fix(menu): iPad scrolling issue #5616
Conversation
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
🦋 Changeset detectedLatest commit: decc0c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| private touchStartY = 0; | ||
| private touchStartTime = 0; | ||
| private isScrolling = false; | ||
| private scrollThreshold = 10; // pixels |
There was a problem hiding this comment.
Regarding scrollThreshold and scrollTimeThreshold: It's great that this addresses the iPad scrolling issue. Could you provide a bit more context on how the scrollThreshold and scrollTimeThreshold values were determined? Are they based on empirical testing, or are there any common best practices for these values that could be referenced? Just want to ensure they're robust enough to cover various scroll speeds and nuances on iPad.
There was a problem hiding this comment.
Yes the 10px and 300ms values were determined through empirical testing on various iPad models. One thing we can do is to make these values configurable but I don't think that is something we can do if we get reports of lags or jitter. Tested this bug in iPad Pro, Air, iPad mini in simulator should cover the most of the blast radius of this issue.
There was a problem hiding this comment.
In the code comments, we need more of an explanation about what this logic does and why it works, so that we avoid future questions on it and regressions. I can't imagine someone else supporting this a year down the road and knowing exactly how to fix it.
packages/menu/src/Menu.ts
Outdated
| this.addEventListener('touchmove', this.handleTouchMove, { | ||
| passive: true, | ||
| }); | ||
| this.addEventListener('touchend', this.handleTouchEnd, { |
There was a problem hiding this comment.
there is already a touchend event listener above this. should that be removed in favor of this?
packages/menu/src/Menu.ts
Outdated
| @@ -925,6 +980,14 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { | |||
| } | |||
|
|
|||
| public override disconnectedCallback(): void { | |||
| // Clean up touch event listeners to prevent memory leaks | |||
There was a problem hiding this comment.
why are we removing these and not the others?
There was a problem hiding this comment.
If we only want these event listeners to exist while attached to the DOM and to remove them during the disconnected phase of the lifecycle, then we should have been adding them on the connected part of the lifecycle instead of the constructor.
Right now, if you were to remove the picker from the DOM and add it back, the listeners would not be re-applied.
packages/menu/test/menu.test.ts
Outdated
| await elementUpdated(el); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const menu = el as any; |
There was a problem hiding this comment.
we should avoid using the any type, I believe this should be Menu base on the signature above
packages/menu/test/menu.test.ts
Outdated
| menu.isScrolling = true; | ||
|
|
||
| // Try to select an item while scrolling | ||
| const clickEvent = new MouseEvent('click', { button: 0 }); |
There was a problem hiding this comment.
this is using a mouse event in an anticipated touch experience, I don't this this is the correct check we want.
packages/menu/test/menu.test.ts
Outdated
| // Wait for the selection to be processed | ||
| await elementUpdated(el); | ||
| await elementUpdated(firstItem); | ||
| await nextFrame(); |
There was a problem hiding this comment.
this shouldnt be needed after an elementUpdated returns. Can you explain why this is needed?
packages/picker/test/index.ts
Outdated
|
|
||
| // Store initial value | ||
| const initialValue = el.value; | ||
| const touchStartEvent = new Event('touchstart', { |
There was a problem hiding this comment.
should we use oneEvent to stub the event so it doesn't introduce side effects?
packages/menu/test/menu.test.ts
Outdated
|
|
||
| it('prevents selection when scrolling is detected', async () => { | ||
| const el = await fixture<Menu>(html` | ||
| <sp-menu selects="single"> |
There was a problem hiding this comment.
there arent enough items to simulate scrolling, I'm wondering if these tests should be present in picker versus just a flat menu?
There was a problem hiding this comment.
For the tests in menu, I'm struggling to understand how its accurately testing scrolling with only two menu items and the menu isn't in a container. These tests feel a little false positive but would like to understand better how they are working.
I've left a number of comments and questions for you. :)
packages/menu/src/Menu.ts
Outdated
| @@ -925,6 +980,14 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { | |||
| } | |||
|
|
|||
| public override disconnectedCallback(): void { | |||
| // Clean up touch event listeners to prevent memory leaks | |||
There was a problem hiding this comment.
If we only want these event listeners to exist while attached to the DOM and to remove them during the disconnected phase of the lifecycle, then we should have been adding them on the connected part of the lifecycle instead of the constructor.
Right now, if you were to remove the picker from the DOM and add it back, the listeners would not be re-applied.
| private touchStartY = 0; | ||
| private touchStartTime = 0; | ||
| private isScrolling = false; | ||
| private scrollThreshold = 10; // pixels |
There was a problem hiding this comment.
In the code comments, we need more of an explanation about what this logic does and why it works, so that we avoid future questions on it and regressions. I can't imagine someone else supporting this a year down the road and knowing exactly how to fix it.
packages/menu/test/menu.test.ts
Outdated
| @@ -707,4 +707,249 @@ describe('Menu', () => { | |||
| await nextFrame(); | |||
| expect(el.selected).to.deep.equal(['3']); | |||
| }); | |||
|
|
|||
| it('prevents selection when scrolling is detected', async () => { | |||
There was a problem hiding this comment.
Why are these tests on menu when the issue is with Picker's dropdown?
Good point Casey, those were working since the mouse events are not trying to simulate a touch Event. I checked the repo and also the web-test-runner docs it lacks support for |
Rajdeepc
requested review from
caseyisonit,
nikkimk,
blunteshwar and
rubencarvalho
Rajdeepc
changed the title
|
@Rajdeepc Is there also a Jira ticket associated with this update? |
|
So i was learning more about the touch events because I haven't used them before and re-read the issue filed. The support request lists Chrome and Safari for the bug and I noticed in the API browser compatibility table touch events aren't supported in safari. I think this solution works for chrome but will need something else for safari. https://developer.mozilla.org/en-US/docs/Web/API/Touch_events#api.touchevent |
|
i just tried to reproduce the bugs on both chrome and safari on my iPhone with force-popover on from the production URL and I'm able to scroll and select items just fine with no jitter and thrashing. |
Summary
Fixes an issue where scrolling through menu items in a picker dropdown on iPad would accidentally select the first touched item and close the picker. This was caused by touch events being interpreted as selection events during scroll gestures.
Problem
When users attempted to scroll through menu items in a picker dropdown on iPad, the item they first touched when starting to scroll would be selected instead of allowing the scroll gesture to complete. This made the picker unusable on iPad devices.
Solution
Added touch event handling to distinguish between scroll gestures and selection gestures:
handlePointerBasedSelectionto prevent selection when scrolling is detectedTechnical details
touchStartY,touchStartTime,isScrollingproperties for scroll detectionhandleTouchStart,handleTouchMove,handleTouchEndmethodspassive: truefor touch event listeners to improve scrolling performancethreshold (10px)and timethreshold (300ms)for scroll detectionhandlePointerBasedSelectionto checkisScrollingstate before processing selectionRelated issue(s)
Screenshots (if appropriate)
Fix
picker-scrolling.mov
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
iPad Scroll test in Chrome, Mozilla, Safari
Device review