fix(picker): stop the click events from reaching the elements below picker-tray #5060

TarunAdobe · 2025-02-03T08:48:29Z

Description

Stop the click events to reach the element below tray when we open a picker in iOS <17

Related issue(s)

fixes [Bug]: (iOS) Tapping on a button in the sp-menu will also tap on a button directly underneath it #5042

Motivation and context

Prevents the event leaking out of the dialog to the element below it.
This is a workaround for a bug in iOS <17 where the event leaks out of the dialog to the element below it.

How has this been tested?

Go here in iOS<17
- Open the picker and make a selection on the tray.
- There should not be any console log from the button getting clicked.
Manually Tested. See the attached Video below for before and After:

Before

screen-capture.webm

After

screen-capture.1.webm

Did it pass in Desktop?
Did it pass in Mobile?
Did it pass in iPad?

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
If my change required a change to the documentation, I have updated the documentation in this pull request.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

changeset-bot · 2025-02-03T08:48:35Z

⚠️ No Changeset found

Latest commit: a413ae8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2025-02-03T08:50:39Z

Branch preview

Review the following VRT differences

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

TarunAdobe · 2025-02-03T08:50:55Z

Since this issue happens only in specific version(s) of iOS combined with the fact that we don't have a good testing infrastructure setup for mobile - I couldn't figure out a way to write tests for this which won't pass always. So I attached a before and after video in the description.
Apart from this I did test and verified manually as well as through our tests that everything else related to the menu and picker behaviour remains exactly the same!

github-actions · 2025-02-03T08:52:56Z

Tachometer results

Currently, no packages are changed by this PR...

github-actions · 2025-02-03T08:54:56Z

Lighthouse scores

Category	Latest (report)	Main (report)	Branch (report)
Performance	0.99	0.98	0.99
Accessibility	1	1	1
Best Practices	1	1	1
SEO	1	0.92	0.92
PWA	1	1	1

What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category	Latest	Main	Branch
Total	250.968 kB	236.237 kB	236.211 kB 🏆
Scripts	61.651 kB	54.401 kB 🏆	54.418 kB
Stylesheet	53.247 kB	47.353 kB 🏆	47.357 kB
Document	6.239 kB	5.468 kB	5.457 kB 🏆
Font	126.892 kB	126.605 kB	126.604 kB 🏆

Request Count

Category	Latest	Main	Branch
Total	52	52	52
Scripts	41	41	41
Stylesheet	5	5	5
Document	1	1	1
Font	2	2	2

coveralls · 2025-02-03T08:57:38Z

Pull Request Test Coverage Report for Build 13114709204

Details

36 of 36 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 98.192%

Totals
Change from base Build 13111336528:	0.002%
Covered Lines:	33099
Relevant Lines:	33528

💛 - Coveralls

rubencarvalho · 2025-02-03T10:13:34Z

packages/overlay/src/OverlayDialog.ts

+             * This is a workaround for a bug in iOS <17 where the event leaks out of the dialog to the element below it. 
+             * Issue - https://github.com/adobe/spectrum-web-components/issues/5042
+             */
+            this.dialogEl.addEventListener('pointerup', (event) => {


is pointerup enough? Have we considered adding touchend?

Either of the two pointerup or touchend was working fine and similar to the other one in simulators... Didn't test it on a real device. Should I stop the propagation for both?

I’m wondering if there’s any downside to having touchend as a “safe fallback”... I don’t think it should be necessary, but without real device testing, it is hard for me to say 🤷

I don't particularly see any other downside than the fact that in the worst case we're adding 1 extra event listener....so

rubencarvalho · 2025-02-03T10:15:07Z

packages/overlay/src/OverlayDialog.ts

Does OverlayDialog only get instantiated once, or is there a chance that the dialog may be re-instantiated? If so, should we define the event handler as a class method and attach/detach it?

Yess! Thanks for the catch. Totally slipped my mind that we need to avoid attaching the event listener multiple times and clean it properly. Will update the code to do that.
I was thinking of having the logic of attaching and removing the listeners abstracted out and then will call that function to attach the listener when the dialog opens and remove it when it closes?

rubencarvalho

LGTM 🚢

caseyisonit · 2025-02-03T22:46:37Z

@TarunAdobe should we add in a test to confirm this behavior? or is this a device emulation issue?

TarunAdobe · 2025-02-04T07:26:11Z

@TarunAdobe should we add in a test to confirm this behavior? or is this a device emulation issue?

I made a comment on this PR abt this that got lost between all the bot comments but TLDR; Since this issue happens only in specific version(s) of iOS combined with the fact that we don't have a good testing infrastructure setup for mobile - I couldn't figure out a way to write tests for this which always end up passing. And so it'd be useless :(

TarunAdobe requested a review from a team as a code owner February 3, 2025 08:48

TarunAdobe linked an issue Feb 3, 2025 that may be closed by this pull request

[Bug]: (iOS) Tapping on a button in the sp-menu will also tap on a button directly underneath it #5042

Closed

1 task

TarunAdobe self-assigned this Feb 3, 2025

TarunAdobe changed the title ~~fix(picker): stop the click events to reach the element below tray~~ fix(picker): stop the click events from reaching the elements below picker-tray Feb 3, 2025

rubencarvalho reviewed Feb 3, 2025

View reviewed changes

TarunAdobe force-pushed the picker/background-click branch from 3c8deae to 42c8612 Compare February 3, 2025 11:11

fix(picker): stop the click events to reach the element below tray

272fc0b

TarunAdobe force-pushed the picker/background-click branch from 42c8612 to 272fc0b Compare February 3, 2025 11:13

TarunAdobe requested a review from rubencarvalho February 3, 2025 11:13

chore(picker): add story to test the background click

a413ae8

TarunAdobe force-pushed the picker/background-click branch from 4d07c4f to a413ae8 Compare February 3, 2025 13:22

Merge branch 'main' into picker/background-click

e11418a

rubencarvalho approved these changes Feb 3, 2025

View reviewed changes

TarunAdobe merged commit 7e4fdbf into main Feb 4, 2025
23 of 27 checks passed

TarunAdobe deleted the picker/background-click branch February 4, 2025 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(picker): stop the click events from reaching the elements below picker-tray #5060

fix(picker): stop the click events from reaching the elements below picker-tray #5060

TarunAdobe commented Feb 3, 2025 •

edited by caseyisonit

Loading

changeset-bot bot commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025

TarunAdobe commented Feb 3, 2025

github-actions bot commented Feb 3, 2025

github-actions bot commented Feb 3, 2025 •

edited

Loading

coveralls commented Feb 3, 2025 •

edited

Loading

rubencarvalho Feb 3, 2025

TarunAdobe Feb 3, 2025

rubencarvalho Feb 3, 2025

TarunAdobe Feb 3, 2025

rubencarvalho Feb 3, 2025

TarunAdobe Feb 3, 2025

rubencarvalho left a comment

caseyisonit commented Feb 3, 2025 •

edited

Loading

TarunAdobe commented Feb 4, 2025

fix(picker): stop the click events from reaching the elements below picker-tray #5060

fix(picker): stop the click events from reaching the elements below picker-tray #5060

Conversation

TarunAdobe commented Feb 3, 2025 • edited by caseyisonit Loading

Description

Related issue(s)

Motivation and context

How has this been tested?

Before

After

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

changeset-bot bot commented Feb 3, 2025 • edited Loading

⚠️ No Changeset found

github-actions bot commented Feb 3, 2025

Branch preview

Review the following VRT differences

TarunAdobe commented Feb 3, 2025

github-actions bot commented Feb 3, 2025

Tachometer results

github-actions bot commented Feb 3, 2025 • edited Loading

Lighthouse scores

Transfer Size

Request Count

coveralls commented Feb 3, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13114709204

Details

💛 - Coveralls

rubencarvalho Feb 3, 2025

Choose a reason for hiding this comment

TarunAdobe Feb 3, 2025

Choose a reason for hiding this comment

rubencarvalho Feb 3, 2025

Choose a reason for hiding this comment

TarunAdobe Feb 3, 2025

Choose a reason for hiding this comment

rubencarvalho Feb 3, 2025

Choose a reason for hiding this comment

TarunAdobe Feb 3, 2025

Choose a reason for hiding this comment

rubencarvalho left a comment

Choose a reason for hiding this comment

caseyisonit commented Feb 3, 2025 • edited Loading

TarunAdobe commented Feb 4, 2025

TarunAdobe commented Feb 3, 2025 •

edited by caseyisonit

Loading

changeset-bot bot commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

coveralls commented Feb 3, 2025 •

edited

Loading

caseyisonit commented Feb 3, 2025 •

edited

Loading