Skip to content

Commit ea38ef0

Browse files
nikkimkcaseyisonit
andauthoredFeb 26, 2025··
fix(action menu): keyboard accessibility omnibus (#5031)
* fix(menu): added rovingTabindexController and removed aria-activedescendant * fix(menu): fixes firefox keyboard navigation and call stack issues * test(menu): updates menu test based on changes * test(menu): adds click test for accessibility * test(menu): menu itself should delegate focus to active item * fix(menu): fixed menu group navigation according WAI ARIA APG * fix(reactive-controllers): complex element delegates focus to active child WAI ARIA APG * fix(menu): components should delegate focus * fix(menu): update keyboard nav to match WAI ARIA APG * fix(menu): updated docs to reflect preferred accessible method * fix(menu): selection and keyboard navigation according to APG * test(menu): updated to recommended a11y behavior from APG * docs(menu): reverted docs but fixed typos * fix(menu): removed unused map * test(menu): updated test to match changed group id * test(menu): keyboard selection is fixed * test(menu): updated to reflect WAI ARIA APG * fix(picker): picker should delegate focus * fix(action-menu): fixed a11y error message * fix(picker): according to APG ArrowDown should focus on first item * test(action-menu): updated tests for focus delegation and APG keyboard nav * fix(picker): ensure overlay is open before setting focus * fix(menu): removed extra focus * chore: added changeset * fix(picker): prevents the picker button from getting focus when menu is first updated * fix(menu): fixes preventDefault behavior when Space is pressed * fix(menu): accounted for action menus with no selection * fix(picker):sets focus on first selected item * fix(picker): focuses on 1st selected item when opened via kbd * test(picker): updated tests based on APG kbd nav * fix(menu): when I menuitem with a submenu closes * fix(picker): leverages RTI for ArrowLeft/ArrowRight * fix(picker): leverages RTI for ArrowLeft/ArrowRight * test(picker): adjusted tests based on a11y requirements * fix(picker): fixed accessibility warning * fix(picker): resolves opening and closing tests * fix(menu): resolves test by setting focus after scrolling * fix(picker): resolves mobile test * test(menu): updated tests for focus delegation * fix(picker): ensures that slotted labels don't get a11y warning * fix(picker): resolves menu close issue * fix(menu): fixed prev and next item * fix(picker): fixed left-right arrow keys * test(tray): debugging VO/iOS tray * fix(menu): resolves submenu tests * fix(menu): added some temporary debugging logic and comments * test(action-menu): removed test that no longer applies to a11y * fix(action-menu): fixed menu closing issue, added docs, and removed temporary debugging * test(action-menu): fixed test based on focus behavior when an item is selected * merge: merge from main * merge: merge from main * test(combobox): removing aria-activedescendant references until a combobox solution is implemented * test(combobox): removing aria-activedescendant references until a combobox solution is implemented * test(combobox): added skipped tests back * fix(combobox): resolves timing issues with updated menu * test(combobox): updated test docs * chore(combobox): removed debugging * docs(combobox): updated webkit escape hatches * test(combobox): removed isWebkit since escape hatch is not needed * test(tabs): removed test that does not follow WAI ARIA APG * fix(controllers): delegates focus setting until all other components using controllers are reviewed for a11y, * fix(controllers): fix(controllers): delegates focus setting until all other components using controllers are reviewed for a11y, * test(action-menu): fixed typo * test(action-menu): fixed typo * chore: fix menu tests with comments for skips * chore: cleanup * chore: rebuilt stylesheet * chore: dialog stylesheet * chore: dialog stylesheet * chore: modified pre-commit hook temporarily and fix spectrum-dialog.css * chore: revert hook * fix(menu): ensures menu used focus delegation * fix(action-group): should delegate focus * fix(action-group): prevents submenu events from reaching action group * fix(action-group): prevents submenu events from bubbling * fix(action-group): preventing submenu actions from being handled by action group * fix(action-menu): prevents submenu key events from being handle by action group * chore: added tons of comments to picker test * chore: added comments to action menu test * chore: added comments to action group test * fix(picker): fixed focus delegation * fix(menu): allows Escape to bubble to action menu and picker * fix(picker): fixes escape key and disabled closing * fix(picker): fixes escape key and disabled closing * test(menu): added more info to submenu test * fix(reactive-controllers): fixed failing reactive-controller tests * fix(picker): resolves focus styles in VRT * docs(tray): added a story to test buttons in tray on mobile * docs(tray): removed unnecessary story * docs(action-menu): added an example for #4623 * fix(menu): fixes for menu focus issues on chromium as well as updating menuitems * fix(menu): resolved all but a WebKit timeout on removal * chore: reverting last commit * fix(menu): fixes failing tests related to mutations * chore: styles from update from main * test(menu): removed skip from removal test * fix(combobox): fixes combobox focus issue * chore: updated CSS * chore: updated golden hash * fix(menu): menu item removal and focus * fix(menu): fixed aria-expanded in submenus * fix(menu): ensures sunmenu recieves focus when opened * fix(picker): ensures focus is not lost when overlay closes * fix(menu): resolves menu removal issue * chore: updated hash for new stories and a11y behaviors * fix(action-menu): resolves action-menu timeout * test(picker): fixed test based on a11y focus * test(picker): fixed timeout on picker test * test(action-menu): replaced removed test * chore(picker): fix missing import from merge * chore(picker): fixed merging issue * chore: reverting hash to main for comparison * fix(picker): focus should be managed by user not initial state * fix(picker): picker overlay should only focus with a user action * fix(picker): picker overlay should only focus with a user action * test(picker): fix timeout * test(picker): visual diffs because focus should only be set by user action * fix(menu): made submenu items focusable after open * fix(menu): fixes menuitem focus * test(menu): temporarily skipping Chrome * chore: reverting to main has for comparison * fix(menu): fixed menu test * fix(menu): fixed menu test * chore: updated golden images hash * fix(menu): fixes VoiceOver submenu focus Issue * fix(picker): retains focus after user closes menu * test(picker): uncommented tests * test(picker): fixed focus throw test * test(picker): removed `only` * test(picker): added test for retaining focus * chore: resolved hash conflict * chore: reverting hash to resolve * chore: updating hash * chore: reverting hash * chore: reverting hash * chore: reverting hash * chore: updating hash again * test(action-menu): added tests for returning focus * test(action-menu): added a11y test * test(action-menu): added a11y test * tes(action-menu): removed an `only` * test(action-menu): fixed linting issue --------- Co-authored-by: Casey Eickhoff <ceickhoff@adobe.com>
1 parent 191a15b commit ea38ef0

35 files changed

+1758
-1211
lines changed
 

‎.changeset/lemon-points-ring.md

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
'@spectrum-web-components/reactive-controllers': minor
3+
'@spectrum-web-components/action-menu': minor
4+
'@spectrum-web-components/picker': minor
5+
'@spectrum-web-components/menu': minor
6+
---
7+
8+
Used WAI ARIA Authoring Practices Guide (APG) to make accessibility improvements for `<sp-action-menu>`, `<sp-menu>`, and `<sp-picker>`, including:
9+
- Numpad keys now work with `<sp-picker>` and `<sp-action-menu>`
10+
-`<sp-action-menu>`'s `<sp-menu-item>` elements can now be read by a screen reader ([#4556](https://github.com/adobe/spectrum-web-components/issues/4556))
11+
- `<sp-menu-item>` href can now be clicked by a screen reader ([#4997](https://github.com/adobe/spectrum-web-components/issues/4997))
12+
- Opening a `<sp-action-menu>`, `<sp-menu>`, and `<sp-picker>` with a keyboard now sets focus on an item within the menu. ([#4557](https://github.com/adobe/spectrum-web-components/issues/4557))
13+
14+
See the following APG examples for more information:
15+
- [Navigation Menu Example](https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-navigation/)
16+
- [Editor Menubar Example](https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-editor/)

‎.circleci/config.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ parameters:
1414
# 3. Commit this change to the PR branch where the changes exist.
1515
current_golden_images_hash:
1616
type: string
17-
default: be83a254993526d25ab8f6dd32639eae6db1c7eb
17+
default: 34923d02e4b68736f367f04be8d5a4f3843bc091
1818
wireit_cache_name:
1919
type: string
2020
default: wireit

‎packages/action-group/src/ActionGroup.ts

+14-5
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import {
2222
property,
2323
query,
2424
} from '@spectrum-web-components/base/src/decorators.js';
25-
import type { ActionButton } from '@spectrum-web-components/action-button';
25+
import { ActionButton } from '@spectrum-web-components/action-button';
2626
import { RovingTabindexController } from '@spectrum-web-components/reactive-controllers/src/RovingTabindex.js';
2727
import { MutationController } from '@lit-labs/observers/mutation-controller.js';
2828

@@ -40,6 +40,10 @@ export class ActionGroup extends SizedMixin(SpectrumElement, {
4040
validSizes: ['xs', 's', 'm', 'l', 'xl'],
4141
noDefaultSize: true,
4242
}) {
43+
static override shadowRootOptions = {
44+
...SpectrumElement.shadowRootOptions,
45+
delegatesFocus: true,
46+
};
4347
public static override get styles(): CSSResultArray {
4448
return [styles];
4549
}
@@ -90,6 +94,7 @@ export class ActionGroup extends SizedMixin(SpectrumElement, {
9094
: firstEnabledIndex;
9195
},
9296
elements: () => this.buttons,
97+
hostDelegatesFocus: true,
9398
isFocusableElement: (el: ActionButton) => !el.disabled,
9499
}
95100
);
@@ -245,7 +250,8 @@ export class ActionGroup extends SizedMixin(SpectrumElement, {
245250
const selections: ActionButton[] = [];
246251
const updates = options.map(async (option) => {
247252
await option.updateComplete;
248-
option.setAttribute('role', 'radio');
253+
if (option instanceof ActionButton)
254+
option.setAttribute('role', 'radio');
249255
option.setAttribute(
250256
'aria-checked',
251257
option.selected ? 'true' : 'false'
@@ -273,7 +279,8 @@ export class ActionGroup extends SizedMixin(SpectrumElement, {
273279
const selections: ActionButton[] = [];
274280
const updates = options.map(async (option) => {
275281
await option.updateComplete;
276-
option.setAttribute('role', 'checkbox');
282+
if (option instanceof ActionButton)
283+
option.setAttribute('role', 'checkbox');
277284
option.setAttribute(
278285
'aria-checked',
279286
option.selected ? 'true' : 'false'
@@ -297,7 +304,8 @@ export class ActionGroup extends SizedMixin(SpectrumElement, {
297304
const selections: ActionButton[] = [];
298305
const updates = options.map(async (option) => {
299306
await option.updateComplete;
300-
option.setAttribute('role', 'button');
307+
if (option instanceof ActionButton)
308+
option.setAttribute('role', 'button');
301309
if (option.selected) {
302310
option.setAttribute('aria-pressed', 'true');
303311
selections.push(option);
@@ -315,7 +323,8 @@ export class ActionGroup extends SizedMixin(SpectrumElement, {
315323
);
316324
} else {
317325
this.buttons.forEach((option) => {
318-
option.setAttribute('role', 'button');
326+
if (option instanceof ActionButton)
327+
option.setAttribute('role', 'button');
319328
});
320329
break;
321330
}

‎packages/action-group/test/action-group.test.ts

+79-45
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@ import {
2222
} from '@open-wc/testing';
2323

2424
import { ActionButton } from '@spectrum-web-components/action-button';
25-
import { MenuItem } from '@spectrum-web-components/menu';
2625
import { ActionMenu } from '@spectrum-web-components/action-menu';
26+
import { MenuItem } from '@spectrum-web-components/menu';
2727
import '@spectrum-web-components/action-button/sp-action-button.js';
2828
import '@spectrum-web-components/action-menu/sp-action-menu.js';
2929
import '@spectrum-web-components/menu/sp-menu.js';
@@ -53,7 +53,6 @@ import { spy } from 'sinon';
5353
import { sendMouse } from '../../../test/plugins/browser.js';
5454
import { HasActionMenuAsChild } from '../stories/action-group.stories.js';
5555
import '../stories/action-group.stories.js';
56-
import { isWebKit } from '@spectrum-web-components/shared';
5756
import sinon from 'sinon';
5857

5958
class QuietActionGroup extends LitElement {
@@ -190,13 +189,28 @@ describe('ActionGroup', () => {
190189
await elementUpdated(el);
191190

192191
// expect the first button to be focused
193-
expect(document.activeElement?.id).to.equal('first');
192+
expect(
193+
document.activeElement?.id,
194+
'should be focused on the first button'
195+
).to.equal('first');
194196

195197
// expect all the elements of the focus group to have a tabIndex of -1 except the first button because it is focused using Tab
196-
expect((el.children[0] as ActionButton)?.tabIndex).to.equal(0);
197-
expect((el.children[1] as ActionButton)?.tabIndex).to.equal(-1);
198-
expect((el.children[2] as ActionButton)?.tabIndex).to.equal(-1);
199-
expect((el.children[3] as ActionMenu)?.tabIndex).to.equal(-1);
198+
expect(
199+
(el.children[0] as ActionButton)?.tabIndex,
200+
'should be focused on the first button'
201+
).to.equal(0);
202+
expect(
203+
(el.children[1] as ActionButton)?.tabIndex,
204+
'should not be focused on the second button'
205+
).to.equal(-1);
206+
expect(
207+
(el.children[2] as ActionButton)?.tabIndex,
208+
'should not be focused on the third button'
209+
).to.equal(-1);
210+
expect(
211+
(el.children[3] as ActionMenu)?.tabIndex,
212+
'should not be focused on the fourth button'
213+
).to.equal(-1);
200214

201215
// navigate to the action-menu using the arrow keys
202216
await sendKeys({ press: 'ArrowRight' });
@@ -206,39 +220,35 @@ describe('ActionGroup', () => {
206220
await elementUpdated(el);
207221

208222
// expect the action-menu to be focused
209-
expect((el.children[3] as ActionMenu)?.focused).to.be.true;
223+
expect(el.children[3]).to.equal(document.activeElement);
210224

211225
// press Enter to open the action-menu
212226
await sendKeys({ press: 'Enter' });
213227

214-
const opened = oneEvent(el.children[3] as ActionMenu, 'sp-opened');
228+
let opened = oneEvent(el.children[3] as ActionMenu, 'sp-opened');
215229
await elementUpdated(el.children[3]);
216230
await opened;
217231

218232
// expect the first menu item to be focused
219-
const firstMenuItem = el.querySelector('#first-menu-item') as MenuItem;
220-
expect(firstMenuItem?.focused).to.be.true;
221-
233+
const firstMenuItem = el.querySelector('#first-menu-item');
234+
const fourthMenuItem = el.querySelector('#fourth-menu-item');
235+
expect(firstMenuItem).to.equal(document.activeElement);
222236
// navigate to the fourth menu item using the arrow keys
223237
await sendKeys({ press: 'ArrowDown' });
224238
await sendKeys({ press: 'ArrowDown' });
225239
await sendKeys({ press: 'ArrowDown' });
226240

241+
opened = oneEvent(fourthMenuItem as MenuItem, 'sp-opened');
242+
227243
// press Enter to select the fourth menu item
228244
await sendKeys({ press: 'Enter' });
229-
230-
await elementUpdated(el.children[3]);
231-
232-
await nextFrame();
233-
await nextFrame();
234-
await nextFrame();
235-
await nextFrame();
245+
await opened;
236246

237247
// expect the second submenu item to be focused
238248
const secondSubMenuItem = el.querySelector(
239249
'#second-sub-menu-item'
240250
) as MenuItem;
241-
expect(secondSubMenuItem?.focused).to.be.true;
251+
expect(secondSubMenuItem).to.equal(document.activeElement);
242252

243253
// press Enter to select the second submenu item
244254
await sendKeys({ press: 'Enter' });
@@ -249,7 +259,7 @@ describe('ActionGroup', () => {
249259
await closed;
250260

251261
// expect the action-menu to be focused
252-
expect((el.children[3] as ActionMenu)?.focused).to.be.true;
262+
expect(el.children[3]).to.equal(document.activeElement);
253263
});
254264

255265
it('action-group with action-menu manages tabIndex correctly while using mouse', async () => {
@@ -280,10 +290,22 @@ describe('ActionGroup', () => {
280290
await aTimeout(500);
281291

282292
// expect all the elements of the focus group to have a tabIndex of -1 except the first button because it is focused using mouse
283-
expect((el.children[0] as ActionButton)?.tabIndex).to.equal(0);
284-
expect((el.children[1] as ActionButton)?.tabIndex).to.equal(-1);
285-
expect((el.children[2] as ActionButton)?.tabIndex).to.equal(-1);
286-
expect((el.children[3] as ActionMenu)?.tabIndex).to.equal(-1);
293+
expect(
294+
(el.children[0] as ActionButton)?.tabIndex,
295+
'mouse1: should be focused on the first button'
296+
).to.equal(0);
297+
expect(
298+
(el.children[1] as ActionButton)?.tabIndex,
299+
'mouse1: should not be focused on the second button'
300+
).to.equal(-1);
301+
expect(
302+
(el.children[2] as ActionButton)?.tabIndex,
303+
'mouse1: should not be focused on the third button'
304+
).to.equal(-1);
305+
expect(
306+
(el.children[3] as ActionMenu)?.tabIndex,
307+
'mouse1: should not be focused on the fourth button'
308+
).to.equal(-1);
287309

288310
// click outside the action-group and it should loose focus and update the tabIndexes
289311
sendMouse({
@@ -299,10 +321,22 @@ describe('ActionGroup', () => {
299321
await aTimeout(500);
300322

301323
// expect the first button to have a tabIndex of 0 and everything else to have a tabIndex of -1
302-
expect((el.children[0] as ActionButton)?.tabIndex).to.equal(0);
303-
expect((el.children[1] as ActionButton)?.tabIndex).to.equal(-1);
304-
expect((el.children[2] as ActionButton)?.tabIndex).to.equal(-1);
305-
expect((el.children[3] as ActionMenu)?.tabIndex).to.equal(-1);
324+
expect(
325+
(el.children[0] as ActionButton)?.tabIndex,
326+
'mouse2: should be focused on the first button'
327+
).to.equal(0);
328+
expect(
329+
(el.children[1] as ActionButton)?.tabIndex,
330+
'mouse2: should not be focused on the second button'
331+
).to.equal(-1);
332+
expect(
333+
(el.children[2] as ActionButton)?.tabIndex,
334+
'mouse2: should not be focused on the third button'
335+
).to.equal(-1);
336+
expect(
337+
(el.children[3] as ActionMenu)?.tabIndex,
338+
'mouse2: should not be focused on the fourth button'
339+
).to.equal(-1);
306340

307341
// get the bounding box of the action-menu
308342
const actionMenu = el.querySelector('#action-menu') as ActionMenu;
@@ -331,23 +365,23 @@ describe('ActionGroup', () => {
331365
const closed = oneEvent(el.children[3] as ActionMenu, 'sp-closed');
332366
await closed;
333367

334-
if (!isWebKit()) {
335-
sendMouse({
336-
steps: [
337-
{
338-
position: [0, 0],
339-
type: 'click',
340-
},
341-
],
342-
});
343-
}
344-
345368
await aTimeout(500);
346-
347-
expect((el.children[0] as ActionButton)?.tabIndex).to.equal(0);
348-
expect((el.children[1] as ActionButton)?.tabIndex).to.equal(-1);
349-
expect((el.children[2] as ActionButton)?.tabIndex).to.equal(-1);
350-
expect((el.children[3] as ActionMenu)?.tabIndex).to.equal(-1);
369+
expect(
370+
(el.children[0] as ActionButton)?.tabIndex,
371+
'final: should NOT be focused on the first button'
372+
).to.equal(-1);
373+
expect(
374+
(el.children[1] as ActionButton)?.tabIndex,
375+
'final: should not be focused on the second button'
376+
).to.equal(-1);
377+
expect(
378+
(el.children[2] as ActionButton)?.tabIndex,
379+
'final: should not be focused on the third button'
380+
).to.equal(-1);
381+
expect(
382+
(el.children[3] as ActionMenu)?.tabIndex,
383+
'final: should be be focused on the fourth button'
384+
).to.equal(0);
351385
});
352386

353387
testForLitDevWarnings(

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

+28
Original file line numberDiff line numberDiff line change
@@ -135,4 +135,32 @@ export class ActionMenu extends ObserveSlotPresence(
135135
}
136136
super.update(changedProperties);
137137
}
138+
139+
protected override hasAccessibleLabel(): boolean {
140+
return (
141+
!!this.label ||
142+
!!this.getAttribute('aria-label') ||
143+
!!this.getAttribute('aria-labelledby') ||
144+
!!this.appliedLabel ||
145+
this.hasLabel ||
146+
this.labelOnly
147+
);
148+
}
149+
150+
protected override warnNoLabel(): void {
151+
window.__swc.warn(
152+
this,
153+
`<${this.localName}> needs one of the following to be accessible:`,
154+
'https://opensource.adobe.com/spectrum-web-components/components/action-menu/#accessibility',
155+
{
156+
type: 'accessibility',
157+
issues: [
158+
`an <sp-field-label> element with a \`for\` attribute referencing the \`id\` of the \`<${this.localName}>\`, or`,
159+
'value supplied to the "label" attribute, which will be displayed visually as placeholder text',
160+
'text content supplied in a <span> with slot="label", or, text content supplied in a <span> with slot="label-only"',
161+
'which will also be displayed visually as placeholder text.',
162+
],
163+
}
164+
);
165+
}
138166
}

‎packages/action-menu/stories/action-menu.stories.ts

+22
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,7 @@ export const groups = ({
406406
onChange(value: string): void;
407407
}): TemplateResult => html`
408408
<sp-action-menu
409+
id="groups"
409410
@change=${({ target: { value } }: Event & { target: ActionMenu }) =>
410411
onChange(value)}
411412
open
@@ -540,3 +541,24 @@ export const withScrollEvent = (): TemplateResult => {
540541
withScrollEvent.parameters = {
541542
chromatic: { disableSnapshot: true },
542543
};
544+
545+
export const MenuItemAlerts = (): TemplateResult => html`
546+
<sp-action-menu size="m">
547+
<span slot="label">More Actions</span>
548+
<sp-menu-item @click=${() => alert('Deselect')}>Deselect</sp-menu-item>
549+
<sp-menu-item @click=${() => alert('Select inverse')}>
550+
Select inverse
551+
</sp-menu-item>
552+
<sp-menu-item @click=${() => alert('Feather...')}>
553+
Feather...
554+
</sp-menu-item>
555+
<sp-menu-item @click=${() => alert('Select and mask...')}>
556+
Select and mask...
557+
</sp-menu-item>
558+
<sp-menu-divider></sp-menu-divider>
559+
<sp-menu-item @click=${() => alert('Save selection')}>
560+
Save selection
561+
</sp-menu-item>
562+
<sp-menu-item disabled>Make work path</sp-menu-item>
563+
</sp-action-menu>
564+
`;

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

+7-3
Original file line numberDiff line numberDiff line change
@@ -22,25 +22,29 @@ describe('Slottable Request Directive', () => {
2222
it('Action Menu requests for options rendering when opening and closing', async function () {
2323
const el = await fixture<ActionMenu>(directive());
2424
const initialNodeLength = el.children.length;
25-
expect(el.open).to.be.false;
25+
26+
expect(el.open, 'should be closed initially').to.be.false;
2627
expect(el.children.length).to.equal(initialNodeLength);
2728

2829
const opened = oneEvent(el, 'sp-opened');
30+
2931
el.click();
3032
await opened;
3133

32-
expect(el.open).to.be.true;
34+
expect(el.open, 'should be open after clicking').to.be.true;
3335
expect(el.children.length).to.be.gt(initialNodeLength);
3436

3537
const closed = oneEvent(el, 'sp-closed');
38+
3639
await sendKeys({
3740
press: 'Escape',
3841
});
3942
await closed;
4043
await nextFrame();
4144
await nextFrame();
4245

43-
expect(el.open).to.be.false;
46+
expect(el.open, 'should be closed after escape key is pressed').to.be
47+
.false;
4448
expect(el.children.length).to.equal(initialNodeLength);
4549
});
4650
});

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

+9-15
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,10 @@ describe('Action Menu - Groups', () => {
2424
groupsWithSelects({ onChange: () => {} })
2525
);
2626

27-
const firstGroup = el.querySelector('sp-menu-group') as HTMLElement;
2827
const firstItem = el.querySelector('sp-menu-item') as MenuItem;
2928

3029
expect(firstItem.focused).to.be.false;
31-
expect(document.activeElement === firstGroup).to.be.false;
30+
expect(document.activeElement === firstItem).to.be.false;
3231

3332
const opened = oneEvent(el, 'sp-opened');
3433
el.focus();
@@ -39,7 +38,7 @@ describe('Action Menu - Groups', () => {
3938

4039
expect(firstItem.focused).to.be.true;
4140
expect(
42-
document.activeElement === firstGroup,
41+
document.activeElement === firstItem,
4342
document.activeElement?.localName
4443
).to.be.true;
4544
});
@@ -57,15 +56,15 @@ describe('Action Menu - Groups', () => {
5756
'sp-menu-item'
5857
) as MenuItem;
5958

60-
expect(firstItem.selected).to.be.false;
59+
expect(firstItem.selected, 'before opening: first item selected?').to.be.false;
6160

6261
let opened = oneEvent(el, 'sp-opened');
6362
el.focus();
6463
await sendKeys({
6564
press: 'ArrowDown',
6665
});
6766
await opened;
68-
expect(el.open).to.be.true;
67+
expect(el.open, 'first opened: open?').to.be.true;
6968

7069
await sendKeys({
7170
press: 'ArrowUp',
@@ -81,8 +80,8 @@ describe('Action Menu - Groups', () => {
8180
await elementUpdated(el);
8281
await elementUpdated(firstItem);
8382

84-
expect(el.open).to.be.false;
85-
expect(firstItem.selected).to.be.true;
83+
expect(el.open, 'first closed: open?').to.be.false;
84+
expect(firstItem.selected, 'after select: first item selected?').to.be.true;
8685
expect(document.activeElement === el, document.activeElement?.localName)
8786
.to.be.true;
8887

@@ -91,12 +90,7 @@ describe('Action Menu - Groups', () => {
9190
press: 'ArrowDown',
9291
});
9392
await opened;
94-
expect(el.open).to.be.true;
95-
96-
await sendKeys({
97-
press: 'ArrowUp',
98-
});
99-
await elementUpdated(el);
93+
expect(el.open, 'reopened: open?').to.be.true;
10094

10195
closed = oneEvent(el, 'sp-closed');
10296
await sendKeys({
@@ -107,7 +101,7 @@ describe('Action Menu - Groups', () => {
107101
await elementUpdated(el);
108102
await elementUpdated(firstItem);
109103

110-
expect(el.open).to.be.false;
111-
expect(firstItem.selected).to.be.false;
104+
expect(el.open, 'reclosed: open?').to.be.false;
105+
expect(firstItem.selected, 'after deselect: first item selected?').to.be.false;
112106
});
113107
});

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

+135-74
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
html,
1818
nextFrame,
1919
oneEvent,
20+
waitUntil
2021
} from '@open-wc/testing';
2122
import { testForLitDevWarnings } from '../../../test/testing-helpers';
2223

@@ -38,7 +39,11 @@ import type { Tooltip } from '@spectrum-web-components/tooltip';
3839
import { sendMouse } from '../../../test/plugins/browser.js';
3940
import type { TestablePicker } from '../../picker/test/index.js';
4041
import type { Overlay } from '@spectrum-web-components/overlay';
41-
import { sendKeys, setViewport } from '@web/test-runner-commands';
42+
import {
43+
a11ySnapshot,
44+
findAccessibilityNode,
45+
sendKeys,
46+
setViewport } from '@web/test-runner-commands';
4247
import { TemplateResult } from '@spectrum-web-components/base';
4348
import { isWebKit } from '@spectrum-web-components/shared';
4449
import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/picker/src/InteractionController.js';
@@ -142,6 +147,34 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => {
142147

143148
await expect(el).to.be.accessible();
144149
});
150+
it('has menuitems in accessibility tree', async() => {
151+
const el = await fixture<ActionMenu>(html`
152+
<sp-action-menu
153+
label="More Actions">
154+
<sp-menu-item>Deselect</sp-menu-item>
155+
<sp-menu-item disabled>Make Work Path</sp-menu-item>
156+
</sp-action-menu>
157+
`);
158+
const opened = oneEvent(el, 'sp-opened');
159+
el.focus();
160+
sendKeys({ press: 'Enter' });
161+
await opened;
162+
await nextFrame();
163+
164+
165+
type NamedNode = { name: string, role: string, disabled: boolean };
166+
const snapshot = (await a11ySnapshot({})) as unknown as NamedNode & {
167+
children: NamedNode[];
168+
};
169+
const button = findAccessibilityNode<NamedNode>(snapshot, (node) => node.name === 'More Actions');
170+
const menu = findAccessibilityNode<NamedNode>(snapshot, (node) => node.role === 'menu');
171+
const deselect = findAccessibilityNode<NamedNode>(snapshot, (node) => node.role === 'menuitem' && node.name === 'Deselect');
172+
const workPath = findAccessibilityNode<NamedNode>(snapshot, (node) => node.role === 'menuitem' && node.name === 'Make Work Path' && node.disabled);
173+
expect(button, 'button').to.not.be.null;
174+
expect(menu, 'menu').to.not.be.null;
175+
expect(deselect, 'first menuitem').to.not.be.null;
176+
expect(workPath, 'second menuitem').to.not.be.null;
177+
});
145178
it('dispatches change events, no [href]', async () => {
146179
const changeSpy = spy();
147180

@@ -404,35 +437,33 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => {
404437
})
405438
);
406439

407-
await elementUpdated(el);
440+
expect(el.open, 'open?').to.be.false;
408441

409-
el.focus();
410-
await elementUpdated(el);
411442
let opened = oneEvent(el, 'sp-opened');
412-
await sendKeys({ press: 'ArrowRight' });
413-
await sendKeys({ press: 'ArrowLeft' });
414-
await sendKeys({ press: 'Space' });
443+
el.click();
415444
await opened;
416445

446+
expect(el.open, 'open?').to.be.true;
447+
417448
const firstRect = (
418449
el as unknown as { overlayElement: Overlay }
419-
).overlayElement.dialogEl.getBoundingClientRect();
450+
)?.overlayElement?.dialogEl?.getBoundingClientRect();
420451

421-
let closed = oneEvent(el, 'sp-closed');
422-
await sendKeys({ press: 'Space' });
452+
const closed = oneEvent(el, 'sp-closed');
453+
el.close();
423454
await closed;
455+
expect(el.open, 'open?').to.be.false;
424456

425457
opened = oneEvent(el, 'sp-opened');
426-
await sendKeys({ press: 'Space' });
458+
el.toggle();
427459
await opened;
460+
expect(el.open, 'open?').to.be.true;
428461

429462
const secondRect = (
430463
el as unknown as { overlayElement: Overlay }
431-
).overlayElement.dialogEl.getBoundingClientRect();
464+
)?.overlayElement?.dialogEl?.getBoundingClientRect();
432465

433-
closed = oneEvent(el, 'sp-closed');
434-
await sendKeys({ press: 'Space' });
435-
await closed;
466+
el.close();
436467

437468
expect(firstRect).to.deep.equal(secondRect);
438469
});
@@ -486,6 +517,44 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => {
486517
expect(el.open).to.be.false;
487518
expect(selected).to.equal(thirdItem.value);
488519
});
520+
it('returns focus on `Escape`', async () => {
521+
const el = await actionMenuFixture();
522+
const thirdItem = el.querySelector(
523+
'sp-menu-item:nth-of-type(3)'
524+
) as MenuItem;
525+
526+
expect(el.value).to.not.equal(thirdItem.value);
527+
const opened = oneEvent(el, 'sp-opened');
528+
el.focus();
529+
await sendKeys({ press: 'Enter'})
530+
await opened;
531+
532+
await sendKeys({ press: 'Escape'});
533+
await waitUntil(
534+
() => document.activeElement === el,
535+
'focused', { timeout: 300 }
536+
);
537+
expect(el.open).to.be.false;
538+
});
539+
it('returns focus on select', async () => {
540+
const el = await actionMenuFixture();
541+
const thirdItem = el.querySelector(
542+
'sp-menu-item:nth-of-type(3)'
543+
) as MenuItem;
544+
545+
expect(el.value).to.not.equal(thirdItem.value);
546+
const opened = oneEvent(el, 'sp-opened');
547+
el.focus();
548+
await sendKeys({ press: 'Enter'})
549+
await opened;
550+
551+
thirdItem.click();
552+
await waitUntil(
553+
() => document.activeElement === el,
554+
'focused', { timeout: 300 }
555+
);
556+
expect(el.open).to.be.false;
557+
});
489558
it('has attribute aria-describedby', async () => {
490559
const name = 'sp-picker';
491560
const description = 'Rendering a Picker';
@@ -581,94 +650,86 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => {
581650
'initially selected item should maintain selection'
582651
).to.be.true;
583652
});
584-
it('allows top-level selection state to change', async () => {
585-
let selected = true;
586-
const handleChange = (
587-
event: Event & { target: ActionMenu }
588-
): void => {
589-
if (event.target.value === 'test') {
590-
selected = !selected;
591-
592-
event.target.updateComplete.then(() => {
593-
event.target.value = selected ? 'test' : '';
594-
});
595-
}
596-
};
653+
it('does not alter submenu selection when top-level menu items are selected', async () => {
597654
const root = await fixture<ActionMenu>(html`
598-
<sp-action-menu label="More Actions" @change=${handleChange}>
599-
<sp-menu-item>One</sp-menu-item>
600-
<sp-menu-item selected value="test" id="root-selected-item">
601-
Two
602-
</sp-menu-item>
603-
<sp-menu-item id="item-with-submenu">
604-
B should be selected
605-
<sp-menu slot="submenu">
606-
<sp-menu-item>A</sp-menu-item>
607-
<sp-menu-item selected id="sub-selected-item">
608-
B
609-
</sp-menu-item>
610-
<sp-menu-item>C</sp-menu-item>
655+
<sp-action-menu id="actionmenu" label="More Actions">
656+
<sp-menu-item id="item-1">One</sp-menu-item>
657+
<sp-menu-item id="item-2">
658+
Two, with B selected
659+
<sp-menu slot="submenu" id="menu-2" selects="single">
660+
<sp-menu-item id="item-2a" selected>A</sp-menu-item>
661+
<sp-menu-item id="item-2b">B</sp-menu-item>
611662
</sp-menu>
612663
</sp-menu-item>
613664
</sp-action-menu>
614665
`);
615666

616-
const unselectedItem = root.querySelector(
617-
'sp-menu-item'
618-
) as MenuItem;
619-
const selectedItem = root.querySelector(
620-
'#root-selected-item'
621-
) as MenuItem;
622-
623-
expect(unselectedItem.textContent).to.include('One');
624-
expect(unselectedItem.selected).to.be.false;
625-
expect(selectedItem.textContent).to.include('Two');
626-
expect(selectedItem.selected).to.be.true;
667+
const item1 = root.querySelector('#item-1') as MenuItem;
668+
const item2 = root.querySelector('#item-2') as MenuItem;
669+
const itemA = root.querySelector('#item-2a') as MenuItem;
670+
const itemB = root.querySelector('#item-2b') as MenuItem;
627671

628672
let opened = oneEvent(root, 'sp-opened');
673+
674+
expect(item1.selected, 'before opening: item1 selected?').to.be
675+
.false;
676+
expect(item2.selected, 'before opening: item2 selected?').to.be
677+
.false;
678+
expect(itemA.selected, 'before opening: itemA selected?').to.be
679+
.true;
680+
expect(item2.selected, 'before opening: itemB selected?').to.be
681+
.false;
629682
root.click();
630683
await opened;
631684

632-
// close by clicking selected
633-
// (with event listener: should set selected = false)
685+
expect(root.open, 'after clicking open: open?').to.be.true;
686+
634687
let closed = oneEvent(root, 'sp-closed');
635-
selectedItem.click();
688+
item1.click();
636689
await closed;
637690

638-
expect(root.open).to.be.false;
691+
expect(item1.selected, 'after clicking item1: item1 selected?').to
692+
.be.false;
693+
expect(itemA.selected, 'after clicking item1: itemA selected?').to
694+
.be.true;
695+
expect(root.open, 'after clicking item1: open?').to.be.false;
696+
639697
opened = oneEvent(root, 'sp-opened');
640698
root.click();
641699
await opened;
642700

643-
// close by clicking unselected
644-
// (no event listener: should remain selected = false)
701+
expect(root.open, 'after reopening: open?').to.be.true;
702+
645703
closed = oneEvent(root, 'sp-closed');
646-
unselectedItem.click();
704+
itemB.click();
705+
root.close();
647706
await closed;
648707

708+
expect(item1.selected, 'after clicking itemB: item1 selected?').to
709+
.be.false;
710+
expect(item2.selected, 'after clicking itemB: item2 selected?').to
711+
.be.false;
712+
expect(itemA.selected, 'after clicking itemB: itemA selected?').to
713+
.be.false;
714+
expect(itemB.selected, 'after clicking itemB: itemB selected?').to
715+
.be.true;
716+
expect(root.open, 'after clicking itemB: open?').to.be.false;
717+
649718
opened = oneEvent(root, 'sp-opened');
650719
root.click();
651720
await opened;
652721

653-
expect(unselectedItem.textContent).to.include('One');
654-
expect(unselectedItem.selected).to.be.false;
655-
expect(selectedItem.textContent).to.include('Two');
656-
expect(selectedItem.selected).to.be.false;
722+
expect(root.open, 'after reopening: open?').to.be.true;
657723

658-
// close by clicking selected
659-
// (with event listener: should set selected = false)
660724
closed = oneEvent(root, 'sp-closed');
661-
selectedItem.click();
725+
itemB.click();
662726
await closed;
663727

664-
opened = oneEvent(root, 'sp-opened');
665-
root.click();
666-
await opened;
667-
668-
expect(unselectedItem.textContent).to.include('One');
669-
expect(unselectedItem.selected).to.be.false;
670-
expect(selectedItem.textContent).to.include('Two');
671-
expect(selectedItem.selected).to.be.true;
728+
expect(item2.selected, 'after clicking item2: item2 selected?').to
729+
.be.false;
730+
expect(itemB.selected, 'after clicking item2: itemB selected?').to
731+
.be.true;
732+
expect(root.open, 'after clicking item2: open?').to.be.false;
672733
});
673734
it('shows tooltip', async function () {
674735
const openSpy = spy();

‎packages/combobox/src/Combobox.ts

+13-2
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,12 @@ export class Combobox extends Textfield {
227227
if (!this.availableOptions[nextActiveIndex].disabled) {
228228
this.activeDescendant = this.availableOptions[nextActiveIndex];
229229
}
230+
this.optionEls.forEach((el) =>
231+
el.setAttribute(
232+
'aria-selected',
233+
el.value === this.activeDescendant?.value ? 'true' : 'false'
234+
)
235+
);
230236
}
231237

232238
public activatePreviousDescendant(): void {
@@ -245,6 +251,12 @@ export class Combobox extends Textfield {
245251
if (!this.availableOptions[previousActiveIndex].disabled) {
246252
this.activeDescendant = this.availableOptions[previousActiveIndex];
247253
}
254+
this.optionEls.forEach((el) =>
255+
el.setAttribute(
256+
'aria-selected',
257+
el.value === this.activeDescendant?.value ? 'true' : 'false'
258+
)
259+
);
248260
}
249261

250262
public selectDescendant(): void {
@@ -283,9 +295,8 @@ export class Combobox extends Textfield {
283295

284296
protected handleMenuChange(event: PointerEvent & { target: Menu }): void {
285297
const { target } = event;
286-
const value = target.selected[0];
287298
const selected = (this.options || this.optionEls).find(
288-
(item) => item.value === value
299+
(item) => item.value === target?.value
289300
);
290301
this.value = selected?.itemText || '';
291302
event.preventDefault();

‎packages/combobox/test/combobox-a11y.test.ts

+7-65
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020

2121
import '@spectrum-web-components/combobox/sp-combobox.js';
2222
import { Combobox } from '@spectrum-web-components/combobox';
23-
import { detectOS, fixture } from '../../../test/testing-helpers.js';
23+
import { fixture } from '../../../test/testing-helpers.js';
2424
import { findDescribedNode } from '../../../test/testing-helpers-a11y.js';
2525
import {
2626
a11ySnapshot,
@@ -29,7 +29,6 @@ import {
2929
} from '@web/test-runner-commands';
3030
import type { AccessibleNamedNode } from './helpers.js';
3131
import { comboboxFixture } from './helpers.js';
32-
import { isWebKit } from '@spectrum-web-components/shared';
3332
import {
3433
withFieldLabel,
3534
withHelpText,
@@ -72,9 +71,6 @@ describe('Combobox accessibility', () => {
7271
<div>${withFieldLabel()}</div>
7372
`);
7473
const el = test.querySelector('sp-combobox') as unknown as Combobox;
75-
const name = 'Pick something';
76-
const webkitName = 'Pick something Banana';
77-
const isWebKitMacOS = isWebKit() && detectOS() === 'Mac OS';
7874

7975
await elementUpdated(el);
8076
await nextFrame();
@@ -89,7 +85,7 @@ describe('Combobox accessibility', () => {
8985
const a11yNode = findAccessibilityNode<AccessibleNamedNode>(
9086
snapshot,
9187
(node) =>
92-
node.name === name && !node.value && node.role === 'combobox'
88+
node.name === 'Pick something' && !node.value && node.role === 'combobox'
9389
);
9490
// by default, is there a combobox that has `name` as the label?
9591
expect(a11yNode, '`name` is the label text').to.not.be.null;
@@ -102,55 +98,26 @@ describe('Combobox accessibility', () => {
10298
)) as unknown as AccessibleNamedNode & {
10399
children: AccessibleNamedNode[];
104100
};
105-
106-
// WebKit doesn't currently associate the `name` via the accessibility tree.
107-
// Instead if lists this data in the description 🤷🏻‍♂️
108-
// Give it an escape hatch for now.
109101
const node = findAccessibilityNode<AccessibleNamedNode>(
110102
snapshot,
111103
(node) =>
112-
(node.name === name ||
113-
(isWebKitMacOS && node.name === webkitName)) &&
104+
node.name === 'Pick something' &&
114105
node.value === 'Banana' &&
115106
node.role === 'combobox'
116107
);
117108

118109
expect(
119110
node,
120-
`pre escape hatch node not available: ${JSON.stringify(
111+
`node not available: ${JSON.stringify(
121112
snapshot,
122113
null,
123114
' '
124115
)}`
125116
).to.not.be.null;
126-
127-
if (isWebKitMacOS) {
128-
// Retest WebKit without the escape hatch, expecting it to fail.
129-
// This way we get notified when the results are as expected, again.
130-
const iOSNode = findAccessibilityNode<AccessibleNamedNode>(
131-
snapshot,
132-
(node) =>
133-
node.name === name &&
134-
node.value === 'Banana' &&
135-
node.role === 'combobox'
136-
);
137-
expect(
138-
iOSNode,
139-
`post escape hatch node available: ${JSON.stringify(
140-
snapshot,
141-
null,
142-
' '
143-
)}`
144-
).to.be.null;
145-
}
146117
});
147118
it('manages its "name" value in the accessibility tree', async () => {
148119
const el = await comboboxFixture();
149120

150-
const name = 'Combobox';
151-
const webkitName = 'Combobox Banana';
152-
const isWebKitMacOS = isWebKit() && detectOS() === 'Mac OS';
153-
154121
await elementUpdated(el);
155122
await nextFrame();
156123
await nextFrame();
@@ -164,7 +131,7 @@ describe('Combobox accessibility', () => {
164131
const a11yNode = findAccessibilityNode<AccessibleNamedNode>(
165132
snapshot,
166133
(node) =>
167-
node.name === name && !node.value && node.role === 'combobox'
134+
node.name === 'Combobox' && !node.value && node.role === 'combobox'
168135
);
169136
// by default, is there a combobox that has `name` as the label?
170137
expect(a11yNode, '`name` is the label text').to.not.be.null;
@@ -177,47 +144,22 @@ describe('Combobox accessibility', () => {
177144
)) as unknown as AccessibleNamedNode & {
178145
children: AccessibleNamedNode[];
179146
};
180-
181-
// WebKit doesn't currently associate the `name` via the accessibility tree.
182-
// Instead if lists this data in the description 🤷🏻‍♂️
183-
// Give it an escape hatch for now.
184147
const node = findAccessibilityNode<AccessibleNamedNode>(
185148
snapshot,
186149
(node) =>
187-
(node.name === name ||
188-
(isWebKitMacOS && node.name === webkitName)) &&
150+
node.name === 'Combobox' &&
189151
node.value === 'Banana' &&
190152
node.role === 'combobox'
191153
);
192154

193155
expect(
194156
node,
195-
`pre escape hatch node not available: ${JSON.stringify(
157+
`node not available: ${JSON.stringify(
196158
snapshot,
197159
null,
198160
' '
199161
)}`
200162
).to.not.be.null;
201-
202-
if (isWebKitMacOS) {
203-
// Retest WebKit without the escape hatch, expecting it to fail.
204-
// This way we get notified when the results are as expected, again.
205-
const iOSNode = findAccessibilityNode<AccessibleNamedNode>(
206-
snapshot,
207-
(node) =>
208-
node.name === name &&
209-
node.value === 'Banana' &&
210-
node.role === 'combobox'
211-
);
212-
expect(
213-
iOSNode,
214-
`post escape hatch node available: ${JSON.stringify(
215-
snapshot,
216-
null,
217-
' '
218-
)}`
219-
).to.be.null;
220-
}
221163
});
222164
it('manages its "description" value with slotted <sp-tooltip>', async () => {
223165
const test = await fixture<HTMLDivElement>(html`

‎packages/combobox/test/combobox.test.ts

+28-18
Original file line numberDiff line numberDiff line change
@@ -488,27 +488,35 @@ describe('Combobox', () => {
488488
await elementUpdated(el);
489489
expect(el.value).to.equal('');
490490
expect(el.activeDescendant).to.be.undefined;
491-
expect(el.open).to.be.false;
491+
expect(el.open, 'open?').to.be.false;
492492

493493
el.focusElement.focus();
494494
const opened = oneEvent(el, 'sp-opened');
495495
el.focusElement.dispatchEvent(arrowDownEvent());
496496
await opened;
497497

498-
expect(el.open).to.be.true;
499-
500-
await elementUpdated(el);
501-
el.focusElement.dispatchEvent(arrowDownEvent());
502-
498+
expect(el.open, 'open?').to.be.true;
503499
await elementUpdated(el);
504-
testActiveElement(el, 'banana');
500+
expect(
501+
el.activeDescendant?.value,
502+
'activeDscendant after open?'
503+
).to.equal('apple');
505504
el.focusElement.dispatchEvent(enterEvent());
506505

507506
await elementUpdated(el);
508-
expect(el.open).to.be.false;
509-
expect(el.activeDescendant).to.be.undefined;
510-
expect(el.value).to.equal('Banana');
511-
expect(el.focusElement.value).to.equal(el.value);
507+
expect(el.open, 'open?').to.be.false;
508+
expect(el.activeDescendant, 'activeDescendant after Enter?').to.be
509+
.undefined;
510+
expect(el.value, 'value after enter').to.equal('Apple');
511+
expect(
512+
el.shadowRoot.querySelector(
513+
'sp-menu-item[aria-selected="true"]'
514+
)?.id,
515+
'aria-selected'
516+
).to.equal('apple');
517+
expect(el.focusElement.value, 'focusElement after enter').to.equal(
518+
el.value
519+
);
512520
});
513521
it('does not set the value when `enter` is pressed and no active descendent', async () => {
514522
const el = await comboboxFixture();
@@ -538,9 +546,10 @@ describe('Combobox', () => {
538546

539547
await elementUpdated(el);
540548

541-
expect(el.value).to.equal('');
542-
expect(el.activeDescendant).to.be.undefined;
543-
expect(el.open).to.be.false;
549+
expect(el.value, 'initial value').to.equal('');
550+
expect(el.activeDescendant, 'initial activeDescendant').to.be
551+
.undefined;
552+
expect(el.open, 'initially open?').to.be.false;
544553

545554
const opened = oneEvent(el, 'sp-opened');
546555
el.focusElement.click();
@@ -549,7 +558,7 @@ describe('Combobox', () => {
549558
const item = el.shadowRoot.querySelector('#cherry') as HTMLElement;
550559
await elementUpdated(item);
551560

552-
expect(el.open).to.be.true;
561+
expect(el.open, 'open after click?').to.be.true;
553562

554563
const itemValue = (item.textContent as string).trim();
555564
const rect = item.getBoundingClientRect();
@@ -567,9 +576,10 @@ describe('Combobox', () => {
567576
});
568577
await elementUpdated(el);
569578

570-
expect(el.value).to.equal(itemValue);
571-
expect(el.open).to.be.false;
572-
expect(el.activeDescendant).to.be.undefined;
579+
expect(el.value, 'value after item click?').to.equal(itemValue);
580+
expect(el.open, 'open after item click?').to.be.false;
581+
expect(el.activeDescendant, 'activeDescendant after item click').to
582+
.be.undefined;
573583
});
574584
it('reflects the selected value in menu on reopening', async () => {
575585
const el = await comboboxFixture();

‎packages/combobox/test/helpers.ts

+5-2
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,12 @@ export const testActiveElement = (
8787
el: TestableCombobox,
8888
testId: string
8989
): void => {
90-
expect(el.activeDescendant?.value).to.equal(testId);
90+
expect(el.activeDescendant?.value, 'active descendant').to.equal(testId);
9191
const activeElement = el.shadowRoot.querySelector(
9292
`#${el.activeDescendant.value}`
9393
) as HTMLElement;
94-
expect(activeElement.getAttribute('aria-selected')).to.equal('true');
94+
expect(
95+
activeElement.getAttribute('aria-selected'),
96+
'aria-selected'
97+
).to.equal('true');
9598
};

‎packages/menu/menu-group.md

+2-2
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ An `<sp-menu-group>` can be used to organize `<sp-menu-item>`s in an `<sp-memu>`
3333
<p>
3434
Your favorite park in New York is: <span id="group-1-value"></span>
3535
<br><br>
36-
Your favorite park in San Fransisco is: <span id="group-2-value"></span>
36+
Your favorite park in San Francisco is: <span id="group-2-value"></span>
3737
</p>
3838
<sp-popover open style="position: relative">
3939
<sp-menu
@@ -62,7 +62,7 @@ An `<sp-menu-group>` can be used to organize `<sp-menu-item>`s in an `<sp-memu>`
6262
id="group-2"
6363
selects="single"
6464
>
65-
<span slot="header">San Fransisco</span>
65+
<span slot="header">San Francisco</span>
6666
<sp-menu-item>
6767
Golden Gate Park
6868
</sp-menu-item>

‎packages/menu/menu-item.md

+28-5
Original file line numberDiff line numberDiff line change
@@ -95,12 +95,35 @@ Content assigned to the `value` slot will be placed at the end of the `<sp-menu-
9595
An `<sp-menu-item>` can also accept content addressed to its `submenu` slot. Using the `<sp-menu>` element with this slot name the options will be surfaced in flyout menu that can be activated by hovering over the root menu item with your pointer or focusing the menu item and pressing the appropriate `ArrowRight` or `ArrowLeft` key based on text direction to move into the submenu.
9696

9797
```html
98-
<sp-menu style="width: 200px;">
98+
<p>
99+
Your favorite park in New York is:
100+
<span id="group-1-value"></span>
101+
<br />
102+
<br />
103+
Your favorite park in San Francisco is:
104+
<span id="group-2-value"></span>
105+
</p>
106+
<sp-menu
107+
style="width: 200px;"
108+
onchange="this.parentElement
109+
.previousElementSibling
110+
.querySelector(`#${arguments[0].target.id}-value`)
111+
.textContent = arguments[0].target.value"
112+
>
113+
<sp-menu-item>
114+
New York
115+
<sp-menu slot="submenu" selects="single">
116+
<sp-menu-item>Central Park</sp-menu-item>
117+
<sp-menu-item>Flushing Meadows Corona Park</sp-menu-item>
118+
<sp-menu-item>Prospect Park</sp-menu-item>
119+
</sp-menu>
120+
</sp-menu-item>
99121
<sp-menu-item>
100-
Item with submenu
101-
<sp-menu slot="submenu">
102-
<sp-menu-item>Additional options</sp-menu-item>
103-
<sp-menu-item>Available on request</sp-menu-item>
122+
San Francisco
123+
<sp-menu slot="submenu" selects="single">
124+
<sp-menu-item>Golden Gate Park</sp-menu-item>
125+
<sp-menu-item>John McLaren Park</sp-menu-item>
126+
<sp-menu-item>Lake Merced Park</sp-menu-item>
104127
</sp-menu>
105128
</sp-menu-item>
106129
</sp-menu>

‎packages/menu/src/Menu.ts

+215-231
Large diffs are not rendered by default.

‎packages/menu/src/MenuGroup.ts

+14-9
Original file line numberDiff line numberDiff line change
@@ -48,15 +48,20 @@ export class MenuGroup extends Menu {
4848
@state()
4949
private headerElement?: HTMLElement;
5050

51+
/**
52+
* a menu group must have the role `group`
53+
* and should never function as a menu
54+
*/
5155
protected override get ownRole(): string {
52-
switch (this.selects) {
53-
case 'multiple':
54-
case 'single':
55-
case 'inherit':
56-
return 'group';
57-
default:
58-
return 'menu';
59-
}
56+
return 'group';
57+
}
58+
59+
/**
60+
* only a menu controls roving tabindex;
61+
* groups should defer navigation to parent menu
62+
*/
63+
protected override get controlsRovingTabindex(): boolean {
64+
return false;
6065
}
6166

6267
protected updateLabel(): void {
@@ -87,7 +92,7 @@ export class MenuGroup extends Menu {
8792
<span class="header" ?hidden=${!this.headerElement}>
8893
<slot name="header" @slotchange=${this.updateLabel}></slot>
8994
</span>
90-
<sp-menu ignore>${this.renderMenuItemSlot()}</sp-menu>
95+
${this.renderMenuItemSlot()}
9196
`;
9297
}
9398
}

‎packages/menu/src/MenuItem.ts

+153-12
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,9 @@ type MenuCascadeItem = {
5252
ancestorWithSelects?: HTMLElement;
5353
};
5454

55+
/**
56+
* Fires when a menu item is added or updated so that a parent menu can track it.
57+
*/
5558
export class MenuItemAddedOrUpdatedEvent extends Event {
5659
constructor(item: MenuItem) {
5760
super('sp-menu-item-added-or-updated', {
@@ -79,6 +82,55 @@ export class MenuItemAddedOrUpdatedEvent extends Event {
7982
currentAncestorWithSelects?: Menu;
8083
}
8184

85+
/**
86+
* Fires to forward keyboard event information to parent menu.
87+
*/
88+
export class MenuItemKeydownEvent extends KeyboardEvent {
89+
root?: MenuItem;
90+
private _event?: KeyboardEvent;
91+
constructor({ root, event }: { root?: MenuItem; event?: KeyboardEvent }) {
92+
super('sp-menu-item-keydown', { bubbles: true, composed: true });
93+
this.root = root;
94+
this._event = event;
95+
}
96+
97+
public override get altKey(): boolean {
98+
return this._event?.altKey || false;
99+
}
100+
101+
public override get code(): string {
102+
return this._event?.code || '';
103+
}
104+
105+
public override get ctrlKey(): boolean {
106+
return this._event?.ctrlKey || false;
107+
}
108+
109+
public override get isComposing(): boolean {
110+
return this._event?.isComposing || false;
111+
}
112+
113+
public override get key(): string {
114+
return this._event?.key || '';
115+
}
116+
117+
public override get location(): number {
118+
return this._event?.location || 0;
119+
}
120+
121+
public override get metaKey(): boolean {
122+
return this._event?.metaKey || false;
123+
}
124+
125+
public override get repeat(): boolean {
126+
return this._event?.repeat || false;
127+
}
128+
129+
public override get shiftKey(): boolean {
130+
return this._event?.shiftKey || false;
131+
}
132+
}
133+
82134
export type MenuItemChildren = { icon: Element[]; content: Node[] };
83135

84136
/**
@@ -100,17 +152,29 @@ export class MenuItem extends LikeAnchor(
100152

101153
abortControllerSubmenu!: AbortController;
102154

155+
/**
156+
* whether the menu item is active or has an active descendant
157+
*/
103158
@property({ type: Boolean, reflect: true })
104159
public active = false;
105160

106161
private dependencyManager = new DependencyManagerController(this);
107162

163+
/**
164+
* whether the menu item has keyboard focus
165+
*/
108166
@property({ type: Boolean, reflect: true })
109167
public focused = false;
110168

169+
/**
170+
* whether the menu item is selected
171+
*/
111172
@property({ type: Boolean, reflect: true })
112173
public selected = false;
113174

175+
/**
176+
* value of the menu item which is used for selection
177+
*/
114178
@property({ type: String })
115179
public get value(): string {
116180
return this._value || this.itemText;
@@ -132,6 +196,7 @@ export class MenuItem extends LikeAnchor(
132196

133197
/**
134198
* @private
199+
* text content of the menu item minus whitespace
135200
*/
136201
public get itemText(): string {
137202
return this.itemChildren.content.reduce(
@@ -140,6 +205,9 @@ export class MenuItem extends LikeAnchor(
140205
);
141206
}
142207

208+
/**
209+
* whether the menu item has a submenu
210+
*/
143211
@property({ type: Boolean, reflect: true, attribute: 'has-submenu' })
144212
public hasSubmenu = false;
145213

@@ -149,6 +217,9 @@ export class MenuItem extends LikeAnchor(
149217
@query('slot[name="icon"]')
150218
iconSlot!: HTMLSlotElement;
151219

220+
/**
221+
* whether menu item text content should not wrap
222+
*/
152223
@property({
153224
type: Boolean,
154225
reflect: true,
@@ -167,6 +238,9 @@ export class MenuItem extends LikeAnchor(
167238

168239
private submenuElement?: HTMLElement;
169240

241+
/**
242+
* the focusable element of the menu item
243+
*/
170244
public override get focusElement(): HTMLElement {
171245
return this;
172246
}
@@ -206,6 +280,8 @@ export class MenuItem extends LikeAnchor(
206280
this.addEventListener('click', this.handleClickCapture, {
207281
capture: true,
208282
});
283+
this.addEventListener('focus', this.handleFocus);
284+
this.addEventListener('blur', this.handleBlur);
209285

210286
new MutationController(this, {
211287
config: {
@@ -227,28 +303,23 @@ export class MenuItem extends LikeAnchor(
227303
});
228304
}
229305

306+
/**
307+
* whether submenu is open
308+
*/
230309
@property({ type: Boolean, reflect: true })
231310
public open = false;
232311

233-
public override click(): void {
234-
if (this.disabled) {
235-
return;
236-
}
237-
238-
if (this.shouldProxyClick()) {
239-
return;
240-
}
241-
242-
super.click();
243-
}
244-
245312
private handleClickCapture(event: Event): void | boolean {
246313
if (this.disabled) {
247314
event.preventDefault();
248315
event.stopImmediatePropagation();
249316
event.stopPropagation();
250317
return false;
251318
}
319+
320+
if (this.shouldProxyClick()) {
321+
return;
322+
}
252323
}
253324

254325
private handleSlottableRequest = (event: SlottableRequestEvent): void => {
@@ -360,6 +431,9 @@ export class MenuItem extends LikeAnchor(
360431
`;
361432
}
362433

434+
/**
435+
* determines if item has a submenu and updates the `aria-haspopup` attribute
436+
*/
363437
protected manageSubmenu(event: Event & { target: HTMLSlotElement }): void {
364438
this.submenuElement = event.target.assignedElements({
365439
flatten: true,
@@ -385,18 +459,52 @@ export class MenuItem extends LikeAnchor(
385459
protected override firstUpdated(changes: PropertyValues): void {
386460
super.firstUpdated(changes);
387461
this.setAttribute('tabindex', '-1');
462+
this.addEventListener('keydown', this.handleKeydown);
388463
this.addEventListener('pointerdown', this.handlePointerdown);
389464
this.addEventListener('pointerenter', this.closeOverlaysForRoot);
390465
if (!this.hasAttribute('id')) {
391466
this.id = `sp-menu-item-${randomID()}`;
392467
}
393468
}
394469

470+
/**
471+
* forward key info from keydown event to parent menu
472+
*/
473+
handleKeydown = (event: KeyboardEvent): void => {
474+
const { target, key } = event;
475+
const openSubmenuKey =
476+
this.hasSubmenu && !this.open && [' ', 'Enter'].includes(key);
477+
if (target === this) {
478+
if (
479+
['ArrowLeft', 'ArrowRight', 'Escape'].includes(key) ||
480+
openSubmenuKey
481+
)
482+
event.preventDefault();
483+
this.dispatchEvent(
484+
new MenuItemKeydownEvent({ root: this, event: event })
485+
);
486+
}
487+
};
488+
395489
protected closeOverlaysForRoot(): void {
396490
if (this.open) return;
397491
this.menuData.parentMenu?.closeDescendentOverlays();
398492
}
399493

494+
protected handleFocus(event: FocusEvent): void {
495+
const { target } = event;
496+
if (target === this) {
497+
this.focused = true;
498+
}
499+
}
500+
501+
protected handleBlur(event: FocusEvent): void {
502+
const { target } = event;
503+
if (target === this) {
504+
this.focused = false;
505+
}
506+
}
507+
400508
protected handleSubmenuClick(event: Event): void {
401509
if (event.composedPath().includes(this.overlayElement)) {
402510
return;
@@ -409,6 +517,7 @@ export class MenuItem extends LikeAnchor(
409517
// Wait till after `closeDescendentOverlays` has happened in Menu
410518
// to reopen (keep open) the direct descendent of this Menu Item
411519
this.overlayElement.open = this.open;
520+
this.focused = false;
412521
});
413522
}
414523

@@ -467,17 +576,21 @@ export class MenuItem extends LikeAnchor(
467576
}
468577

469578
protected handleSubmenuOpen(event: Event): void {
579+
const shouldFocus = this.matches(':focus, :focus-within') || this.focused;
470580
this.focused = false;
471581
const parentOverlay = event.composedPath().find((el) => {
472582
return (
473583
el !== this.overlayElement &&
474584
(el as HTMLElement).localName === 'sp-overlay'
475585
);
476586
}) as Overlay;
587+
if (shouldFocus)
588+
this.submenuElement?.focus();
477589
this.overlayElement.parentOverlayToForceClose = parentOverlay;
478590
}
479591

480592
protected cleanup(): void {
593+
this.setAttribute('aria-expanded', 'false');
481594
this.open = false;
482595
this.active = false;
483596
}
@@ -511,6 +624,20 @@ export class MenuItem extends LikeAnchor(
511624
this.updateAriaSelected();
512625
}
513626

627+
protected override willUpdate(changes: PropertyValues<this>): void {
628+
super.updated(changes);
629+
630+
// make sure focus returns to the anchor element when submenu is closed
631+
if (
632+
changes.has('open') &&
633+
!this.open &&
634+
this.hasSubmenu &&
635+
this.hasVisibleFocusInTree()
636+
) {
637+
this.focus();
638+
}
639+
}
640+
514641
protected override updated(changes: PropertyValues<this>): void {
515642
super.updated(changes);
516643
if (
@@ -597,6 +724,18 @@ export class MenuItem extends LikeAnchor(
597724
this.dispatchUpdate();
598725
}
599726

727+
public override focus(): void {
728+
super.focus();
729+
// ensure focus event fires in Chromium for tests
730+
this.dispatchEvent(new FocusEvent('focus'));
731+
}
732+
733+
public override blur(): void {
734+
// ensure focus event fires in Chromium for tests
735+
this.dispatchEvent(new FocusEvent('blur'));
736+
super.blur();
737+
}
738+
600739
public dispatchUpdate(): void {
601740
if (!this.isConnected) {
602741
return;
@@ -611,8 +750,10 @@ export class MenuItem extends LikeAnchor(
611750
selectionRoot?: Menu;
612751
cleanupSteps: ((item: MenuItem) => void)[];
613752
} = {
753+
// menu that controls ArrowUp/ArrowDown navigation
614754
focusRoot: undefined,
615755
parentMenu: undefined,
756+
// menu or menu group that controls selection
616757
selectionRoot: undefined,
617758
cleanupSteps: [],
618759
};

‎packages/menu/stories/index.ts

+17-5
Original file line numberDiff line numberDiff line change
@@ -53,19 +53,25 @@ export class ComplexSlottedGroup extends LitElement {
5353
get menu(): Menu {
5454
return this.renderRoot.querySelector('sp-menu') as Menu;
5555
}
56+
57+
static override shadowRootOptions = {
58+
...LitElement.shadowRootOptions,
59+
delegatesFocus: true,
60+
};
61+
5662
protected override render(): TemplateResult {
5763
return html`
58-
<sp-menu>
59-
<sp-menu-group>
64+
<sp-menu id="sp-menu">
65+
<sp-menu-group id="sp-menu-group-1">
6066
<sp-menu-item id="i-1">Before First</sp-menu-item>
6167
<slot name="before"></slot>
6268
</sp-menu-group>
63-
<sp-menu-group>
69+
<sp-menu-group id="sp-menu-group-2">
6470
<sp-menu-item id="i-4">Sibling 1</sp-menu-item>
6571
<slot></slot>
6672
<sp-menu-item id="i-10">Sibling 2</sp-menu-item>
6773
</sp-menu-group>
68-
<sp-menu-group>
74+
<sp-menu-group id="sp-menu-group-3">
6975
<sp-menu-item id="i-11">After 1</sp-menu-item>
7076
<sp-menu-item id="i-12">After 2</sp-menu-item>
7177
</sp-menu-group>
@@ -84,9 +90,15 @@ export class ComplexSlottedMenu extends LitElement {
8490
) as ComplexSlottedGroup
8591
).menu;
8692
}
93+
94+
static override shadowRootOptions = {
95+
...LitElement.shadowRootOptions,
96+
delegatesFocus: true,
97+
};
98+
8799
protected override render(): TemplateResult {
88100
return html`
89-
<complex-slotted-group id="group">
101+
<complex-slotted-group id="complex-slotted-group">
90102
<sp-menu-item id="i-5">Middle 1</sp-menu-item>
91103
<sp-menu-item id="i-6">Middle 2</sp-menu-item>
92104
<sp-menu-item id="i-7">Middle 3</sp-menu-item>

‎packages/menu/stories/menu.stories.ts

+22-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ governing permissions and limitations under the License.
1111
*/
1212
import { html, TemplateResult } from '@spectrum-web-components/base';
1313

14-
import type { Menu } from '@spectrum-web-components/menu';
14+
import type { Menu, MenuItem } from '@spectrum-web-components/menu';
1515
import '@spectrum-web-components/menu/sp-menu.js';
1616
import '@spectrum-web-components/popover/sp-popover.js';
1717
import '@spectrum-web-components/action-menu/sp-action-menu.js';
@@ -431,3 +431,24 @@ export const menuWithValueSlots = (): TemplateResult => {
431431
</sp-popover>
432432
`;
433433
};
434+
435+
headersAndIcons.storyName = 'Dynamic MenuItems';
436+
437+
export const dynamicRemoval = (): TemplateResult => {
438+
const removeItem = async function (event: FocusEvent) {
439+
await (event.target as MenuItem)?.updateComplete;
440+
(event.target as MenuItem)?.remove();
441+
};
442+
return html`
443+
<sp-menu id="casey" selects="single">
444+
<sp-menu-item>Deselect</sp-menu-item>
445+
<sp-menu-item>Select Inverse</sp-menu-item>
446+
<sp-menu-item id="nikkimk" @focus=${removeItem}>
447+
Feather...
448+
</sp-menu-item>
449+
<sp-menu-item selected>Select and Mask...</sp-menu-item>
450+
<sp-menu-item>Save Selection</sp-menu-item>
451+
<sp-menu-item disabled>Make Work Path</sp-menu-item>
452+
</sp-menu>
453+
`;
454+
};

‎packages/menu/test/menu-group.test.ts

+89-93
Original file line numberDiff line numberDiff line change
@@ -43,28 +43,7 @@ const focusableItems = (menu: Menu | MenuGroup): MenuItem[] => {
4343
describe('Menu group', () => {
4444
testForLitDevWarnings(
4545
async () =>
46-
await fixture<Menu>(
47-
html`
48-
<sp-menu selects="single">
49-
<sp-menu-group selects="inherit">
50-
<span slot="header">Section Heading</span>
51-
<sp-menu-item>Action 1</sp-menu-item>
52-
<sp-menu-item>Action 2</sp-menu-item>
53-
<sp-menu-item>Action 3</sp-menu-item>
54-
</sp-menu-group>
55-
<sp-menu-divider></sp-menu-divider>
56-
<sp-menu-group selects="inherit">
57-
<span slot="header">Section Heading</span>
58-
<sp-menu-item>Save</sp-menu-item>
59-
<sp-menu-item disabled>Download</sp-menu-item>
60-
</sp-menu-group>
61-
</sp-menu>
62-
`
63-
)
64-
);
65-
it('renders', async () => {
66-
const el = await fixture<Menu>(
67-
html`
46+
await fixture<Menu>(html`
6847
<sp-menu selects="single">
6948
<sp-menu-group selects="inherit">
7049
<span slot="header">Section Heading</span>
@@ -79,22 +58,40 @@ describe('Menu group', () => {
7958
<sp-menu-item disabled>Download</sp-menu-item>
8059
</sp-menu-group>
8160
</sp-menu>
82-
`
83-
);
61+
`)
62+
);
63+
it('renders', async () => {
64+
const el = await fixture<Menu>(html`
65+
<sp-menu selects="single">
66+
<sp-menu-group selects="inherit">
67+
<span slot="header">Section Heading</span>
68+
<sp-menu-item>Action 1</sp-menu-item>
69+
<sp-menu-item>Action 2</sp-menu-item>
70+
<sp-menu-item>Action 3</sp-menu-item>
71+
</sp-menu-group>
72+
<sp-menu-divider></sp-menu-divider>
73+
<sp-menu-group selects="inherit">
74+
<span slot="header">Section Heading</span>
75+
<sp-menu-item>Save</sp-menu-item>
76+
<sp-menu-item disabled>Download</sp-menu-item>
77+
</sp-menu-group>
78+
</sp-menu>
79+
`);
8480

85-
await waitUntil(() => {
86-
return managedItems(el).length === 5;
87-
}, `expected menu group to manage 5 children, received ${managedItems(el).length} of ${el.childItems.length}`);
81+
await waitUntil(
82+
() => {
83+
return managedItems(el).length === 5;
84+
},
85+
`expected menu group to manage 5 children, received ${managedItems(el).length} of ${el.childItems.length}`
86+
);
8887
await elementUpdated(el);
8988

9089
await expect(el).to.be.accessible();
9190
});
9291
it('manages [slot="header"] content', async () => {
93-
const el = await fixture<MenuGroup>(
94-
html`
95-
<sp-menu-group></sp-menu-group>
96-
`
97-
);
92+
const el = await fixture<MenuGroup>(html`
93+
<sp-menu-group></sp-menu-group>
94+
`);
9895
await elementUpdated(el);
9996
const slot = el.shadowRoot.querySelector(
10097
'[name="header"'
@@ -116,46 +113,44 @@ describe('Menu group', () => {
116113
expect(header.id).to.equal('');
117114
});
118115
it('handles selects for nested menu groups', async () => {
119-
const el = await fixture<Menu>(
120-
html`
121-
<sp-menu selects="single">
122-
<sp-menu-item selected>First</sp-menu-item>
123-
<!-- 1 -->
124-
<sp-menu-item>Second</sp-menu-item>
125-
<!-- 1 -->
126-
<sp-menu-group id="mg-multi" selects="multiple">
127-
<sp-menu-item selected>Multi1</sp-menu-item>
116+
const el = await fixture<Menu>(html`
117+
<sp-menu selects="single">
118+
<sp-menu-item selected>First</sp-menu-item>
119+
<!-- 1 -->
120+
<sp-menu-item>Second</sp-menu-item>
121+
<!-- 1 -->
122+
<sp-menu-group id="mg-multi" selects="multiple">
123+
<sp-menu-item selected>Multi1</sp-menu-item>
124+
<!-- 2 -->
125+
<sp-menu-item>Multi2</sp-menu-item>
126+
<!-- 2 -->
127+
<sp-menu-group id="mg-sub-inherit" selects="inherit">
128+
<sp-menu-item>SubInherit1</sp-menu-item>
128129
<!-- 2 -->
129-
<sp-menu-item>Multi2</sp-menu-item>
130+
<sp-menu-item>SubInherit2</sp-menu-item>
130131
<!-- 2 -->
131-
<sp-menu-group id="mg-sub-inherit" selects="inherit">
132-
<sp-menu-item>SubInherit1</sp-menu-item>
133-
<!-- 2 -->
134-
<sp-menu-item>SubInherit2</sp-menu-item>
135-
<!-- 2 -->
136-
</sp-menu-group>
137132
</sp-menu-group>
138-
<sp-menu-group id="mg-single" selects="single">
139-
<sp-menu-item selected>Single1</sp-menu-item>
140-
<!-- 3 -->
141-
<sp-menu-item>Single2</sp-menu-item>
142-
<!-- 3 -->
143-
</sp-menu-group>
144-
<sp-menu-group id="mg-none">
145-
<sp-menu-item>Inherit1</sp-menu-item>
146-
<!-- - -->
147-
<sp-menu-item>Inherit2</sp-menu-item>
148-
<!-- - -->
149-
</sp-menu-group>
150-
<sp-menu-group id="mg-inherit" selects="inherit">
151-
<sp-menu-item>Inherit1</sp-menu-item>
152-
<!-- 1 -->
153-
<sp-menu-item>Inherit2</sp-menu-item>
154-
<!-- 1 -->
155-
</sp-menu-group>
156-
</sp-menu>
157-
`
158-
);
133+
</sp-menu-group>
134+
<sp-menu-group id="mg-single" selects="single">
135+
<sp-menu-item selected>Single1</sp-menu-item>
136+
<!-- 3 -->
137+
<sp-menu-item>Single2</sp-menu-item>
138+
<!-- 3 -->
139+
</sp-menu-group>
140+
<sp-menu-group id="mg-none">
141+
<sp-menu-item>Inherit1</sp-menu-item>
142+
<!-- - -->
143+
<sp-menu-item>Inherit2</sp-menu-item>
144+
<!-- - -->
145+
</sp-menu-group>
146+
<sp-menu-group id="mg-inherit" selects="inherit">
147+
<sp-menu-item>Inherit1</sp-menu-item>
148+
<!-- 1 -->
149+
<sp-menu-item>Inherit2</sp-menu-item>
150+
<!-- 1 -->
151+
</sp-menu-group>
152+
</sp-menu>
153+
`);
159154

160155
// 1 & 3 should be menuitemradio
161156
// 2 shouwl menuitemcheckbox
@@ -317,22 +312,20 @@ describe('Menu group', () => {
317312
});
318313

319314
it('handles changing managed items for selects changes', async () => {
320-
const el = await fixture<Menu>(
321-
html`
322-
<sp-menu selects="multiple" value-separator="--">
323-
<sp-menu-item selected>First</sp-menu-item>
324-
<sp-menu-item>Second</sp-menu-item>
325-
<sp-menu-group id="mg-inherit" selects="inherit">
326-
<sp-menu-item>Inherit1</sp-menu-item>
327-
<sp-menu-item>Inherit2</sp-menu-item>
328-
<sp-menu-group id="mg-sub-inherit" selects="inherit">
329-
<sp-menu-item>SubInherit1</sp-menu-item>
330-
<sp-menu-item selected>SubInherit2</sp-menu-item>
331-
</sp-menu-group>
315+
const el = await fixture<Menu>(html`
316+
<sp-menu selects="multiple" value-separator="--">
317+
<sp-menu-item selected>First</sp-menu-item>
318+
<sp-menu-item>Second</sp-menu-item>
319+
<sp-menu-group id="mg-inherit" selects="inherit">
320+
<sp-menu-item>Inherit1</sp-menu-item>
321+
<sp-menu-item>Inherit2</sp-menu-item>
322+
<sp-menu-group id="mg-sub-inherit" selects="inherit">
323+
<sp-menu-item>SubInherit1</sp-menu-item>
324+
<sp-menu-item selected>SubInherit2</sp-menu-item>
332325
</sp-menu-group>
333-
</sp-menu>
334-
`
335-
);
326+
</sp-menu-group>
327+
</sp-menu>
328+
`);
336329

337330
await waitUntil(
338331
() => managedItems(el).length == 6,
@@ -381,9 +374,12 @@ describe('Menu group', () => {
381374
await elementUpdated(inheritGroup);
382375
await elementUpdated(el);
383376

384-
await waitUntil(() => {
385-
return managedItems(inheritGroup).length === 4;
386-
}, `expected new single sub-group to manage 4 items, received ${managedItems(inheritGroup).length} because "selects === ${inheritGroup.selects}`);
377+
await waitUntil(
378+
() => {
379+
return managedItems(inheritGroup).length === 4;
380+
},
381+
`expected new single sub-group to manage 4 items, received ${managedItems(inheritGroup).length} because "selects === ${inheritGroup.selects}`
382+
);
387383

388384
await waitUntil(
389385
() => managedItems(el).length === 2,
@@ -419,13 +415,13 @@ describe('Menu group', () => {
419415
items.i6 = el.renderRoot.querySelector('#i-6') as MenuItem;
420416
items.i7 = el.renderRoot.querySelector('#i-7') as MenuItem;
421417
const group = el.renderRoot.querySelector(
422-
'#group'
418+
'#complex-slotted-group'
423419
) as ComplexSlottedGroup;
424-
items.i1 = group.renderRoot.querySelector('#i-1') as MenuItem;
425-
items.i4 = group.renderRoot.querySelector('#i-4') as MenuItem;
426-
items.i10 = group.renderRoot.querySelector('#i-10') as MenuItem;
427-
items.i11 = group.renderRoot.querySelector('#i-11') as MenuItem;
428-
items.i12 = group.renderRoot.querySelector('#i-12') as MenuItem;
420+
items.i1 = group?.renderRoot.querySelector('#i-1') as MenuItem;
421+
items.i4 = group?.renderRoot.querySelector('#i-4') as MenuItem;
422+
items.i10 = group?.renderRoot.querySelector('#i-10') as MenuItem;
423+
items.i11 = group?.renderRoot.querySelector('#i-11') as MenuItem;
424+
items.i12 = group?.renderRoot.querySelector('#i-12') as MenuItem;
429425

430426
const rect = items.i9.getBoundingClientRect();
431427
await sendMouse({

‎packages/menu/test/menu-item.test.ts

+49-2
Original file line numberDiff line numberDiff line change
@@ -117,11 +117,59 @@ describe('Menu item', () => {
117117
).anchorElement.dispatchEvent(new FocusEvent('focus'));
118118

119119
await elementUpdated(item);
120-
expect(el === document.activeElement).to.be.true;
120+
121+
expect(item === document.activeElement).to.be.true;
121122
item.click();
122123

123124
expect(clickTargetSpy.calledWith(anchorElement)).to.be.true;
124125
});
126+
it('allows link click', async () => {
127+
const clickTargetSpy = spy();
128+
const el = await fixture<Menu>(html`
129+
<sp-menu
130+
@click=${(event: Event) => {
131+
clickTargetSpy(
132+
event.composedPath()[0] as HTMLAnchorElement
133+
);
134+
event.stopPropagation();
135+
event.preventDefault();
136+
}}
137+
>
138+
<sp-menu-item
139+
href="https://opensource.adobe.com/spectrum-web-components"
140+
>
141+
Selected Text
142+
</sp-menu-item>
143+
</sp-menu>
144+
`);
145+
146+
const item = el.querySelector('sp-menu-item') as MenuItem;
147+
const { anchorElement } = item as unknown as {
148+
anchorElement: HTMLAnchorElement;
149+
};
150+
(
151+
item as unknown as { anchorElement: HTMLAnchorElement }
152+
).anchorElement.dispatchEvent(new FocusEvent('focus'));
153+
154+
await elementUpdated(item);
155+
expect(item === document.activeElement).to.be.true;
156+
157+
// tests mouse click events, and by extension VoiceOver CRTL+Option+Space click
158+
const rect = el.getBoundingClientRect();
159+
await sendMouse({
160+
steps: [
161+
{
162+
position: [
163+
rect.left + rect.width / 2,
164+
rect.top + rect.height / 2,
165+
],
166+
type: 'click',
167+
},
168+
],
169+
});
170+
171+
expect(clickTargetSpy.calledWith(anchorElement)).to.be.true;
172+
});
125173
it('value attribute', async () => {
126174
const el = await fixture<MenuItem>(html`
127175
<sp-menu-item value="selected" selected>Selected Text</sp-menu-item>
@@ -193,7 +241,6 @@ describe('Menu item', () => {
193241

194242
expect(el.hasSubmenu).to.be.false;
195243
});
196-
197244
it('should not allow text-align to cascade when used inside an overlay', async () => {
198245
const element = await fixture<HTMLDivElement>(html`
199246
<div style="text-align: center">

‎packages/menu/test/menu-selects.test.ts

+43-50
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,13 @@ describe('Menu [selects]', () => {
2929
let el!: Menu;
3030
let options!: MenuItem[];
3131
beforeEach(async () => {
32-
el = await fixture<Menu>(
33-
html`
34-
<sp-menu selects="single">
35-
<sp-menu-item value="1">Option 1</sp-menu-item>
36-
<sp-menu-item value="2">Option 2</sp-menu-item>
37-
<sp-menu-item value="3">Option 3</sp-menu-item>
38-
</sp-menu>
39-
`
40-
);
32+
el = await fixture<Menu>(html`
33+
<sp-menu selects="single">
34+
<sp-menu-item value="1">Option 1</sp-menu-item>
35+
<sp-menu-item value="2">Option 2</sp-menu-item>
36+
<sp-menu-item value="3">Option 3</sp-menu-item>
37+
</sp-menu>
38+
`);
4139
options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[];
4240
await Promise.all(options.map((option) => option.updateComplete));
4341
await nextFrame();
@@ -151,17 +149,15 @@ describe('Menu [selects] w/ group', () => {
151149
let el!: Menu;
152150
let options!: MenuItem[];
153151
beforeEach(async () => {
154-
el = await fixture<Menu>(
155-
html`
156-
<sp-menu selects="single">
157-
<sp-menu-group selects="inherit">
158-
<sp-menu-item value="1">Option 1</sp-menu-item>
159-
<sp-menu-item value="2">Option 2</sp-menu-item>
160-
<sp-menu-item value="3">Option 3</sp-menu-item>
161-
</sp-menu-group>
162-
</sp-menu>
163-
`
164-
);
152+
el = await fixture<Menu>(html`
153+
<sp-menu selects="single">
154+
<sp-menu-group selects="inherit">
155+
<sp-menu-item value="1">Option 1</sp-menu-item>
156+
<sp-menu-item value="2">Option 2</sp-menu-item>
157+
<sp-menu-item value="3">Option 3</sp-menu-item>
158+
</sp-menu-group>
159+
</sp-menu>
160+
`);
165161
options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[];
166162
await Promise.all(options.map((option) => option.updateComplete));
167163
await nextFrame();
@@ -275,17 +271,15 @@ describe('Menu w/ group [selects]', () => {
275271
let group!: MenuGroup;
276272
let options!: MenuItem[];
277273
beforeEach(async () => {
278-
el = await fixture<Menu>(
279-
html`
280-
<sp-menu>
281-
<sp-menu-group selects="single">
282-
<sp-menu-item value="1">Option 1</sp-menu-item>
283-
<sp-menu-item value="2">Option 2</sp-menu-item>
284-
<sp-menu-item value="3">Option 3</sp-menu-item>
285-
</sp-menu-group>
286-
</sp-menu>
287-
`
288-
);
274+
el = await fixture<Menu>(html`
275+
<sp-menu>
276+
<sp-menu-group selects="single">
277+
<sp-menu-item value="1">Option 1</sp-menu-item>
278+
<sp-menu-item value="2">Option 2</sp-menu-item>
279+
<sp-menu-item value="3">Option 3</sp-menu-item>
280+
</sp-menu-group>
281+
</sp-menu>
282+
`);
289283
group = el.querySelector('sp-menu-group') as MenuGroup;
290284
options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[];
291285
await Promise.all(options.map((option) => option.updateComplete));
@@ -403,22 +397,20 @@ describe('Menu w/ groups [selects]', () => {
403397
let groupB!: MenuGroup;
404398
let options!: MenuItem[];
405399
beforeEach(async () => {
406-
el = await fixture<Menu>(
407-
html`
408-
<sp-menu>
409-
<sp-menu-group selects="single" id="group-1">
410-
<sp-menu-item value="1a">Option 1a</sp-menu-item>
411-
<sp-menu-item value="2a">Option 2a</sp-menu-item>
412-
<sp-menu-item value="3a">Option 3a</sp-menu-item>
413-
</sp-menu-group>
414-
<sp-menu-group selects="single" id="group-2">
415-
<sp-menu-item value="1b">Option 1b</sp-menu-item>
416-
<sp-menu-item value="2b">Option 2b</sp-menu-item>
417-
<sp-menu-item value="3b">Option 3b</sp-menu-item>
418-
</sp-menu-group>
419-
</sp-menu>
420-
`
421-
);
400+
el = await fixture<Menu>(html`
401+
<sp-menu>
402+
<sp-menu-group selects="single" id="group-1">
403+
<sp-menu-item value="1a">Option 1a</sp-menu-item>
404+
<sp-menu-item value="2a">Option 2a</sp-menu-item>
405+
<sp-menu-item value="3a">Option 3a</sp-menu-item>
406+
</sp-menu-group>
407+
<sp-menu-group selects="single" id="group-2">
408+
<sp-menu-item value="1b">Option 1b</sp-menu-item>
409+
<sp-menu-item value="2b">Option 2b</sp-menu-item>
410+
<sp-menu-item value="3b">Option 3b</sp-menu-item>
411+
</sp-menu-group>
412+
</sp-menu>
413+
`);
422414
groupA = el.querySelector('sp-menu-group:first-child') as MenuGroup;
423415
groupB = el.querySelector('sp-menu-group:last-child') as MenuGroup;
424416
options = [...el.querySelectorAll('sp-menu-item')] as MenuItem[];
@@ -638,16 +630,17 @@ describe('Menu w/ groups [selects]', () => {
638630
input.focus();
639631
expect(document.activeElement === input).to.be.true;
640632
await sendKeys({ press: 'Shift+Tab' });
641-
expect(document.activeElement === groupA).to.be.true;
633+
expect(document.activeElement === options[0]).to.be.true;
642634
await sendKeys({ press: 'ArrowDown' });
635+
expect(document.activeElement === options[1]).to.be.true;
643636
await sendKeys({ press: 'ArrowUp' });
644637

645638
await elementUpdated(el);
646639
let optionCount = 0;
647640
for (const option of options) {
648641
const parentElement = option.parentElement as Menu;
649-
expect(document.activeElement === parentElement, 'parent focused')
650-
.to.be.true;
642+
expect(document.activeElement === option, 'option focused').to.be
643+
.true;
651644
expect(option.focused, `option ${optionCount} visually focused`).to
652645
.be.true;
653646
await sendKeys({ press: 'Space' });

‎packages/menu/test/menu.test.ts

+125-110
Large diffs are not rendered by default.

‎packages/menu/test/submenu.test.ts

+74-34
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import '@spectrum-web-components/overlay/sp-overlay.js';
3232
import '@spectrum-web-components/icons-workflow/icons/sp-icon-show-menu.js';
3333
import { TemplateResult } from 'lit-html';
3434
import { slottableRequest } from '@spectrum-web-components/overlay/src/slottable-request-directive.js';
35+
import { isWebKit } from '@spectrum-web-components/shared';
3536

3637
type SelectsWithKeyboardTest = {
3738
dir: 'ltr' | 'rtl' | 'auto';
@@ -110,48 +111,70 @@ describe('Submenu', () => {
110111
this.el.parentElement.dir = testData.dir;
111112

112113
await elementUpdated(this.el);
113-
expect(this.rootItem.open).to.be.false;
114+
expect(
115+
this.rootItem.open,
116+
`rootItem open before ${testData.openKey}`
117+
).to.be.false;
114118
const input = document.createElement('input');
115119
this.el.insertAdjacentElement('beforebegin', input);
116-
input.focus();
117-
await sendKeys({
118-
press: 'Tab',
119-
});
120+
this.el.focus();
121+
122+
// by default, Safari doesn't tab to some elements
123+
if (!isWebKit) {
124+
await sendKeys({
125+
press: 'Shift+Tab',
126+
});
127+
128+
expect(document.activeElement).to.equal(input);
129+
await sendKeys({
130+
press: 'Tab',
131+
});
132+
133+
expect(document.activeElement).to.equal(this.el);
134+
}
120135
await sendKeys({
121136
press: 'ArrowDown',
122137
});
123138
await elementUpdated(this.rootItem);
124139

125-
expect(this.rootItem.focused).to.be.true;
140+
expect(
141+
this.rootItem.focused,
142+
`rootItem focused before ${testData.openKey}`
143+
).to.be.true;
126144

127145
let opened = oneEvent(this.rootItem, 'sp-opened');
128146
await sendKeys({
129147
press: testData.openKey,
130148
});
131149
await opened;
132150

151+
const rootItem = this.el.querySelector('.root') as MenuItem;
133152
let submenu = this.el.querySelector('[slot="submenu"]') as Menu;
134153
let submenuItem = this.el.querySelector(
135-
'.submenu-item-2'
154+
'.submenu-item-1'
136155
) as MenuItem;
137156

138-
expect(this.rootItem.open).to.be.true;
139157
expect(
140-
submenu === document.activeElement,
141-
`${document.activeElement?.id}`
158+
this.rootItem.open,
159+
`rootItem open after ${testData.openKey}`
142160
).to.be.true;
143161

162+
//opening a menu via keyboard should set focus on first item
163+
expect(document.activeElement).to.equal(submenuItem);
164+
144165
let closed = oneEvent(this.rootItem, 'sp-closed');
145166
await sendKeys({
146167
press: testData.closeKey,
147168
});
148169
await closed;
149170

150-
expect(this.rootItem.open).to.be.false;
151171
expect(
152-
this.el === document.activeElement,
153-
`${document.activeElement?.id}`
154-
).to.be.true;
172+
this.rootItem.open,
173+
`rootItem open after ${testData.closeKey}`
174+
).to.be.false;
175+
176+
//closing a submenu via keyboard should set focus on its parent menuitem
177+
expect(document.activeElement).to.equal(rootItem);
155178

156179
opened = oneEvent(this.rootItem, 'sp-opened');
157180
await sendKeys({
@@ -160,19 +183,21 @@ describe('Submenu', () => {
160183
await opened;
161184

162185
submenu = this.el.querySelector('[slot="submenu"]') as Menu;
163-
submenuItem = this.el.querySelector('.submenu-item-2') as MenuItem;
164186

165187
expect(this.rootItem.open).to.be.true;
188+
expect(submenuItem.focused).to.be.true;
189+
expect(document.activeElement).to.equal(submenuItem);
166190

167191
await sendKeys({
168192
press: 'ArrowDown',
169193
});
170194
await elementUpdated(submenu);
171195
await elementUpdated(submenuItem);
172196

173-
expect(submenu.getAttribute('aria-activedescendant')).to.equal(
174-
submenuItem.id
175-
);
197+
submenuItem = this.el.querySelector('.submenu-item-2') as MenuItem;
198+
expect(submenuItem.focused, `submenu focused`).to.be.true;
199+
expect(document.activeElement === submenuItem, `submenu active`).to
200+
.be.true;
176201

177202
closed = oneEvent(this.rootItem, 'sp-closed');
178203
await sendKeys({
@@ -193,13 +218,20 @@ describe('Submenu', () => {
193218
it('returns visible focus when submenu closed', async function () {
194219
const input = document.createElement('input');
195220
this.el.insertAdjacentElement('beforebegin', input);
196-
input.focus();
197-
await sendKeys({
198-
press: 'Tab',
199-
});
200-
await elementUpdated(this.el);
201-
await nextFrame();
202-
await nextFrame();
221+
// by default, Safari doesn't tab to some elements
222+
if (!isWebKit) {
223+
await sendKeys({
224+
press: 'Shift+Tab',
225+
});
226+
227+
expect(document.activeElement).to.equal(input);
228+
await sendKeys({
229+
press: 'Tab',
230+
});
231+
232+
expect(document.activeElement).to.equal(this.el);
233+
}
234+
this.el.focus();
203235
await sendKeys({
204236
press: 'ArrowDown',
205237
});
@@ -212,6 +244,7 @@ describe('Submenu', () => {
212244
`focused: ${document.activeElement?.localName}`
213245
).to.be.true;
214246
expect(this.rootItem.open, 'not open').to.be.false;
247+
expect(document.activeElement).to.equal(this.rootItem);
215248

216249
const opened = oneEvent(this.rootItem, 'sp-opened');
217250
await sendKeys({
@@ -330,7 +363,6 @@ describe('Submenu', () => {
330363
],
331364
});
332365
await opened;
333-
334366
expect(this.rootItem.open).to.be.true;
335367

336368
const closed = oneEvent(this.rootItem, 'sp-closed');
@@ -552,7 +584,7 @@ describe('Submenu', () => {
552584
this.rootItem = this.el.querySelector('.root') as MenuItem;
553585
await elementUpdated(this.rootItem);
554586
});
555-
describe.skip('selects', () => {
587+
describe('selects', () => {
556588
selectWithPointer();
557589
selectsWithKeyboardData.map((testData) => {
558590
selectsWithKeyboard(testData);
@@ -716,7 +748,7 @@ describe('Submenu', () => {
716748
expect(subSubmenuChanged.calledWith('C'), 'sub submenu changed').to.be
717749
.true;
718750
});
719-
it('closes all decendent submenus when closing a ancestor menu', async () => {
751+
it('closes all descendant submenus when closing a ancestor menu', async () => {
720752
const el = await fixture<ActionMenu>(html`
721753
<sp-action-menu label="Closing ancestors will close submenus">
722754
<sp-icon-show-menu slot="icon"></sp-icon-show-menu>
@@ -879,7 +911,7 @@ describe('Submenu', () => {
879911
});
880912
await closed;
881913
});
882-
it('closes decendent menus when Menu Item in ancestor without a submenu is pointerentered', async function () {
914+
it('closes descendant menus when Menu Item in ancestor without a submenu is pointerentered', async function () {
883915
const rootMenu = this.el.querySelector(
884916
'#submenu-item-1'
885917
) as MenuItem;
@@ -903,7 +935,7 @@ describe('Submenu', () => {
903935
);
904936
await closed;
905937
});
906-
it('closes decendent menus when Menu Item in ancestor is clicked', async function () {
938+
it('closes descendant menus when Menu Item in ancestor is clicked', async function () {
907939
const rootMenu1 = this.el.querySelector(
908940
'#submenu-item-1'
909941
) as MenuItem;
@@ -1135,9 +1167,15 @@ describe('Submenu', () => {
11351167
<sp-menu-item>
11361168
Parent Item
11371169
<div role="menu" slot="submenu">
1138-
${Array(20).fill(0).map((_, i) => html`
1139-
<sp-menu-item>Submenu Item ${i + 1}</sp-menu-item>
1140-
`)}
1170+
${Array(20)
1171+
.fill(0)
1172+
.map(
1173+
(_, i) => html`
1174+
<sp-menu-item>
1175+
Submenu Item ${i + 1}
1176+
</sp-menu-item>
1177+
`
1178+
)}
11411179
</div>
11421180
</sp-menu-item>
11431181
</sp-menu>
@@ -1146,7 +1184,9 @@ describe('Submenu', () => {
11461184
await elementUpdated(el);
11471185

11481186
const menuItem = el.querySelector('sp-menu-item') as MenuItem;
1149-
const submenu = menuItem.querySelector('[slot="submenu"]') as HTMLElement;
1187+
const submenu = menuItem.querySelector(
1188+
'[slot="submenu"]'
1189+
) as HTMLElement;
11501190

11511191
// Open the submenu
11521192
const opened = oneEvent(menuItem, 'sp-opened');

‎packages/overlay/stories/overlay.stories.ts

+1
Original file line numberDiff line numberDiff line change
@@ -473,6 +473,7 @@ class ComplexModalReady extends HTMLElement {
473473
handlePickerOpen = async (): Promise<void> => {
474474
const picker = document.querySelector('#test-picker') as Picker;
475475
const actions = [nextFrame, picker.updateComplete];
476+
picker.focus();
476477

477478
await Promise.all(actions);
478479

‎packages/picker/src/InteractionController.ts

+10-1
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,10 @@ export class InteractionController implements ReactiveController {
137137
this.host.isMobile.matches && !this.host.forcePopover
138138
? undefined
139139
: this.host.placement;
140-
this.overlay.receivesFocus = 'true';
140+
// We should not be applying open is set programmatically via the picker's open.property.
141+
// Focus should only be applied if a user action causes the menu to open. Otherwise,
142+
// we could be pulling focus from a user when an picker with an open menu loads.
143+
this.overlay.receivesFocus = 'false';
141144
this.overlay.willPreventClose =
142145
this.preventNextToggle !== 'no' && this.open;
143146
this.overlay.addEventListener(
@@ -158,6 +161,7 @@ export class InteractionController implements ReactiveController {
158161
) {
159162
this.preventNextToggle = 'yes';
160163
}
164+
if (this.preventNextToggle === 'no') this.host.close();
161165
}
162166

163167
public handleActivate(_event: Event): void {}
@@ -172,6 +176,11 @@ export class InteractionController implements ReactiveController {
172176

173177
hostConnected(): void {
174178
this.init();
179+
this.host.addEventListener('sp-closed', ()=> {
180+
if(!this.open && this.host.optionsMenu.matches(':focus-within') && !this.host.button.matches(':focus')) {
181+
this.host.button.focus();
182+
}
183+
});
175184
}
176185

177186
hostDisconnected(): void {

‎packages/picker/src/MobileController.ts

+5-1
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,12 @@ export class MobileController extends InteractionController {
2020
override type = InteractionTypes.mobile;
2121

2222
handleClick(): void {
23+
if (this.host.disabled) {
24+
return;
25+
}
2326
if (this.preventNextToggle == 'no') {
24-
this.open = !this.open;
27+
28+
this.host.toggle();
2529
}
2630
this.preventNextToggle = 'no';
2731
}

‎packages/picker/src/Picker.ts

+170-84
Large diffs are not rendered by default.

‎packages/picker/test/index.ts

+297-266
Large diffs are not rendered by default.

‎packages/picker/test/picker-reparenting.test.ts

+12-3
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,14 @@ import '@spectrum-web-components/menu/sp-menu-item.js';
1515
import '@spectrum-web-components/menu/sp-menu-divider.js';
1616
import { Picker } from '@spectrum-web-components/picker';
1717
import { MenuItem } from '@spectrum-web-components/menu';
18-
import { expect, fixture, html, nextFrame, oneEvent } from '@open-wc/testing';
18+
import {
19+
elementUpdated,
20+
expect,
21+
fixture,
22+
html,
23+
nextFrame,
24+
oneEvent,
25+
} from '@open-wc/testing';
1926

2027
import '@spectrum-web-components/theme/sp-theme.js';
2128
import '@spectrum-web-components/theme/src/themes.js';
@@ -79,6 +86,7 @@ describe('Reparented Picker', () => {
7986
const { picker, before, after } = await fixtureElements();
8087

8188
expect(picker.value).to.equal('');
89+
picker.id = 'nikki';
8290

8391
const item2 = picker.querySelector('[value="2"]') as MenuItem;
8492
const item3 = picker.querySelector('[value="3"]') as MenuItem;
@@ -110,11 +118,12 @@ describe('Reparented Picker', () => {
110118
opened = oneEvent(picker, 'sp-opened');
111119
picker.click();
112120
await opened;
121+
await nextFrame();
113122
expect(picker.open).to.be.true;
114123
expect(picker.value).to.equal('3');
115-
closed = oneEvent(picker, 'sp-closed');
124+
opened = oneEvent(picker, 'sp-opened');
116125
before.append(picker);
117-
await closed;
126+
await elementUpdated(picker);
118127

119128
expect(
120129
(picker as unknown as TestablePicker).optionsMenu.value

‎packages/tabs/test/tabs.test.ts

-42
Original file line numberDiff line numberDiff line change
@@ -353,48 +353,6 @@ describe('Tabs', () => {
353353
await elementUpdated(el);
354354
expect(el.selected).to.be.equal('first');
355355
});
356-
it('prevents [tabindex=0] while `focusin`', async () => {
357-
const el = await fixture<Tabs>(html`
358-
<sp-tabs>
359-
<sp-tab label="Tab 1" value="first" selected>
360-
<sp-icon-checkmark slot="icon"></sp-icon-checkmark>
361-
</sp-tab>
362-
<sp-tab label="Tab 2" value="second">
363-
<sp-icon-checkmark slot="icon"></sp-icon-checkmark>
364-
</sp-tab>
365-
</sp-tabs>
366-
`);
367-
368-
const selected = el.querySelector('[value="first"]') as Tab;
369-
const toBeSelected = el.querySelector('[value="second"]') as Tab;
370-
371-
await elementUpdated(el);
372-
await waitUntil(() => el.selected === 'first', 'wait for selection');
373-
374-
expect(el.selected).to.equal('first');
375-
expect(selected.tabIndex).to.equal(0);
376-
377-
toBeSelected.dispatchEvent(new Event('focusin', { bubbles: true }));
378-
379-
await elementUpdated(el);
380-
381-
expect(el.selected).to.equal('first');
382-
expect(selected.tabIndex).to.equal(-1);
383-
384-
toBeSelected.dispatchEvent(new Event('focusout', { bubbles: true }));
385-
386-
await elementUpdated(el);
387-
388-
expect(el.selected).to.equal('first');
389-
expect(selected.tabIndex).to.equal(0);
390-
391-
toBeSelected.click();
392-
393-
await elementUpdated(el);
394-
395-
expect(el.selected).to.equal('second');
396-
expect(toBeSelected.tabIndex).to.equal(0);
397-
});
398356
it('accepts keyboard based selection', async () => {
399357
const el = await fixture<Tabs>(html`
400358
<sp-tabs selected="Unknown">

‎tools/reactive-controllers/src/FocusGroup.ts

+55-8
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import type { ReactiveController, ReactiveElement } from 'lit';
1313

1414
type DirectionTypes = 'horizontal' | 'vertical' | 'both' | 'grid';
1515
export type FocusGroupConfig<T> = {
16+
hostDelegatesFocus?: boolean;
1617
focusInIndex?: (_elements: T[]) => number;
1718
direction?: DirectionTypes | (() => DirectionTypes);
1819
elementEnterAction?: (el: T) => void;
@@ -63,6 +64,8 @@ export class FocusGroupController<T extends HTMLElement>
6364

6465
public directionLength = 5;
6566

67+
public hostDelegatesFocus = false;
68+
6669
elementEnterAction = (_el: T): void => {
6770
return;
6871
};
@@ -120,6 +123,7 @@ export class FocusGroupController<T extends HTMLElement>
120123
constructor(
121124
host: ReactiveElement,
122125
{
126+
hostDelegatesFocus,
123127
direction,
124128
elementEnterAction,
125129
elements,
@@ -131,6 +135,7 @@ export class FocusGroupController<T extends HTMLElement>
131135
this.mutationObserver = new MutationObserver(() => {
132136
this.handleItemMutation();
133137
});
138+
this.hostDelegatesFocus = hostDelegatesFocus || false;
134139
this.host = host;
135140
this.host.addController(this);
136141
this._elements = elements;
@@ -180,18 +185,60 @@ export class FocusGroupController<T extends HTMLElement>
180185
this.manage();
181186
}
182187

183-
focus(options?: FocusOptions): void {
188+
/**
189+
* resets the focusedItem to initial item
190+
*/
191+
reset(): void {
184192
const elements = this.elements;
185193
if (!elements.length) return;
194+
this.setCurrentIndexCircularly(this.focusInIndex - this.currentIndex);
186195
let focusElement = elements[this.currentIndex];
196+
if (this.currentIndex < 0) {
197+
return;
198+
}
187199
if (!focusElement || !this.isFocusableElement(focusElement)) {
188200
this.setCurrentIndexCircularly(1);
189201
focusElement = elements[this.currentIndex];
190202
}
191203
if (focusElement && this.isFocusableElement(focusElement)) {
192204
elements[this.prevIndex]?.setAttribute('tabindex', '-1');
205+
focusElement.setAttribute('tabindex', '0');
206+
}
207+
}
208+
209+
focusOnItem(item?: T, options?: FocusOptions): void {
210+
if (
211+
item &&
212+
this.isFocusableElement(item) &&
213+
this.elements.indexOf(item)
214+
) {
215+
const diff = this.elements.indexOf(item) - this.currentIndex;
216+
this.setCurrentIndexCircularly(diff);
217+
this.elements[this.prevIndex]?.setAttribute('tabindex', '-1');
218+
}
219+
this.focus(options);
220+
}
221+
222+
focus(options?: FocusOptions): void {
223+
const elements = this.elements;
224+
if (!elements.length) return;
225+
let focusElement = elements[this.currentIndex];
226+
if (!focusElement || !this.isFocusableElement(focusElement)) {
227+
this.setCurrentIndexCircularly(1);
228+
focusElement = elements[this.currentIndex];
229+
}
230+
if (focusElement && this.isFocusableElement(focusElement)) {
231+
if (
232+
!this.hostDelegatesFocus ||
233+
elements[this.prevIndex] !== focusElement
234+
) {
235+
elements[this.prevIndex]?.setAttribute('tabindex', '-1');
236+
}
193237
focusElement.tabIndex = 0;
194238
focusElement.focus(options);
239+
if (this.hostDelegatesFocus && !this.focused) {
240+
this.hostContainsFocus();
241+
}
195242
}
196243
}
197244

@@ -298,28 +345,28 @@ export class FocusGroupController<T extends HTMLElement>
298345
}
299346
};
300347

301-
acceptsEventCode(code: string): boolean {
302-
if (code === 'End' || code === 'Home') {
348+
acceptsEventKey(key: string): boolean {
349+
if (key === 'End' || key === 'Home') {
303350
return true;
304351
}
305352
switch (this.direction) {
306353
case 'horizontal':
307-
return code === 'ArrowLeft' || code === 'ArrowRight';
354+
return key === 'ArrowLeft' || key === 'ArrowRight';
308355
case 'vertical':
309-
return code === 'ArrowUp' || code === 'ArrowDown';
356+
return key === 'ArrowUp' || key === 'ArrowDown';
310357
case 'both':
311358
case 'grid':
312-
return code.startsWith('Arrow');
359+
return key.startsWith('Arrow');
313360
}
314361
}
315362

316363
handleKeydown = (event: KeyboardEvent): void => {
317-
if (!this.acceptsEventCode(event.code) || event.defaultPrevented) {
364+
if (!this.acceptsEventKey(event.key) || event.defaultPrevented) {
318365
return;
319366
}
320367
let diff = 0;
321368
this.prevIndex = this.currentIndex;
322-
switch (event.code) {
369+
switch (event.key) {
323370
case 'ArrowRight':
324371
diff += 1;
325372
break;

‎tools/reactive-controllers/src/RovingTabindex.ts

+1-2
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ export class RovingTabindexController<
4545
}
4646

4747
manageTabindexes(): void {
48-
if (this.focused) {
48+
if (this.focused && !this.hostDelegatesFocus) {
4949
this.updateTabindexes(() => ({ tabIndex: -1 }));
5050
} else {
5151
this.updateTabindexes((el: HTMLElement): UpdateTabIndexes => {
@@ -72,7 +72,6 @@ export class RovingTabindexController<
7272
}
7373
return;
7474
}
75-
el.removeAttribute('tabindex');
7675
const updatable = el as unknown as {
7776
requestUpdate?: () => void;
7877
};

‎tools/reactive-controllers/test/roving-tabindex-integration.test.ts

+12-20
Original file line numberDiff line numberDiff line change
@@ -272,35 +272,27 @@ describe('tabIndex is cached properly', () => {
272272
</sp-action-menu>
273273
`);
274274

275-
expect(menuEl.tabIndex).to.equal(0);
276-
expect(menuEl.focusElement?.tabIndex).to.equal(0);
277-
278-
menuEl.tabIndex = 1;
279-
280-
await elementUpdated(menuEl);
281-
282-
expect(menuEl.tabIndex).to.equal(1);
283-
expect(menuEl.focusElement?.tabIndex).to.equal(1);
275+
expect(
276+
menuEl.focusElement?.tabIndex,
277+
'button tabindex before disabling'
278+
).to.equal(0);
284279

285280
menuEl.disabled = true;
286281

287282
await elementUpdated(menuEl);
288283

289-
expect(menuEl.tabIndex).to.equal(-1);
290-
expect(menuEl.focusElement?.tabIndex).to.equal(-1);
291-
292-
menuEl.tabIndex = 2;
293-
294-
await elementUpdated(menuEl);
295-
296-
expect(menuEl.tabIndex).to.equal(-1);
297-
expect(menuEl.focusElement?.tabIndex).to.equal(-1);
284+
expect(
285+
menuEl.focusElement?.tabIndex,
286+
'button tabindex after disabling'
287+
).to.equal(-1);
298288

299289
menuEl.disabled = false;
300290

301291
await elementUpdated(menuEl);
302292

303-
expect(menuEl.tabIndex).to.equal(2);
304-
expect(menuEl.focusElement?.tabIndex).to.equal(2);
293+
expect(
294+
menuEl.focusElement?.tabIndex,
295+
'button tabindex after setting to 0'
296+
).to.equal(0);
305297
});
306298
});

0 commit comments

Comments
 (0)
Please sign in to comment.