Repository navigation
fix(menu): don't show focus ring when MenuItem is focused by hover - #4368
udit (UditDewan) wants to merge 3 commits into
Conversation
On hover, onHoverIn hides the focus ring and useHoverFocusEffect then focuses the item. onFocus decided whether the focus came from hover by reading pressable.state.focused, which is stale at that point, so it always re-enabled the focus ring. Track hover-initiated focus with a ref instead so onFocus sees it synchronously. Fixes microsoft#4083
|
Please rebase or merge the latest changes from main to fix the test repo links error. |
Bundle size reportTree-shaken, minified production esbuild bundles with React and React Native runtimes externalized.
The job is advisory: size changes are reported but do not fail the pull request. Bundle or analysis errors still fail. |
| pressable.props.onHoverIn(e); | ||
| // when it's a hover focus, set enableFocusRing explicitly to false | ||
| if (!pressable.state.focused) { | ||
| isHoverFocus.current = true; |
There was a problem hiding this comment.
[P2] Cancel pending hover-focus intent when the hover ends without focus
This ref is cleared only by a subsequent onFocus, not by onHoverOut or cancellation of the focus request. If hover-in and hover-out are processed before a commit, useHoverFocusEffect sees the final hovered=false and never requests focus, but isHoverFocus.current remains true. The next genuine keyboard onFocus then consumes the stale flag and leaves enableFocusRing=false. The same stale-intent issue occurs when the imperative focus request does not deliver a focus event. Please clear or otherwise invalidate pending hover intent when that hover/request ends, and extend the new hook tests with hover-in, hover-out, then keyboard focus without an intervening hover-focus event. The existing second test always supplies that event, so it misses this path.
Platforms Impacted
Description of changes
Fixes #4083.
When the mouse hovers a
MenuItem,onHoverInhides the focus ring anduseHoverFocusEffectthen calls.focus()on the item.onFocustried to detect "this focus came from hover" by checking!pressable.state.focused, but that value is the pre-hover render's state, so the check always passed andonFocusre-enabled the focus ring, overwriting thefalsethatonHoverInhad just set. The result is a focus ring on hovered menu items.packages/components/Menu/src/MenuItem/useMenuItem.ts:isHoverFocus) set inonHoverIn. Refs are read synchronously, so they aren't affected by React not having re-rendered yet.onFocusconsumes the flag: if focus came from hover the ring stays hidden, otherwise (keyboard/programmatic focus) the ring is enabled as before.macOS is unaffected (it already uses the plain pressable handlers and never shows the ring).
Verification
src/MenuItem/useMenuItem.test.win32.tsxcovering:enableFocusRingfalsemainand pass with this change.yarn build,yarn lint,yarn testfor@fluentui-react-native/menu;yarn format:check.Pull request checklist
This PR has considered (when applicable):