Update scroll lock logic to lock everything but disabled targets #858
Conversation
…ontal scrolling element
src/utils/scroll-lock.js
Outdated
| @@ -73,23 +74,30 @@ function advancedUnlock(targetElement) { | |||
| document.removeEventListener('touchmove', preventDefault); | |||
| } | |||
|
|
|||
| const isVerticalScroll = ({ scrollHeight, scrollWidth }) => scrollHeight > scrollWidth; | |||
|
|
|||
There was a problem hiding this comment.
If I understood this correctly, I'm not sure if we can determine if an element allows vertical or horizontal scroll just by comparing its scroll height to its width. I think we should compare scrollHeight to clientHeight maybe?
There was a problem hiding this comment.
Thinking out loud here: what you implemented here determines if a container is more "portrait" or "landscape", but that may not translate exactly to the direction of the scroll. For example, a longer code block in a narrow viewport would be considered "portrait" but we most likely need it to scroll horizontally
There was a problem hiding this comment.
In the example you give, I don't think that would happen because in the case of a code block, the scrollWidth would be greater than the scrollHeight.
A narrow viewport wouldn't affect the scrollWidth since it's a measurement of the width of an element's content, including content not visible on the screen due to overflow. Source.
There was a problem hiding this comment.
Ops... I was reading from here 😅
But I think my point still stands... The shape of the container doesn't necessarily translate to the desired scroll direction
| } else if (Math.abs(clientY) > Math.abs(clientX)) { | ||
| // prevent event if user tries to perform vertical scroll in an horizontal scrolling element |
There was a problem hiding this comment.
So smart :PP
Is this how WebKit determines what's a vertical scroll?
There was a problem hiding this comment.
I don't have idea about how WebKit really works but here I'm just comparing the absolute value of clientY (how much the user has moved to the Y axis since it started the movement) vs the absolute value of clientX(how much the user has moved to the X axis since it started the movement).
So if the user has moved more in the Y axis than to the X axis, it's a vertical scroll.
There was a problem hiding this comment.
I understand what you are doing here, but there are examples when a gesture that moved more in the X axis than the Y axis be considered a vertical scroll on an iOS device.
But realizing now, it's probably not possible to imitate the iOS implementation exactly.
I don't have a better idea on a better heuristic to use to determine the scroll direction either...
| @@ -106,7 +107,7 @@ | |||
| </div> | |||
| </div> | |||
| <TagList | |||
| v-if="displaySuggestedTags" | |||
| v-show="displaySuggestedTags" | |||
| :id="SuggestedTagsId" | |||
| ref="suggestedTags" | |||
| :ariaLabel="$tc('filter.suggested-tags', suggestedTags.length)" | |||
There was a problem hiding this comment.
Do you think it could solve our problem if we applied the SCROLL_LOCK_DISABLE_ATTR here, on the entire taglist component, instead of on individual tags within it?
And also conditionally apply the disable attribute onto the input element by checking if modelValue is overflowing? This way we don't disable the lock when FilterInput is not overflowing.
marinaaisa
force-pushed
the
r129588759/scroll-container
branch
from
07b81dc
to
6924446
Compare
Bug/issue #129588759, if applicable:
Summary
Update scroll lock logic to lock everything but disabled targets
Dependencies
NA
Testing
Steps:
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
npm test, and it succeeded