Repository navigation
fix(richtext): text reads the way it is written, in either document direction - #7723
potatowagon wants to merge 6 commits into
Conversation
…-right document A block kept the surface's `direction` while Lexical's `dir="auto"` reordered its glyphs, so a Hebrew paragraph in an English document sat as a right-to-left run inside a left-to-right line box: it flushed left, its quote bar, list markers and indent sat on the wrong edge, and the caret walked backwards -- press Right and it moved left. Google Docs and Lexical's own playground both read that paragraph right-to-left. A block now follows the surface until its own text is strongly right-to-left, which `:dir(rtl)` on a `dir="auto"` block detects. CSS cannot tell a strongly left-to-right block from one holding digits, punctuation, an image or nothing at all, so everything else still follows the document and an empty paragraph keeps its caret on the document's start edge. Table parts are exempt: their columns are read in the document's order.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report
Modified ComponentsRichTextEditor (@astryxdesign/richtext)
RichTextView (@astryxdesign/richtext)
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression40 of 482 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 20 more. Generated by PR Enrichment workflow | View full report |
There was a problem hiding this comment.
Thanks for chasing down the inverted caret: it is real, and this change fixes it for top-level Hebrew paragraphs. Approval waits on a maintainer decision. spec:AST-061 FR6 currently says a surface does not choose a block's direction from its text, so a maintainer needs to either update FR6 to allow this exception (and say whether core Markdown follows it) or keep FR6 as written. Separately, these need fixing whichever way that goes:
- A list whose first item is Hebrew now turns every item right-to-left in an English document, so
Buy milk.renders as.Buy milk; keepingul/olon the surface direction avoids it. - A fenced code block whose first line is a Hebrew comment turns right-to-left, so
const x = 1;renders as;const x = 1and the copy button covers the first line; keepingcodeon the surface direction avoids it. - The changeset says such a block reads right-to-left "wherever it sits", but a Hebrew item in a list that starts in English is unchanged, and the parity test should keep its nested-direction check for core
Markdown.
[Automated review]
| // its own text is strongly right-to-left. | ||
| const block = (...styles: Array<stylex.StyleXStyles>): string => | ||
| stylex.props(...styles, editorTheme.providerDirection).className ?? ''; | ||
| stylex.props(...styles, editorTheme.blockDirection).className ?? ''; |
There was a problem hiding this comment.
block() also builds the ul/ol classes (lines 521-522) and the fenced-code class (line 548). Lexical gives list items no dir, so a list whose first item is Hebrew turns every item right-to-left (Buy milk. renders as .Buy milk). A code block whose first line is a Hebrew comment renders const x = 1; as ;const x = 1, with the copy button over its first line. Keeping lists and code on the surface direction, as tablePart does, avoids both.
| const otherDirection = geometry.blocks | ||
| .filter(block => block.direction !== direction) | ||
| .map(block => block.key); | ||
| expect(otherDirection, MARKDOWN).toEqual([]); |
There was a problem hiding this comment.
This now checks only Markdown's top-level blocks. The base also checked tr, th, td, li, p inside Markdown, whose output this PR does not change. Please keep that nested check, and consider adding a Hebrew-first list and a Hebrew-commented code block to the fixture so the RichText rule is exercised on lists and code too.
…cument Follow-up in the same change: the mirror case. An English paragraph in a Hebrew document was broken the same way a Hebrew paragraph in an English document was, and `:dir()` alone cannot fix it -- under `dir="auto"` a block of English, digits, an image and an empty line are all `:dir(ltr)`, so a CSS rule that catches the first catches the other three and moves an empty paragraph's caret to the wrong edge. BlockDirectionPlugin writes the direction out instead: each top-level block gets `dir="ltr"` or `dir="rtl"` from `getTextDirection` of its own text, and keeps `dir="auto"` when it has no strong character, which the theme styles as the document's direction. Lexical serializes the value and $setDirectionFromDOM already preserves it on paste, so a stored document reopens the way it was written. The plugin runs in the editor and the view, so both read a document the same way. The RTL image-resize test moves its image into the document's Hebrew paragraph: the last paragraph is English, so under this change it reads left-to-right even in a right-to-left document, and an image in it has its handle on the right.
There was a problem hiding this comment.
Thanks for the careful write-up: the inverted caret is real on the base, and this head fixes it for top-level Hebrew paragraphs. Approval waits on a maintainer decision, because spec:AST-061 FR6 currently says a surface does not choose a block's direction from its text, so a maintainer needs to amend FR6 and its verification row (and say whether core Markdown follows) or keep it as written. Independently of that decision, these need fixing:
- Undo gets stuck once a keystroke changes a block's own direction: typing
- Helloand pressing Ctrl+Z stops atHinstead of removing the list, because the direction sweep inBlockDirectionPluginruns as its own history step. - A list whose first item is Hebrew, or a code block whose first line is a Hebrew comment, still turns right-to-left as a whole, so
Buy milk.renders as.Buy milkandconst x = 1;as;const x = 1. - The Markdown half of the FR6 parity test should keep its nested
tr, th, td, li, pcheck, since this change does not touch Markdown.
[Automated review]
| editor.registerUpdateListener(({editorState}) => { | ||
| const stale = editorState.read(() => | ||
| $getRoot() | ||
| .getChildren() | ||
| .some( | ||
| node => | ||
| $isOwnDirectionBlock(node) && | ||
| node.getDirection() !== $ownDirection(node), | ||
| ), | ||
| ); | ||
| if (stale) { | ||
| editor.update(() => { | ||
| $getRoot().getChildren().forEach($writeOwnDirection); | ||
| }); | ||
| } | ||
| }), | ||
| ); |
There was a problem hiding this comment.
This follow-up editor.update lands as its own undo step whenever a keystroke inside an existing text node changes the block's first strong character (Lexical runs no element transform for that keystroke, so this listener is what fixes the direction). Undoing past it restores the stale state, the listener sweeps again and pushes a new step, so undo never gets further: in the Default story, - + Hello then Ctrl+Z ×3 leaves H, and 123 + שלום then Ctrl+Z ×4 leaves 123 ש; the base undoes both to an empty editor. Writing the direction inside the update that caused it (for example a TextNode transform that updates its top-level block) and tagging any whole-state sweep history-merge would avoid it.
| const TEXT_BLOCKS: ReadonlyArray<Klass<ElementNode>> = [ | ||
| ParagraphNode, | ||
| HeadingNode, | ||
| QuoteNode, | ||
| ListNode, | ||
| CodeNode, | ||
| ]; |
There was a problem hiding this comment.
ListNode and CodeNode take the direction of their whole text, so a list whose first item is Hebrew turns every item right-to-left (Buy milk. renders .Buy milk), and a code block whose first line is a Hebrew comment renders const x = 1; as ;const x = 1, with the copy button over the comment. Leaving lists and code on the surface direction, as tables are, avoids both.
… direction write out of history Review feedback from astracat-bot on facebook#7723. A list and a fenced code block hold several lines under one element, so one `dir` on the element imposes the first line's direction on all of them: under a Hebrew first item `Buy milk.` rendered `.Buy milk`, and under a Hebrew first comment `const x = 1;` rendered `;const x = 1`. They leave `dir="auto"` now, which the theme styles as the document's direction and under which the browser reads each line on its own. ParagraphNode, HeadingNode and QuoteNode are read as one piece of prose and keep their own direction. The direction write also cost an undo step. Typing inside an existing text node dirties the text node, not the block, so no element transform ran and the update listener was doing the work -- in an `editor.update` of its own, which became its own history entry. Undo restored the stale direction, the listener swept again, and undo never got past it: `- ` + `Hello` then Ctrl+Z left `H`. A TextNode transform now writes the direction inside the update that changed the text, and the whole-state sweep that RichTextView needs is tagged `history-merge`. The Markdown half of the FR6 parity test keeps its nested `tr, th, td, li, p` check: this change does not touch core Markdown.
|
Thanks — all three are real, and all three are fixed in Lists and fenced code blocks. You are right, and the fix is the one you suggested. Worth noting what the browser then does, because it is better than "no direction handling": under Per-item direction inside a list is a real improvement and is reachable — the reconciler emits Undo. Good catch, and your diagnosis was exactly right, including why: typing inside an existing text node dirties the text node and not the block, so no element transform ran for that keystroke and the update listener was doing the work — in an Fixed the way you suggested: a Both of your repros are now regression tests in a new The parity test's nested check. Restored. The Markdown half keeps the base's Changeset. Narrowed. "Wherever it sits" now says paragraph, heading or quote, and names what keeps the document's direction. On the FR6 decision, which is yours and not mine: the PR body sets out why I think the editor should be narrower than FR6 reads today, and I have no attachment to winning it. If FR6 is meant to bind Suite after the changes: 94 chromium, 600 unit, no new typecheck errors, lint and repo checks clean. |
There was a problem hiding this comment.
Thanks for the careful fixes: the undo regression and the Hebrew-first list and code block cases from the last round are resolved, and the caret now follows the arrow keys in a right-to-left paragraph. Approval waits on a maintainer, because spec:AST-061 FR6 currently says content blocks take the provider's direction and that a surface does not choose a block's direction from its text, so a maintainer needs to update FR6 and its verification row to allow own-text direction for paragraphs, headings and quotes (and say whether that covers RichTextEditor only, RichTextView too, or core Markdown as well) or keep FR6 as written. Independently of that decision, one fix is needed:
- A list, code block or table-cell paragraph that arrives with an explicit
dir(pasted HTML or a stored value) now keeps it, becauseblock()still buildsul,olandcodeon the new rule: a pasted<ul dir="rtl">rendersBuy milk.as.Buy milkin an English document, so keeping those classes on the unconditional document direction, astablePartdoes, and clearingdiron nested paragraphs would fix it.
[Automated review]
| // the document while it still says `auto`. | ||
| const block = (...styles: Array<stylex.StyleXStyles>): string => | ||
| stylex.props(...styles, editorTheme.providerDirection).className ?? ''; | ||
| stylex.props(...styles, editorTheme.blockDirection).className ?? ''; |
There was a problem hiding this comment.
block() still builds ul, ol (lines 518-519) and code (line 545), so with blockDirection they now keep any explicit dir that arrives with them, which the base's unconditional direction: inherit neutralized. A list pasted as <ul dir="rtl"> into an English document turns right-to-left and renders Buy milk. as .Buy milk, and cell paragraphs pasted or stored with dir="ltr" sit flush left in a right-to-left table. Building ul, ol and code on the unconditional surface direction, as tablePart does, and clearing __dir on a paragraph that is not a root child would keep them on the document direction, as the PR intends.
Reported on the published build: in a right-to-left document every English list item and table cell still flushed right with its arrow keys reversed. Only the containers written by BlockDirectionPlugin behaved, and it wrote to top-level blocks alone. Lexical writes no `dir` below the top level, so anything deeper simply inherits the document's: an English list item in a Hebrew document is a left-to-right run in a right-to-left box, which is the same inverted caret this stack set out to fix, one level down. Measured on the playground in a right-to-left document: 18 of 18 list items and 8 of 8 cell paragraphs disagreed with their own text. A prose container now carries its own direction at any depth -- paragraph, heading, quote, and list item -- so a paragraph inside a table cell reads its own way too. Their containers still order in the document's direction: a list's markers and indent, a table's columns. This also makes the previous commit's list carve-out unnecessary. One `dir` on the `ul` imposed the first item's direction on every item, which is why it was dropped; a direction per item cannot, so a Hebrew first item still leaves `Buy milk.` reading `Buy milk.` and an English item in a right-to-left document now reads correctly as well. A fenced code block keeps the document's direction: it has no per-line node to carry one, and core Markdown's renders the same way, so changing it belongs in a change that moves both surfaces together.
Reported on the published build: in a right-to-left document the English text in a code block still flushed right with its arrow keys reversed. I had left this alone on the grounds that the block has no per-line node to carry a direction and that core Markdown renders it the same way. The first half is true, the second is not: measured in a right-to-left document, core Markdown's `pre` starts its code at the left edge while RichText's started at the right, so the two surfaces already disagreed and leaving it was not protecting parity. CSS can do per line what the node tree cannot. `unicode-bidi: plaintext` puts every preserved line in its own bidi paragraph, taking the direction of its own first strong character, so a code line reads left to right with its semicolon at the end in either document and a Hebrew comment line still reads right to left. That is also the answer to the Hebrew-comment case raised in review, which a single direction on the block could not have given: measured on the base, a code block whose first line is a Hebrew comment renders `const x = 1;` as `;const x = 1` in a right-to-left document.
Review feedback from astracat-bot on facebook#7723. `ul`, `ol` and the table parts were built on the conditional rule, which leaves an explicit `dir` alone. Nothing in the editor writes one onto a container, but $setDirectionFromDOM does when HTML is pasted, and an older stored document can carry one -- so a list pasted as `<ul dir="rtl">` kept its markers and indent on the right in an English document. Measured before this change: `ul=rtl li=ltr`. Containers are built on the unconditional document direction now, as the tables already were, and BlockDirectionPlugin clears a direction that arrives on a list, a code block or a table part, so the stored state says what the document shows. The item inside still carries its own, which is what keeps `Buy milk.` reading `Buy milk.`. A cell paragraph that arrives with an explicit `dir` keeps it, which the review also raised: since the previous commit a cell's prose carries its own direction by design, so an English paragraph in a right-to-left table reading left-to-right is the intended behaviour, not a leftover.
|
Thanks — the pasted-container case is real and is fixed in The list. Confirmed and fixed as you suggested. The symptom had already narrowed by the time I measured it: since The cell paragraph. This one I have deliberately left, because it is now the intended behaviour rather than a leftover. Since Also fixed since your review, both reported against the published build:
Suite: 96 chromium, 601 unit, no new typecheck errors, lint and repo checks clean. The FR6 scope question is still yours. |
The bug
Text in
RichTextEditoris laid out by the direction of the document around it, not by the direction of the text itself. In a left-to-right document a Hebrew paragraph flushes left, and its arrow keys move the caret the wrong way — press Right and the caret goes left. In a right-to-left document every English paragraph, list item, table cell and code line does the mirror.Google Docs and Lexical's own playground read each paragraph the way its own text reads. A Hebrew blockquote here also draws its bar and indent on the wrong edge.
Repro
פסקה בעברית לבדיקת כיוון הטקסט.Then flip the document to right-to-left and try the same in an English list item, a table cell and a code block.
Why
Lexical marks every top-level block
dir="auto"and writes nothing below that.editorTheme.tsthen overrode the result on every block:dir="auto"gives a block two things: the computeddirectionits first strong character asks for, andunicode-bidi: isolate. An author declaration beats the UA's, so the override took the first and left the second. The Hebrew text stayed an isolated run, reordering right-to-left inside itself, while its paragraph became left-to-right — andtext-align: startand the logical insets resolve againstdirection, so the text flushes left, the quote bar lands on the wrong edge, and caret motion follows the paragraph rather than the text.Measured in Chromium, the same markup with and without the override:
directionborder-inline-startlandsltrrtlBelow the top level there was no override to blame — Lexical simply writes no
dirthere, so a list item or table cell inherits the document's direction and an English one in a right-to-left document is a left-to-right run in a right-to-left box. Same symptom, one level down. Measured on the playground in a right-to-left document: 18 of 18 list items and 8 of 8 cell paragraphs disagreed with their own text.Why CSS alone cannot fix it
A
:dir()rule fixes the Hebrew-in-English case and nothing else. Underdir="auto"a block of English, a block of digits, an image-only block and an empty block are all:dir(ltr), so a rule that catches the first catches the other three. Measured in Chromium inside a right-to-left document::dir(rtl)rule only:dir(ltr)An English paragraph.rtl❌ltr✅<br>)rtl✅ltr❌ caret jumps to the leftrtl✅ltr❌ resize handle flips sideThe last two are not hypothetical: an earlier draft of this PR used the symmetric CSS rule and broke
the resize handle follows the drag in a right-to-left document.Reaching for
unicode-bidi: plaintexton prose is not the answer either. It makestext-align: startfollow the content's own direction, so the text lands correctly — but logical properties still resolve againstdirection, so a Hebrew blockquote in an English document would flush its text right while keeping its bar on the left.The fix
Write the direction out, which is the one thing that can express "this block has no direction of its own".
BlockDirectionPlugingives each prose container an explicitdirfromgetTextDirection()of its own text — a public Lexical export — and leaves a container with no strong character alone:The theme then reads the three states apart:
A Hebrew paragraph reads right-to-left and an English one left-to-right, wherever each sits, and an empty paragraph still keeps its caret on a right-to-left document's start edge.
Lexical serializes
dir, so a stored document reopens the way it was written, and$setDirectionFromDOMalready preserves the attribute when HTML is pasted in.Prose carries a direction; its container orders it
ParagraphNode,HeadingNode,QuoteNodeandListItemNodetake part, at any depth — so a paragraph inside a table cell does too.Their containers do not, because they order their contents rather than reading as one piece of prose:
<ul>keeps it. A singlediron the<ul>would also impose the first item's direction on every item — under a Hebrew first item,Buy milk.would render.Buy milk— which a direction per item cannot do.Containers take the document's direction unconditionally, and the plugin clears a direction that arrives on one. Nothing in the editor writes a
dironto a container, but$setDirectionFromDOMdoes when HTML is pasted and an older stored document can carry one, so a list pasted as<ul dir="rtl">would otherwise keep its markers and indent on the right in an English document.unicode-bidi: plaintextputs each preserved line in its own bidi paragraph, so every line reads the way its own first strong character reads. A Hebrew comment line reads right-to-left whileconst x = 1;keeps its semicolon at the end, in either document — which no single direction on the block could have given.This is spec:AST-061 FR6 narrowed to prose containers; the comments in
editorTheme.tsandBlockDirectionPlugin.tsxand the parity tests now state it.Where the write happens matters for undo
Typing inside an existing paragraph dirties its text node, not the container, so no element transform runs for that keystroke. Doing the work from an update listener instead costs an
editor.updateof its own, which becomes its own history entry: undo restores the stale direction, the listener sweeps again and pushes a new entry, and undo never gets past it (-thenHello, then Ctrl+Z, stops atH).A
TextNodetransform now writes the direction inside the update that changed the text, so it rides in that keystroke's own history entry. The update listener remains only for a whole-state replacement, which runs no transforms and is whatRichTextViewdoes on everyvaluechange, and it is taggedhistory-merge. Writing the direction settles the container, so the sweep does not loop.Relationship to #7001
This narrows #7001 rather than reverting it. That PR fixed a real problem, and its description states it exactly:
Switching a page between
MarkdownandRichTextEditorgenuinely did make blocks jump sides, and that is worth fixing. The scope is what I would like to revisit, on two points.Its stated invariant does not hold as written. #7001 says "text inside a block still lays out by the Unicode bidi algorithm, so a Hebrew phrase inside an English sentence still reads correctly". A Hebrew phrase inside a sentence does, but
directionis itself an input to that algorithm: it sets the paragraph embedding level. Overriding it whiledir="auto"keepsunicode-bidi: isolateleaves the block's text an isolated right-to-left run inside a left-to-right paragraph, where alignment, logical insets and caret motion all disagree with the text. That intermediate state is worse than either behaviour on its own.Parity is the renderer's requirement, not the field's.
RichTextViewand coreMarkdownshow authored content, and matching them block for block is clearly right.RichTextEditorhas a typist. A field whose chrome overrules the direction of what someone types cannot be correct for a bilingual author, which is why Google Docs and Lexical's playground both let the block win.So this PR keeps #7001's parity everywhere it is about rendering — core
Markdownis untouched,RichTextViewand the editor still agree with each other, and a list's markers, a table's columns and a code block's frame all still take the document's direction — and drops it only for text that has a direction of its own.One correction worth recording: I first left the fenced code block alone on the grounds that core
Markdownrendered it the same way. Measured, it does not — in a right-to-left document core Markdown'sprestarts its code at the left edge while RichText's started at the right. The two surfaces already disagreed, so leaving it was not protecting parity. They agree now.Worth noting for the test strategy: #7001's RTL coverage flips the whole surface to
direction: rtland asserts both surfaces reportrtl. Both the old behaviour and this one pass that, because a uniform-direction document cannot distinguish them. The mixed-direction case is the one that separates them, and this PR adds it.@cixzhang, since this is your area and your spec reading — happy to take the other decision if FR6 is meant to bind the editor as strictly as the renderer. In that case the minimum fix is still needed: the override has to neutralise
dir="auto"rather than take half of it, or the caret keeps walking backwards through right-to-left text.Tests
MarkdownParitySandbox.a11y.chromium.spec.tsasserted that every block takes the provider direction, which is the behaviour being fixed. It is now split and tightened:markdown blocks take the provider direction (ltr|rtl)— coreMarkdownsets nodir, so it is unchanged, and this keeps the base's nestedtr, th, td, li, pcheck.a rich text block reads the way its own text reads (ltr|rtl)— the Hebrew block isrtland an English block isltrin either document direction; the thematic break, which has no direction of its own, follows the document; so do the table's parts.each line of a fenced code block reads its own way (ltr|rtl)— the block isunicode-bidi: plaintext, its code line readsconst x = 1;starting at the left edge in either document, and its Hebrew comment line reads right-to-left starting at the right. Characters are read out of client rects and grouped into visual lines, so syntax-highlighting spans cannot hide a reordering.a list orders in the document direction and its items read their own (ltr|rtl)— the<ul>and the code block staydir="auto"and compute to the document's direction, while the Hebrew item isrtland the English oneltr, which is what keepsBuy milk.readingBuy milk..a table orders in the document direction and its cells read their own (ltr|rtl)— the cell box follows the document so columns keep their order; the paragraph inside reads the way the cell's own text reads.New
BlockDirection.a11y.chromium.spec.tsis the undo guard, in a real browser:-+Hello+ Ctrl+Z and123+שלום+ Ctrl+Z both walk back to an empty document, and a paragraph picks upauto→ltr→rtlas the text typed into it changes. Both undo tests fail on the commit before the fix and pass after it.New
blockDirection.test.tsxcovers the attribute itself in jsdom:ltr/rtl/autowritten onto the right containers, the same directions inRichTextView, a rewrite when the view is handed a new document, a list whose three items come outrtl,ltrand none, the list, table and code block left to the document, and a stored<ul dir="rtl">giving its direction up while its English item keepsltr(this one fails on the commit before the fix).Run against a built Storybook:
packages/richtextchromium suite: 96 passed.packages/richtextunit tests: 601 passed.eslint src: 0 errors.tsc --noEmit: no new errors against a stashed baseline.check:changesets,check:sync,check:use-clientandcheck:package-boundaries: clean.Commits
006f870— the inverted caret for a top-level Hebrew paragraph.c0a0e9e— the mirror case, via an explicitdirper block rather than CSS.0ddeb45— review feedback: the direction write no longer costs an undo step.7c17e20— list items and table cells, at any depth.fcf55a8— each line of a fenced code block.e0fcff5— a container gives up a direction that arrives with pasted HTML or a stored document.