Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(Menu): Do not rely on tabster restorer for focus restore #32840

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
+27 −8
Draft

fix(Menu): Do not rely on tabster restorer for focus restore #32840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec03df2
fix(Menu): Do not rely on tabster restorer for focus restore
ling1726 Sep 16, 2024
d35e170
changefile
ling1726 Sep 16, 2024
e2fd724
update snapshots
ling1726 Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@fluentui-react-menu-6534522e-919d-4ebb-973b-b93815bd1abe.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕵🏾‍♀️ visual regressions to review in the fluentuiv9 Visual Regression Report

Avatar Converged 2 screenshots
Image Name Diff(in Pixels) Image Type
Avatar Converged.badgeMask.chromium.png 8 Changed
Avatar Converged.Badge Mask RTL.chromium.png 6 Changed
Drawer 1 screenshots
Image Name Diff(in Pixels) Image Type
Drawer.Full Overlay Dark Mode.chromium.png 996 Changed

"type": "patch",
"comment": "fix(Menu): Do not rely on tabster restorer for focus restore",
"packageName": "@fluentui/react-menu",
"email": "lingfangao@hotmail.com",
"dependentChangeType": "patch"
}
1 change: 0 additions & 1 deletion .../react-components/react-menu/library/src/components/Menu/__snapshots__/Menu.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ exports[`Menu renders a default state 1`] = `
<div>
<button
aria-haspopup="menu"
data-tabster="{\\"restorer\\":{\\"type\\":1}}"
id="menu1"
>
Menu trigger
Expand Down
20 changes: 19 additions & 1 deletion packages/react-components/react-menu/library/src/components/Menu/useMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
useOnScrollOutside,
elementContains,
useTimeout,
useFirstMount,
} from '@fluentui/react-utilities';
import { useFluent_unstable as useFluent } from '@fluentui/react-shared-contexts';
import { useFocusFinders } from '@fluentui/react-tabster';
Expand Down Expand Up @@ -173,6 +174,8 @@ const useMenuOpenState = (
> &
Pick<MenuProps, 'open' | 'defaultOpen' | 'onOpenChange'>,
) => {
'use no memo';

const { targetDocument } = useFluent();
const parentSetOpen = useMenuContext_unstable(context => context.setOpen);
const onOpenChange: MenuProps['onOpenChange'] = useEventCallback((e, data) => state.onOpenChange?.(e, data));
Expand Down Expand Up @@ -265,11 +268,26 @@ const useMenuOpenState = (
firstFocusable?.focus();
}, [findFirstFocusable, state.menuPopoverRef]);

const firstMount = useFirstMount();
React.useEffect(() => {
if (open) {
focusFirst();
} else {
if (!firstMount) {
if (targetDocument?.activeElement === targetDocument?.body) {
// We know that React effects are sync so we focus the trigger here
// after any event handler (event handlers will update state and re-render).
// Since the browser only performs the default behaviour for the Tab key once
// keyboard events have fully bubbled up the window, the browser will move
// focus to the next tabbable element before/after the trigger if needed.
// If the Tab key was not pressed, focus will remain on the trigger as expected.
state.triggerRef.current?.focus();
}
}
}
}, [open, focusFirst]);
// firstMount change should not re-run this effect
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [state.triggerRef, state.isSubmenu, open, focusFirst, targetDocument, state.menuPopoverRef]);

return [open, setOpen] as const;
};
2 changes: 0 additions & 2 deletions packages/react-components/react-menu/library/src/components/MenuTrigger/MenuTrigger.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ describe('MenuTrigger', () => {
Array [
<button
aria-haspopup="menu"
data-tabster="{\\"restorer\\":{\\"type\\":1}}"
id="id"
>
Trigger
Expand Down Expand Up @@ -88,7 +87,6 @@ describe('MenuTrigger', () => {
Array [
<button
aria-haspopup="menu"
data-tabster="{\\"restorer\\":{\\"type\\":1}}"
id="id"
>
Trigger
Expand Down
1 change: 0 additions & 1 deletion ...nts/react-menu/library/src/components/MenuTrigger/__snapshots__/MenuTrigger.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
exports[`MenuTrigger renders a default state 1`] = `
<button
aria-haspopup="menu"
data-tabster="{\\"restorer\\":{\\"type\\":1}}"
id="id"
onClick={[Function]}
onContextMenu={[Function]}
Expand Down
4 changes: 1 addition & 3 deletions packages/react-components/react-menu/library/src/components/MenuTrigger/useMenuTrigger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';
import { MenuTriggerProps, MenuTriggerState } from './MenuTrigger.types';
import { useMenuContext_unstable } from '../../contexts/menuContext';
import { useIsSubmenu } from '../../utils/useIsSubmenu';
import { useFocusFinders, useRestoreFocusTarget } from '@fluentui/react-tabster';
import { useFocusFinders } from '@fluentui/react-tabster';
import { useFluent_unstable as useFluent } from '@fluentui/react-shared-contexts';
import { ArrowRight, ArrowLeft, Escape, ArrowDown } from '@fluentui/keyboard-keys';
import {
Expand Down Expand Up @@ -31,7 +31,6 @@ export const useMenuTrigger_unstable = (props: MenuTriggerProps): MenuTriggerSta
const triggerId = useMenuContext_unstable(context => context.triggerId);
const openOnHover = useMenuContext_unstable(context => context.openOnHover);
const openOnContext = useMenuContext_unstable(context => context.openOnContext);
const restoreFocusTargetAttribute = useRestoreFocusTarget();

const isSubmenu = useIsSubmenu();

Expand Down Expand Up @@ -125,7 +124,6 @@ export const useMenuTrigger_unstable = (props: MenuTriggerProps): MenuTriggerSta

const contextMenuProps = {
id: triggerId,
...restoreFocusTargetAttribute,
...child?.props,
ref: useMergedRefs(triggerRef, child?.ref),
onMouseEnter: useEventCallback(mergeCallbacks(child?.props.onMouseEnter, onMouseEnter)),
Expand Down
Loading