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.

Sign up for GitHub

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

Jump to bottom

DOM: Update getScrollContainer to be aware of horizontal scroll #49787

Merged
merged 2 commits into from
Apr 16, 2023
Merged

DOM: Update getScrollContainer to be aware of horizontal scroll #49787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6352bd
DOM: Update getScrollContainer to be aware of horizontal scroll
andrewserong Apr 13, 2023
dda80bd
Update to add a direction param
andrewserong Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/dom/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ Given a DOM node, finds the closest scrollable container node or the node itself
_Parameters_

- _node_ `Element | null`: Node from which to start.
- _direction_ `?string`: Direction of scrollable container to search for ('vertical', 'horizontal', 'all'). Defaults to 'vertical'.

_Returns_

Expand Down
38 changes: 28 additions & 10 deletions packages/dom/src/dom/get-scroll-container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,37 @@ import getComputedStyle from './get-computed-style';
* Given a DOM node, finds the closest scrollable container node or the node
* itself, if scrollable.
*
* @param {Element | null} node Node from which to start.
*
* @param {Element | null} node Node from which to start.
* @param {?string} direction Direction of scrollable container to search for ('vertical', 'horizontal', 'all').
* Defaults to 'vertical'.
* @return {Element | undefined} Scrollable container node, if found.
*/
export default function getScrollContainer( node ) {
export default function getScrollContainer( node, direction = 'vertical' ) {
if ( ! node ) {
return undefined;
}

// Scrollable if scrollable height exceeds displayed...
if ( node.scrollHeight > node.clientHeight ) {
// ...except when overflow is defined to be hidden or visible
const { overflowY } = getComputedStyle( node );
if ( direction === 'vertical' || direction === 'all' ) {
// Scrollable if scrollable height exceeds displayed...
if ( node.scrollHeight > node.clientHeight ) {
// ...except when overflow is defined to be hidden or visible
const { overflowY } = getComputedStyle( node );

if ( /(auto|scroll)/.test( overflowY ) ) {
return node;
Copy link
Contributor

Choose a reason for hiding this comment

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

If "direction" is "all", won't we need to check if the closest horizontal container isn't closer than the closest vertical before returning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, but I'm not sure if I understand the problem. If we're searching by all, I didn't think it'd matter which it encounters first, as it should return the first scrollable container of any kind of direction. So if it doesn't match against vertical, then the horizontal block below will be run and it'll return the node as expected.

Can you think of a case where that logic would slip up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: I think I understand the question now. getScrollContainer works iteratively only through a single level at a time, recursing via calling itself with node.parentNode, so we should be safe to check vertical then check horizontal, as in each iteration we're only looking one level up, rather than looking all the way up in each vertical/horizontal code block.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh got it, that should work then!

}
}
}

if ( direction === 'horizontal' || direction === 'all' ) {
// Scrollable if scrollable width exceeds displayed...
if ( node.scrollWidth > node.clientWidth ) {
// ...except when overflow is defined to be hidden or visible
const { overflowX } = getComputedStyle( node );

if ( /(auto|scroll)/.test( overflowY ) ) {
return node;
if ( /(auto|scroll)/.test( overflowX ) ) {
return node;
}
}
}

Expand All @@ -31,5 +46,8 @@ export default function getScrollContainer( node ) {
}

// Continue traversing.
return getScrollContainer( /** @type {Element} */ ( node.parentNode ) );
return getScrollContainer(
/** @type {Element} */ ( node.parentNode ),
direction
);
}