-
Notifications
You must be signed in to change notification settings - Fork 233
fix(menu): MenuItem focus stealing from input elements on mouseover #5732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: e36b58b 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 Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick first pass review; I'll validate once the test case is written up!
`; | ||
}; | ||
|
||
export const InputsWithMenu = (): TemplateResult => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great test case but I don't think we need a new story for it. If you wanted to replicate the behavior in a story, we could probably use the play functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a comprehensive test coverage for the focus behavior in packages/menu/test/menu.test.ts (lines 848-984)
with the test 'does not steal focus from input elements on mouseover' that validates all the same input types and scenarios. Let me know if I am missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main thought with not adding this as a story is that it's a lot of manual instructions for the viewer of the page to complete to get the experience you're demonstrating. Maybe we defer adding any new stories until we have a chance to talk about Storybook docs more as a team?
<!-- Color Field Input --> | ||
<div> | ||
<label for="demo-color">Color Field:</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<label for="demo-color">Color Field:</label> | |
<label for="demo-color">Color field:</label> |
Minor nit: try to use sentence case headings in our demos to reinforce the design system content standards where possible!
tools/base/src/constants.ts
Outdated
* Regular expression pattern to match Spectrum Web Components input elements. | ||
* Used to identify components that should maintain focus during menu interactions. | ||
*/ | ||
export const INPUT_COMPONENT_PATTERN = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rubencarvalho Let me know what you think about this! I dig it since we do reuse this logic and it feels like a more global concept to have updated in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can add a manual test case to the PR description so I can do that last bit of validation, the rest of this looks really great.
@castastrophe I’ve already added a manual test case to verify this behavior with multiple input elements. Could you let me know if you’re looking to validate something different? |
Can you update the Image Hash for this. |
Tachometer resultsChromemenu permalinktest-basic
Firefoxmenu permalinktest-basic
|
@Rajdeepc Oh I see it now! The title was the template language so I thought it hadn't been updated. I validated and it looks great! |
}; | ||
|
||
InputsWithMenu.parameters = { | ||
tags: ['!dev'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Description
Fixes MenuItem focus stealing from input elements on mouseover
Solution
isInputElement()
method that detects all input typesgetRootNode().activeElement
for reliable focus detection across shadow boundariestextbox
,searchbox
,combobox
,spinbutton
,slider
) for generic detection/^(SP-SEARCH|SP-TEXTFIELD|SP-NUMBER-FIELD|SP-COMBOBOX|SP-COLOR-FIELD)$/
)No Breaking Changes
Technical Details
Before
After
Motivation and context
MenuItem was stealing focus from input elements (searchboxes, textfields, etc.) when users hovered over menu items while typing. This created a poor user experience where:
sp-search
,sp-textfield
,sp-number-field
,sp-combobox
,sp-color-field
)input
,textarea
,select
) were also affectedRelated issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch
,minor
, ormajor
featuresManual review test cases
Descriptive Test Statement
Device review