chore(picker): added a new interaction strategy #4396

TarunAdobe · 2024-05-03T10:05:11Z

Abstracted a new interaction strategy for Picker. This will allow our picker to be stateless and handle all the interactions through a separate mobile and desktop controllers instead.

Related issue(s)

[Bug]: Picker in mobile that opens under the options that will be displayed will automatically close (make a selection) #4227
[Bug]: (iOS) sp-card (supplied with an actions slot) action menu opens tray and on menu item selection in tray, if other card is behind it registers click on it as well. #4461
[Bug]: sp-tray retracts automatically when opened with less items in iOS simulator #4572
[Bug]: Multiple change events dispatched from ActionMenu tray in mobile device #4459

Motivation and context

How has this been tested?

Test 1 (Desktop)
1. Go here
2. click on the picker button
3. verify that the picker opens
4. click on the picker button again
5. verify that the picker is closed
Test 2 (Desktop)
1. Go here
2. longpress on the picker button and make an item selection by dragging the mouse to the menu-item and releasing the pointer
3. verify the picker opened and actually made a selection before closing.
Test 3 (Mobile)
1. Go here.
2. Click on the action-menu button and verify a tray opens up.
3. Select a menu-item from the tray and the tray should close after making a selection.

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.

github-actions · 2024-05-03T10:07:27Z

Branch preview

Visual regression test results

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

github-actions · 2024-05-03T10:26:43Z

Tachometer results

Chrome

action-menu permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	647 kB	128.58ms - 131.58ms	-	faster ✔ 9% - 12% 13.28ms - 17.70ms
branch	638 kB	143.96ms - 147.20ms	slower ❌ 10% - 14% 13.28ms - 17.70ms	-

test-directive permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	605 kB	60.41ms - 61.57ms	-	faster ✔ 12% - 14% 8.11ms - 10.09ms
branch	595 kB	69.29ms - 70.89ms	slower ❌ 13% - 17% 8.11ms - 10.09ms	-

test-lazy permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	604 kB	58.17ms - 59.44ms	-	faster ✔ 11% - 14% 7.61ms - 9.39ms
branch	595 kB	66.69ms - 67.92ms	slower ❌ 13% - 16% 7.61ms - 9.39ms	-

test-open-close-directive permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	791 kB	1872.29ms - 1875.09ms	-	unsure 🔍 -0% - -0% -8.35ms - -4.38ms
branch	781 kB	1878.65ms - 1881.46ms	unsure 🔍 +0% - +0% +4.38ms - +8.35ms	-

test-open-close permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	789 kB	1857.96ms - 1861.00ms	-	faster ✔ 1% - 1% 12.74ms - 17.10ms
branch	779 kB	1872.84ms - 1875.96ms	slower ❌ 1% - 1% 12.74ms - 17.10ms	-

combobox permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	711 kB	34.70ms - 35.18ms	-	faster ✔ 4% - 6% 1.47ms - 2.09ms
branch	698 kB	36.52ms - 36.91ms	slower ❌ 4% - 6% 1.47ms - 2.09ms	-

light-dom-test permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	712 kB	370.52ms - 376.13ms	-	faster ✔ 3% - 5% 12.21ms - 20.66ms
branch	698 kB	386.59ms - 392.92ms	slower ❌ 3% - 6% 12.21ms - 20.66ms	-

menu permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	476 kB	199.79ms - 204.24ms	-	faster ✔ 2% - 4% 3.93ms - 9.10ms
branch	463 kB	207.22ms - 209.85ms	slower ❌ 2% - 5% 3.93ms - 9.10ms	-

picker permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	514 kB	501.72ms - 511.31ms	-	faster ✔ 2% - 4% 10.16ms - 23.35ms
branch	504 kB	518.74ms - 527.80ms	slower ❌ 2% - 5% 10.16ms - 23.35ms	-

split-button permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	724 kB	1858.62ms - 1862.39ms	-	unsure 🔍 -0% - -0% -5.84ms - -0.20ms
branch	714 kB	1861.42ms - 1865.62ms	unsure 🔍 +0% - +0% +0.20ms - +5.84ms	-

Firefox

action-menu permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	647 kB	272.83ms - 275.13ms	-	faster ✔ 18% - 20% 59.44ms - 66.40ms
branch	638 kB	333.62ms - 340.18ms	slower ❌ 22% - 24% 59.44ms - 66.40ms	-

test-directive permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	605 kB	135.58ms - 140.02ms	-	unsure 🔍 -0% - +4% -0.21ms - +5.29ms
branch	595 kB	133.64ms - 136.88ms	unsure 🔍 -4% - +0% -5.29ms - +0.21ms	-

test-lazy permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	604 kB	152.33ms - 163.63ms	-	slower ❌ 9% - 18% 12.62ms - 25.02ms
branch	595 kB	136.62ms - 141.70ms	faster ✔ 8% - 15% 12.62ms - 25.02ms	-

test-open-close-directive permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	791 kB	1891.88ms - 1902.24ms	-	unsure 🔍 -1% - +0% -10.40ms - +0.60ms
branch	781 kB	1900.09ms - 1903.83ms	unsure 🔍 -0% - +1% -0.60ms - +10.40ms	-

test-open-close permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	789 kB	1890.24ms - 1899.60ms	-	faster ✔ 0% - 1% 5.56ms - 19.72ms
branch	779 kB	1902.25ms - 1912.87ms	slower ❌ 0% - 1% 5.56ms - 19.72ms	-

combobox permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	711 kB	60.65ms - 63.87ms	-	unsure 🔍 -4% - +2% -2.59ms - +1.23ms
branch	698 kB	61.92ms - 63.96ms	unsure 🔍 -2% - +4% -1.23ms - +2.59ms	-

light-dom-test permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	712 kB	760.48ms - 778.72ms	-	slower ❌ 3% - 7% 20.97ms - 48.71ms
branch	698 kB	724.31ms - 745.21ms	faster ✔ 3% - 6% 20.97ms - 48.71ms	-

menu permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	476 kB	447.00ms - 461.24ms	-	unsure 🔍 -5% - +0% -21.90ms - +0.54ms
branch	463 kB	456.13ms - 473.47ms	unsure 🔍 -0% - +5% -0.54ms - +21.90ms	-

picker permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	514 kB	1032.04ms - 1044.80ms	-	faster ✔ 5% - 8% 51.04ms - 91.88ms
branch	504 kB	1090.49ms - 1129.27ms	slower ❌ 5% - 9% 51.04ms - 91.88ms	-

split-button permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	724 kB	1874.75ms - 1878.77ms	-	unsure 🔍 -0% - +0% -3.10ms - +3.90ms
branch	714 kB	1873.50ms - 1879.22ms	unsure 🔍 -0% - +0% -3.90ms - +3.10ms	-

github-actions · 2024-05-23T07:14:38Z

Lighthouse scores

Category	Latest (report)	Main (report)	Branch (report)
Performance	0.99	0.99	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	224.346 kB	211.872 kB 🏆	212.151 kB
Scripts	56.226 kB	49.642 kB 🏆	49.752 kB
Stylesheet	34.96 kB	30.291 kB 🏆	30.446 kB
Document	6.15 kB	5.326 kB 🏆	5.338 kB
Font	127.01 kB	126.613 kB 🏆	126.615 kB

Request Count

Category	Latest	Main	Branch
Total	48	48	48
Scripts	40	40	40
Stylesheet	5	5	5
Document	1	1	1
Font	2	2	2

TarunAdobe · 2024-06-03T07:44:47Z

@spdev3000 Need your eyes on these failing tests as I'm trying to separate out the interaction strategy from the sp-picker component.

spdev3000 · 2024-06-03T10:46:26Z

I can only see that in the Storybook the Picker opens twice after the first loading and usage.
So first trigger seems correctly open the picker, but on sp-close of the overlay, maybe there is something not correctly reseted.
After that if I click again on the picker, it opens correctly, but as soon as I release the pointer on the picker it re-opens again.

Can you also pls add 2 new stories to the tests, testing each (desktop and mobile) use case? Is this somehow possible with test framework?

Rajdeepc · 2024-06-05T03:17:12Z

I can only see that in the Storybook the Picker opens twice after the first loading and usage. So first trigger seems correctly open the picker, but on sp-close of the overlay, maybe there is something not correctly reseted. After that if I click again on the picker, it opens correctly, but as soon as I release the pointer on the picker it re-opens again.

Can you also pls add 2 new stories to the tests, testing each (desktop and mobile) use case? Is this somehow possible with test framework?

@spdev3000 ,

It appears that the issue may be caused by a race condition related to the open setter, which is affecting the closure of the picker component. The bug likely stems from how the overlay reference is created whenever it isn't already instantiated. Currently, I haven't found an effective method to reset the overlay when the sp-closed event is triggered. Additionally, some parts of the code seem to be introducing delays in the picker’s closing state.
the bug gets created here.

Could you please perform a thorough review of this branch on your local setup to help identify and resolve the issue?

Thank you!

spdev3000 · 2024-06-11T12:36:09Z

I debugged into the issue and as far as I can see the problem seems to be that at second (and any further) calls to set open to overlay immediately closes (and reopens again). This seems to be, that the function initOverlay with
this.overlay.willPreventClose = this.preventNextToggle !== 'no' && this.open; is just called on first open.
This could be solved by adding this into

spectrum-web-components/packages/picker/src/InteractionController.ts

Line 54 in 28a86be

this.overlay.open = open;

so that function could look like:

/**
     * Set `open`
     */
    public set open(open: boolean) {
        if (this._open === open) return;
        this._open = open;
        if (this.overlay) {
            // If there already is an Overlay, apply the value of `open` directly.
            if (this.host.dependencyManager.loaded) {
                this.overlay.willPreventClose = this.preventNextToggle !== 'no';
                this.overlay.open = open;
            }
            this.host.open = open;
            return;
        }
        if (!open) {
        ...

Rajdeepc

LGTM!!

nikkimk

lgtm

* chore: testing a click controller strategy for picker component * chore: testing a new interaction strategy for picker * fix: separated interaction strategy for picker * chore: updated the strategy to include the dependency controller * chore: updated open condition in interactioncontroller * chore(picker): removed double event handling from action menu * chore: added slottable-request handling in interactionController * chore: change trigger button for split-button * chore: updated split-button test * chore: fixed conflicts after merging main * chore: updated action-group test to remove flakiness * chore: refractored interaction controller code * chore: added delay for click event tests * chore: added hacky mobile tests for mobilecontroller picker * chore: removed unexpected only from picker responsive test * chore: updated menu interaction model * chore: don't dispatch synthetic click event on menu selection * chore: removed split-button tests cz its deprecated --------- Co-authored-by: Michael Jordan <mijordan@adobe.com>

TarunAdobe force-pushed the bug/picker-auto-slection branch from a5d671b to 106aa6c Compare May 23, 2024 06:53

TarunAdobe changed the title ~~[DRAFT]: Testing an Interaction Controller Strategy for Picker Component~~ chore: Adding a Interaction Controller Strategy for Picker Component May 23, 2024

TarunAdobe changed the title ~~chore: Adding a Interaction Controller Strategy for Picker Component~~ chore: Adding an Interaction Controller Strategy for Picker Component May 23, 2024

TarunAdobe marked this pull request as ready for review May 23, 2024 06:55

TarunAdobe self-assigned this May 23, 2024

TarunAdobe added the Component: Picker label May 23, 2024

TarunAdobe force-pushed the bug/picker-auto-slection branch from 106aa6c to 757b90e Compare May 23, 2024 07:10

TarunAdobe changed the title ~~chore: Adding an Interaction Controller Strategy for Picker Component~~ chore(picker): added a new interaction strategy May 28, 2024

TarunAdobe requested a review from a team May 28, 2024 05:39

TarunAdobe force-pushed the bug/picker-auto-slection branch from 814916d to 28a86be Compare May 30, 2024 12:50

TarunAdobe requested a review from spdev3000 June 3, 2024 07:43

TarunAdobe added 5 commits June 25, 2024 10:34

chore: testing a click controller strategy for picker component

b9ab321

chore: testing a new interaction strategy for picker

f350d1a

fix: separated interaction strategy for picker

1b83611

chore: updated the strategy to include the dependency controller

1290ba7

chore: updated open condition in interactioncontroller

f3e64e9

TarunAdobe force-pushed the bug/picker-auto-slection branch from c895143 to f3e64e9 Compare June 25, 2024 05:04

chore(picker): removed double event handling from action menu

f61dc96

TarunAdobe force-pushed the bug/picker-auto-slection branch from 68641bd to aaef850 Compare July 8, 2024 08:39

chore: added slottable-request handling in interactionController

b163d0c

TarunAdobe force-pushed the bug/picker-auto-slection branch from aaef850 to b163d0c Compare July 8, 2024 08:44

TarunAdobe added 2 commits July 8, 2024 16:59

chore: change trigger button for split-button

4fee105

chore: updated split-button test

88a52d8

chore: removed unexpected only from picker responsive test

344344e

TarunAdobe requested review from rubencarvalho, Rajdeepc, a team and nikkimk July 24, 2024 09:52

Rajdeepc added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Jul 24, 2024

Rajdeepc previously approved these changes Jul 24, 2024

View reviewed changes

nikkimk previously approved these changes Jul 24, 2024

View reviewed changes

rubencarvalho previously approved these changes Jul 24, 2024

View reviewed changes

TarunAdobe dismissed stale reviews from rubencarvalho, nikkimk, and Rajdeepc via bcc18dd July 25, 2024 05:50

chore: updated menu interaction model

7f051a3

TarunAdobe force-pushed the bug/picker-auto-slection branch from bcc18dd to 7f051a3 Compare July 25, 2024 06:28

TarunAdobe requested review from rubencarvalho and Rajdeepc July 25, 2024 06:31

TarunAdobe added 2 commits July 25, 2024 18:35

chore: don't dispatch synthetic click event on menu selection

6b214b8

chore: removed split-button tests cz its deprecated

fee37bd

Rajdeepc approved these changes Jul 29, 2024

View reviewed changes

Rajdeepc merged commit 56366ce into main Jul 29, 2024
58 checks passed

Rajdeepc deleted the bug/picker-auto-slection branch July 29, 2024 09:21

chore(picker): added a new interaction strategy #4396

chore(picker): added a new interaction strategy #4396

Conversation

TarunAdobe commented May 3, 2024 • edited Loading

Related issue(s)

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

github-actions bot commented May 3, 2024 • edited Loading

Branch preview

github-actions bot commented May 3, 2024 • edited Loading

Tachometer results

action-menu permalink

test-basic

test-directive permalink

test-lazy permalink

test-open-close-directive permalink

test-open-close permalink

combobox permalink

basic-test

light-dom-test permalink

menu permalink

test-basic

picker permalink

basic-test

split-button permalink

basic-test

action-menu permalink

test-basic

test-directive permalink

test-lazy permalink

test-open-close-directive permalink

test-open-close permalink

combobox permalink

basic-test

light-dom-test permalink

menu permalink

test-basic

picker permalink

basic-test

split-button permalink

basic-test

github-actions bot commented May 23, 2024 • edited Loading

Lighthouse scores

Transfer Size

Request Count

TarunAdobe commented Jun 3, 2024

spdev3000 commented Jun 3, 2024

Rajdeepc commented Jun 5, 2024

spdev3000 commented Jun 11, 2024

Rajdeepc left a comment

Choose a reason for hiding this comment

nikkimk left a comment

Choose a reason for hiding this comment

TarunAdobe commented May 3, 2024 •

edited

Loading

github-actions bot commented May 3, 2024 •

edited

Loading

github-actions bot commented May 3, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading