fix: (WIP) properly scroll body if keyboard focusing a item with no other scroll parents#9780
fix: (WIP) properly scroll body if keyboard focusing a item with no other scroll parents#9780
Conversation
| let parentElements: Element[] = []; | ||
| let root = document.scrollingElement || document.documentElement; | ||
|
|
||
| do { |
There was a problem hiding this comment.
old logic wasn't including the root element except if the initial node provided was the root. isScrollable should properly check if scrolling is being prevented on the root so we don't actually need the node ! == root check I believe
There was a problem hiding this comment.
Meh, our test suite was exactly calling with root as the container. Sorry about that.
There was a problem hiding this comment.
all good haha, I didn't catch this either on my original review. So many different scenarios to test too
| let scrollBarOffsetY = scrollView === root ? 0 : borderTopWidth + borderBottomWidth; | ||
| let scrollBarWidth = scrollView.offsetWidth - scrollView.clientWidth - scrollBarOffsetX; | ||
| let scrollBarHeight = scrollView.offsetHeight - scrollView.clientHeight - scrollBarOffsetY; | ||
| let scrollBarHeight = scrollView === root ? 0 : scrollView.offsetHeight - scrollView.clientHeight - scrollBarOffsetY; |
There was a problem hiding this comment.
we already use scrollView.clientHeight when calculating viewBottom as used below. That omits the scroll bar already if scrollView is the root so we can set to 0 here
There was a problem hiding this comment.
I think we would want to do that for scrollBarWidth too, right? In case the root is horizontally scrollable.
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?
There was a problem hiding this comment.
hm, good points I'll see about trying that soon
There was a problem hiding this comment.
actually for the scrollPort do we need to accommodate for that? The scrollPort values should always be the coordinates of the scroll region we are currently positioning the target with respect to so it shouldn't care about the actual viewport and thus should always take the border and scrollPadding into view right? The border of the scroll region/port being not in view should get handled by scrolling all parents up to the root in scrollIntoViewport?
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?
There was a problem hiding this comment.
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 (0, 0, clientWidth, clientHeight). Here, clientWidth and clientHeight correspond to viewport - scrollbar, while offsetHeight continues to be the "full" rect, including borders.
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.
There was a problem hiding this comment.
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?
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;
That way we remain in the same coordinate system w/ regards to the scrollPort values and the scrollArea values.
https://codesandbox.io/p/sandbox/2x4wtp?file=%2Findex.html%3A14%2C16 seems to work well now with those changes
There was a problem hiding this comment.
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.
| for (let scrollParent of getScrollParents(targetElement, true)) { | ||
| scrollIntoView(scrollParent as HTMLElement, targetElement as HTMLElement); | ||
| } |
There was a problem hiding this comment.
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 scrollIntoView and one via scrollIntoViewport ) and the second of which scrolls the containing element into view first
|
Build successful! 🎉 |
|
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:GridList GridList <T extends {}> {
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
autoFocus?: boolean | FocusStrategy
children?: ReactNode | ({}) => ReactNode
className?: ClassNameOrFunction<GridListRenderProps> = 'react-aria-GridList'
defaultSelectedKeys?: 'all' | Iterable<Key>
dependencies?: ReadonlyArray<any>
disabledBehavior?: DisabledBehavior = "all"
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
disallowTypeAhead?: boolean = false
dragAndDropHooks?: DragAndDropHooks<NoInfer<{}>>
escapeKeyBehavior?: 'clearSelection' | 'none' = 'clearSelection'
id?: string
items?: Iterable<T>
keyboardNavigationBehavior?: 'arrow' | 'tab' = 'arrow'
layout?: 'stack' | 'grid' = 'stack'
onAction?: (Key) => void
onSelectionChange?: (Selection) => void
- orientation?: 'horizontal' | 'vertical' = 'vertical'
render?: DOMRenderFunction<keyof React.JSX.IntrinsicElements, GridListRenderProps>
renderEmptyState?: (GridListRenderProps) => ReactNode
selectedKeys?: 'all' | Iterable<Key>
selectionBehavior?: SelectionBehavior = "toggle"
shouldSelectOnPressUp?: boolean
slot?: string | null
style?: StyleOrFunction<GridListRenderProps>
}/react-aria-components:GridListProps GridListProps <T> {
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
autoFocus?: boolean | FocusStrategy
children?: ReactNode | (T) => ReactNode
className?: ClassNameOrFunction<GridListRenderProps> = 'react-aria-GridList'
defaultSelectedKeys?: 'all' | Iterable<Key>
dependencies?: ReadonlyArray<any>
disabledBehavior?: DisabledBehavior = "all"
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
disallowTypeAhead?: boolean = false
dragAndDropHooks?: DragAndDropHooks<NoInfer<T>>
escapeKeyBehavior?: 'clearSelection' | 'none' = 'clearSelection'
id?: string
items?: Iterable<T>
keyboardNavigationBehavior?: 'arrow' | 'tab' = 'arrow'
layout?: 'stack' | 'grid' = 'stack'
onAction?: (Key) => void
onSelectionChange?: (Selection) => void
- orientation?: 'horizontal' | 'vertical' = 'vertical'
render?: DOMRenderFunction<keyof React.JSX.IntrinsicElements, GridListRenderProps>
renderEmptyState?: (GridListRenderProps) => ReactNode
selectedKeys?: 'all' | Iterable<Key>
selectionBehavior?: SelectionBehavior = "toggle"
shouldSelectOnPressUp?: boolean
slot?: string | null
style?: StyleOrFunction<GridListRenderProps>
}/react-aria-components:TreeEmptyStateRenderProps-TreeEmptyStateRenderProps {
- allowsDragging: boolean
- isFocusVisible: boolean
- isFocused: boolean
- selectionMode: SelectionMode
- state: TreeState<unknown>
-}@react-aria/utils/@react-aria/utils:getNonce-getNonce {
- doc?: Document
- returnVal: undefined
-}@react-spectrum/s2/@react-spectrum/s2:CardView CardView <T extends {}> {
UNSAFE_className?: UnsafeClassName
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
autoFocus?: boolean | FocusStrategy
children?: ReactNode | (T) => ReactNode
defaultSelectedKeys?: 'all' | Iterable<Key>
density?: 'compact' | 'regular' | 'spacious' = 'regular'
dependencies?: ReadonlyArray<any>
disabledBehavior?: DisabledBehavior = "all"
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
disallowTypeAhead?: boolean = false
dragAndDropHooks?: DragAndDropHooks<NoInfer<T>>
escapeKeyBehavior?: 'clearSelection' | 'none' = 'clearSelection'
id?: string
items?: Iterable<T>
layout?: 'grid' | 'waterfall' = 'grid'
loadingState?: LoadingState
onAction?: (Key) => void
onLoadMore?: () => void
onSelectionChange?: (Selection) => void
- orientation?: 'horizontal' | 'vertical' = 'vertical'
renderActionBar?: ('all' | Set<Key>) => ReactElement
renderEmptyState?: (GridListRenderProps) => ReactNode
selectedKeys?: 'all' | Iterable<Key>
selectionMode?: SelectionMode
shouldSelectOnPressUp?: boolean
size?: 'XS' | 'S' | 'M' | 'L' | 'XL' = 'M'
slot?: string | null
styles?: StylesPropWithHeight
variant?: 'primary' | 'secondary' | 'tertiary' | 'quiet' = 'primary'
}/@react-spectrum/s2:ListView ListView <T extends {}> {
UNSAFE_className?: UnsafeClassName
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
autoFocus?: boolean | FocusStrategy
children: ReactNode | ({}) => ReactNode
defaultSelectedKeys?: 'all' | Iterable<Key>
dependencies?: ReadonlyArray<any>
disabledBehavior?: DisabledBehavior = "all"
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
disallowTypeAhead?: boolean = false
escapeKeyBehavior?: 'clearSelection' | 'none' = 'clearSelection'
hideLinkOutIcon?: boolean
id?: string
isQuiet?: boolean
items?: Iterable<T>
loadingState?: LoadingState
onAction?: (Key) => void
onLoadMore?: () => void
onSelectionChange?: (Selection) => void
- orientation?: 'horizontal' | 'vertical' = 'vertical'
overflowMode?: 'wrap' | 'truncate' = 'truncate'
renderActionBar?: ('all' | Set<Key>) => ReactElement
renderEmptyState?: (GridListRenderProps) => ReactNode
selectedKeys?: 'all' | Iterable<Key>
selectionStyle?: 'highlight' | 'checkbox' = 'checkbox'
shouldSelectOnPressUp?: boolean
slot?: string | null
styles?: StylesPropWithHeight
}/@react-spectrum/s2:CardViewProps CardViewProps <T> {
UNSAFE_className?: UnsafeClassName
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
autoFocus?: boolean | FocusStrategy
children?: ReactNode | (T) => ReactNode
defaultSelectedKeys?: 'all' | Iterable<Key>
density?: 'compact' | 'regular' | 'spacious' = 'regular'
dependencies?: ReadonlyArray<any>
disabledBehavior?: DisabledBehavior = "all"
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
disallowTypeAhead?: boolean = false
dragAndDropHooks?: DragAndDropHooks<NoInfer<T>>
escapeKeyBehavior?: 'clearSelection' | 'none' = 'clearSelection'
id?: string
items?: Iterable<T>
layout?: 'grid' | 'waterfall' = 'grid'
loadingState?: LoadingState
onAction?: (Key) => void
onLoadMore?: () => void
onSelectionChange?: (Selection) => void
- orientation?: 'horizontal' | 'vertical' = 'vertical'
renderActionBar?: ('all' | Set<Key>) => ReactElement
renderEmptyState?: (GridListRenderProps) => ReactNode
selectedKeys?: 'all' | Iterable<Key>
selectionMode?: SelectionMode
shouldSelectOnPressUp?: boolean
size?: 'XS' | 'S' | 'M' | 'L' | 'XL' = 'M'
slot?: string | null
styles?: StylesPropWithHeight
variant?: 'primary' | 'secondary' | 'tertiary' | 'quiet' = 'primary'
}/@react-spectrum/s2:ListViewProps ListViewProps <T> {
UNSAFE_className?: UnsafeClassName
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
autoFocus?: boolean | FocusStrategy
children: ReactNode | (T) => ReactNode
defaultSelectedKeys?: 'all' | Iterable<Key>
dependencies?: ReadonlyArray<any>
disabledBehavior?: DisabledBehavior = "all"
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
disallowTypeAhead?: boolean = false
escapeKeyBehavior?: 'clearSelection' | 'none' = 'clearSelection'
hideLinkOutIcon?: boolean
id?: string
isQuiet?: boolean
items?: Iterable<T>
loadingState?: LoadingState
onAction?: (Key) => void
onLoadMore?: () => void
onSelectionChange?: (Selection) => void
- orientation?: 'horizontal' | 'vertical' = 'vertical'
overflowMode?: 'wrap' | 'truncate' = 'truncate'
renderActionBar?: ('all' | Set<Key>) => ReactElement
renderEmptyState?: (GridListRenderProps) => ReactNode
selectedKeys?: 'all' | Iterable<Key>
selectionStyle?: 'highlight' | 'checkbox' = 'checkbox'
shouldSelectOnPressUp?: boolean
slot?: string | null
styles?: StylesPropWithHeight
} |
3 participants
Closes #8977
✅ Pull Request Checklist:
📝 Test Instructions:
Go to "Table with React Transition" story in the RAC storybook and make sure the window is small enough that the body is scrollable. Keyboard navigate downwards through the items and make sure they get scrolled into view properly.
🧢 Your Project:
RSP