DOM: Update getScrollContainer to be aware of horizontal scroll #49787
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Bug
andrewserong
requested review from
youknowriad,
apeatling,
ramonjd and
tellthemachines
|
Size Change: +85 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
A question for the reviewer — I believe this is a bug fix, but could this change be unexpected in different use cases? If so, would it be worth us creating an additional function, or adding an additional param were consumers can specify which directions they'd like to look for (and we'd default to vertical)? For now, I've left this PR as a simple bug fix.
Good question! I'm inclined to consider this a breaking change because we do have horizontal scroll in some parts of the UI, especially on smaller screens, so it's possible this might change existing behaviour.
An optional param defaulting to vertical sounds good. Would there ever be a need to find the closest scrollable container independent of whether it's vertical or horizontal? I can't think of any offhand but I'm not that familiar with existing uses of this function.
Great point! And it'd be hard for us to know based on this PR alone whether or not anything would be likely to break.
I think my assumption is that in most cases you'd be looking for either scrollbar, rather than specifically looking for horizontal or vertical. But now that you mention it, the list view PR is really just looking for a horizontal scrollbar, and there are likely many places that don't care if there's a horizontal scrollbar and are only looking for vertical 🤔 Happy to try any options here. I might leave this PR for today, and will update tomorrow — I'm a little on the fence between updating this function, or adding another function called |
|
I agree with @tellthemachines it seems that this change could potentially have unexpected results. Maybe an argument to switch between vertical and horizontal (and potentially both) while the default should be "vertical"? cc @ellatrix who I believe had to implement/use this in a few places. |
|
Thanks for the feedback @tellthemachines and @youknowriad! I've updated this PR to include an optional I went with |
|
Flaky tests detected in dda80bd. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4695079987
|
| const { overflowY } = getComputedStyle( node ); | ||
|
|
||
| if ( /(auto|scroll)/.test( overflowY ) ) { | ||
| return node; |
There was a problem hiding this comment.
If "direction" is "all", won't we need to check if the closest horizontal container isn't closer than the closest vertical before returning?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh got it, that should work then!
tellthemachines
approved these changes
Apr 14, 2023
|
Thanks for reviewing @tellthemachines, much appreciated! I'll wait until Monday to see if there's any other feedback, and if not, will merge it in then 🙂 |
|
Thanks :) |
andrewserong
deleted the
fix/get-scroll-container-to-recognise-horizontal-scroll
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR updates the
getScrollContainerutility function to add adirectionparameter, accepting valuesvertical,horizontalorall, and defaulting tovertical. It adds the ability for horizontally scrolling containers to be returned by the traversal.For backwards compatibility, when called without the
directionparam, only vertically scrolling containers will be returned.Why?
While working on #49786, where I'm using
getScrollContainerwith the expectation of it returning the list view container once a horizontal scrollbar is introduced, I noticed thatgetScrollContainerwould only return the expected container in lists that went far enough vertically to create a vertical scrollbar.A question for the reviewer — I believe this is a bug fix, but could this change be unexpected in different use cases? If so, would it be worth us creating an additional function, or adding an additional param were consumers can specify which directions they'd like to look for (and we'd default toUpdate: I've updated this PR to add an extra param, for backwards compatibility.vertical)? For now, I've left this PR as a simple bug fix.How?
getScrollContainerthat looks for a vertical scrollbar and apply to horizontal valuesdirectionparam and check it before applying the vertical and horizontal logic.directiontovertical, but also allowhorizontalandallvalues.Testing Instructions
I'm not too sure of a good way to test this manually, however I've updated #49786 to include this code change so that the list view drag and drop can be tested with it.
Also, I attempted to write a unit test for this, but discovered quickly that
clientWidthis always returned as0in tests, so I couldn't quite think of a good way to write a test for this without a lot of mocking, which might not be all that useful. Very happy for feedback here if folks have a good idea of how to address it!