[lexical] Bug Fix: scroll collapsed selection horizontally in overflow containers#8495
[lexical] Bug Fix: scroll collapsed selection horizontally in overflow containers#8495Frank20021 wants to merge 3 commits into
Conversation
|
Hi @Frank20021! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
| if (selectionTarget instanceof Text) { | ||
| return selectionTarget.parentElement; | ||
| } | ||
| if (selectionTarget instanceof HTMLElement) { | ||
| return selectionTarget; | ||
| } |
There was a problem hiding this comment.
We should never use instanceof with DOM because it does not work across iframes, this is why we have guard functions like isDOMNode, isDOMTextNode, etc.
|
Thanks, fixed by replacing the DOM instanceof checks with Lexical’s cross-frame-safe DOM guard helpers.
________________________________
From: Bob Ippolito ***@***.***>
Sent: Monday, May 11, 2026 2:26 PM
To: facebook/lexical ***@***.***>
Cc: FRANK NAMBEH ***@***.***>; Mention ***@***.***>
Subject: Re: [facebook/lexical] [lexical] Bug Fix: scroll collapsed selection horizontally in overflow containers (PR #8495)
EXTERNAL EMAIL: Please do not click links or open attachments unless you recognize the sender and know the content is safe.
@etrepum commented on this pull request.
________________________________
In packages/lexical/src/LexicalSelection.ts<https://urldefense.com/v3/__https://github.com/facebook/lexical/pull/8495*discussion_r3221570495__;Iw!!NGa5sw!dGyHKWrJ9kXfbij68cAl548mpib1Alg9U-uaFVZtMyZvTsTgI8CbuyBuk2f8jB3_wDwJqiTeivzzCdyOpF-G76vTwuEXDSE$>:
+ if (selectionTarget instanceof Text) {
+ return selectionTarget.parentElement;
+ }
+ if (selectionTarget instanceof HTMLElement) {
+ return selectionTarget;
+ }
We should never use instanceof with DOM because it does not work across iframes, this is why we have guard functions like isDOMNode, isDOMTextNode, etc.
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/facebook/lexical/pull/8495*pullrequestreview-4266744258__;Iw!!NGa5sw!dGyHKWrJ9kXfbij68cAl548mpib1Alg9U-uaFVZtMyZvTsTgI8CbuyBuk2f8jB3_wDwJqiTeivzzCdyOpF-G76vTvDf3_T4$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/BKYUZDPHNSTSP3OFAIGCLY342ISQHAVCNFSM6AAAAACYYTXTR6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DENRWG42DIMRVHA__;!!NGa5sw!dGyHKWrJ9kXfbij68cAl548mpib1Alg9U-uaFVZtMyZvTsTgI8CbuyBuk2f8jB3_wDwJqiTeivzzCdyOpF-G76vTkaaeONo$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
| const element: HTMLElement | null = | ||
| scrollChainStartElement != null ? scrollChainStartElement : rootElement; |
There was a problem hiding this comment.
| const element: HTMLElement | null = | |
| scrollChainStartElement != null ? scrollChainStartElement : rootElement; | |
| let element: HTMLElement | null = scrollChainStartElement || rootElement; |
I don't see a reason to have both an element and a walk variable, let's get rid of this walk variable and use element like we did before. element isn't re-used anywhere so it doesn't make sense to keep the initial value around as a constant.
| let walk: HTMLElement | null = element; | ||
|
|
||
| while (walk !== null) { | ||
| const isBodyElement = walk === doc.body; |
There was a problem hiding this comment.
| let walk: HTMLElement | null = element; | |
| while (walk !== null) { | |
| const isBodyElement = walk === doc.body; | |
| while (element !== null) { | |
| const isBodyElement = element === doc.body; |
| } | ||
| } else { | ||
| const targetRect = element.getBoundingClientRect(); | ||
| const targetRect = walk.getBoundingClientRect(); |
There was a problem hiding this comment.
| const targetRect = walk.getBoundingClientRect(); | |
| const targetRect = element.getBoundingClientRect(); |
| break; | ||
| } | ||
| element = getParentElement(element); | ||
| walk = getParentElement(walk); |
There was a problem hiding this comment.
| walk = getParentElement(walk); | |
| element = getParentElement(element); |
etrepum
left a comment
There was a problem hiding this comment.
While this does follow the input text to and scroll the right it doesn't seem to scroll correctly to the left. For example, create a CodeBlock and type enough text to scroll and then press enter. It will scroll to the left but not all the way. Possibly due to the line numbers.
potatowagon
left a comment
There was a problem hiding this comment.
Review — Scroll Collapsed Selection Horizontally in Overflow Containers
Assessment: Looks good to land ✅
What I verified:
-
Bug fix logic: The existing
scrollIntoViewIfNeededonly handled vertical scrolling. This PR extends it to also handle horizontal scrolling, which is important for code blocks or any container withoverflow-x: auto/scroll. The fix addscurrentLeft/currentRight/targetLeft/targetRighttracking parallel to the existing vertical logic. -
Scroll chain improvement: A new
scrollChainStartElementparameter lets the walk begin at the caret's immediate parent rather than the root, so intermediate scrollable containers (e.g., a code block) get scrolled first before bubbling up to the document. -
Edge cases: The
maxScrollX > 0guard prevents unnecessary scroll attempts on non-scrollable elements. CSS scroll-padding is correctly accounted for on both axes. ThevisualViewportpath also gets horizontal bounds. -
Test coverage: Unit test added that creates an overflow container, positions a selection rect beyond the right edge, and verifies
scrollIntoViewIfNeededscrolls it into view. -
CI status: Full CI suite green (40 checks pass).
-
Risk: Low — extends existing scroll logic in a consistent, parallel manner. The added
scrollChainStartElementparameter is optional, so existing callers are unaffected.
— via Navi on behalf of potatowagon
Description
Current behavior: When the caret sits inside a horizontally scrollable block (for example a code block with long lines and
overflow-x: auto), moving the collapsed selection with keyboard shortcuts such as Ctrl+ArrowLeft / Ctrl+ArrowRight can place the caret outside the visible scroll region.scrollIntoViewIfNeededonly adjusted vertical scroll and only walked ancestors starting from the editor root, so inner scroll containers were not scrolled on the X axis.This PR: Extends
scrollIntoViewIfNeededto:scrollWidth > clientWidth), updatingscrollLeftsimilarly to the existingscrollToplogic.scrollBy, usingvisualViewportwhen present and applying horizontalscroll-paddingon the document element in addition to the existing vertical padding.Collapsed selection updates in
LexicalSelectionpass the chain start derived from the activeRange/Text/HTMLElementtarget.Related to #8459 (addresses the horizontal auto-scroll / caret visibility part; VS Code–style word wrap remains a possible follow-up).
Test plan
Automated
pnpm exec vitest run packages/lexical/src/__tests__/unit/LexicalUtils.test.ts -t "scrollIntoViewIfNeeded" --project=unitManual
Before
Caret could move off-screen horizontally inside the code block while the block’s
scrollLeftdid not follow.After
Caret stays within the visible horizontal range of the code block (and other scrollable ancestors) when the collapsed selection is updated.