Fixes focus issue for contextual menus by rebeccaballantyne · Pull Request #4744 · microsoft/fluentui

rebeccaballantyne · 2018-05-02T21:57:33Z

Pull request checklist

Addresses an existing issue: Fixes When opening a contextual menu, first menu item does not receive focus #4717, [Accessibility][ContextualMenu] Initial focus is set on menu, not first menu item #4207
Include a change request file using $ npm run change

Description of changes

Prior to fix, when a contextual menu was opened, focus always went to the contextual menu container.

The correct behavior (determined via discussion with Jeremy Spurlin/ Ben Truelove/ Chris Schlechty/ Jon Schectman/ Thomas Michon) is as follows:

If user activates contextual menu via keyboarding (pressing enter on a button for example), focus should go to the first focusable menu item.
If user activates contextual menu via mouse click, focus should go to the contextual menu container.

The following is a summary of changes to each file:

BaseButton.tsx: Pipes through to ContextualMenu via new param (shouldFocusOnContainer) how the menu was opened. Right now we disregard user input for "shouldFocusOnContainer" if contextual menu is created via menu params to button. I think that is actually desired behavior as it means we will enforce accessibility. Tweaked _isValidMenuOpenKey so that way "Enter" is considered the valid menu open key for non split button items. This means that on enter key press, _onMenuKeyDown will be executed instead of _onMenuClick.
ContextualMenu.tsx: Responds to new param shouldFocusOnContainer, setting roles and tab indexes appropriately. Note that aria label is read regardless of if focus is put on container or first menu item. Added state expandedByMouseClick so that way the same focus behavior applies to sub-menus.

Focus areas to test

With Narrator on and off (in Edge), test contextual menus, activating via keyboard or via click. Observe different focus behaviors.

…Button.tsx

ThomasMichon · 2018-05-02T22:47:55Z

        onMenuClick(ev, this);
      }

+      const menuProps = this.props.menuProps;


Should do:

const menuProps = { ...this.props.menuProps, shouldFocusOnContainer: true };

Avoid modifying existing props objects.

ThomasMichon · 2018-05-02T22:48:15Z


    if (!ev.defaultPrevented) {
+      const menuProps = this.props.menuProps;
+      if (menuProps) {


Same comment regarding using ... to mix in a new prop.

ThomasMichon · 2018-05-02T22:48:55Z

        >
          <div
-            role='menu'
+            role={ shouldFocusOnContainer ? 'menu' : undefined }


Why should the role change based on whether or not it is focusable? What gets labeled as a menu in the other case?

So when focus is NOT on the container (and is instead on the first menu item), adding role='menu' to the container stopped Narrator from reading the aria-label on the menu. Role='group' was fine but so was no role at all for narration behavior. Currently nothing gets labeled as a menu in the case where focus is on the first menu item (and narration works optimally). Is it a big accessibility no-no to not have role='menu' somewhere on a menu?

betrue-final-final · 2018-05-03T18:37:56Z

This works great. Tested in Narrator on Edge and with a mouse.

chang47

Adding some comments

chang47 · 2018-05-02T23:35:47Z

                handleTabKey={ FocusZoneTabbableElements.all }
              >
                <ul
-                  role='presentation'


Why are we getting rid of the role?

At the very least shouldn't it be something like:

role = { shouldFocusOnContainer ? 'presentation' : undefined }

to match with your changes above.

This role='presentation' was useless and it's presence didn't impact screen reader behavior at all.

chang47 · 2018-05-03T00:12:24Z

            id={ id }
            className={ this._classNames.container }
-            tabIndex={ 0 }
+            tabIndex={ shouldFocusOnContainer ? 0 : undefined }


For my understanding, this is the part of the code where we prevent focus being added to the container for the contextual menu?

chang47 · 2018-05-03T00:13:44Z

        this._onSubMenuDismiss(ev);
      } else { // This has a collapsed sub menu. Expand it.
+        this.setState({
+          expandedByMouseClick: (ev.nativeEvent.detail !== 0)


Since we added a comment in BaseButton regarding why we're using:

ev.nativeEvent.detail

Let's put it here too.

rebeccaballantyne · 2018-05-03T21:30:18Z

  "bundlesize": [
    {
      "path": "../apps/test-bundle-button/dist/test-bundle-button.min.js",
-      "maxSize": "47.6 kB"


Worth noting that the current size in the most up to date version of master is 49.6 so this is not a bump of 2KB

* master: (52 commits) Marqueeselection style update (microsoft#4803) Applying package updates. FocusZone: Add the ability to stop focus from propagating outside the FocusZone (microsoft#4823) Unknown persona coin (microsoft#4809) Applying package updates. BaseButton: Allow Alt + Down on menu buttons to open the menu (microsoft#4807) Applying package updates. Website: Scroll to top of page on navigation (microsoft#4810) Applying package updates. ActivityItem: fix getstyles (microsoft#4802) Mark Slider#ValuePosition enum as deprecated as it is unused. (microsoft#4799) Icon: ensure `styles` still works. (microsoft#4805) Popup: Added check for onBlur to prevent focus setting focus to be incorrectly disabled when closing a menu in Chrome (microsoft#4804) Update package.json Move <Layer> to use React portals when available (microsoft#4724) Fix breadcrumb rendering issue when overflow item is at last index (microsoft#4787) Fixes focus issue for contextual menus (microsoft#4744) Remove redundant selected items prop on BaseExtendedPicker (microsoft#4769) SearchBox: New prop for turning off icon animation (microsoft#4794) Add functions to ContextualMenuItem to open and close menus on command (microsoft#4741) ...

rebeccaballantyne01 added 9 commits May 1, 2018 13:44

Best behavior for keyboarding

9f877be

Adds in new parameter, and basics of different aria roles

2947a4d

Removing unnecessary group role for when focus moves to first menu item

3e99a25

Adding in differences between keyboard and mouse/click action in Base…

8432116

…Button.tsx

Fixes issue with Edge + Narrator combination

f29bfd4

Fixing up comment

fde5885

Moving fix for Edge + Narrator to ContextualMenu.tsx

19e4d27

Removing mistaken commit for testing

fd759fb

Rush change file

2205afc

rebeccaballantyne requested a review from joschect as a code owner May 2, 2018 21:57

ThomasMichon reviewed May 2, 2018

View reviewed changes

rebeccaballantyne01 added 2 commits May 2, 2018 18:24

Avoid modifying state directly

a6a7095

moving prop to proper place

c929768

betrue-final-final self-assigned this May 3, 2018

betrue-final-final approved these changes May 3, 2018

View reviewed changes

chang47 reviewed May 3, 2018

View reviewed changes

joschect approved these changes May 3, 2018

View reviewed changes

Adding in comment in ContextualMenu.tsx

34c3bb2

chang47 approved these changes May 3, 2018

View reviewed changes

Updating package.json bundle size

3ef3590

rebeccaballantyne requested a review from dzearing as a code owner May 3, 2018 21:29

rebeccaballantyne commented May 3, 2018

View reviewed changes

dzearing and others added 4 commits May 7, 2018 13:15

Merge branch 'master' into rebeba/contextual-menu-focus

c871515

Fixing accidental removal of path

0fe31a7

Deleting old rush change file, should be minor, not patch

222a46b

New rush change file for minor change

0a8c819

joschect approved these changes May 7, 2018

View reviewed changes

joschect merged commit 8b36d24 into microsoft:master May 7, 2018

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Uh oh!

Conversation

rebeccaballantyne commented May 2, 2018 • edited by cliffkoh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebeccaballantyne May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betrue-final-final commented May 3, 2018

Uh oh!

chang47 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rebeccaballantyne commented May 2, 2018 •

edited by cliffkoh

Loading

rebeccaballantyne May 3, 2018 •

edited

Loading