fix: refactor some focus logic in MenuButton and SplitButton

CHANGELOG.md

@@ -33,6 +33,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Accessibility `splitButton` & `menuButton` - screen reader fixes @kolaps33 ([#2090](https://github.com/microsoft/fluent-ui-react/pull/2090))
 - Accessibility `menuButton` add aria-controls attribute based on `open` prop @kolaps33 ([#2107](https://github.com/microsoft/fluent-ui-react/pull/2107))
 - Fix the 500 color variant for `steelLight` in category color scheme @natashamayurshah ([#2089](https://github.com/microsoft/fluent-ui-react/pull/2089))
+- Fix focus handling cases in `MenuButton` and `SplitButton` @silviuavram ([#2145](https://github.com/microsoft/fluent-ui-react/pull/2145))
 
 ### Features
 - Add `Table` component base implementation @pompomon ([#2099](https://github.com/microsoft/fluent-ui-react/pull/2099))

packages/accessibility/src/behaviors/SplitButton/splitButtonBehavior.ts

@@ -12,35 +12,35 @@ import menuButtonBehavior from '../MenuButton/menuButtonBehavior'
  * Adds attribute 'id=trigger-id' based on the property 'triggerId' to 'trigger' slot.
  * Adds attribute 'id=menu-id' based on the property 'menuId' to 'menu' slot.
  * Adds attribute 'aria-labelledby=trigger-id' based on the property 'triggerId' to 'menu' slot.
- * Triggers 'closeAndFocusTrigger' action with 'Escape' or 'altKey'+'ArrowUp' or 'Tab' or 'Shift'+'Tab' on 'popup' slot.
- * Triggers 'open' action with 'altKey'+'ArrowDown' on 'trigger' slot.
+ * Triggers 'closeAndFocusTrigger' action with 'Escape' or 'altKey'+'ArrowUp'.
+ * Triggers 'openAndFocusFirst' action with 'altKey'+'ArrowDown' on 'root' slot.
  *
  * @specification
  * Adds attribute 'tabIndex=-1' to 'toggleButton' slot.
  * Adds attribute 'aria-haspopup=true' to 'toggleButton' slot.
  */
 const splitButtonBehavior: Accessibility = props => {
-  const splitButtonMenuButtonBehavior = props => {
+  const splitButtonMenuButtonBehavior = () => {
     const menuButtonBehaviorData = menuButtonBehavior(props)
     menuButtonBehaviorData.attributes.trigger['aria-haspopup'] = undefined
 
     return _.merge(menuButtonBehaviorData, {
       keyActions: {
-        trigger: {
-          open: {
-            keyCombinations: [{ keyCode: keyboardKey.ArrowDown, altKey: true }],
-          },
-        },
         popup: {
           closeAndFocusTrigger: {
             keyCombinations: [
               { keyCode: keyboardKey.Escape },
               { keyCode: keyboardKey.ArrowUp, altKey: true },
-              { keyCode: keyboardKey.Tab, shiftKey: false },
-              { keyCode: keyboardKey.Tab, shiftKey: true },
             ],
           },
         },
+        root: {
+          ...(!props.open && {
+            openAndFocusFirst: {
+              keyCombinations: [{ keyCode: keyboardKey.ArrowDown, altKey: true }],
+            },
+          }),
+        },
       },
     })
   }

packages/accessibility/test/behaviors/splitButtonBehavior-test.tsx

@@ -37,7 +37,7 @@ describe('SplitButtonBehavior.ts', () => {
     const property = {}
     const supportedKeys = [{ altKey: true, keyCode: keyboardKey.ArrowDown }]
     const keysFromBehavior = splitButtonBehavior(property)['childBehaviors']['menuButton'](property)
-      .keyActions.trigger.open.keyCombinations
+      .keyActions.root.openAndFocusFirst.keyCombinations
     verifyKeys(supportedKeys, keysFromBehavior)
   })
 })

packages/react/src/components/MenuButton/MenuButton.tsx

@@ -169,24 +169,23 @@ export default class MenuButton extends AutoControlledComponent<MenuButtonProps,
   menuRef = React.createRef<HTMLElement>()
 
   actionHandlers = {
-    closeMenu: () => this.closeMenu(),
+    closeMenu: e => this.closeMenu(e),
     openAndFocusFirst: e => this.openAndFocus(e, 'first'),
     openAndFocusLast: e => this.openAndFocus(e, 'last'),
   }
 
-  closeMenu() {
-    this.setState({ open: false })
+  closeMenu(e: React.KeyboardEvent) {
+    this.handleOpenChange(e, false)
   }
 
   openAndFocus(e: React.KeyboardEvent, which: 'first' | 'last') {
-    const renderCallback = () => focusMenuItem(this.menuRef.current, which)
-    this.setState({ open: true }, renderCallback)
     e.preventDefault()
+    this.handleOpenChange(e, true, () => focusMenuItem(this.menuRef.current, which))
   }
 
-  handleOpenChange = (e, { open }: PopupProps) => {
+  handleOpenChange = (e: React.SyntheticEvent, open: boolean, callback?: () => void) => {
     _.invoke(this.props, 'onOpenChange', e, { ...this.props, open })
-    this.setState({ open })
+    this.setState({ open }, callback)
   }
 
   handleMenuOverrides = (predefinedProps?: MenuProps) => ({
@@ -195,7 +194,7 @@ export default class MenuButton extends AutoControlledComponent<MenuButtonProps,
       _.invoke(this.props, 'onMenuItemClick', e, itemProps)
       if (!itemProps || !itemProps.menu) {
         // do not close if clicked on item with submenu
-        this.setState({ open: false })
+        this.handleOpenChange(e, false)
       }
     },
   })
@@ -266,7 +265,9 @@ export default class MenuButton extends AutoControlledComponent<MenuButtonProps,
     const overrideProps: PopupProps = {
       accessibility: () => accessibility,
       open: this.state.open,
-      onOpenChange: this.handleOpenChange,
+      onOpenChange: (e, { open }) => {
+        this.handleOpenChange(e, open)
+      },
       content: {
         styles: styles.popupContent,
         content: content && <Ref innerRef={this.menuRef}>{content}</Ref>,

packages/react/src/components/Popup/Popup.tsx

@@ -195,6 +195,7 @@ export default class Popup extends AutoControlledComponent<PopupProps, PopupStat
 
   actionHandlers = {
     closeAndFocusTrigger: e => {
+      e.preventDefault()
       this.close(e, () => _.invoke(this.triggerFocusableDomElement, 'focus'))
     },
     close: e => {
@@ -214,7 +215,12 @@ export default class Popup extends AutoControlledComponent<PopupProps, PopupStat
   }
 
   componentDidMount() {
-    const { inline, trapFocus, autoFocus } = this.props
+    const { inline, trapFocus, autoFocus, open } = this.props
+
+    if (open) {
+      // when new state 'open' === 'true', save the last focused element
+      this.updateTriggerFocusableDomElement()
+    }
 
     if (process.env.NODE_ENV !== 'production') {
       if (inline && trapFocus) {
@@ -230,6 +236,13 @@ export default class Popup extends AutoControlledComponent<PopupProps, PopupStat
     }
   }
 
+  componentDidUpdate({ open }) {
+    if (open) {
+      // when new state 'open' === 'true', save the last focused element
+      this.updateTriggerFocusableDomElement()
+    }
+  }
+
   renderComponent({
     classes,
     rtl,

packages/react/test/specs/components/SplitButton/SplitButton-test.tsx

@@ -29,10 +29,10 @@ describe('SplitButton', () => {
       const toggleButton = getToggleButton(wrapper)
 
       toggleButton.simulate('click')
-      expect(getMenuItems(wrapper).length).toBe(mockMenu.items.length)
+      expect(getMenuItems(wrapper)).toHaveLength(mockMenu.items.length)
 
       toggleButton.simulate('click')
-      expect(getMenuItems(wrapper).length).toBe(0)
+      expect(getMenuItems(wrapper)).toHaveLength(0)
     })
 
     test('is false when clicking menu item', () => {
@@ -41,31 +41,60 @@ describe('SplitButton', () => {
       getMenuItems(wrapper)
         .at(0)
         .simulate('click')
-      expect(getMenuItems(wrapper).length).toBe(0)
+      expect(getMenuItems(wrapper)).toHaveLength(0)
     })
 
-    test('is open when Alt+ArrowDown is sent to the main button', () => {
+    test('is true when Alt+ArrowDown is sent to the main button', () => {
       const wrapper = mountWithProvider(<SplitButton menu={mockMenu} button="test" />)
 
       getMainButton(wrapper).simulate('keydown', { keyCode: keyboardKey.ArrowDown, altKey: true })
 
-      expect(getMenuItems(wrapper).length).toBe(mockMenu.items.length)
+      expect(getMenuItems(wrapper)).toHaveLength(mockMenu.items.length)
     })
 
-    test('is open when Alt+ArrowUp is sent to the menu', () => {
+    test('is false when Alt+ArrowUp is sent to the menu', () => {
       const wrapper = mountWithProvider(<SplitButton menu={mockMenu} button="test" defaultOpen />)
 
       getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.ArrowUp, altKey: true })
 
-      expect(getMenuItems(wrapper).length).toBe(0)
+      expect(getMenuItems(wrapper)).toHaveLength(0)
+      expect(document.activeElement).toBe(getMainButton(wrapper).getDOMNode())
     })
 
-    test('is open when Escape is sent to the menu', () => {
+    test('is false when Escape is sent to the menu', () => {
       const wrapper = mountWithProvider(<SplitButton menu={mockMenu} button="test" defaultOpen />)
 
       getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.Escape })
 
-      expect(getMenuItems(wrapper).length).toBe(0)
+      expect(getMenuItems(wrapper)).toHaveLength(0)
+      expect(document.activeElement).toBe(getMainButton(wrapper).getDOMNode())
+    })
+
+    test('is false when Tab is sent to the menu', () => {
+      const wrapper = mountWithProvider(<SplitButton menu={mockMenu} button="test" defaultOpen />)
+
+      getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.Tab, shiftKey: false })
+
+      expect(getMenuItems(wrapper)).toHaveLength(0)
+    })
+
+    test('is false when Enter is sent to the menu', () => {
+      const wrapper = mountWithProvider(<SplitButton menu={mockMenu} button="test" defaultOpen />)
+
+      getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.Enter })
+      getMenuItems(wrapper)
+        .at(0)
+        .simulate('click')
+
+      expect(getMenuItems(wrapper)).toHaveLength(0)
+    })
+
+    test('is false when Shift+Tab is sent to the menu', () => {
+      const wrapper = mountWithProvider(<SplitButton menu={mockMenu} button="test" defaultOpen />)
+
+      getMenu(wrapper).simulate('keydown', { keyCode: keyboardKey.Tab, shiftKey: true })
+
+      expect(getMenuItems(wrapper)).toHaveLength(0)
     })
   })