Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] [lexical] Bug Fix: don't set 'ltr' direction on node with no directioned text #7330

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

james-atticus
Copy link
Contributor

@james-atticus james-atticus commented Mar 14, 2025

Description

LexicalReconciler tries to determine whether text should be LTR or RTL based on a regex.

If a given block has no directioned text, then the activeEditorDirection is used. This more-or-less-global variable is set to null at the start of reconciliation but updated when a node with directioned text is encountered.

That's fine when processing the whole tree: node get updated in reading order. However, if you process only some nodes in the tree, then you can end up changing the direction of a node in an unexpected way. This is shown in the third test case where marking the first and third nodes as dirty changes the direction of the latter to RTL.

Potential fixes

Don't set node direction to LTR if no directioned text

This makes it so that the node direction is more in sync with the dir property set in the DOM. Currently the reconciler clears the dir property if direction is null OR if it's LTR and the node has no directioned text: https://github.com/facebook/lexical/blob/main/packages/lexical/src/LexicalReconciler.ts#L390-L395

This approach is the one I've gone with in the current PR. The result is that the two editors have slightly different DOMs ('ltr' is removed in left editor but kept in right editor), however I don't think this is a huge issue. That said, a bunch of unit tests are failing.

Mark __dir as an excluded property for Yjs syncing

Based on what the reconciler is doing, figuring out direction from the text content, arguably __dir is a derived node property in the same way that __cachedText is on the root node: https://github.com/facebook/lexical/blob/main/packages/lexical-yjs/src/Utils.ts#L50-L54

I tried adding __dir to the excluded list for element nodes and it was still only the Table test that needed updating.

This feels like a reasonable approach to me, given __dir is derived state, however it would mean you could end up with two editors that display a given node with different directions. Take the example from the third test case - the two nodes were marked dirty but had no changes, so nothing would be synced to Lexical, so other editors would still have null as the direction for the third paragraph.

Backtrack through the tree to determine activeEditorDirection

This would be the most complete fix, however could be a significant change. Didn't want to go down this path without first discussing (and ruling out) the two options above.

The idea here would be to not use activeEditorState but instead look at the previous block node in the tree (direct sibling, or sibling of a parent) and use its direction, assuming you were processing a node with no directioned text. That should give the same result as processing the whole tree in order.

Process the whole tree when reconciling direction

This would give consistent results and would be a small code change, but is a bad idea for obvious performance reasons.

Copy link

vercel bot commented Mar 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2025 4:13am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2025 4:13am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 14, 2025
@etrepum
Copy link
Collaborator

etrepum commented Mar 14, 2025

I don't personally have any domain expertise with bidi text so will have to wait for some feedback from other contributors to see what the ideal semantics are. I agree that it seems like it should primarily be a derived/computed property, but without full context I think we need to start with declaring what the DOM should be in the ideal scenario and then work backwards to come up with an implementation strategy that matches that expectation efficiently.

I suspect that probably the complexity here is to make sure that if the user is entering rtl text in one paragraph and then creates another paragraph then it should probably start off as rtl even when empty so the selection is visually in the expected place?

Some questions to answer:

  • Is there a use case for persisting dir outside of the DOM?
  • What should it be set to when ltr and rtl text ends up in the same element?
  • How should that get reconciled when the timing of those events are interleaved due to collab?

@zurfyx - who is the best resource for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants