fix: RTL content option no longer flips the whole page (#7900) by JohnMcLear · Pull Request #7901 · ether/etherpad

JohnMcLear · 2026-06-05T11:40:14Z

Note: the related "enforce settings" override bug discovered during testing has been split into #7902.

Problem

Enabling the pad's RTL content option flips the entire page — the toolbar and surrounding chrome — not just the pad's inner text contents, which is all a per-pad content option should affect.

Root cause

The rtlistrue setter in src/static/js/ace2_inner.ts wrote the base direction to document.documentElement. In that file document resolves to the top-level pad page (the editor iframes are derived from it via document.getElementsByName("ace_outer")), so toggling RTL content overwrote the whole page's dir attribute.

The page direction is owned solely by the UI language — src/static/js/l10n.ts sets document.documentElement.dir = html10n.getDirection(). The content option must not clobber it.

Fix

Apply the content direction to the inner editor document (targetDoc.documentElement) instead of the top-level document. The inner body already gets the .rtl/.ltr class; this keeps the dir attribute on the inner <html> where it belongs, so the toolbar/chrome stay governed by the UI language.

Test

Added a frontend test (rtl_url_param.spec.ts) asserting that with rtl=true the inner editor body flips to RTL while the top-level <html> dir stays ltr. Verified red→green: the test fails against the old code (top-level <html dir="rtl">) and passes with the fix.

🤖 Generated with Claude Code

The pad's RTL content option (rtlIsTrue) is a per-pad setting that should only change the direction of the editor's text contents. The setter in ace2_inner.ts wrote the direction to the top-level `document.documentElement` (`document` there resolves to the main pad page, since the editor iframes are derived from it), which flipped the entire page — toolbar and chrome included. The page direction is owned solely by the UI language (l10n.ts sets it from html10n.getDirection()). Apply the content direction to the inner editor document (`targetDoc.documentElement`) instead, so enabling RTL content leaves the surrounding interface untouched. Adds a frontend test asserting the inner editor flips to RTL while the top-level <html> dir stays ltr. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

qodo-code-review · 2026-06-05T11:40:19Z

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

qodo-free-for-open-source-projects · 2026-06-05T11:40:30Z

Review Summary by Qodo

Fix RTL content option flipping entire page chrome

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Fixed RTL content option flipping entire page instead of just pad contents
• Changed rtlistrue setter to apply direction to inner editor document only
• Added frontend test verifying inner editor flips while toolbar stays LTR
• Prevents per-pad RTL setting from overriding UI language page direction

Diagram

flowchart LR
  A["RTL Content Option"] --> B["ace2_inner.ts rtlistrue setter"]
  B --> C["Apply to targetDoc.documentElement"]
  C --> D["Inner editor flips to RTL"]
  C --> E["Top-level page stays LTR"]
  F["UI Language Direction"] --> E

File Changes

1. src/static/js/ace2_inner.ts 🐞 Bug fix +5/-1

Apply RTL direction to inner editor document only

• Changed document.documentElement.dir to targetDoc.documentElement.dir in rtlistrue setter
• Added explanatory comment clarifying why inner document is targeted instead of top-level document
• Prevents RTL content option from affecting page chrome and toolbar

src/static/js/ace2_inner.ts

2. src/tests/frontend-new/specs/rtl_url_param.spec.ts 🧪 Tests +16/-0

Add test for RTL content option isolation

• Added new test case verifying RTL content option behavior
• Asserts inner editor body has rtl class and computed direction is rtl
• Verifies top-level ` element retains dir="ltr"` attribute
• Ensures toolbar and chrome remain unaffected by pad's RTL setting

src/tests/frontend-new/specs/rtl_url_param.spec.ts

qodo-free-for-open-source-projects · 2026-06-05T11:40:31Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. ~~Flaky ace_inner frame access~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new RTL test dereferences page.frame('ace_inner')! immediately after navigation, but
appendQueryParams() does not wait for the inner iframe/body to be ready, so the test can
intermittently throw (null frame) or race against iframe initialization.

Code

src/tests/frontend-new/specs/rtl_url_param.spec.ts[43]
+    const innerBody = page.frame('ace_inner')!.locator('#innerdocbody');

Evidence
appendQueryParams() waits only for the outer iframe and #editorcontainer.initialized, and the
helper comments explicitly warn that #editorcontainer.initialized can be reached before the inner
editor body is ready. The new test uses a non-null assertion on page.frame('ace_inner'), which can
race with that readiness window.
src/tests/frontend-new/specs/rtl_url_param.spec.ts[38-47]
src/tests/frontend-new/helper/padHelper.ts[107-115]
src/tests/frontend-new/helper/padHelper.ts[117-133]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test uses `page.frame('ace_inner')!` without waiting for the inner frame to exist/be ready. `appendQueryParams()` only waits for the outer iframe and `#editorcontainer.initialized`, and the repo already documents that this is not sufficient to guarantee the inner editor is ready.
### Issue Context
There is an existing `waitForEditorReady()` implementation that waits for `#innerdocbody[contenteditable="true"]`, but `appendQueryParams()` does not call it.
### Fix Focus Areas
- src/tests/frontend-new/specs/rtl_url_param.spec.ts[38-47]
- src/tests/frontend-new/helper/padHelper.ts[107-115]
- src/tests/frontend-new/helper/padHelper.ts[117-133]
### Suggested fix
Use a `frameLocator` chain (which Playwright will auto-wait for) and/or explicitly wait for `#innerdocbody` readiness before asserting:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Hardcoded LTR dir assertion~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

The test asserts the top-level ` has dir="ltr"`, but page direction is set by localization (cookie
/ navigator.language) and can legitimately be rtl, making the test environment-dependent and
prone to false failures.

Code

src/tests/frontend-new/specs/rtl_url_param.spec.ts[51]
+    await expect(page.locator('html')).toHaveAttribute('dir', 'ltr');

Evidence
The app explicitly sets the top-level page dir from localization, which can yield RTL depending on
cookie/navigator.language, and the Playwright config does not specify a locale. Therefore,
asserting a fixed ltr value is not invariant across environments.
src/tests/frontend-new/specs/rtl_url_param.spec.ts[49-51]
src/static/js/l10n.ts[11-18]
src/playwright.config.ts[46-69]
src/static/js/pad_editor.ts[239-251]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test hard-codes `dir="ltr"` on the top-level `<html>`, but the app sets `document.documentElement.dir` from `html10n.getDirection()` which depends on localization inputs. This can be `rtl` on RTL locales, so the assertion is brittle.
### Issue Context
`l10n.ts` sets the page `dir` when localization completes, and Playwright config does not force a locale globally.
### Fix Focus Areas
- src/tests/frontend-new/specs/rtl_url_param.spec.ts[38-52]
- src/static/js/l10n.ts[11-18]
- src/playwright.config.ts[46-69]
### Suggested fix
Instead of asserting LTR specifically, assert that toggling the pad RTL option does not change whatever the UI language chose:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

…t page-dir assertion (#7900) - Use a frameLocator chain (Playwright auto-waits) instead of page.frame('ace_inner')! which can race with inner-editor readiness. - Don't hardcode dir='ltr' on the top-level <html> (the UI language can legitimately pick rtl). Capture the initial page dir and assert it is unchanged after toggling the pad RTL option — that is the real invariant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

qodo-free-for-open-source-projects Bot reviewed Jun 5, 2026

View reviewed changes

Comment thread src/tests/frontend-new/specs/rtl_url_param.spec.ts Outdated

JohnMcLear changed the title ~~fix: RTL content option no longer flips the whole page (#7900)~~ fix: RTL pad-wide settings — page-flip and enforce-override bugs (#7900) Jun 5, 2026

JohnMcLear mentioned this pull request Jun 5, 2026

fix: pad-wide view settings apply to the creator's own view (#7900) #7902

Merged

JohnMcLear force-pushed the fix/7900-rtl-flips-whole-page branch from 7ea7321 to a164a63 Compare June 5, 2026 12:06

JohnMcLear changed the title ~~fix: RTL pad-wide settings — page-flip and enforce-override bugs (#7900)~~ fix: RTL content option no longer flips the whole page (#7900) Jun 5, 2026

qodo-code-review Bot deleted a comment from qodo-free-for-open-source-projects Bot Jun 5, 2026

JohnMcLear merged commit 61aee16 into develop Jun 5, 2026
32 checks passed

JohnMcLear deleted the fix/7900-rtl-flips-whole-page branch June 5, 2026 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: RTL content option no longer flips the whole page (#7900)#7901

fix: RTL content option no longer flips the whole page (#7900)#7901
JohnMcLear merged 2 commits into
developfrom
fix/7900-rtl-flips-whole-page

JohnMcLear commented Jun 5, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jun 5, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 5, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

JohnMcLear commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root cause

Fix

Test

Uh oh!

qodo-code-review Bot commented Jun 5, 2026

Qodo reviews are paused for this user.

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 5, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

JohnMcLear commented Jun 5, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Jun 5, 2026 •

edited

Loading