-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: (WIP) properly scroll body if keyboard focusing a item with no other scroll parents #9780
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,7 @@ export function scrollIntoView(scrollView: HTMLElement, element: HTMLElement, op | |
| let itemStyle = window.getComputedStyle(element); | ||
| let viewStyle = window.getComputedStyle(scrollView); | ||
| let root = document.scrollingElement || document.documentElement; | ||
| let isRoot = scrollView === root; | ||
|
|
||
| let viewTop = scrollView === root ? 0 : view.top; | ||
| let viewBottom = scrollView === root ? scrollView.clientHeight : view.bottom; | ||
|
|
@@ -72,13 +73,13 @@ export function scrollIntoView(scrollView: HTMLElement, element: HTMLElement, op | |
|
|
||
| let scrollBarOffsetX = scrollView === root ? 0 : borderLeftWidth + borderRightWidth; | ||
| let scrollBarOffsetY = scrollView === root ? 0 : borderTopWidth + borderBottomWidth; | ||
| let scrollBarWidth = scrollView.offsetWidth - scrollView.clientWidth - scrollBarOffsetX; | ||
| let scrollBarHeight = scrollView.offsetHeight - scrollView.clientHeight - scrollBarOffsetY; | ||
| let scrollBarWidth = scrollView === root ? 0 : scrollView.offsetWidth - scrollView.clientWidth - scrollBarOffsetX; | ||
| let scrollBarHeight = scrollView === root ? 0 : scrollView.offsetHeight - scrollView.clientHeight - scrollBarOffsetY; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we already use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we would want to do that for Also I think we need to adjust the scrollPort for the root element too actually. Since the border may not be in view, it might not make sense to always add/subtract it, right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm, good points I'll see about trying that soon
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually for the For instance take a table with a border that scrolled partially out of view such that its border isn't in view: Scrolling a table row into view would be to first scroll the table itself into view, then the item into view within the table right so we wouldn't need to accommodate for the border being in view or not?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I think for normal containers that's right, but not for the root element. I will have to test this more in depth, but here is my thinking: The Scrollport is defined as the unclipped rect of a scroll container, adjusted by scroll-padding. For the root element, this is always the viewport, hence why we set the view to The client values on the root are unaffected when a border is added/removed on the root. Hence why I think we need to adjust when the border is in view vs not. On normal elements, a border wouldn't affect the inner scroll offset, but on the root it does. Edit: Confirmed via sandbox.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh I see, in that case I think we can adjust the scrollPort values to include the border if we are currently scrolling w/ respect to the root? That way we remain in the same coordinate system w/ regards to the https://codesandbox.io/p/sandbox/2x4wtp?file=%2Findex.html%3A14%2C16 seems to work well now with those changes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's what my first thought was too, but I don't think that works fully - scrolling both to red start and red end is broken. I haven't fully grasped why, because I haven't spent time on this today, but I think its because the border falls within the target area, which is what I meant by it being visible or not. |
||
|
|
||
| let scrollPortTop = viewTop + borderTopWidth + scrollPaddingTop; | ||
| let scrollPortBottom = viewBottom - borderBottomWidth - scrollPaddingBottom - scrollBarHeight; | ||
| let scrollPortLeft = viewLeft + borderLeftWidth + scrollPaddingLeft; | ||
| let scrollPortRight = viewRight - borderRightWidth - scrollPaddingRight; | ||
| let scrollPortTop = viewTop + (isRoot ? 0 : borderTopWidth) + scrollPaddingTop; | ||
| let scrollPortBottom = viewBottom - (isRoot ? 0 : borderBottomWidth) - scrollPaddingBottom - scrollBarHeight; | ||
| let scrollPortLeft = viewLeft + (isRoot ? 0 : borderLeftWidth) + scrollPaddingLeft; | ||
| let scrollPortRight = viewRight - (isRoot ? 0 : borderRightWidth) - scrollPaddingRight; | ||
|
|
||
| // IOS always positions the scrollbar on the right ¯\_(ツ)_/¯ | ||
| if (viewStyle.direction === 'rtl' && !isIOS()) { | ||
|
|
@@ -159,9 +160,13 @@ export function scrollIntoViewport(targetElement: Element | null, opts: ScrollIn | |
| // Account for sub pixel differences from rounding | ||
| if ((Math.abs(originalLeft - newLeft) > 1) || (Math.abs(originalTop - newTop) > 1)) { | ||
| scrollParents = containingElement ? getScrollParents(containingElement, true) : []; | ||
| // scroll containing element into view first, then rescroll target element into view like the non chrome flow above | ||
| for (let scrollParent of scrollParents) { | ||
| scrollIntoView(scrollParent as HTMLElement, containingElement as HTMLElement, {block: 'center', inline: 'center'}); | ||
| } | ||
| for (let scrollParent of getScrollParents(targetElement, true)) { | ||
| scrollIntoView(scrollParent as HTMLElement, targetElement as HTMLElement); | ||
| } | ||
|
Comment on lines
+167
to
+169
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mirrors the flow above, we need to first scroll the containing element into view if necessary, then re-scroll the target element into view to compensate. There is some logic in useSelectableCollection that revealed that this was a problem because we do two separate scroll in views (one via |
||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| /* | ||
| * Copyright 2026 Adobe. All rights reserved. | ||
| * This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. You may obtain a copy | ||
| * of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under | ||
| * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS | ||
| * OF ANY KIND, either express or implied. See the License for the specific language | ||
| * governing permissions and limitations under the License. | ||
| */ | ||
|
|
||
| import {getScrollParents} from '../src/getScrollParents'; | ||
|
|
||
| describe('getScrollParents', () => { | ||
| let root: Element; | ||
|
|
||
| beforeEach(() => { | ||
| root = document.documentElement; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| document.body.innerHTML = ''; | ||
| jest.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it('includes root as a scroll parent for a node in the document', () => { | ||
| let div = document.createElement('div'); | ||
| document.body.appendChild(div); | ||
|
|
||
| let parents = getScrollParents(div); | ||
| expect(parents).toContain(root); | ||
| }); | ||
|
|
||
| it('does not include root when root has overflow: hidden', () => { | ||
| let div = document.createElement('div'); | ||
| document.body.appendChild(div); | ||
|
|
||
| jest.spyOn(window, 'getComputedStyle').mockImplementation((el) => { | ||
| if (el === root) { | ||
| return {overflow: 'hidden'} as CSSStyleDeclaration; | ||
| } | ||
| return {overflow: 'visible'} as CSSStyleDeclaration; | ||
| }); | ||
|
|
||
| let parents = getScrollParents(div); | ||
| expect(parents).not.toContain(root); | ||
| }); | ||
|
|
||
| it('includes a scrollable intermediate parent', () => { | ||
| let scrollable = document.createElement('div'); | ||
| let child = document.createElement('div'); | ||
| document.body.appendChild(scrollable); | ||
| scrollable.appendChild(child); | ||
|
|
||
| jest.spyOn(window, 'getComputedStyle').mockImplementation((el) => { | ||
| if (el === scrollable) { | ||
| return {overflow: 'auto'} as CSSStyleDeclaration; | ||
| } | ||
| return {overflow: 'visible'} as CSSStyleDeclaration; | ||
| }); | ||
|
|
||
| let parents = getScrollParents(child); | ||
| expect(parents).toContain(scrollable); | ||
| expect(parents).toContain(root); | ||
| }); | ||
|
|
||
| it('excludes non-scrollable ancestors', () => { | ||
| let plain = document.createElement('div'); | ||
| let child = document.createElement('div'); | ||
| document.body.appendChild(plain); | ||
| plain.appendChild(child); | ||
|
|
||
| let parents = getScrollParents(child); | ||
| expect(parents).not.toContain(plain); | ||
| expect(parents).not.toContain(document.body); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
old logic wasn't including the root element except if the initial node provided was the root.
isScrollableshould properly check if scrolling is being prevented on the root so we don't actually need thenode ! == rootcheck I believeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, our test suite was exactly calling with
rootas the container. Sorry about that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all good haha, I didn't catch this either on my original review. So many different scenarios to test too