diff --git a/e2e/links-open.spec.ts b/e2e/links-open.spec.ts new file mode 100644 index 0000000..b7ee758 --- /dev/null +++ b/e2e/links-open.spec.ts @@ -0,0 +1,95 @@ +import { test, expect, type Page } from '@playwright/test' + +// Live-preview links must open on a real click, in a real browser. +// +// jsdom cannot catch this class of break: PR #302 moved the open from +// mousedown to mouseup and its jsdom test kept a stale reference to the link +// element, so it stayed green while every markdown link on beta went dead. +// Here Chromium does the press and the release itself. + +test.beforeEach(async ({ page }) => { + await page.addInitScript(() => { + try { window.localStorage.clear() } catch { /* ignore */ } + try { for (const n of ['noteser', 'keyval-store']) indexedDB.deleteDatabase(n) } catch { /* ignore */ } + try { + window.localStorage.setItem('noteser-settings', JSON.stringify({ + state: { + onboardingShown: true, + sidebarGroups: [{ id: 'g-files', tabs: ['files'], activeTab: 'files', collapsed: false }], + }, + version: 3, + })) + } catch { /* ignore */ } + // Record window.open instead of actually opening a tab. + ;(window as unknown as { __opened: string[] }).__opened = [] + window.open = ((url?: string | URL) => { + ;(window as unknown as { __opened: string[] }).__opened.push(String(url)) + return null + }) as typeof window.open + }) +}) + +async function opened(page: Page): Promise { + return page.evaluate(() => (window as unknown as { __opened: string[] }).__opened) +} + +test('markdown link, bare URL and wikilink all open on a real click', async ({ page }) => { + await page.goto('/') + await expect(page.getByTestId('folder-tree')).toBeVisible() + await page.waitForFunction(() => !!window.__noteser_test?.stores?.noteStore) + + const id = await page.evaluate(() => { + const ns = window.__noteser_test!.stores.noteStore.getState() + ns.addNote({ title: 'Target', content: 'target body', folderId: null }) + return ns.addNote({ + title: 'Links', + content: 'md [Example](https://example.com/md) end\n\nbare https://example.com/bare end\n\nwiki [[Target]] end\n', + folderId: null, + }).id + }) + await page.evaluate((noteId) => { + const el = document.querySelector(`[data-testid="note-row"][data-note-id="${noteId}"]`) as HTMLElement + el.click(); el.click() + }, id) + + await expect(page.locator('.cm-lp-link').first()).toBeVisible() + + // 1. markdown link + await page.locator('.cm-lp-link[data-cm-lp-href="https://example.com/md"]').click() + await page.waitForTimeout(150) + expect(await opened(page)).toContain('https://example.com/md') + + // 2. bare URL + await page.locator('.cm-lp-link[data-cm-lp-href="https://example.com/bare"]').click() + await page.waitForTimeout(150) + expect(await opened(page)).toContain('https://example.com/bare') + + // 3. wikilink + await page.locator('.cm-lp-wikilink').click() + await page.waitForTimeout(300) + const tab = await page.locator('.border-t-obsidianAccentPurple span.truncate').first().textContent() + expect(tab).toContain('Target') +}) + +test('drag starting on a markdown link selects, does not navigate (#300)', async ({ page }) => { + await page.goto('/') + await expect(page.getByTestId('folder-tree')).toBeVisible() + await page.waitForFunction(() => !!window.__noteser_test?.stores?.noteStore) + const id = await page.evaluate(() => window.__noteser_test!.stores.noteStore.getState() + .addNote({ title: 'Links2', content: 'md [Example](https://example.com/md) trailing text here\n', folderId: null }).id) + await page.evaluate((noteId) => { + const el = document.querySelector(`[data-testid="note-row"][data-note-id="${noteId}"]`) as HTMLElement + el.click(); el.click() + }, id) + const link = page.locator('.cm-lp-link[data-cm-lp-href="https://example.com/md"]') + await expect(link).toBeVisible() + const box = (await link.boundingBox())! + await page.mouse.move(box.x + 2, box.y + box.height / 2) + await page.mouse.down() + await page.mouse.move(box.x + 120, box.y + box.height / 2, { steps: 8 }) + await page.mouse.up() + await page.waitForTimeout(150) + const sel = await page.evaluate(() => window.getSelection()?.toString() ?? '') + expect(await opened(page)).toHaveLength(0) + expect(sel.length).toBeGreaterThan(0) +}) diff --git a/src/__tests__/linksLivePreview.test.ts b/src/__tests__/linksLivePreview.test.ts index 42b1674..cb00c66 100644 --- a/src/__tests__/linksLivePreview.test.ts +++ b/src/__tests__/linksLivePreview.test.ts @@ -32,6 +32,7 @@ import { shouldOpenOnRelease, type LinksLivePreviewDeps, } from '../components/editor/linksLivePreview' +import type { Note } from '../types' const deps: LinksLivePreviewDeps = { getActiveNotes: () => [], @@ -219,45 +220,47 @@ describe('linksLivePreview — produced decoration types', () => { // so a text selection that starts on a link selects instead of navigating. describe('shouldOpenOnRelease', () => { - const el = {} // stand-in for the link element (identity is all that matters) - const other = {} + const key = 'https://example.com' + const other = 'https://other.example' test('press then release in place on the same link opens', () => { expect(shouldOpenOnRelease( - { x: 10, y: 10, el }, - { x: 11, y: 12, el, button: 0 }, + { x: 10, y: 10, key }, + { x: 11, y: 12, key, button: 0 }, )).toBe(true) }) test('a 10 px drag before release does NOT open', () => { expect(shouldOpenOnRelease( - { x: 10, y: 10, el }, - { x: 20, y: 10, el, button: 0 }, + { x: 10, y: 10, key }, + { x: 20, y: 10, key, button: 0 }, )).toBe(false) }) test('release without a recorded press does NOT open', () => { - expect(shouldOpenOnRelease(null, { x: 10, y: 10, el, button: 0 })).toBe(false) + expect(shouldOpenOnRelease(null, { x: 10, y: 10, key, button: 0 })).toBe(false) }) test('releasing over a DIFFERENT link does NOT open', () => { expect(shouldOpenOnRelease( - { x: 10, y: 10, el }, - { x: 10, y: 10, el: other, button: 0 }, + { x: 10, y: 10, key }, + { x: 10, y: 10, key: other, button: 0 }, )).toBe(false) }) - test('releasing outside any link does NOT open', () => { + test('releasing where the link no longer renders STILL opens', () => { + // reveal-on-cursor destroys the pressed span during the press, so a + // release over no link is the normal case for a click, not a miss. expect(shouldOpenOnRelease( - { x: 10, y: 10, el }, - { x: 10, y: 10, el: null, button: 0 }, - )).toBe(false) + { x: 10, y: 10, key }, + { x: 10, y: 10, key: null, button: 0 }, + )).toBe(true) }) test('the right button never opens', () => { expect(shouldOpenOnRelease( - { x: 10, y: 10, el }, - { x: 10, y: 10, el, button: 2 }, + { x: 10, y: 10, key }, + { x: 10, y: 10, key, button: 2 }, )).toBe(false) }) }) @@ -331,3 +334,92 @@ describe('linksLivePreview external-link DOM handlers', () => { expect(openSpy).not.toHaveBeenCalled() }) }) + +// ── Regression: reveal-on-cursor destroys the pressed span (#302 / issue #300) ─ +// CodeMirror's own mousedown observer runs BEFORE our domEventHandlers, so the +// press moves the caret into the link, `selectionTouches` fires, and the +// `.cm-lp-link` element that received the mousedown is gone by mouseup. + +describe('linksLivePreview — link survives reveal-on-cursor between press and release', () => { + let view: EditorView + let openSpy: jest.SpyInstance + + function mouse(type: string, x: number, y: number, target: Element, button = 0): void { + target.dispatchEvent(new MouseEvent(type, { + bubbles: true, cancelable: true, clientX: x, clientY: y, button, + })) + } + + /** Whatever the pointer is over now — the link if it survived, else the line. */ + function underPointer(): Element { + return view.contentDOM.querySelector('.cm-lp-link') + ?? view.contentDOM.firstElementChild as Element + } + + beforeAll(() => { + const rect = new DOMRect(0, 0, 200, 16) + Range.prototype.getClientRects = () => ([rect] as unknown as DOMRectList) + Range.prototype.getBoundingClientRect = () => rect + }) + + beforeEach(() => { + openSpy = jest.spyOn(window, 'open').mockImplementation(() => null) + view = new EditorView({ + state: EditorState.create({ + doc: 'go [text](https://example.com) now\n', + selection: { anchor: 0 }, + extensions: [markdown({ base: markdownLanguage }), linksLivePreview(deps)], + }), + parent: document.body, + }) + }) + + afterEach(() => { + view.destroy() + openSpy.mockRestore() + }) + + test('the press really does destroy the rendered link element', () => { + // Pins the mechanism this regression test exists for. + const pressed = view.contentDOM.querySelector('.cm-lp-link') + expect(pressed).not.toBeNull() + mouse('mousedown', 5, 5, pressed!) + expect(view.contentDOM.querySelector('.cm-lp-link')).toBeNull() + }) + + test('clicking a markdown link opens it even though its span was re-rendered', () => { + mouse('mousedown', 5, 5, view.contentDOM.querySelector('.cm-lp-link')!) + mouse('mouseup', 5, 5, underPointer()) + expect(openSpy).toHaveBeenCalledWith('https://example.com', '_blank', 'noopener,noreferrer') + }) + + test('a wikilink widget still navigates on press + release', () => { + // The widget swallows the mousedown, so its span survives — but it shares + // the same press slot, so pin it here too. + const nav = jest.fn() + const notes = [{ id: 'n1', title: 'My Note', content: '' }] as unknown as Note[] + const wikiView = new EditorView({ + state: EditorState.create({ + doc: 'go [[My Note]] now\n', + selection: { anchor: 0 }, + extensions: [ + markdown({ base: markdownLanguage }), + linksLivePreview({ getActiveNotes: () => notes, onWikilinkNavigate: nav }), + ], + }), + parent: document.body, + }) + const widget = wikiView.contentDOM.querySelector('.cm-lp-wikilink')! + mouse('mousedown', 5, 5, widget) + mouse('mouseup', 5, 5, widget) + expect(nav).toHaveBeenCalledWith(expect.objectContaining({ id: 'n1' })) + wikiView.destroy() + }) + + test('a drag off a markdown link still selects instead of navigating (#300)', () => { + mouse('mousedown', 5, 5, view.contentDOM.querySelector('.cm-lp-link')!) + mouse('mousemove', 60, 5, underPointer()) + mouse('mouseup', 60, 5, underPointer()) + expect(openSpy).not.toHaveBeenCalled() + }) +}) diff --git a/src/components/editor/linksLivePreview.tsx b/src/components/editor/linksLivePreview.tsx index 314618d..6363451 100644 --- a/src/components/editor/linksLivePreview.tsx +++ b/src/components/editor/linksLivePreview.tsx @@ -56,17 +56,26 @@ export const LINK_DRAG_SLOP_PX = 4 export interface LinkPress { x: number y: number - /** Identity of the pressed link element. */ - el: unknown + /** What was pressed: the external href, or `wikilink:`. */ + key: string } -/** Pure decision: does this release open the link the press started on? */ +/** + * Pure decision: does this release open the link the press started on? + * + * NOT compared by element identity. CodeMirror's own mousedown observer runs + * before our handlers, so the press moves the caret into the link, + * reveal-on-cursor rebuilds the decorations, and the pressed `.cm-lp-link` + * span is already detached by mouseup. We compare the link's key instead, and + * a release over no link at all still counts — only a DIFFERENT link cancels. + * The slop check is what keeps a drag-selection from navigating (issue #300). + */ export function shouldOpenOnRelease( press: LinkPress | null, - release: { x: number; y: number; el: unknown; button: number }, + release: { x: number; y: number; key: string | null; button: number }, ): boolean { if (!press || release.button !== 0) return false - if (release.el == null || release.el !== press.el) return false + if (release.key !== null && release.key !== press.key) return false return Math.hypot(release.x - press.x, release.y - press.y) < LINK_DRAG_SLOP_PX } @@ -111,16 +120,17 @@ class WikilinkWidget extends WidgetType { ? `Open: ${note.title}${fragment ? ` → ${fragment}` : ''}` : `Note not found: ${title}` if (!note) span.classList.add('cm-lp-wikilink-missing') + const key = `wikilink:${this.target}` // mousedown (not click): stop CodeMirror from moving the caret into the // widget. Navigation waits for the release (issue #300). span.addEventListener('mousedown', e => { e.preventDefault() e.stopPropagation() - linkPress = e.button === 0 ? { x: e.clientX, y: e.clientY, el: span } : null + linkPress = e.button === 0 ? { x: e.clientX, y: e.clientY, key } : null }) span.addEventListener('mouseup', e => { const open = shouldOpenOnRelease(linkPress, { - x: e.clientX, y: e.clientY, el: span, button: e.button, + x: e.clientX, y: e.clientY, key, button: e.button, }) linkPress = null if (!open) return @@ -317,36 +327,36 @@ const linksTheme = EditorView.baseTheme({ // Plain left-click opens the link — matching Obsidian's live-preview, where a // rendered link is clickable directly (no modifier needed) — but only on // RELEASE, so dragging a selection off a link selects instead of navigating. -function linkAt(event: MouseEvent): HTMLElement | null { +function hrefAt(event: MouseEvent): string | null { const target = event.target as HTMLElement | null - return (target?.closest?.('.cm-lp-link') as HTMLElement | null) ?? null + const el = target?.closest?.('.cm-lp-link') as HTMLElement | null + return el?.getAttribute('data-cm-lp-href') ?? null } const externalLinkClickHandler = EditorView.domEventHandlers({ mousedown(event) { // No preventDefault: CodeMirror still gets to start a selection here. - const el = event.button === 0 ? linkAt(event) : null - linkPress = el ? { x: event.clientX, y: event.clientY, el } : null + const href = event.button === 0 ? hrefAt(event) : null + linkPress = href ? { x: event.clientX, y: event.clientY, key: href } : null return false }, mousemove(event) { - // Dragging away (or off the link) cancels the pending open. + // Dragging away cancels the pending open. Distance only — the pressed + // element is gone by now, so "still over the same link" is unanswerable. if (!linkPress) return false const moved = Math.hypot(event.clientX - linkPress.x, event.clientY - linkPress.y) - if (moved >= LINK_DRAG_SLOP_PX || linkAt(event) !== linkPress.el) linkPress = null + if (moved >= LINK_DRAG_SLOP_PX) linkPress = null return false }, mouseup(event) { - const el = linkAt(event) - const open = shouldOpenOnRelease(linkPress, { - x: event.clientX, y: event.clientY, el, button: event.button, + const press = linkPress + const open = shouldOpenOnRelease(press, { + x: event.clientX, y: event.clientY, key: hrefAt(event), button: event.button, }) linkPress = null - if (!open || !el) return false - const href = el.getAttribute('data-cm-lp-href') - if (!href) return false + if (!open || !press) return false event.preventDefault() - openExternal(href) + openExternal(press.key) return true }, })