Skip to content

Commit 56366ce

Browse files
TarunAdobeMichael Jordan
TarunAdobe
and
Michael Jordan
authored
chore(picker): added a new interaction strategy (#4396)
* 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>
1 parent ec30b7f commit 56366ce

File tree

14 files changed

+598
-407
lines changed

14 files changed

+598
-407
lines changed

‎packages/action-menu/src/ActionMenu.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ export class ActionMenu extends ObserveSlotPresence(
6464
return this.slotContentIsPresent;
6565
}
6666

67-
protected override handleSlottableRequest = (
67+
public override handleSlottableRequest = (
6868
event: SlottableRequestEvent
6969
): void => {
7070
this.dispatchEvent(new SlottableRequestEvent(event.name, event.data));
@@ -113,8 +113,6 @@ export class ActionMenu extends ObserveSlotPresence(
113113
class="button"
114114
size=${this.size}
115115
@blur=${this.handleButtonBlur}
116-
@click=${this.handleActivate}
117-
@pointerdown=${this.handleButtonPointerdown}
118116
@focus=${this.handleButtonFocus}
119117
@keydown=${{
120118
handleEvent: this.handleEnterKeydown,

‎packages/action-menu/test/index.ts

Lines changed: 97 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -43,58 +43,50 @@ import { sendKeys } from '@web/test-runner-commands';
4343
ignoreResizeObserverLoopError(before, after);
4444

4545
const deprecatedActionMenuFixture = async (): Promise<ActionMenu> =>
46-
await fixture<ActionMenu>(
47-
html`
48-
<sp-action-menu label="More Actions">
49-
<sp-menu>
50-
<sp-menu-item>Deselect</sp-menu-item>
51-
<sp-menu-item>Select Inverse</sp-menu-item>
52-
<sp-menu-item>Feather...</sp-menu-item>
53-
<sp-menu-item>Select and Mask...</sp-menu-item>
54-
<sp-menu-divider></sp-menu-divider>
55-
<sp-menu-item>Save Selection</sp-menu-item>
56-
<sp-menu-item disabled>Make Work Path</sp-menu-item>
57-
</sp-menu>
58-
</sp-action-menu>
59-
`
60-
);
61-
62-
const actionMenuFixture = async (): Promise<ActionMenu> =>
63-
await fixture<ActionMenu>(
64-
html`
65-
<sp-action-menu label="More Actions">
46+
await fixture<ActionMenu>(html`
47+
<sp-action-menu label="More Actions">
48+
<sp-menu>
6649
<sp-menu-item>Deselect</sp-menu-item>
6750
<sp-menu-item>Select Inverse</sp-menu-item>
6851
<sp-menu-item>Feather...</sp-menu-item>
6952
<sp-menu-item>Select and Mask...</sp-menu-item>
7053
<sp-menu-divider></sp-menu-divider>
7154
<sp-menu-item>Save Selection</sp-menu-item>
7255
<sp-menu-item disabled>Make Work Path</sp-menu-item>
73-
</sp-action-menu>
74-
`
75-
);
56+
</sp-menu>
57+
</sp-action-menu>
58+
`);
59+
60+
const actionMenuFixture = async (): Promise<ActionMenu> =>
61+
await fixture<ActionMenu>(html`
62+
<sp-action-menu label="More Actions">
63+
<sp-menu-item>Deselect</sp-menu-item>
64+
<sp-menu-item>Select Inverse</sp-menu-item>
65+
<sp-menu-item>Feather...</sp-menu-item>
66+
<sp-menu-item>Select and Mask...</sp-menu-item>
67+
<sp-menu-divider></sp-menu-divider>
68+
<sp-menu-item>Save Selection</sp-menu-item>
69+
<sp-menu-item disabled>Make Work Path</sp-menu-item>
70+
</sp-action-menu>
71+
`);
7672

7773
const actionSubmenuFixture = async (): Promise<ActionMenu> =>
78-
await fixture<ActionMenu>(
79-
html`
80-
<sp-action-menu label="More Actions">
81-
<sp-menu-item>One</sp-menu-item>
82-
<sp-menu-item selected id="root-selected-item">
83-
Two
84-
</sp-menu-item>
85-
<sp-menu-item id="item-with-submenu">
86-
B should be selected
87-
<sp-menu slot="submenu">
88-
<sp-menu-item>A</sp-menu-item>
89-
<sp-menu-item selected id="sub-selected-item">
90-
B
91-
</sp-menu-item>
92-
<sp-menu-item>C</sp-menu-item>
93-
</sp-menu>
94-
</sp-menu-item>
95-
</sp-action-menu>
96-
`
97-
);
74+
await fixture<ActionMenu>(html`
75+
<sp-action-menu label="More Actions">
76+
<sp-menu-item>One</sp-menu-item>
77+
<sp-menu-item selected id="root-selected-item">Two</sp-menu-item>
78+
<sp-menu-item id="item-with-submenu">
79+
B should be selected
80+
<sp-menu slot="submenu">
81+
<sp-menu-item>A</sp-menu-item>
82+
<sp-menu-item selected id="sub-selected-item">
83+
B
84+
</sp-menu-item>
85+
<sp-menu-item>C</sp-menu-item>
86+
</sp-menu>
87+
</sp-menu-item>
88+
</sp-action-menu>
89+
`);
9890

9991
export const testActionMenu = (mode: 'sync' | 'async'): void => {
10092
describe(`Action menu: ${mode}`, () => {
@@ -108,20 +100,18 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => {
108100
await expect(el).to.be.accessible();
109101
});
110102
it('loads - [slot="label"]', async () => {
111-
const el = await fixture<ActionMenu>(
112-
html`
113-
<sp-action-menu>
114-
<span slot="label">More Actions</span>
115-
<sp-menu-item>Deselect</sp-menu-item>
116-
<sp-menu-item>Select Inverse</sp-menu-item>
117-
<sp-menu-item>Feather...</sp-menu-item>
118-
<sp-menu-item>Select and Mask...</sp-menu-item>
119-
<sp-menu-divider></sp-menu-divider>
120-
<sp-menu-item>Save Selection</sp-menu-item>
121-
<sp-menu-item disabled>Make Work Path</sp-menu-item>
122-
</sp-action-menu>
123-
`
124-
);
103+
const el = await fixture<ActionMenu>(html`
104+
<sp-action-menu>
105+
<span slot="label">More Actions</span>
106+
<sp-menu-item>Deselect</sp-menu-item>
107+
<sp-menu-item>Select Inverse</sp-menu-item>
108+
<sp-menu-item>Feather...</sp-menu-item>
109+
<sp-menu-item>Select and Mask...</sp-menu-item>
110+
<sp-menu-divider></sp-menu-divider>
111+
<sp-menu-item>Save Selection</sp-menu-item>
112+
<sp-menu-item disabled>Make Work Path</sp-menu-item>
113+
</sp-action-menu>
114+
`);
125115

126116
await elementUpdated(el);
127117
await nextFrame();
@@ -130,20 +120,18 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => {
130120
await expect(el).to.be.accessible();
131121
});
132122
it('loads - [custom icon]', async () => {
133-
const el = await fixture<ActionMenu>(
134-
html`
135-
<sp-action-menu label="More Actions">
136-
<sp-icon-settings slot="icon"></sp-icon-settings>
137-
<sp-menu-item>Deselect</sp-menu-item>
138-
<sp-menu-item>Select Inverse</sp-menu-item>
139-
<sp-menu-item>Feather...</sp-menu-item>
140-
<sp-menu-item>Select and Mask...</sp-menu-item>
141-
<sp-menu-divider></sp-menu-divider>
142-
<sp-menu-item>Save Selection</sp-menu-item>
143-
<sp-menu-item disabled>Make Work Path</sp-menu-item>
144-
</sp-action-menu>
145-
`
146-
);
123+
const el = await fixture<ActionMenu>(html`
124+
<sp-action-menu label="More Actions">
125+
<sp-icon-settings slot="icon"></sp-icon-settings>
126+
<sp-menu-item>Deselect</sp-menu-item>
127+
<sp-menu-item>Select Inverse</sp-menu-item>
128+
<sp-menu-item>Feather...</sp-menu-item>
129+
<sp-menu-item>Select and Mask...</sp-menu-item>
130+
<sp-menu-divider></sp-menu-divider>
131+
<sp-menu-item>Save Selection</sp-menu-item>
132+
<sp-menu-item disabled>Make Work Path</sp-menu-item>
133+
</sp-action-menu>
134+
`);
147135

148136
await elementUpdated(el);
149137
await nextFrame();
@@ -154,27 +142,25 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => {
154142
it('dispatches change events, no [href]', async () => {
155143
const changeSpy = spy();
156144

157-
const el = await fixture<ActionMenu>(
158-
html`
159-
<sp-action-menu
160-
label="More Actions"
161-
@change=${({
162-
target: { value },
163-
}: Event & { target: ActionMenu }) => {
164-
changeSpy(value);
165-
}}
166-
>
167-
<sp-icon-settings slot="icon"></sp-icon-settings>
168-
<sp-menu-item>Deselect</sp-menu-item>
169-
<sp-menu-item>Select Inverse</sp-menu-item>
170-
<sp-menu-item>Feather...</sp-menu-item>
171-
<sp-menu-item>Select and Mask...</sp-menu-item>
172-
<sp-menu-divider></sp-menu-divider>
173-
<sp-menu-item>Save Selection</sp-menu-item>
174-
<sp-menu-item disabled>Make Work Path</sp-menu-item>
175-
</sp-action-menu>
176-
`
177-
);
145+
const el = await fixture<ActionMenu>(html`
146+
<sp-action-menu
147+
label="More Actions"
148+
@change=${({
149+
target: { value },
150+
}: Event & { target: ActionMenu }) => {
151+
changeSpy(value);
152+
}}
153+
>
154+
<sp-icon-settings slot="icon"></sp-icon-settings>
155+
<sp-menu-item>Deselect</sp-menu-item>
156+
<sp-menu-item>Select Inverse</sp-menu-item>
157+
<sp-menu-item>Feather...</sp-menu-item>
158+
<sp-menu-item>Select and Mask...</sp-menu-item>
159+
<sp-menu-divider></sp-menu-divider>
160+
<sp-menu-item>Save Selection</sp-menu-item>
161+
<sp-menu-item disabled>Make Work Path</sp-menu-item>
162+
</sp-action-menu>
163+
`);
178164

179165
expect(changeSpy.callCount).to.equal(0);
180166
expect(el.open).to.be.false;
@@ -200,27 +186,25 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => {
200186
it('closes when Menu Item has [href]', async () => {
201187
const changeSpy = spy();
202188

203-
const el = await fixture<ActionMenu>(
204-
html`
205-
<sp-action-menu
206-
label="More Actions"
207-
@change=${() => {
208-
changeSpy();
209-
}}
210-
>
211-
<sp-icon-settings slot="icon"></sp-icon-settings>
212-
<sp-menu-item href="#">Deselect</sp-menu-item>
213-
<sp-menu-item href="#">Select Inverse</sp-menu-item>
214-
<sp-menu-item href="#">Feather...</sp-menu-item>
215-
<sp-menu-item href="#">Select and Mask...</sp-menu-item>
216-
<sp-menu-divider></sp-menu-divider>
217-
<sp-menu-item href="#">Save Selection</sp-menu-item>
218-
<sp-menu-item href="#" disabled>
219-
Make Work Path
220-
</sp-menu-item>
221-
</sp-action-menu>
222-
`
223-
);
189+
const el = await fixture<ActionMenu>(html`
190+
<sp-action-menu
191+
label="More Actions"
192+
@change=${() => {
193+
changeSpy();
194+
}}
195+
>
196+
<sp-icon-settings slot="icon"></sp-icon-settings>
197+
<sp-menu-item href="#">Deselect</sp-menu-item>
198+
<sp-menu-item href="#">Select Inverse</sp-menu-item>
199+
<sp-menu-item href="#">Feather...</sp-menu-item>
200+
<sp-menu-item href="#">Select and Mask...</sp-menu-item>
201+
<sp-menu-divider></sp-menu-divider>
202+
<sp-menu-item href="#">Save Selection</sp-menu-item>
203+
<sp-menu-item href="#" disabled>
204+
Make Work Path
205+
</sp-menu-item>
206+
</sp-action-menu>
207+
`);
224208

225209
expect(changeSpy.callCount).to.equal(0);
226210
expect(el.open).to.be.false;

‎packages/menu/src/Menu.ts

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -342,22 +342,20 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
342342
}
343343
}
344344

345-
private willSynthesizeClick = 0;
345+
// if the click and pointerup events are on the same target, we should not
346+
// handle the click event.
347+
private pointerUpTarget = null as EventTarget | null;
346348

347349
private handleClick(event: Event): void {
348-
if (this.willSynthesizeClick) {
349-
cancelAnimationFrame(this.willSynthesizeClick);
350-
this.willSynthesizeClick = 0;
350+
if (this.pointerUpTarget === event.target) {
351+
this.pointerUpTarget = null;
351352
return;
352353
}
353354
this.handlePointerBasedSelection(event);
354355
}
355356

356357
private handlePointerup(event: Event): void {
357-
this.willSynthesizeClick = requestAnimationFrame(() => {
358-
event.target?.dispatchEvent(new Event('click'));
359-
this.willSynthesizeClick = 0;
360-
});
358+
this.pointerUpTarget = event.target;
361359
this.handlePointerBasedSelection(event);
362360
}
363361

‎packages/picker/package.json

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,18 @@
2525
"default": "./src/index.js"
2626
},
2727
"./package.json": "./package.json",
28+
"./src/DesktopController.js": {
29+
"development": "./src/DesktopController.dev.js",
30+
"default": "./src/DesktopController.js"
31+
},
32+
"./src/InteractionController.js": {
33+
"development": "./src/InteractionController.dev.js",
34+
"default": "./src/InteractionController.js"
35+
},
36+
"./src/MobileController.js": {
37+
"development": "./src/MobileController.dev.js",
38+
"default": "./src/MobileController.js"
39+
},
2840
"./src/Picker.js": {
2941
"development": "./src/Picker.dev.js",
3042
"default": "./src/Picker.js"
@@ -34,6 +46,10 @@
3446
"default": "./src/index.js"
3547
},
3648
"./src/picker.css.js": "./src/picker.css.js",
49+
"./src/strategies.js": {
50+
"development": "./src/strategies.dev.js",
51+
"default": "./src/strategies.js"
52+
},
3753
"./sync/index.js": {
3854
"development": "./sync/index.dev.js",
3955
"default": "./sync/index.js"

0 commit comments

Comments
 (0)