Skip to content

Commit a69accb

Browse files
nikkimkRajdeepccaseyisonitrenovate[bot]castastrophe
authored
fix(menu): menuitem focus and hover styles (#5270)
* fix(menu): hover styling supersedes focus * fix(menu): differentiates pointer from keyboard * fix(menu): pointer focus should happen before overlay open * test: added helpers for sending mouse to or from an element or rect * test: added helpers for sending mouse to or from an element or rect * test: added helpers for sending mouse to or from an element or rect * test(menu): updated menu tests and fixed some flakiness * test(menu): updated tests based on how pointer and keyboard should work * fix(menu): keyboard actions should focus on submenu item * chore: updated golden images hash and changeset * chore: update dependency @spectrum-css/tray to v4.0.1 * fix(tray,styles): use latest CSS * chore: update dependency @spectrum-css/card to v10.0.1 * fix(card,styles): use latest CSS * chore: update dependency @spectrum-css/underlay to v6.1.0 (#5296) * chore: update dependency @spectrum-css/underlay to v5.0.1 * fix(underlay,styles): use latest CSS --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: [ Cassondra ] <[email protected]> * chore: update dependency @spectrum-css/colorarea to v7.1.0 (#5297) * chore: update dependency @spectrum-css/colorarea to v6.0.1 * fix(color-area,styles): use latest CSS --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: [ Cassondra ] <[email protected]> * chore: update dependency @spectrum-css/buttongroup to v8.0.1 * fix(buttongroup,styles): use latest CSS * fix(overlay): prevent overlay close on document scroll (#5308) * fix(overlay): prevent overlay close on document scroll * chore: added changeset * fix(overlay): updated overlay position during scroll * chore: updated with waitUntil and removed nextFrame for predictibility --------- Co-authored-by: Rajdeep Chandra <[email protected]> * chore: update dependency @spectrum-css/divider to v5.1.0 (#5298) * chore: update dependency @spectrum-css/divider to v4.0.1 * fix(divider,styles): use latest CSS --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: [ Cassondra ] <[email protected]> * chore: align and update changesets * chore: update dependency @spectrum-css/colorslider to v8 * fix(color-slider,styles): remove unneeded system layer * chore: update dependency @spectrum-css/colorwheel to v5.0.1 * fix(color-wheel,styles): use latest CSS * chore: update dependency @spectrum-css/taggroup to v7 * fix(tags,styles): remove unneeded system layer * chore: update dependency @spectrum-css/helptext to v7 * fix(help-text,styles): remove unneeded system layer * chore: update dependency @spectrum-css/sidenav to v7 * fix(sidenav,styles): remove unneeded system layer * fix(menu): capture touchend on mobile (#5313) * fix(menu): capture touchend on mobile * chore: add changeset * chore: updated code comments * chore: update comment Co-authored-by: Rúben Carvalho <[email protected]> --------- Co-authored-by: Rúben Carvalho <[email protected]> * chore: run constraints fix on all packages; eslint sorting * fix(menu): differentiates pointer from keyboard * fix(menu): removed dups from merging * fix(color-wheel,styles): use latest CSS * fix(menu): differentiates pointer from keyboard * chore: updated golden images hash and changeset * fix(color-wheel,styles): use latest CSS * chore: updated golden images hash and changeset * fix(menu): fixes menu focus and hover * test(menu): focus styling should not be applied with mouse hover * test(menu): added button option to testing helpers * test: fix linting issue * test(menu): fixed menu test for middle button * chore: updated golden images hash * fix(menu): fixes menu focus and hover * fix(menu): removed commented out sections * test(menu): fixed typo Co-authored-by: [ Cassondra ] <[email protected]> * chore: update changeset --------- Co-authored-by: Rajdeep Chandra <[email protected]> Co-authored-by: Casey Eickhoff <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: [ Cassondra ] <[email protected]> Co-authored-by: Rajdeep Chandra <[email protected]> Co-authored-by: TaraT <[email protected]> Co-authored-by: Rúben Carvalho <[email protected]>
1 parent fdb37bf commit a69accb

File tree

8 files changed

+200
-458
lines changed

8 files changed

+200
-458
lines changed

‎.changeset/spicy-papers-fold.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@spectrum-web-components/menu': patch
3+
---
4+
5+
correctly applies menuitem hover styling with pointerenter actions and only applies menuitem focus styling with keyboard/click action [#5269](https://github.com/adobe/spectrum-web-components/issues/5269)

‎.circleci/config.yml

Lines changed: 1 addition & 1 deletion
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: eeca2959e19eb0289b2f9c77e8b792fc0accf9de
17+
default: 2b6914161171d5a49857cebd254a9fbca3ed0ca4
1818
wireit_cache_name:
1919
type: string
2020
default: wireit

‎packages/menu/src/Menu.ts

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -393,10 +393,8 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
393393
capture: true,
394394
}
395395
);
396-
397396
this.addEventListener('click', this.handleClick);
398397
this.addEventListener('touchend', this.handlePointerup);
399-
this.addEventListener('mouseover', this.handleMouseover);
400398
this.addEventListener('focusout', this.handleFocusout);
401399
this.addEventListener('sp-menu-item-keydown', this.handleKeydown);
402400
this.addEventListener('pointerup', this.handlePointerup);
@@ -449,17 +447,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
449447
// handle the click event.
450448
private pointerUpTarget = null as EventTarget | null;
451449

452-
private handleMouseover(event: MouseEvent): void {
453-
const { target } = event;
454-
const menuItem = target as MenuItem;
455-
if (
456-
this.childItems.includes(menuItem) &&
457-
this.isFocusableElement(menuItem)
458-
) {
459-
this.rovingTabindexController?.focusOnItem(menuItem);
460-
}
461-
}
462-
463450
private handleFocusout(): void {
464451
if (!this.matches(':focus-within'))
465452
this.rovingTabindexController?.reset();
@@ -696,7 +683,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
696683
if (lastFocusedItem?.hasSubmenu) {
697684
//open submenu and set focus
698685
event.stopPropagation();
699-
lastFocusedItem.openOverlay();
686+
lastFocusedItem.openOverlay(true);
700687
}
701688
} else if (shouldCloseSelfAsSubmenu && this.isSubmenu) {
702689
event.stopPropagation();
@@ -735,7 +722,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
735722
// Remove focus while opening overlay from keyboard or the visible focus
736723
// will slip back to the first item in the menu.
737724
event.preventDefault();
738-
root.openOverlay();
725+
root.openOverlay(true);
739726
return;
740727
}
741728
if (key === ' ' || key === 'Enter') {

‎packages/menu/src/MenuItem.ts

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,16 @@ export class MenuItem extends LikeAnchor(
309309
@property({ type: Boolean, reflect: true })
310310
public open = false;
311311

312+
/**
313+
* whether menu item's submenu is opened via keyboard
314+
*/
315+
private _openedViaKeyboard = false;
316+
317+
/**
318+
* whether menu item's submenu is closed via pointer leave
319+
*/
320+
private _closedViaPointer = false;
321+
312322
private handleClickCapture(event: Event): void | boolean {
313323
if (this.disabled) {
314324
event.preventDefault();
@@ -369,12 +379,14 @@ export class MenuItem extends LikeAnchor(
369379
import('@spectrum-web-components/popover/sp-popover.js');
370380
return html`
371381
<sp-overlay
382+
receives-focus="false"
372383
.triggerElement=${this as HTMLElement}
373384
?disabled=${!this.hasSubmenu}
374385
?open=${this.hasSubmenu &&
375386
this.open &&
376387
this.dependencyManager.loaded}
377388
.placement=${this.isLTR ? 'right-start' : 'left-start'}
389+
receives-focus="false"
378390
.offset=${[-10, -5] as [number, number]}
379391
.type=${'auto'}
380392
@close=${(event: Event) => event.stopPropagation()}
@@ -460,13 +472,20 @@ export class MenuItem extends LikeAnchor(
460472
super.firstUpdated(changes);
461473
this.setAttribute('tabindex', '-1');
462474
this.addEventListener('keydown', this.handleKeydown);
475+
this.addEventListener('mouseover', this.handleMouseover);
463476
this.addEventListener('pointerdown', this.handlePointerdown);
464477
this.addEventListener('pointerenter', this.closeOverlaysForRoot);
465478
if (!this.hasAttribute('id')) {
466479
this.id = `sp-menu-item-${randomID()}`;
467480
}
468481
}
469-
482+
handleMouseover(event: MouseEvent): void {
483+
const target = event.target as HTMLElement;
484+
if (target === this) {
485+
this.focus();
486+
this.focused = false;
487+
}
488+
}
470489
/**
471490
* forward key info from keydown event to parent menu
472491
*/
@@ -509,7 +528,7 @@ export class MenuItem extends LikeAnchor(
509528
if (event.composedPath().includes(this.overlayElement)) {
510529
return;
511530
}
512-
this.openOverlay();
531+
this.openOverlay(true);
513532
}
514533

515534
protected handleSubmenuFocus(): void {
@@ -536,15 +555,18 @@ export class MenuItem extends LikeAnchor(
536555
if (this.leaveTimeout) {
537556
clearTimeout(this.leaveTimeout);
538557
delete this.leaveTimeout;
558+
this.recentlyLeftChild = false;
539559
return;
540560
}
561+
this.focus();
541562
this.openOverlay();
542563
}
543564

544565
protected leaveTimeout?: ReturnType<typeof setTimeout>;
545566
protected recentlyLeftChild = false;
546567

547568
protected handlePointerleave(): void {
569+
this._closedViaPointer = true;
548570
if (this.open && !this.recentlyLeftChild) {
549571
this.leaveTimeout = setTimeout(() => {
550572
delete this.leaveTimeout;
@@ -570,38 +592,37 @@ export class MenuItem extends LikeAnchor(
570592
}
571593

572594
protected async handleSubmenuPointerleave(): Promise<void> {
573-
requestAnimationFrame(() => {
574-
this.recentlyLeftChild = false;
575-
});
595+
this.recentlyLeftChild = false;
576596
}
577597

578598
protected handleSubmenuOpen(event: Event): void {
579-
const shouldFocus = this.matches(':focus, :focus-within') || this.focused;
580-
this.focused = false;
581599
const parentOverlay = event.composedPath().find((el) => {
582600
return (
583601
el !== this.overlayElement &&
584602
(el as HTMLElement).localName === 'sp-overlay'
585603
);
586604
}) as Overlay;
587-
if (shouldFocus)
605+
if (this._openedViaKeyboard) {
588606
this.submenuElement?.focus();
607+
}
589608
this.overlayElement.parentOverlayToForceClose = parentOverlay;
590609
}
591610

592611
protected cleanup(): void {
612+
this._closedViaPointer = false;
593613
this.setAttribute('aria-expanded', 'false');
594614
this.open = false;
595615
this.active = false;
596616
}
597617

598-
public async openOverlay(): Promise<void> {
618+
public async openOverlay(shouldFocus: boolean = false): Promise<void> {
599619
if (!this.hasSubmenu || this.open || this.disabled) {
600620
return;
601621
}
602622
this.open = true;
603623
this.active = true;
604624
this.setAttribute('aria-expanded', 'true');
625+
this._openedViaKeyboard = shouldFocus;
605626
this.addEventListener('sp-closed', this.cleanup, {
606627
once: true,
607628
});
@@ -632,7 +653,8 @@ export class MenuItem extends LikeAnchor(
632653
changes.has('open') &&
633654
!this.open &&
634655
this.hasSubmenu &&
635-
this.hasVisibleFocusInTree()
656+
!this._closedViaPointer &&
657+
this.matches(':focus-within')
636658
) {
637659
this.focus();
638660
}

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

Lines changed: 15 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,11 @@ import '@spectrum-web-components/menu/sp-menu.js';
2727
import { isFirefox, isWebKit } from '@spectrum-web-components/shared';
2828
import { sendKeys } from '@web/test-runner-commands';
2929
import { spy } from 'sinon';
30-
import { sendMouse } from '../../../test/plugins/browser.js';
3130
import {
3231
arrowDownEvent,
3332
arrowUpEvent,
3433
fixture,
34+
sendMouseTo,
3535
tabEvent,
3636
testForLitDevWarnings,
3737
tEvent,
@@ -338,29 +338,25 @@ describe('Menu', () => {
338338

339339
expect(document.activeElement).to.equal(firstItem);
340340
expect(firstItem.focused, 'first item focused').to.be.true;
341-
const rect = secondItem.getBoundingClientRect();
342-
343-
await sendMouse({
344-
steps: [
345-
{
346-
position: [
347-
rect.left + rect.width / 2,
348-
rect.top + rect.height / 2,
349-
],
350-
type: 'move',
351-
},
352-
],
353-
});
341+
342+
await sendMouseTo(secondItem);
354343

355344
expect(document.activeElement, 'active element after hover').to.equal(
356345
secondItem
357346
);
358-
expect(secondItem.focused, 'second item focused').to.be.true;
347+
expect(document.activeElement).to.equal(secondItem);
348+
expect(
349+
secondItem.focused,
350+
'second item should not have focus styling on hover'
351+
).to.be.false;
359352

360353
await sendKeys({ press: 'ArrowUp' });
361354

362355
expect(document.activeElement).to.equal(firstItem);
363-
expect(firstItem.focused, 'first item focused').to.be.true;
356+
expect(
357+
firstItem.focused,
358+
'first item should have focus styling after keyboard'
359+
).to.be.true;
364360
});
365361

366362
it('handle focus and late descendant additions', async () => {
@@ -562,6 +558,7 @@ describe('Menu', () => {
562558
const changeSpy = spy();
563559
const el = await fixture<Menu>(html`
564560
<sp-menu
561+
id="debug"
565562
selects="single"
566563
@change=${() => {
567564
changeSpy();
@@ -584,41 +581,14 @@ describe('Menu', () => {
584581
) as MenuItem;
585582

586583
// send right mouse click to the secondItem
587-
const rect = secondItem.getBoundingClientRect();
588-
sendMouse({
589-
steps: [
590-
{
591-
position: [
592-
rect.left + rect.width / 2,
593-
rect.top + rect.height / 2,
594-
],
595-
type: 'click',
596-
options: {
597-
button: 'right',
598-
},
599-
},
600-
],
601-
});
584+
sendMouseTo(secondItem, 'click', 'right');
602585
await elementUpdated(el);
603586
await elementUpdated(secondItem);
604587
await aTimeout(150);
605588
expect(changeSpy.callCount, 'no change').to.equal(0);
606589

607590
// send middle mouse click to the secondItem
608-
sendMouse({
609-
steps: [
610-
{
611-
position: [
612-
rect.left + rect.width / 2,
613-
rect.top + rect.height / 2,
614-
],
615-
type: 'click',
616-
options: {
617-
button: 'middle',
618-
},
619-
},
620-
],
621-
});
591+
await sendMouseTo(secondItem, 'click', 'middle');
622592
await elementUpdated(el);
623593
await elementUpdated(secondItem);
624594
await aTimeout(150);

0 commit comments

Comments
 (0)