fix(transcription): harden re-edit pencil hit-testing + disable sync (#628 review)
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 1m17s
CI / OCR Service Tests (pull_request) Successful in 22s
CI / Backend Unit Tests (pull_request) Successful in 3m31s
CI / fail2ban Regex (pull_request) Successful in 50s
CI / Semgrep Security Scan (pull_request) Successful in 29s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m5s
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 1m17s
CI / OCR Service Tests (pull_request) Successful in 22s
CI / Backend Unit Tests (pull_request) Successful in 3m31s
CI / fail2ban Regex (pull_request) Successful in 50s
CI / Semgrep Security Scan (pull_request) Successful in 29s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m5s
Addresses the clean-agent review of PR #717: - C1: the hidden pencil was opacity-0 only, which still hit-tests; its 44px box overhangs adjacent text, so a click in the gap between two mentions could land on the invisible button and spuriously open the dropdown (AC-8 hole). Add pointer-events-none while hidden, re-enabled with the opacity reveal on hover/focus. - C2/N1: editor.setEditable() emits "update", not a ProseMirror transaction, so the NodeView's 'transaction' listener missed a mid-session disable flip (stale aria-disabled/tabindex; the comment was wrong). Listen on 'update' instead — which also skips selection-only changes, so it fires far less often. - N2: track the node across update() so the pencil opens with the live displayName (hardening; relink only swaps personId today). Tests: structural guard that the hidden pencil is pointer-events-none + reveals, and a mid-session disable-flip test (fixture gains an onReady setDisabled hook). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -1333,3 +1333,54 @@ describe('PersonMentionEditor — #628 editing announce', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ─── #628 review fixes: hidden-pencil hit-testing + mid-session disable ───────
|
||||||
|
|
||||||
|
describe('PersonMentionEditor — #628 review fixes', () => {
|
||||||
|
it('keeps the hidden pencil out of hit-testing (pointer-events-none until revealed)', async () => {
|
||||||
|
// opacity-0 alone still hit-tests, and the 44px button overhangs adjacent
|
||||||
|
// text — so a click in the gap between mentions would otherwise land on the
|
||||||
|
// invisible pencil and open the dropdown (AC-8). It must be pointer-events-none
|
||||||
|
// while hidden and only become clickable together with the opacity reveal.
|
||||||
|
renderHost({ value: '@Aug ', mentionedPersons: [{ personId: 'p-aug', displayName: 'Aug' }] });
|
||||||
|
await vi.waitFor(() =>
|
||||||
|
expect(document.querySelector('[data-type="mention"] button')).not.toBeNull()
|
||||||
|
);
|
||||||
|
const btn = document.querySelector('[data-type="mention"] button') as HTMLElement;
|
||||||
|
expect(btn.className).toContain('pointer-events-none');
|
||||||
|
expect(btn.className).toContain('group-hover/mention:pointer-events-auto');
|
||||||
|
expect(btn.className).toContain('group-focus-within/mention:pointer-events-auto');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('re-syncs the pencil to inert when the editor is disabled mid-session', async () => {
|
||||||
|
// editor.setEditable() emits "update", not a transaction — the NodeView must
|
||||||
|
// listen on "update" or a runtime disable flip leaves the pencil stale.
|
||||||
|
let setDisabled!: (value: boolean) => void;
|
||||||
|
render(PersonMentionEditorHost, {
|
||||||
|
initialValue: '@Aug ',
|
||||||
|
initialMentions: [{ personId: 'p-aug', displayName: 'Aug' }],
|
||||||
|
disabled: false,
|
||||||
|
onChange: () => {},
|
||||||
|
onReady: (api: { setDisabled: (value: boolean) => void }) => {
|
||||||
|
setDisabled = api.setDisabled;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
await vi.waitFor(() => {
|
||||||
|
const btn = document.querySelector(
|
||||||
|
'[data-type="mention"] button'
|
||||||
|
) as HTMLButtonElement | null;
|
||||||
|
expect(btn).not.toBeNull();
|
||||||
|
expect(btn!.disabled).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
setDisabled(true);
|
||||||
|
|
||||||
|
await vi.waitFor(() => {
|
||||||
|
const btn = document.querySelector('[data-type="mention"] button') as HTMLButtonElement;
|
||||||
|
expect(btn.disabled).toBe(true);
|
||||||
|
expect(btn.getAttribute('aria-disabled')).toBe('true');
|
||||||
|
expect(btn.tabIndex).toBe(-1);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
<script lang="ts">
|
<script lang="ts">
|
||||||
import { untrack } from 'svelte';
|
import { onMount, untrack } from 'svelte';
|
||||||
import PersonMentionEditor from './PersonMentionEditor.svelte';
|
import PersonMentionEditor from './PersonMentionEditor.svelte';
|
||||||
import type { components } from '$lib/generated/api';
|
import type { components } from '$lib/generated/api';
|
||||||
|
|
||||||
@@ -11,6 +11,9 @@ type Props = {
|
|||||||
placeholder?: string;
|
placeholder?: string;
|
||||||
disabled?: boolean;
|
disabled?: boolean;
|
||||||
onChange: (snapshot: { value: string; mentionedPersons: PersonMention[] }) => void;
|
onChange: (snapshot: { value: string; mentionedPersons: PersonMention[] }) => void;
|
||||||
|
/** Hands the test a setter so it can flip `disabled` after mount — used to
|
||||||
|
* exercise the editor's reactive `disabled` $effect (mid-session toggle). */
|
||||||
|
onReady?: (api: { setDisabled: (value: boolean) => void }) => void;
|
||||||
};
|
};
|
||||||
|
|
||||||
let {
|
let {
|
||||||
@@ -18,13 +21,19 @@ let {
|
|||||||
initialMentions = [],
|
initialMentions = [],
|
||||||
placeholder,
|
placeholder,
|
||||||
disabled = false,
|
disabled = false,
|
||||||
onChange
|
onChange,
|
||||||
|
onReady
|
||||||
}: Props = $props();
|
}: Props = $props();
|
||||||
|
|
||||||
// initial* props seed mount-time state; reading them inside untrack signals
|
// initial* props seed mount-time state; reading them inside untrack signals
|
||||||
// the intentional one-shot capture and silences state_referenced_locally.
|
// the intentional one-shot capture and silences state_referenced_locally.
|
||||||
let value = $state(untrack(() => initialValue));
|
let value = $state(untrack(() => initialValue));
|
||||||
let mentionedPersons = $state<PersonMention[]>(untrack(() => [...initialMentions]));
|
let mentionedPersons = $state<PersonMention[]>(untrack(() => [...initialMentions]));
|
||||||
|
let disabledState = $state(untrack(() => disabled));
|
||||||
|
|
||||||
|
onMount(() => {
|
||||||
|
onReady?.({ setDisabled: (next: boolean) => (disabledState = next) });
|
||||||
|
});
|
||||||
|
|
||||||
$effect(() => {
|
$effect(() => {
|
||||||
onChange({ value, mentionedPersons: [...mentionedPersons] });
|
onChange({ value, mentionedPersons: [...mentionedPersons] });
|
||||||
@@ -35,5 +44,5 @@ $effect(() => {
|
|||||||
bind:value={value}
|
bind:value={value}
|
||||||
bind:mentionedPersons={mentionedPersons}
|
bind:mentionedPersons={mentionedPersons}
|
||||||
placeholder={placeholder}
|
placeholder={placeholder}
|
||||||
disabled={disabled}
|
disabled={disabledState}
|
||||||
/>
|
/>
|
||||||
|
|||||||
@@ -57,6 +57,11 @@ function buildPencilIcon(): SVGSVGElement {
|
|||||||
*/
|
*/
|
||||||
export function createMentionNodeView(requestRelink: RelinkRequest) {
|
export function createMentionNodeView(requestRelink: RelinkRequest) {
|
||||||
return ({ node, editor, getPos }: NodeViewArgs) => {
|
return ({ node, editor, getPos }: NodeViewArgs) => {
|
||||||
|
// Tracks the node's current attrs across update()s so the pencil always
|
||||||
|
// opens with the live displayName (relink only swaps personId today, but
|
||||||
|
// this keeps openRelink honest if displayName ever becomes mutable).
|
||||||
|
let currentNode = node;
|
||||||
|
|
||||||
const dom = document.createElement('span');
|
const dom = document.createElement('span');
|
||||||
dom.className = 'mention-token group/mention';
|
dom.className = 'mention-token group/mention';
|
||||||
dom.setAttribute('data-type', 'mention');
|
dom.setAttribute('data-type', 'mention');
|
||||||
@@ -81,11 +86,18 @@ export function createMentionNodeView(requestRelink: RelinkRequest) {
|
|||||||
button.setAttribute('aria-label', m.person_mention_edit_label());
|
button.setAttribute('aria-label', m.person_mention_edit_label());
|
||||||
// Visible glyph stays ~16px; the 44px tap target comes from padding pulled
|
// Visible glyph stays ~16px; the 44px tap target comes from padding pulled
|
||||||
// back with negative margin so the larger hit box does not reflow the line.
|
// back with negative margin so the larger hit box does not reflow the line.
|
||||||
|
// While hidden the button must be pointer-events-none — opacity-0 alone
|
||||||
|
// still hit-tests, and the 44px box overhangs the adjacent text, so a click
|
||||||
|
// in the gap between two mentions would otherwise land on this invisible
|
||||||
|
// button and spuriously open the dropdown (AC-8). Pointer events are
|
||||||
|
// re-enabled together with the opacity reveal on hover/focus.
|
||||||
button.className = [
|
button.className = [
|
||||||
'mention-edit-btn',
|
'mention-edit-btn',
|
||||||
'-mx-3 -my-2 inline-flex h-11 w-11 items-center justify-center rounded-sm text-ink-2',
|
'-mx-3 -my-2 inline-flex h-11 w-11 items-center justify-center rounded-sm text-ink-2',
|
||||||
'opacity-0 transition-none',
|
'pointer-events-none opacity-0 transition-none',
|
||||||
'group-hover/mention:opacity-100 group-focus-within/mention:opacity-100 focus:opacity-100',
|
'group-hover/mention:pointer-events-auto group-hover/mention:opacity-100',
|
||||||
|
'group-focus-within/mention:pointer-events-auto group-focus-within/mention:opacity-100',
|
||||||
|
'focus:pointer-events-auto focus:opacity-100',
|
||||||
'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-brand-navy',
|
'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-brand-navy',
|
||||||
'disabled:cursor-not-allowed'
|
'disabled:cursor-not-allowed'
|
||||||
].join(' ');
|
].join(' ');
|
||||||
@@ -101,7 +113,7 @@ export function createMentionNodeView(requestRelink: RelinkRequest) {
|
|||||||
if (!editor.isEditable) return;
|
if (!editor.isEditable) return;
|
||||||
const pos = getPos();
|
const pos = getPos();
|
||||||
if (pos === undefined) return;
|
if (pos === undefined) return;
|
||||||
requestRelink(() => text.getBoundingClientRect(), node.attrs.displayName ?? '', pos);
|
requestRelink(() => text.getBoundingClientRect(), currentNode.attrs.displayName ?? '', pos);
|
||||||
};
|
};
|
||||||
|
|
||||||
// Prevent mousedown from moving the editor selection before the click
|
// Prevent mousedown from moving the editor selection before the click
|
||||||
@@ -124,9 +136,11 @@ export function createMentionNodeView(requestRelink: RelinkRequest) {
|
|||||||
}
|
}
|
||||||
};
|
};
|
||||||
syncEditable();
|
syncEditable();
|
||||||
// setEditable (and any doc change) dispatches a transaction — re-sync the
|
// editor.setEditable() emits "update" (not a ProseMirror transaction), so
|
||||||
// pencil's inert state so a mid-session disable flip takes effect.
|
// we listen on "update" to re-sync the pencil's inert state when `disabled`
|
||||||
editor.on('transaction', syncEditable);
|
// flips mid-session. "update" also skips selection-only changes, so this
|
||||||
|
// fires far less often than a "transaction" listener would.
|
||||||
|
editor.on('update', syncEditable);
|
||||||
|
|
||||||
return {
|
return {
|
||||||
dom,
|
dom,
|
||||||
@@ -139,13 +153,14 @@ export function createMentionNodeView(requestRelink: RelinkRequest) {
|
|||||||
stopEvent: (event: Event) => button.contains(event.target as Node),
|
stopEvent: (event: Event) => button.contains(event.target as Node),
|
||||||
update(updatedNode: ProseMirrorNode) {
|
update(updatedNode: ProseMirrorNode) {
|
||||||
if (updatedNode.type.name !== 'mention') return false;
|
if (updatedNode.type.name !== 'mention') return false;
|
||||||
|
currentNode = updatedNode;
|
||||||
text.textContent = `@${updatedNode.attrs.displayName}`;
|
text.textContent = `@${updatedNode.attrs.displayName}`;
|
||||||
dom.setAttribute('data-person-id', updatedNode.attrs.personId ?? '');
|
dom.setAttribute('data-person-id', updatedNode.attrs.personId ?? '');
|
||||||
dom.setAttribute('data-display-name', updatedNode.attrs.displayName ?? '');
|
dom.setAttribute('data-display-name', updatedNode.attrs.displayName ?? '');
|
||||||
return true;
|
return true;
|
||||||
},
|
},
|
||||||
destroy() {
|
destroy() {
|
||||||
editor.off('transaction', syncEditable);
|
editor.off('update', syncEditable);
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
};
|
};
|
||||||
|
|||||||
Reference in New Issue
Block a user