fix(picker): menuitem focus when opened via mouse #5358
Conversation
🦋 Changeset detectedLatest commit: 9f49efc The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
nikkimk
changed the title
Tachometer resultsChromepicker permalinkbasic-test
Firefoxpicker permalinkbasic-test
|
| @@ -269,7 +265,7 @@ describe('ActionGroup', () => { | |||
|
|
|||
| await elementUpdated(el); | |||
|
|
|||
| await aTimeout(500); | |||
| await aTimeout(100); | |||
There was a problem hiding this comment.
At some point we really need to come up with something less brittle than aTimeout and nextFrame. There has to be a waitUntil condition we can use instead.
There was a problem hiding this comment.
does changing from 500ms to 100ms removes any redundancy in this unit test case?
| @@ -338,9 +331,13 @@ describe('ActionGroup', () => { | |||
| 'mouse2: should not be focused on the fourth button' | |||
| ).to.equal(-1); | |||
|
|
|||
| await aTimeout(100); | |||
There was a problem hiding this comment.
Firefox wasn't opening without this.
|
|
||
| await elementUpdated(el); | ||
| await aTimeout(500); | ||
| await elementUpdated(firstButton); |
There was a problem hiding this comment.
waiting for the first button to update eliminated the need for aTimeout
| @@ -338,9 +331,13 @@ describe('ActionGroup', () => { | |||
| 'mouse2: should not be focused on the fourth button' | |||
| ).to.equal(-1); | |||
|
|
|||
| await elementUpdated(el); | |||
There was a problem hiding this comment.
waiting for the element to update eliminated the need for aTimeout on line 321
packages/picker/src/Picker.ts
Outdated
| @@ -368,12 +368,12 @@ export class PickerBase extends SizedMixin(SpectrumElement, { | |||
| item.selected = value; | |||
| } | |||
|
|
|||
| public toggle(target?: boolean): void { | |||
| public toggle(target?: boolean, focusOnOpen?: boolean): void { | |||
There was a problem hiding this comment.
Keyboard actions need to focus on open.
| // Make a selection again | ||
| closed = oneEvent(el, 'sp-closed'); | ||
| firstItem.click(); | ||
| await sendKeys({ |
There was a problem hiding this comment.
focus happens with keyboard
| await nextFrame(); | ||
| await nextFrame(); | ||
| await nextFrame(); | ||
| await waitUntil(() => el.children.length === 4); |
|
@nikkimk I tested it on my iPad and desktop and it works perfectly! I would suggest in your description, adding a step to press tab first and changing |
| @@ -311,7 +312,20 @@ export class PickerBase extends SizedMixin(SpectrumElement, { | |||
| }; | |||
|
|
|||
| protected async keyboardOpen(): Promise<void> { | |||
| this.toggle(true); | |||
| // if the menu is not open, we need to toggle it and wait for it to open to focus on the first selected item | |||
| if (!this.open || !this.strategy.open) { | |||
There was a problem hiding this comment.
Opening via keyboard set focus on first item. If menu isn't already open, menu will be opened first.
| @@ -373,14 +387,6 @@ export class PickerBase extends SizedMixin(SpectrumElement, { | |||
| return; | |||
| } | |||
| const open = typeof target !== 'undefined' ? target : !this.open; | |||
| if (open && !this.open) | |||
There was a problem hiding this comment.
We only focus on first item if opened via keyboard.
|
@nikkimk i just realized this needs a change set added before we merge |
An automation here would make life more easier: https://danger.systems/ to help us here |
|
|
||
| // use keyboard to navigate to the second menu item and select it | ||
| await sendKeys({ press: 'ArrowDown' }); | ||
| expect(actionMenu.children[0]).to.equal(document.activeElement); |
There was a problem hiding this comment.
Test was still timing out on CI event though it passed locally, so I wanted to make sure a menuitem was focused before selecting it can closing the menu,
Description
Only sets focus if keyboard opens menu
Related issue(s)
Motivation and context
Focus should not be set on menuitems when opened via mouse.
How has this been tested?
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.