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.

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

Update scroll lock logic to lock everything but disabled targets #858

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
5 changes: 4 additions & 1 deletion src/components/Filter/FilterInput.vue
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
</button>
<div
:class="['filter__input-box-wrapper', { 'scrolling': isScrolling }]"
v-bind="{[SCROLL_LOCK_DISABLE_ATTR]: true}"
@scroll="handleScroll"
>
<TagList
Expand Down Expand Up @@ -106,7 +107,7 @@
</div>
</div>
<TagList
v-if="displaySuggestedTags"
v-show="displaySuggestedTags"
:id="SuggestedTagsId"
ref="suggestedTags"
:ariaLabel="$tc('filter.suggested-tags', suggestedTags.length)"
Copy link
Contributor

@hqhhuang hqhhuang Jun 14, 2024

Choose a reason for hiding this comment

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

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.

Expand All @@ -128,6 +129,7 @@
import ClearRoundedIcon from 'theme/components/Icons/ClearRoundedIcon.vue';
import multipleSelection from 'docc-render/mixins/multipleSelection';
import handleScrollbar from 'docc-render/mixins/handleScrollbar';
import { SCROLL_LOCK_DISABLE_ATTR } from 'docc-render/utils/scroll-lock';
import FilterIcon from 'theme/components/Icons/FilterIcon.vue';
import TagList from './TagList.vue';

Expand Down Expand Up @@ -225,6 +227,7 @@ export default {
SuggestedTagsId,
AXinputProperties,
showSuggestedTags: false,
SCROLL_LOCK_DISABLE_ATTR,
};
},
computed: {
Expand Down
7 changes: 7 additions & 0 deletions src/components/Filter/TagList.vue
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
role="listbox"
:aria-multiselectable="areTagsRemovable ? 'true' : 'false'"
aria-orientation="horizontal"
v-bind="{[SCROLL_LOCK_DISABLE_ATTR]: true}"
@keydown.left.capture.prevent="focusPrev"
@keydown.right.capture.prevent="focusNext"
@keydown.up.capture.prevent="focusPrev"
Expand Down Expand Up @@ -61,6 +62,7 @@
import { isSingleCharacter } from 'docc-render/utils/input-helper';
import handleScrollbar from 'docc-render/mixins/handleScrollbar';
import keyboardNavigation from 'docc-render/mixins/keyboardNavigation';
import { SCROLL_LOCK_DISABLE_ATTR } from 'docc-render/utils/scroll-lock';
import Tag from './Tag.vue';

export default {
Expand All @@ -69,6 +71,11 @@ export default {
handleScrollbar,
keyboardNavigation,
],
data() {
return {
SCROLL_LOCK_DISABLE_ATTR,
};
},
props: {
tags: {
type: Array,
Expand Down
34 changes: 25 additions & 9 deletions src/utils/scroll-lock.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

let isLocked = false;
let initialClientY = -1;
let initialClientX = -1;
let scrolledClientY = 0;
// Adds this attribute to an inner scrollable element to allow it to scroll
export const SCROLL_LOCK_DISABLE_ATTR = 'data-scroll-lock-disable';
Expand Down Expand Up @@ -73,23 +74,30 @@ function advancedUnlock(targetElement) {
document.removeEventListener('touchmove', preventDefault);
}

const isVerticalScroll = ({ scrollHeight, scrollWidth }) => scrollHeight > scrollWidth;

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

/**
* Handles the scrolling of the targetElement
* @param {TouchEvent} event
* @param {HTMLElement} targetElement
* @return {boolean}
*/
function handleScroll(event, targetElement) {
function handleScroll(event, target) {
const clientY = event.targetTouches[0].clientY - initialClientY;
// check if any parent has a scroll-lock disable, if not use the targetElement
const target = event.target.closest(`[${SCROLL_LOCK_DISABLE_ATTR}]`) || targetElement;
if (target.scrollTop === 0 && clientY > 0) {
// element is at the top of its scroll.
return preventDefault(event);
}
const clientX = event.targetTouches[0].clientX - initialClientX;

if (isTargetElementTotallyScrolled(target) && clientY < 0) {
// element is at the bottom of its scroll.
if (isVerticalScroll(target)) {
if (target.scrollTop === 0 && clientY > 0) {
// element is at the top of its scroll.
return preventDefault(event);
}

if (isTargetElementTotallyScrolled(target) && clientY < 0) {
// element is at the bottom of its scroll.
return preventDefault(event);
}
} else if (Math.abs(clientY) > Math.abs(clientX)) {
// prevent event if user tries to perform vertical scroll in an horizontal scrolling element
Comment on lines +99 to +100
Copy link
Contributor

@hqhhuang hqhhuang Jun 13, 2024

Choose a reason for hiding this comment

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

So smart :PP
Is this how WebKit determines what's a vertical scroll?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

return preventDefault(event);
}

Expand All @@ -112,6 +120,7 @@ function advancedLock(targetElement) {
if (event.targetTouches.length === 1) {
// detect single touch.
initialClientY = event.targetTouches[0].clientY;
initialClientX = event.targetTouches[0].clientX;
}
};
targetElement.ontouchmove = (event) => {
Expand All @@ -138,7 +147,11 @@ export default {
if (!isIosDevice()) {
simpleLock();
} else {
// lock everything but target element
advancedLock(targetElement);
// lock everything but disabled targets
const disabledTargets = document.querySelectorAll(`[${SCROLL_LOCK_DISABLE_ATTR}]`);
disabledTargets.forEach(target => advancedLock(target));
}
isLocked = true;
},
Expand All @@ -152,6 +165,9 @@ export default {
if (isIosDevice()) {
// revert the old scroll position
advancedUnlock(targetElement);
// revert the old scroll position for disabled targets
const disabledTargets = document.querySelectorAll(`[${SCROLL_LOCK_DISABLE_ATTR}]`);
disabledTargets.forEach(target => advancedUnlock(target));
} else {
// remove all inline styles added by the `simpleLock` function
document.body.style.removeProperty('overflow');
Expand Down
12 changes: 6 additions & 6 deletions tests/unit/components/Filter/FilterInput.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ describe('FilterInput', () => {
});
expect(selectedTags.props('activeTags')).toEqual(tags);
// assert we tried to ficus first item
expect(spy).toHaveBeenCalledWith(0);
expect(spy).not.toHaveBeenCalled();
});

it('on paste, handles clipboard in HTML format, when copying and pasting from search directly', () => {
Expand Down Expand Up @@ -563,7 +563,7 @@ describe('FilterInput', () => {
await wrapper.vm.$nextTick();
// first time was `true`, from `focus`, then `blur` made it `false`
expect(wrapper.emitted('show-suggested-tags')).toEqual([[true], [false]]);
expect(suggestedTags.exists()).toBe(false);
expect(suggestedTags.attributes('style')).toContain('display: none');
});

it('deletes `suggestedTags` component when `deleteButton` looses its focus on an external component', async () => {
Expand All @@ -576,7 +576,7 @@ describe('FilterInput', () => {
});
await wrapper.vm.$nextTick();
expect(wrapper.emitted('show-suggested-tags')).toEqual([[true], [false]]);
expect(suggestedTags.exists()).toBe(false);
expect(suggestedTags.attributes('style')).toContain('display: none');
});

it('does not hide the tags, if the new focus target matches `input, button`, inside the main component', async () => {
Expand All @@ -598,7 +598,7 @@ describe('FilterInput', () => {

await flushPromises();

expect(suggestedTags.exists()).toBe(false);
expect(suggestedTags.attributes('style')).toContain('display: none');
expect(wrapper.emitted('show-suggested-tags')).toEqual([[true], [false]]);
});

Expand All @@ -613,7 +613,7 @@ describe('FilterInput', () => {
});

await wrapper.vm.$nextTick();
expect(suggestedTags.exists()).toBe(false);
expect(suggestedTags.attributes('style')).toContain('display: none');
expect(wrapper.emitted('show-suggested-tags')).toEqual([[true], [false]]);
expect(wrapper.emitted('blur')).toBeTruthy();
});
Expand All @@ -637,7 +637,7 @@ describe('FilterInput', () => {
});

it('deletes `suggestedTags` component when no tags available', () => {
expect(suggestedTags.exists()).toBe(false);
expect(suggestedTags.attributes('style')).toContain('display: none');
});

it('does not render `deleteButton` when there are no tags and `input` is empty', () => {
Expand Down
59 changes: 39 additions & 20 deletions tests/unit/utils/scroll-lock.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@ describe('scroll-lock', () => {
DOM = parseHTMLString(`
<div class="container">
<div class="scrollable">long</div>
<div ${SCROLL_LOCK_DISABLE_ATTR}="true" class="disabled-target"></div>
</div>
`);
document.body.appendChild(DOM);
container = DOM.querySelector('.container');
Object.defineProperty(container, 'scrollHeight', { value: 10, writable: true });
});
afterEach(() => {
window.navigator.platform = platform;
Expand Down Expand Up @@ -70,12 +72,7 @@ describe('scroll-lock', () => {
container.ontouchmove(touchMoveEvent);
expect(preventDefault).toHaveBeenCalledTimes(1);
expect(stopPropagation).toHaveBeenCalledTimes(0);
expect(touchMoveEvent.target.closest).toHaveBeenCalledTimes(1);
expect(touchMoveEvent.target.closest).toHaveBeenCalledWith(`[${SCROLL_LOCK_DISABLE_ATTR}]`);

// simulate scroll middle
// simulate we have enough to scroll
Object.defineProperty(container, 'scrollHeight', { value: 10, writable: true });
container.ontouchmove({ ...touchMoveEvent, targetTouches: [{ clientY: -10 }] });
expect(preventDefault).toHaveBeenCalledTimes(1);
expect(stopPropagation).toHaveBeenCalledTimes(1);
Expand All @@ -86,26 +83,48 @@ describe('scroll-lock', () => {
expect(preventDefault).toHaveBeenCalledTimes(2);
expect(stopPropagation).toHaveBeenCalledTimes(1);

// simulate there is a scroll-lock-disable target
container.ontouchmove({
...touchMoveEvent,
targetTouches: [{ clientY: -10 }],
target: {
closest: jest.fn().mockReturnValue({
...container,
clientHeight: 150,
}),
},
});
// assert scrolling was allowed
expect(preventDefault).toHaveBeenCalledTimes(2);
expect(stopPropagation).toHaveBeenCalledTimes(2);

scrollLock.unlockScroll(container);
expect(container.ontouchmove).toBeFalsy();
expect(container.ontouchstart).toBeFalsy();
});

it('adds event listeners to the disabled targets too', () => {
const disabledTarget = DOM.querySelector('.disabled-target');
// init the scroll lock
scrollLock.lockScroll(container);
// assert event listeners are attached
expect(disabledTarget.ontouchstart).toEqual(expect.any(Function));
expect(disabledTarget.ontouchmove).toEqual(expect.any(Function));

scrollLock.unlockScroll(container);
expect(disabledTarget.ontouchmove).toBeFalsy();
expect(disabledTarget.ontouchstart).toBeFalsy();
});

it('prevent event if user tries to perform vertical scroll in an horizontal scrolling element', () => {
Object.defineProperty(container, 'scrollWidth', { value: 100, writable: true });

const touchStartEvent = {
targetTouches: [{ clientY: 0, clientX: 0 }],
};
const touchMoveEvent = {
targetTouches: [{ clientY: -10, clientX: 0 }],
preventDefault,
stopPropagation,
touches: [1],
target: {
closest: jest.fn(),
},
};

scrollLock.lockScroll(container);
container.ontouchstart(touchStartEvent);
container.ontouchmove(touchMoveEvent);

expect(preventDefault).toHaveBeenCalledTimes(1);
expect(stopPropagation).toHaveBeenCalledTimes(0);
});

it('prevents body scrolling', () => {
scrollLock.lockScroll(container);
// assert body scroll is getting prevented when swiping up/down
Expand Down