Partially port #5385 & #9003 to improve Dropdown accessibility in office-ui-fabric-react/5.79.1 #18075

Phoebe-0319 · 2021-05-06T03:36:08Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ rush change

Description of changes

Partially port #5385 & #9003 to improve Dropdown accessibility in office-ui-fabric-react/5.79.1

Focus areas to test

(optional)

Update Office UI fabric react v5.79.1

…cessibility

ecraig12345 · 2021-05-06T17:47:33Z

Would appreciate if @smhigley or someone else from @microsoft/fluentui-react-pickers-and-more could help review this, keeping in mind that this is a hotfix to a VERY old version and so changes need to be more minimal.

…i-fabric-react/5.79.1

Phoebe-0319 · 2021-05-12T03:31:08Z

@ecraig12345 could you please help to assign this PR to the specific expertise? It seems this team's hovercard is not available. Thank you so much

…i-fabric-react/5.79.1

Phoebe-0319 · 2021-05-14T06:46:16Z

@ecraig12345 Could you please help to assign this PR to more expertise? Since our SharePoint OnPrem team is going to release new SharePoint Public Preview at the end of May, it is urgent for us to merge this PR to resolve some critical Accessibility bugs.
I really appreciate your support all the times.

smhigley

Thanks for the PR! I have some requests around role structure and attributes to have it better reflect the combobox pattern with the best support now.

This pattern is a good ref: https://w3c.github.io/aria-practices/examples/combobox/combobox-select-only.html. It shows a single-select combo, but could be updated to a multiselect by adding aria-multiselectable="true" to the root.

smhigley · 2021-05-14T18:39:09Z

packages/office-ui-fabric-react/src/components/Dropdown/Dropdown.tsx

+
+    if (!isOpen && selectedIndices.length === 0 && !multiSelect && !disabled) {
+      // Per aria
+      this._moveIndex(1, 0, -1);


This changes the value of the dropdown on focus, which we'd want to avoid. It looks like the only thing this function should do is update the hasFocus state

@smhigley Thanks, will remove it.

smhigley · 2021-05-14T18:41:23Z

packages/office-ui-fabric-react/src/components/Dropdown/Dropdown.tsx

-            aria-readonly='true'
+            role={ ariaAttrs.childRole }
+            aria-live={ !hasFocus || disabled || multiSelect || isOpen ? 'off' : 'assertive' }
+            aria-label={ selectedOptions.length ? selectedOptions[0].text : this.props.placeHolder }


I'd suggest not adding an aria-label at all. It'll have an inconsistent effect on the live region (technically the live region should read the text content and not the aria-label, but it varies in practice), and I can't think of a reason a screen reader user should get different information about the current value than a sighted user.

Additionally, elements without nameable roles (e.g. <div>s and <span>s) don't support aria-label or aria-labelledby.

Since I add 'textbox' role in span, I also add 'title' in span in order to avoid errors from Accessibility Insights.

smhigley · 2021-05-14T18:43:58Z

packages/office-ui-fabric-react/src/components/Dropdown/Dropdown.tsx

@@ -191,8 +206,9 @@ export class Dropdown extends BaseComponent<IDropdownInternalProps, IDropdownSta
              (errorMessage && errorMessage.length > 0 ? styles.titleIsError : null))
            }
            aria-atomic={ true }
-            role='listbox'
-            aria-readonly='true'
+            role={ ariaAttrs.childRole }


While the role of this definitely shouldn't be listbox, it also can't be option once the parent is updated back to combobox. This element actually doesn't need any role -- it can just be a text node and live region, and that's enough to provide the parent combobox with a value.

Will keep 'combobox'

packages/office-ui-fabric-react/src/components/Dropdown/Dropdown.tsx

smhigley · 2021-05-14T19:30:48Z

packages/office-ui-fabric-react/src/components/Dropdown/Dropdown.tsx

@@ -160,27 +175,27 @@ export class Dropdown extends BaseComponent<IDropdownInternalProps, IDropdownSta
          id={ id }
          tabIndex={ disabled ? -1 : 0 }
          aria-expanded={ isOpen ? 'true' : 'false' }
-          role='combobox'


This element would actually need to stay a combobox -- it should be a combo whether it's single or multiselect, the only difference is we should add aria-multiselectable="true/false".

The issue with having a single-select be a listbox is that listboxes do not have an expand/collapse functionality, and would set up a screen reader user's expectations that it behave like a listbox. The issue with not having a role at all is then we have a focusable generic element that still has a name/description and provides no information on how to interact with it.

@smhigley Thanks for your comments. I have a question here: If I use role='combobox', but Accessibility Insights will scan a bug: Required ARIA child role not present: textbox. So how about I add role="textbox' in <span?
And this error also occurs in https://w3c.github.io/aria-practices/examples/combobox/combobox-select-only.html and https://developer.microsoft.com/en-us/fluentui#/controls/web/dropdown

Ah, I should've mentioned that off the bat 😅. Including the textbox role as a child of a combobox is the ARIA 1.1 pattern, and actually has significantly worse screen reader support in practice than leaving it off (and if included, it would need to be the element that gets focus). Having the combobox be the single interactive element is the ARIA 1.2 pattern, which has good screen reader support across the board.

Unfortunately, axe-core has yet to update its rules for combobox, so it still flags it as an error 😭. The issue open against them is here: dequelabs/axe-core#2505

For now, it'd still be better to go with the pattern with good support (no textbox), and ignore/skip the automated error until they fix it.

Thank you @smhigley for letting me know about this. It is really a good chance for me to learn more about ARIA pattern and axe-core. Really appreciate your support.

smhigley · 2021-05-14T19:33:59Z

packages/office-ui-fabric-react/src/components/Dropdown/Dropdown.tsx

          aria-label={ ariaLabel }
          aria-describedby={ id + '-option' }
-          aria-activedescendant={ isOpen && selectedIndices.length === 1 && selectedIndices[0] >= 0 ? (this._id + '-list' + selectedIndices[0]) : null }
+          aria-labelledby={ id + '-label' }


…hub.com/Phoebe-0319/fluentui into hotfix_15/office-ui-fabric-react/5.79.1

smhigley · 2021-05-17T17:48:56Z

packages/office-ui-fabric-react/src/components/Dropdown/Dropdown.tsx

            className={ css(
              'ms-Dropdown-title', styles.title,
              !selectedOptions.length && 'ms-Dropdown-titleIsPlaceHolder',
              !selectedOptions.length && styles.titleIsPlaceHolder,
              (errorMessage && errorMessage.length > 0 ? styles.titleIsError : null))
            }
            aria-atomic={ true }
-            role='listbox'
-            aria-readonly='true'
+            aria-live='off'


The aria-live fix you had here earlier actually looked quite good to me (unless you removed it for some other comment that I missed) 😅. While often aria-live is misused, in this case the logic you used for it locked it down well, and I think it should give some benefit for screen reader users changing the value with the combo collapsed

Will keep the previous design.

smhigley

🎉

….79.1 to hotfix/5.135.6

…18075 (Partially port #5385 & #9003 to improve Dropdown accessibility) from office-ui-fabric-react_v5.79.1 to hotfix/5.135.6 (#22169) * Port #13659 & #18075 from office-ui-fabric-react_v5.79.1 to hotfix/5.135.6 * add commit log

Phoebe-0319 and others added 3 commits April 28, 2021 12:30

Merge pull request #1 from microsoft/office-ui-fabric-react_v5.79.1

fc13834

Update Office UI fabric react v5.79.1

Partially port microsoft#5385 & microsoft#9003 to improve Dropdown ac…

db244f6

…cessibility

add change log

1998ae7

Phoebe-0319 requested a review from TimKaibingchen as a code owner May 6, 2021 03:36

msft-fluent-ui-bot added the Fabric 5 label May 6, 2021

Phoebe-0319 requested a review from ecraig12345 May 6, 2021 07:53

ecraig12345 requested a review from a team May 6, 2021 17:47

ecraig12345 closed this May 6, 2021

ecraig12345 reopened this May 6, 2021

Merge branch 'office-ui-fabric-react_v5.79.1' into hotfix_15/office-u…

311b4a1

…i-fabric-react/5.79.1

Merge branch 'office-ui-fabric-react_v5.79.1' into hotfix_15/office-u…

e1bf8c3

…i-fabric-react/5.79.1

smhigley requested changes May 14, 2021

View reviewed changes

msft-fluent-ui-bot added Needs: Author Feedback and removed Needs: Author Feedback labels May 14, 2021

Phoebe-0319 added 2 commits May 17, 2021 03:43

resolve some comments

8721bd1

Merge branch 'hotfix_15/office-ui-fabric-react/5.79.1' of https://git…

3cf044f

…hub.com/Phoebe-0319/fluentui into hotfix_15/office-ui-fabric-react/5.79.1

Phoebe-0319 requested a review from smhigley May 17, 2021 10:50

smhigley reviewed May 17, 2021

View reviewed changes

resolve some comments

73e6446

Phoebe-0319 requested a review from smhigley May 18, 2021 07:23

smhigley approved these changes May 19, 2021

View reviewed changes

smhigley merged commit 539b05a into microsoft:office-ui-fabric-react_v5.79.1 May 19, 2021

Phoebe-0319 deleted the hotfix_15/office-ui-fabric-react/5.79.1 branch November 1, 2021 06:24

Phoebe-0319 added a commit to Phoebe-0319/fluentui that referenced this pull request Mar 19, 2022

Port microsoft#13659 & microsoft#18075 from office-ui-fabric-react_v5…

318ffa3

….79.1 to hotfix/5.135.6

Phoebe-0319 mentioned this pull request Mar 19, 2022

Port #13659 (Port #10020 to remove role=presentation in FocusZone) & #18075 (Partially port #5385 & #9003 to improve Dropdown accessibility) from office-ui-fabric-react_v5.79.1 to hotfix/5.135.6 #22169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partially port #5385 & #9003 to improve Dropdown accessibility in office-ui-fabric-react/5.79.1 #18075

Partially port #5385 & #9003 to improve Dropdown accessibility in office-ui-fabric-react/5.79.1 #18075

Phoebe-0319 commented May 6, 2021

ecraig12345 commented May 6, 2021

Phoebe-0319 commented May 12, 2021

Phoebe-0319 commented May 14, 2021

smhigley left a comment

smhigley May 14, 2021

Phoebe-0319 May 17, 2021

smhigley May 14, 2021

Phoebe-0319 May 17, 2021

smhigley May 14, 2021

Phoebe-0319 May 17, 2021

smhigley May 14, 2021

Phoebe-0319 May 17, 2021 •

edited

Loading

smhigley May 17, 2021

Phoebe-0319 May 18, 2021

smhigley May 14, 2021

smhigley May 17, 2021

Phoebe-0319 May 18, 2021

smhigley left a comment

Partially port #5385 & #9003 to improve Dropdown accessibility in office-ui-fabric-react/5.79.1 #18075

Partially port #5385 & #9003 to improve Dropdown accessibility in office-ui-fabric-react/5.79.1 #18075

Conversation

Phoebe-0319 commented May 6, 2021

Pull request checklist

Description of changes

Focus areas to test

ecraig12345 commented May 6, 2021

Phoebe-0319 commented May 12, 2021

Phoebe-0319 commented May 14, 2021

smhigley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phoebe-0319 May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smhigley left a comment

Choose a reason for hiding this comment

Phoebe-0319 May 17, 2021 •

edited

Loading