diff --git a/src/web/public/app.js b/src/web/public/app.js index 65503816..f3479f2d 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -2845,26 +2845,11 @@ class CodemanApp { a.setAttribute('target', '_blank'); a.setAttribute('rel', 'noopener noreferrer'); }); - // Right-to-left text (issue #715). With no `dir`, every block was laid - // out left to right, so a Persian, Arabic or Hebrew answer was - // left-aligned, its punctuation landed on the wrong end and its lists - // and quotes hung off the left edge. Each OUTERMOST prose block now - // takes its direction from its own first letter (`dir="auto"`, what - // GitHub does for rendered markdown), so an RTL paragraph beside an - // English one reads correctly in both; nested blocks inherit, which - // keeps a list's markers and indent on one side. A `dir` the agent - // wrote (the sanitizer allows it) is kept, and so is what it covers. - // // Code and preformatted text are always left to right. `dir="ltr"` // also isolates them, and `dir="auto"` skips any descendant that // carries a `dir`, so `npm install` at the head of a Persian sentence - // no longer turns the whole sentence left to right. - tmpl.content - .querySelectorAll('p, h1, h2, h3, h4, h5, h6, ul, ol, dl, blockquote, .rv-table-wrap') - .forEach((el) => { - if (el.parentElement?.closest('[dir]') || el.hasAttribute('dir')) return; - el.setAttribute('dir', 'auto'); - }); + // no longer turns the whole sentence left to right (issue #715). The + // `dir="auto"` itself is `_applyBlockDirection`'s, never this string's. tmpl.content.querySelectorAll('pre, code').forEach((el) => { if (!el.hasAttribute('dir')) el.setAttribute('dir', 'ltr'); }); @@ -3025,10 +3010,38 @@ class CodemanApp { renderedText.className = 'rv-text'; renderedText.innerHTML = this._renderMarkdown(text); this._linkifyFilePaths(renderedText); + this._applyBlockDirection(renderedText); div.appendChild(renderedText); return div; } + /** + * Right-to-left text (issue #715). With no `dir`, every block was laid out + * left to right, so a Persian, Arabic or Hebrew answer was left-aligned, its + * punctuation landed on the wrong end and its lists and quotes hung off the + * left edge. Each OUTERMOST prose block now takes its direction from its own + * first letter (`dir="auto"`, what GitHub does for rendered markdown), so an + * RTL paragraph beside an English one reads correctly in both; nested blocks + * inherit, which keeps a list's markers and indent on one side. A `dir` the + * agent wrote (the sanitizer allows it) is kept, and so is what it covers. + * + * ⚠️ Call this LAST, on the DOM, once nothing else inserts under `root`: an + * element that already carries `dir="auto"` re-resolves its direction on every + * insertion beneath it, and a block whose letters all sit in `dir` children + * (a code-only or path-only list) walks its whole subtree each time. Set + * during `_renderMarkdown` or before the linkifier, that made rendering + * quadratic, for English documents too: a 200 KB list of inline code froze + * the tab for 40 s. Not part of `_linkifyFilePaths`, which returns early when + * a stale constants.js has no `findFilePathLinks`. + */ + _applyBlockDirection(root) { + if (!root) return; + for (const el of root.querySelectorAll('p, h1, h2, h3, h4, h5, h6, ul, ol, dl, blockquote, .rv-table-wrap')) { + if (el.parentElement?.closest('[dir]') || el.hasAttribute('dir')) continue; + el.setAttribute('dir', 'auto'); + } + } + /** * Make file paths in a rendered message clickable: absolute, `~/` and * relative ones (`findFilePathLinks`, constants.js). @@ -3130,7 +3143,7 @@ class CodemanApp { link.dataset.linkified = '1'; if (folder) link.dataset.folder = '1'; // A path reads left to right, and in a right-to-left sentence it must - // not vote on the sentence's direction (`_renderMarkdown`, issue #715). + // not vote on the sentence's direction (`_applyBlockDirection`, issue #715). link.dir = 'ltr'; link.title = path; link.textContent = path; diff --git a/src/web/public/panels-ui.js b/src/web/public/panels-ui.js index e46baa4a..a0c231d2 100644 --- a/src/web/public/panels-ui.js +++ b/src/web/public/panels-ui.js @@ -5071,6 +5071,7 @@ Object.assign(CodemanApp.prototype, { const doc = tmpl.content.firstElementChild; this._rebaseFilePreviewMarkdownRefs(doc, info); this._linkifyFilePaths(doc); + this._applyBlockDirection(doc); // The linkifier's absolute paths name no session; give them the // preview's, like the rebased links, or they open in the active tab's. for (const a of doc.querySelectorAll('a.rv-path:not([data-session-id])')) a.dataset.sessionId = info.sessionId; diff --git a/test/file-preview-markdown.test.ts b/test/file-preview-markdown.test.ts index 4599c2f8..e55820b0 100644 --- a/test/file-preview-markdown.test.ts +++ b/test/file-preview-markdown.test.ts @@ -151,6 +151,7 @@ function loadApp(prefs: Record = {}) { app.filePreviewContent = ''; app._renderMarkdown = vi.fn(() => MARKDOWN_HTML); app._linkifyFilePaths = vi.fn(); + app._applyBlockDirection = vi.fn(); app._bindResponseViewerInteractions = vi.fn(); const byId = (id: string) => document.getElementById(id) as HTMLButtonElement; @@ -177,6 +178,11 @@ describe('file viewer rendered markdown', () => { expect(app._renderMarkdown).toHaveBeenCalledWith(MD_CONTENT, { breaks: false }); // Identity, not deep equality: DOM nodes are compared by reference here. expect(app._linkifyFilePaths.mock.calls[0][0]).toBe(doc); + // After the linkifier: a dir="auto" block re-resolves on every insertion under it (#715). + expect(app._applyBlockDirection.mock.calls[0][0]).toBe(doc); + expect(app._applyBlockDirection.mock.invocationCallOrder[0]).toBeGreaterThan( + app._linkifyFilePaths.mock.invocationCallOrder[0] + ); expect(app._bindResponseViewerInteractions.mock.calls[0][0]).toBe(body); // The source stays what Copy copies. expect(app.filePreviewContent).toBe(MD_CONTENT); @@ -463,6 +469,14 @@ describe('file viewer markdown line breaks', () => { expect(chatPara.querySelectorAll('br')).toHaveLength(1); expect(chatPara.textContent).toBe(WRAPPED.replace('\n', '')); }); + + it('lets a document read right to left through the shipped viewer (#715)', async () => { + const app = loadShippingApp('سلام، این یک تست است.\n'); + + await app.openFilePreview('docs/README.md', 's1'); + + expect(document.querySelector('#filePreviewBody .file-preview-md p')?.getAttribute('dir')).toBe('auto'); + }); }); describe('FILE_PREVIEW_EXTENSIONS', () => { diff --git a/test/response-viewer-rtl.test.ts b/test/response-viewer-rtl.test.ts index a06ab7f9..9ec559ff 100644 --- a/test/response-viewer-rtl.test.ts +++ b/test/response-viewer-rtl.test.ts @@ -1,15 +1,21 @@ /** - * @fileoverview Right-to-left text in rendered markdown (`CodemanApp._renderMarkdown`, issue #715). + * @fileoverview Right-to-left text in rendered markdown (`_applyBlockDirection`, issue #715). * * The Response Viewer and the File Viewer's markdown view rendered every block left to * right, so a Persian, Arabic or Hebrew answer was left-aligned, its punctuation landed on - * the wrong end and its list markers sat on the left. `_renderMarkdown` now gives each + * the wrong end and its list markers sat on the left. `_applyBlockDirection` now gives each * OUTERMOST prose block `dir="auto"` (the browser picks the direction from the block's own - * first letter) and pins code to `dir="ltr"`, which also keeps a code span from deciding a - * sentence's direction, since `dir="auto"` skips descendants that carry a `dir`. + * first letter), and `_renderMarkdown` pins code to `dir="ltr"`, which also keeps a code + * span from deciding a sentence's direction, since `dir="auto"` skips descendants that + * carry a `dir`. * - * jsdom does not resolve `dir="auto"`, so this pins the attributes the browser acts on; - * what Chromium makes of them was checked by eye when the change was made. + * The pass runs LAST, on the DOM, after the linkifier: an element that already carries + * `dir="auto"` re-resolves its direction on every insertion under it, which made a + * code-only or path-only document quadratic. Two tests below pin that order. + * + * jsdom does not resolve `dir="auto"`, so this pins the attributes the browser acts on and + * the order, never the cost; what Chromium makes of them was measured when the change was + * made. * * Drives the SHIPPING artifacts (vendored `marked`, vendored DOMPurify + `sanitize-html.js`, * `constants.js` and `app.js`) in a `vm` with a jsdom document, the technique from @@ -54,6 +60,8 @@ function loadShippingMarked(): { parse: (src: string, opts?: unknown) => string type RenderApp = { _renderMarkdown(text: string, opts?: { breaks?: boolean }): string; _linkifyFilePaths(root: unknown): void; + _applyBlockDirection(root: unknown): void; + _buildResponseViewerMessage(text: string, role: string, agentLabel: string): HTMLElement; }; function loadCodemanAppClass(): { prototype: RenderApp } { @@ -86,11 +94,13 @@ function loadCodemanAppClass(): { prototype: RenderApp } { const CodemanApp = loadCodemanAppClass(); const app = Object.create(CodemanApp.prototype) as RenderApp; -/** Render markdown the way the response viewer does and return the resulting element. */ +/** Render markdown the way both viewers do (pipeline, linkifier, direction pass) and return the element. */ function render(markdown: string, opts?: { breaks?: boolean }): HTMLElement { const root = document.createElement('div'); root.className = 'rv-text'; root.innerHTML = app._renderMarkdown(markdown, opts); + app._linkifyFilePaths(root); + app._applyBlockDirection(root); return root as unknown as HTMLElement; } @@ -171,7 +181,6 @@ describe('response viewer right-to-left text (#715)', () => { it('pins a linkified file path left to right, so it does not decide the sentence direction', () => { const root = render('فایل /home/user/project/config.json را باز کن.'); - app._linkifyFilePaths(root); const link = root.querySelector('a.rv-path'); expect(link?.textContent).toBe('/home/user/project/config.json'); @@ -180,10 +189,35 @@ describe('response viewer right-to-left text (#715)', () => { it('does not pin a markdown link label, which is prose in the sentence language', () => { const root = render('[گزارش حادثه](/tmp/report.json)'); - app._linkifyFilePaths(root); const link = root.querySelector('a.rv-path-link'); expect(link, 'a path-target link takes the rv-path shape').not.toBeNull(); expect(link?.hasAttribute('dir')).toBe(false); }); + + it('never puts dir="auto" in the rendered string, where it would make the parse and the linkifier quadratic', () => { + // An element carrying dir="auto" re-resolves its direction on every insertion + // under it. A document whose letters all sit in `dir` children walks the whole + // block each time, which froze the tab for seconds on an all-code list. + const html = app._renderMarkdown('- `f1`\n- `f2`\n\nسلام\n\n| ستون |\n| --- |\n| الف |'); + + expect(html).not.toContain('dir="auto"'); + expect(html).toContain('f1'); + }); + + it('gives a Response Viewer message its direction only after the linkifier finished', () => { + const probe = Object.create(app) as RenderApp; + let autoDuringLinkify = -1; + probe._linkifyFilePaths = (root) => { + autoDuringLinkify = (root as Element).querySelectorAll('[dir="auto"]').length; + app._linkifyFilePaths(root); + }; + + const message = probe._buildResponseViewerMessage('/home/user/project/config.json را ببین.', 'assistant', 'Claude'); + + expect(autoDuringLinkify).toBe(0); + const para = message.querySelector('.rv-text p'); + expect(dirOf(para)).toBe('auto'); + expect(dirOf(para?.querySelector('a.rv-path') ?? null)).toBe('ltr'); + }); });