diff --git a/packages/super-editor/src/editors/v1/assets/styles/elements/prosemirror.css b/packages/super-editor/src/editors/v1/assets/styles/elements/prosemirror.css index 29b0e02c0c..090dbbbff2 100644 --- a/packages/super-editor/src/editors/v1/assets/styles/elements/prosemirror.css +++ b/packages/super-editor/src/editors/v1/assets/styles/elements/prosemirror.css @@ -432,11 +432,15 @@ https://github.com/ProseMirror/prosemirror-tables/blob/master/demo/index.html margin-top: -5px; } +/* AIDEV-NOTE: SD-3045. `!important` so the transient search highlight wins + * over any inline `style.backgroundColor` painted from a source-doc highlight + * mark on the same span. See packages/superdoc/src/assets/styles/elements/superdoc.css + * for the full rationale. */ .sd-editor-scoped .ProseMirror-search-match { - background-color: #ffff0054; + background-color: #ffff0054 !important; } .sd-editor-scoped .ProseMirror-active-search-match { - background-color: #ff6a0054; + background-color: #ff6a0054 !important; } .sd-editor-scoped .ProseMirror span.sd-custom-selection::selection { background: transparent; diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts index 082e439855..fbab844b04 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts @@ -3533,7 +3533,37 @@ export class PresentationEditor extends EventEmitter { if (pageEl) { // Find the specific element containing this position for precise centering const targetEl = this.#findElementAtPosition(pageEl, clampedPos); - (targetEl ?? pageEl).scrollIntoView({ block, inline: 'nearest', behavior }); + const elToScroll = targetEl ?? pageEl; + elToScroll.scrollIntoView({ block, inline: 'nearest', behavior }); + // AIDEV-NOTE: SD-3045. Search nav (and any other caller of + // scrollToPosition) places the viewport intentionally — usually + // centring the match. The next #updateSelection that runs as part + // of the dispatched setSelection transaction would otherwise call + // #scrollActiveEndIntoView and re-scroll the caret to its minimal + // visible position (often the top of the viewport), undoing our + // centring. Consume the pending scroll-into-view request so that + // selection sync renders the caret overlay without moving the + // scroll back. Other selection updates (Shift+Arrow, typing) re-set + // this flag themselves before they need scroll, so this consume is + // safe. + this.#shouldScrollSelectionIntoView = false; + // Re-assert the scroll on the next animation frame. The flag we + // cleared above defends against handleSelection that has already + // run, but a *later* selectionUpdate (e.g. focus blur fired when + // the user moves focus back to the find input) re-sets the flag to + // true before the RAF-scheduled #updateSelection fires, and that + // pass scrolls the caret to its minimal-visibility position — + // visibly snapping the match out of view. Re-running scrollIntoView + // on the same element a frame later overrides that snap; the no-op + // case (no late scroll happened) just re-centres the same element + // and is cheap. + const win = this.#visibleHost.ownerDocument?.defaultView; + if (win) { + win.requestAnimationFrame(() => { + elToScroll.scrollIntoView({ block, inline: 'nearest', behavior }); + this.#shouldScrollSelectionIntoView = false; + }); + } return true; } } diff --git a/packages/superdoc/src/assets/styles/elements/search-match-precedence.test.js b/packages/superdoc/src/assets/styles/elements/search-match-precedence.test.js new file mode 100644 index 0000000000..d523338e94 --- /dev/null +++ b/packages/superdoc/src/assets/styles/elements/search-match-precedence.test.js @@ -0,0 +1,102 @@ +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); + +// SD-3045 (cross-package CSS invariant). The DomPainter writes +// `style.backgroundColor = run.highlight` inline on the same span the search +// DecorationBridge tags with `.ProseMirror-search-match`. Without `!important`, +// the inline style wins and the search highlight is invisible on every run +// whose source rPr carries a highlight mark (e.g. ``). +// These tests guard the two CSS sites that paint the transient search colour. + +const repoRoot = join(__dirname, '..', '..', '..', '..', '..', '..'); + +const superdocCss = readFileSync( + join(repoRoot, 'packages', 'superdoc', 'src', 'assets', 'styles', 'elements', 'superdoc.css'), + 'utf8', +); + +const editorScopedCss = readFileSync( + join(repoRoot, 'packages', 'super-editor', 'src', 'editors', 'v1', 'assets', 'styles', 'elements', 'prosemirror.css'), + 'utf8', +); + +const extractRuleBody = (css, selector) => { + const idx = css.indexOf(selector); + if (idx === -1) return null; + const open = css.indexOf('{', idx); + const close = css.indexOf('}', open); + if (open === -1 || close === -1) return null; + return css.slice(open + 1, close); +}; + +describe('search-match CSS precedence (SD-3045)', () => { + describe('packages/superdoc/src/assets/styles/elements/superdoc.css', () => { + it('`.superdoc .ProseMirror-search-match` background uses !important', () => { + const body = extractRuleBody(superdocCss, '.superdoc .ProseMirror-search-match'); + expect(body, '.superdoc .ProseMirror-search-match rule must exist').not.toBeNull(); + expect(body).toMatch(/background\s*:[^;]*!important/); + }); + + it('`.superdoc .ProseMirror-active-search-match` background uses !important', () => { + const body = extractRuleBody(superdocCss, '.superdoc .ProseMirror-active-search-match'); + expect(body, '.superdoc .ProseMirror-active-search-match rule must exist').not.toBeNull(); + expect(body).toMatch(/background\s*:[^;]*!important/); + }); + }); + + describe('packages/super-editor/.../prosemirror.css', () => { + it('`.sd-editor-scoped .ProseMirror-search-match` background-color uses !important', () => { + const body = extractRuleBody(editorScopedCss, '.sd-editor-scoped .ProseMirror-search-match'); + expect(body, '.sd-editor-scoped .ProseMirror-search-match rule must exist').not.toBeNull(); + expect(body).toMatch(/background-color\s*:[^;]*!important/); + }); + + it('`.sd-editor-scoped .ProseMirror-active-search-match` background-color uses !important', () => { + const body = extractRuleBody(editorScopedCss, '.sd-editor-scoped .ProseMirror-active-search-match'); + expect(body, '.sd-editor-scoped .ProseMirror-active-search-match rule must exist').not.toBeNull(); + expect(body).toMatch(/background-color\s*:[^;]*!important/); + }); + }); + + describe('JSDOM specificity sanity check', () => { + it('class-level `background !important` beats inline `style="background-color: white"`', () => { + const styleEl = document.createElement('style'); + styleEl.textContent = `.search-test { background: rgba(255, 213, 0, 0.4) !important; }`; + document.head.appendChild(styleEl); + + const span = document.createElement('span'); + span.className = 'search-test'; + span.setAttribute('style', 'background-color: rgb(255, 255, 255);'); + document.body.appendChild(span); + + const bg = getComputedStyle(span).backgroundColor; + + styleEl.remove(); + span.remove(); + + expect(bg).toBe('rgba(255, 213, 0, 0.4)'); + }); + + it('without !important, inline `background-color: white` overrides class background', () => { + const styleEl = document.createElement('style'); + styleEl.textContent = `.search-test-noimp { background: rgba(255, 213, 0, 0.4); }`; + document.head.appendChild(styleEl); + + const span = document.createElement('span'); + span.className = 'search-test-noimp'; + span.setAttribute('style', 'background-color: rgb(255, 255, 255);'); + document.body.appendChild(span); + + const bg = getComputedStyle(span).backgroundColor; + + styleEl.remove(); + span.remove(); + + expect(bg).toBe('rgb(255, 255, 255)'); + }); + }); +}); diff --git a/packages/superdoc/src/assets/styles/elements/superdoc.css b/packages/superdoc/src/assets/styles/elements/superdoc.css index 1686029510..eff079537d 100644 --- a/packages/superdoc/src/assets/styles/elements/superdoc.css +++ b/packages/superdoc/src/assets/styles/elements/superdoc.css @@ -40,12 +40,21 @@ /* --- Search highlights (transient, used in both editor and presentation mode) --- */ +/* AIDEV-NOTE: SD-3045. The DomPainter writes `style.backgroundColor = run.highlight` + * inline on the same span the search DecorationBridge tags. Inline styles win over + * class selectors, so on every run whose source rPr carries a highlight mark + * (e.g. ``), the search highlight was invisible — + * the inline background painted over the class's transient yellow/orange. + * `!important` is the desired semantic here: while a search session is live, the + * find-match colour must override any source-doc highlight. The class is only + * present during a search; clearing the session removes it and the original + * highlight is restored. */ .superdoc .ProseMirror-search-match { - background: var(--sd-ui-search-match-bg); + background: var(--sd-ui-search-match-bg) !important; } .superdoc .ProseMirror-active-search-match { - background: var(--sd-ui-search-match-active-bg); + background: var(--sd-ui-search-match-active-bg) !important; } /* Contained Mode - fixed-height container embedding with internal scrolling */ diff --git a/packages/superdoc/src/components/surfaces/FindReplaceSurface.test.js b/packages/superdoc/src/components/surfaces/FindReplaceSurface.test.js new file mode 100644 index 0000000000..c6791199c2 --- /dev/null +++ b/packages/superdoc/src/components/surfaces/FindReplaceSurface.test.js @@ -0,0 +1,271 @@ +// @ts-nocheck +import { describe, it, expect, vi } from 'vitest'; +import { mount } from '@vue/test-utils'; +import { nextTick, ref, computed } from 'vue'; +import FindReplaceSurface from './FindReplaceSurface.vue'; + +const DEFAULT_TEXTS = { + findPlaceholder: 'Find', + findAriaLabel: 'Find text', + replacePlaceholder: 'Replace', + replaceAriaLabel: 'Replace text', + noResultsLabel: 'No results', + previousMatchLabel: 'Previous', + previousMatchAriaLabel: 'Previous', + nextMatchLabel: 'Next', + nextMatchAriaLabel: 'Next', + closeLabel: 'Close', + closeAriaLabel: 'Close', + replaceLabel: 'Replace', + replaceAllLabel: 'All', + toggleReplaceLabel: 'Toggle replace', + toggleReplaceAriaLabel: 'Toggle replace', + matchCaseLabel: 'Aa', + matchCaseAriaLabel: 'Match case', + ignoreDiacriticsLabel: 'ä≡a', + ignoreDiacriticsAriaLabel: 'Ignore diacritics', +}; + +/** + * Build a ref-shaped findReplace handle that mirrors what useFindReplace provides. + * goNext / goPrev simulate the real Search extension behaviour of focusing the + * editor view (which is what causes SD-3045 in production) by moving focus to a + * detached element supplied via `stealFocusInto`. + */ +function createHandle(overrides = {}) { + const findQuery = ref('Lorem'); + const replaceText = ref(''); + const caseSensitive = ref(false); + const ignoreDiacritics = ref(false); + const showReplace = ref(false); + const matchCount = ref(16); + const activeMatchIndex = ref(0); + + return { + findQuery, + replaceText, + caseSensitive, + ignoreDiacritics, + showReplace, + matchCount, + activeMatchIndex, + matchLabel: computed(() => `${activeMatchIndex.value + 1} of ${matchCount.value}`), + hasMatches: computed(() => matchCount.value > 0), + replaceEnabled: true, + texts: { ...DEFAULT_TEXTS }, + goNext: vi.fn(() => overrides.stealFocusInto?.focus()), + goPrev: vi.fn(() => overrides.stealFocusInto?.focus()), + replaceCurrent: vi.fn(), + replaceAll: vi.fn(), + registerFocusFn: vi.fn(), + close: vi.fn(), + ...overrides.handle, + }; +} + +function mountSurface(handle) { + return mount(FindReplaceSurface, { + attachTo: document.body, + props: { + surfaceId: 'fr-1', + mode: 'floating', + request: {}, + resolve: vi.fn(), + close: vi.fn(), + findReplace: handle, + }, + }); +} + +describe('FindReplaceSurface — keyboard focus (SD-3045)', () => { + it('keeps focus on the find input after pressing Enter, even when goNext steals focus to another element', async () => { + // The editor view focus steal is the real-world cause of SD-3045 — simulate it + // with a detached div that goNext focuses synchronously, matching the Search + // extension's editor.view.focus() side effect. + const stealTarget = document.createElement('div'); + stealTarget.tabIndex = -1; + document.body.appendChild(stealTarget); + + try { + const handle = createHandle({ stealFocusInto: stealTarget }); + const wrapper = mountSurface(handle); + const input = wrapper.find('.sd-find-replace__input').element; + input.focus(); + expect(document.activeElement).toBe(input); + + await wrapper.find('.sd-find-replace__input').trigger('keydown', { key: 'Enter' }); + await nextTick(); + + expect(handle.goNext).toHaveBeenCalledTimes(1); + expect(document.activeElement).toBe(input); + + wrapper.unmount(); + } finally { + stealTarget.remove(); + } + }); + + it('keeps focus on the find input after Shift+Enter (previous match)', async () => { + const stealTarget = document.createElement('div'); + stealTarget.tabIndex = -1; + document.body.appendChild(stealTarget); + + try { + const handle = createHandle({ stealFocusInto: stealTarget }); + const wrapper = mountSurface(handle); + const input = wrapper.find('.sd-find-replace__input').element; + input.focus(); + + await wrapper.find('.sd-find-replace__input').trigger('keydown', { key: 'Enter', shiftKey: true }); + await nextTick(); + + expect(handle.goPrev).toHaveBeenCalledTimes(1); + expect(document.activeElement).toBe(input); + + wrapper.unmount(); + } finally { + stealTarget.remove(); + } + }); + + it('prevents the default Enter behaviour so the editor never receives the keystroke', () => { + const handle = createHandle(); + const wrapper = mountSurface(handle); + const input = wrapper.find('.sd-find-replace__input').element; + input.focus(); + + const event = new KeyboardEvent('keydown', { key: 'Enter', cancelable: true, bubbles: true }); + input.dispatchEvent(event); + expect(event.defaultPrevented).toBe(true); + wrapper.unmount(); + }); + + // SD-3045 follow-up (Luccas's PR review comment on #3240): pressing Enter + // when the next match is on a different page must not undo the + // PresentationEditor.scrollToPosition that goNext just performed. The + // surface is rendered in the normal document flow, so the browser's + // default "scroll input into view" behaviour on .focus() snaps the + // document back to wherever the find input is, hiding the new match. The + // fix is to restore focus with { preventScroll: true } so the document + // scroll stays where goNext placed it. + it('restores focus without scrolling the document back to the find input', async () => { + const stealTarget = document.createElement('div'); + stealTarget.tabIndex = -1; + document.body.appendChild(stealTarget); + + try { + const handle = createHandle({ stealFocusInto: stealTarget }); + const wrapper = mountSurface(handle); + const input = wrapper.find('.sd-find-replace__input').element; + + const focusSpy = vi.spyOn(input, 'focus'); + input.focus(); + + await wrapper.find('.sd-find-replace__input').trigger('keydown', { key: 'Enter' }); + await nextTick(); + + // After Enter, the surface restores focus to the input. That focus call + // must pass preventScroll so the browser does not scroll the document + // back to the input — otherwise the goNext scroll is undone for any + // match on a different page. + const calls = focusSpy.mock.calls; + expect(calls.length).toBeGreaterThan(0); + const restoreCall = calls[calls.length - 1]; + expect(restoreCall[0]).toEqual(expect.objectContaining({ preventScroll: true })); + + focusSpy.mockRestore(); + wrapper.unmount(); + } finally { + stealTarget.remove(); + } + }); + + it('uses preventScroll on Shift+Enter focus restore too', async () => { + const stealTarget = document.createElement('div'); + stealTarget.tabIndex = -1; + document.body.appendChild(stealTarget); + + try { + const handle = createHandle({ stealFocusInto: stealTarget }); + const wrapper = mountSurface(handle); + const input = wrapper.find('.sd-find-replace__input').element; + + const focusSpy = vi.spyOn(input, 'focus'); + input.focus(); + + await wrapper.find('.sd-find-replace__input').trigger('keydown', { key: 'Enter', shiftKey: true }); + await nextTick(); + + const calls = focusSpy.mock.calls; + const restoreCall = calls[calls.length - 1]; + expect(restoreCall[0]).toEqual(expect.objectContaining({ preventScroll: true })); + + focusSpy.mockRestore(); + wrapper.unmount(); + } finally { + stealTarget.remove(); + } + }); + + // SD-3045 holistic: clicking the next/prev buttons must also restore focus to + // the input — otherwise the button receives focus and the browser scrolls + // the document back to the (off-screen) find bar, undoing the goNext scroll + // exactly the same way pressing Enter without focus restore did. + it('restores focus to the find input after clicking the next-match button', async () => { + const stealTarget = document.createElement('div'); + stealTarget.tabIndex = -1; + document.body.appendChild(stealTarget); + + try { + const handle = createHandle({ stealFocusInto: stealTarget }); + const wrapper = mountSurface(handle); + const input = wrapper.find('.sd-find-replace__input').element; + const focusSpy = vi.spyOn(input, 'focus'); + + const buttons = wrapper.findAll('.sd-find-replace__btn--icon'); + // First two icon buttons are prev (▲) and next (▼); third is close. + const nextBtn = buttons[1]; + expect(nextBtn.exists()).toBe(true); + + await nextBtn.trigger('click'); + await nextTick(); + + expect(handle.goNext).toHaveBeenCalledTimes(1); + const focusCall = focusSpy.mock.calls.find((args) => args[0]?.preventScroll === true); + expect(focusCall).toBeDefined(); + + focusSpy.mockRestore(); + wrapper.unmount(); + } finally { + stealTarget.remove(); + } + }); + + it('restores focus to the find input after clicking the previous-match button', async () => { + const stealTarget = document.createElement('div'); + stealTarget.tabIndex = -1; + document.body.appendChild(stealTarget); + + try { + const handle = createHandle({ stealFocusInto: stealTarget }); + const wrapper = mountSurface(handle); + const input = wrapper.find('.sd-find-replace__input').element; + const focusSpy = vi.spyOn(input, 'focus'); + + const buttons = wrapper.findAll('.sd-find-replace__btn--icon'); + const prevBtn = buttons[0]; + + await prevBtn.trigger('click'); + await nextTick(); + + expect(handle.goPrev).toHaveBeenCalledTimes(1); + const focusCall = focusSpy.mock.calls.find((args) => args[0]?.preventScroll === true); + expect(focusCall).toBeDefined(); + + focusSpy.mockRestore(); + wrapper.unmount(); + } finally { + stealTarget.remove(); + } + }); +}); diff --git a/packages/superdoc/src/components/surfaces/FindReplaceSurface.vue b/packages/superdoc/src/components/surfaces/FindReplaceSurface.vue index 96cc56f390..53bcbbf5e0 100644 --- a/packages/superdoc/src/components/surfaces/FindReplaceSurface.vue +++ b/packages/superdoc/src/components/surfaces/FindReplaceSurface.vue @@ -17,20 +17,70 @@ const findInputRef = ref(null); function handleFindKeydown(e) { if (e.key === 'Enter' && !e.shiftKey) { e.preventDefault(); - props.findReplace.goNext(); + handleGoNext(); } else if (e.key === 'Enter' && e.shiftKey) { e.preventDefault(); - props.findReplace.goPrev(); + handleGoPrev(); } } +// Single path for advancing a match. Both the Enter key and the next/prev +// buttons must route through here so the focus restore — which preserves the +// goNext / PresentationEditor.scrollToPosition we just performed — runs in +// every navigation case. Clicking a button without restoring focus lets the +// button keep focus, and the browser scrolls the (off-screen) find bar back +// into view, undoing the navigation scroll on cross-page matches. +function handleGoNext() { + props.findReplace.goNext(); + focusFindInput(); +} + +function handleGoPrev() { + props.findReplace.goPrev(); + focusFindInput(); +} + function handleClose() { props.findReplace.close('user-closed'); } +function collectScrollableAncestors(element) { + const result = []; + let cur = element?.parentElement ?? null; + while (cur) { + const style = cur.ownerDocument?.defaultView?.getComputedStyle?.(cur); + if (style) { + const overflow = `${style.overflowY} ${style.overflowX}`; + if (overflow.includes('auto') || overflow.includes('scroll')) { + result.push(cur); + } + } + cur = cur.parentElement; + } + const root = element?.ownerDocument?.scrollingElement; + if (root && !result.includes(root)) result.push(root); + return result; +} + function focusFindInput() { - findInputRef.value?.focus(); - findInputRef.value?.select(); + // The surface lives in the document's normal flow. After goNext, the + // PresentationEditor has scrolled the SuperDoc container to the active + // match — which can be on a different page. Anything that gives focus to a + // descendant of the (now off-screen) find bar can trigger the browser's + // "scroll element into view" behaviour and snap that scroll back, hiding + // the match. `preventScroll: true` covers `.focus()`; we additionally pin + // every scrollable ancestor's scroll position across the call as a belt + // for `.focus()` implementations or related side effects that ignore + // preventScroll (SD-3045 review — match on different page never appeared). + const input = findInputRef.value; + if (!input) return; + const scrollables = collectScrollableAncestors(input); + const saved = scrollables.map((el) => ({ el, top: el.scrollTop, left: el.scrollLeft })); + input.focus({ preventScroll: true }); + for (const { el, top, left } of saved) { + if (el.scrollTop !== top) el.scrollTop = top; + if (el.scrollLeft !== left) el.scrollLeft = left; + } } onMounted(() => { @@ -66,7 +116,7 @@ onMounted(() => { :disabled="!findReplace.hasMatches.value" :title="findReplace.texts.previousMatchLabel" :aria-label="findReplace.texts.previousMatchAriaLabel" - @click="findReplace.goPrev()" + @click="handleGoPrev()" > ▲ @@ -76,7 +126,7 @@ onMounted(() => { :disabled="!findReplace.hasMatches.value" :title="findReplace.texts.nextMatchLabel" :aria-label="findReplace.texts.nextMatchAriaLabel" - @click="findReplace.goNext()" + @click="handleGoNext()" > ▼