Skip to content

[lexical-selection] Bug Fix: Correct backward inversion for RTL#7686

Merged
etrepum merged 3 commits intofacebook:mainfrom
noamzaks:fix-rtl-selection
Jul 12, 2025
Merged

[lexical-selection] Bug Fix: Correct backward inversion for RTL#7686
etrepum merged 3 commits intofacebook:mainfrom
noamzaks:fix-rtl-selection

Conversation

@noamzaks
Copy link
Copy Markdown
Contributor

Description

This updates the $shouldOverrideDefaultCharacterSelection to the correct "backward" value when inside an RTL element.

Closes #7685.

Test plan

Before

Screen.Recording.2025-07-11.at.0.20.34.mov

After

Screen.Recording.2025-07-11.at.0.28.17.mov

@vercel
Copy link
Copy Markdown

vercel bot commented Jul 10, 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 Jul 12, 2025 8:56am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 12, 2025 8:56am

@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 Jul 10, 2025
@noamzaks
Copy link
Copy Markdown
Contributor Author

As can be seen in the "After" video, exiting the decorator nodes should probably be inverted in some cases as well. However this is more tricky, as it's probably possible that before+after aren't the same direction, in which case it's unknown what should occur.

In any case, I think when before+after are both RTL it's expected that a right arrow from the decorator node will move to the previous node instead of the next one and the left arrow will move to the next node, where I think changes can be made to the lexical-rich-text/src/index.ts file in the appropriate command registrations.

I will probably open an issue (and hopefully PR) for this after #7685 is resolved.

@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Jul 10, 2025
Copy link
Copy Markdown
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a reasonable approach, although it would be fantastic to add an e2e test to show that it does what it's supposed to. Should be fairly straightforward since this is reproducible without adding anything to the playground, there are other tests in packages/lexical-playground/__tests__/e2e/Selection.spec.mjs (but nothing that tests rtl behavior yet)

@noamzaks
Copy link
Copy Markdown
Contributor Author

I added a test case for this (and verified it fails before adding the change).
I had an issue using moveToLineEnd or moveToLineBeginning for some reason.

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Jul 11, 2025

It’s likely that move line to end/beginning has the same class of issue with rtl that needs a different workaround. The e2e test helpers tend to use input rather than some back door to force a specific state. It does look like these tests are failing, maybe only on some specific browser/platform combinations.

@noamzaks
Copy link
Copy Markdown
Contributor Author

Looks like plain text and collab should be disabled for this test.

Copy link
Copy Markdown
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thank you!

@etrepum etrepum added this pull request to the merge queue Jul 12, 2025
Merged via the queue into facebook:main with commit 2f093c6 Jul 12, 2025
39 checks passed
@noamzaks
Copy link
Copy Markdown
Contributor Author

I think this is still broken when its in the end of the paragraph for instance. Investigating now.

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. extended-tests Run extended e2e tests on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Selection movement incorrect for RTL entering decorator node

3 participants