Coachmark accessibility by leddie24 · Pull Request #5155 · microsoft/fluentui

leddie24 · 2018-06-08T22:57:08Z

Pull request checklist

[ ] Addresses an existing issue: Fixes #0000
[ ] Include a change request file using $ npm run change

Description of changes

There's some inadvertent changes from formatting due to Prettier; I'm not sure how to reset this, and pulling from master doesn't seem to change it results for me.

This PR adds accessibility features to Coachmark and TeachingBubble:

Keyboarding controls
ARIA labels and roles
Narrator support

Remaining To Do's

Check keyboarding accessibility
Fix teachingBubbleRef prop in CoachmarkTypes, or change method on how to focus TeachingBubble's root element.

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

…ing.

…c-react into coachmarkAccessibility # Conflicts: # packages/office-ui-fabric-react/src/components/TeachingBubble/TeachingBubbleContent.tsx

…ange from FocusZone to FocusTrapZone.

…for Coachmark

…set.

micahgodbolt · 2018-06-12T23:56:41Z

looks like snapshots are failing

…c-react into coachmarkAccessibility # Conflicts: # packages/office-ui-fabric-react/src/components/ColorPicker/ColorPicker.tsx # packages/office-ui-fabric-react/src/components/ColorPicker/ColorRectangle/ColorRectangle.tsx # packages/office-ui-fabric-react/src/components/ColorPicker/ColorSlider/ColorSlider.tsx

JasonGore

Thanks for the effort. My comments revolve mostly around understanding the difference in shortcomings of ARIA vs. Narrator.

JasonGore · 2018-06-18T16:16:16Z

packages/office-ui-fabric-react/src/components/Coachmark/Beak/Beak.tsx


    return (
-      <div className={css('ms-Beak', classNames.root)}>
+      <div className={css('ms-Beak', classNames.root)} aria-hidden="true">


Is there a strict need for aria-hidden? I'm curious since this element doesn't have an explicit role

Not sure here. Is aria-hidden only required if an element has a role attribute? I haven't done a lot of accessibility work before, so advice is appreciated here. It looks like narrator focuses on the Beak and reads aloud as "image" if it's in scan mode, which I don't think is desirable.

Aria-hidden hides all the subelements so it won't try to read the svg. Seems reasonable.

Can you try role='presentation' and see if that solves the issue? aria-hidden is kinda like a last resort hack.

Sure, updated to role="presentation"

You likely need it on the svg itself.

Instead of the root element, or both?

I thought it needed to be on the element and wasn't inherited. Give it a try w/ narrator+edge

I think we can leave it on the root element only. We have a FocusTrapZone on Coachmark anyways, so we can't tab out to the Beak element.

JasonGore · 2018-06-18T16:24:20Z

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

+  }
+
+  private _onKeyDown = (e: KeyboardEvent): void => {
+    // Open coachmark if user presses ALT + C (arbitrary keypress for now, will change later)


This answers a question I had.. how a user selects or activates the coachmark. It appears there is not a clearly defined role for this in ARIA. I'm curious what this could be changed to? Is there a standard of behavior for coachmarks that you know of?

Can you elaborate here? Not entirely sure what your ask is. Are you asking if we can/should customize the shortcut to open Coachmark? @cschlechty , can you chime in here if you have a chance please?

No, I'm asking how a user using Narrator actually enters and highlights the Coachmark using keyboard navigation. Tabbing seems to go past it. In the ARIA standard, I couldn't find a specified way to highlight a non-modal dialog.

Alt+C highlights it, but I'm curious how a user using assistive technology would know this.. is it standardized anywhere?

It's not a stand control or pattern. We're inventing it, and pushing its usage instructions via embedded usage string. @leddie24 be sure this is included in the examples.

Is that embedded usage string in this PR? I'm assuming it informs users about hitting ALT+C. I couldn't find it.. thanks

It'll be included in the prop for the control.

So it will only be set from outside the control? The default selection behavior is embedded in the control, so I'm wondering if the default notification on usage should be as well. (Similar to how "A coachmark has appeared" is.)

"A coachmark has appeared" and the usage instructions need to be localize-able, so they'll need to be passed in.

I understand that, but some are defaulted to English within the component and others are not. I feel like the navigation info is probably the most important one to be present (if any will be.)

JasonGore · 2018-06-18T16:24:58Z

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

        }, this.props.delayBeforeMouseOpen!);
+
+        // SetTimeout is hacky solution.  ARIA text doesn't read aloud unless it is appended or changed after the component
+        // is mounted


What would be the fix for this? Is this an issue with ARIA spec or with Narrator?

It looks like it's behaving correctly now? I removed the hacky solution and just had the text render directly inside of the <div>

False alarm, I tested this incorrectly. I switched to using setTimeout and appending a textNode. See here:

https://developer.paciellogroup.com/blog/2012/06/html5-accessibility-chops-aria-rolealert-browser-support/

…out hacky solution for ARIA text; seems to be working fine now in narrator.

…office-ui-fabric-react into coachmarkAccessibility

cschlechty · 2018-06-18T21:57:21Z

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

+          {ariaAlertText && (
+            <div
+              className={classNames.ariaContainer}
+              aria-live="assertive"


If you used role="alert" will it read? We could then remove the timeout, or is the timeout to focus the dialog content?

I reverted the change back to a setTimeout and appending a textNode. It doesn't seem to work unless it's one of these hacky methods:

https://developer.paciellogroup.com/blog/2012/06/html5-accessibility-chops-aria-rolealert-browser-support/

cschlechty · 2018-06-18T21:58:28Z

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

+                  >
+                    {isCollapsed &&
+                      ariaLabelledBy && (
+                        <h1 id={ariaLabelledBy} className={classNames.ariaContainer}>


why H1? I thought we decided to not use a header

Changed to a <p> element

cschlechty · 2018-06-18T22:02:35Z

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

+                    className={classNames.entityHost}
+                    tabIndex={-1}
+                    data-is-focusable={true}
+                    onClick={this._onFocusHandler}


Do we need a click?

Removed onClick

…c-react into coachmarkAccessibility

…file for fabric-website. Set default true for isPositionForced.

leddie24 · 2018-06-20T20:05:37Z

@JasonGore @cschlechty , is there anything else needed on this PR to get it merged? Thanks again for looking.

joschect · 2018-06-26T18:46:35Z

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

-                <FocusZone>
-                  <div className={classNames.entityHost} data-is-focusable={true} onFocus={this._onFocusHandler}>
-                    <div className={classNames.entityInnerHost} ref={this._entityInnerHostElement}>
+                <FocusTrapZone isClickableOutsideFocusTrap={true} forceFocusInsideTrap={false}>


What's the point of having this focustrap zone if you can click out of it and it doesn't force focus within it?

Removed the forceFocusInsideTrap prop. I think I inadvertently added this and forgot to remove it. I think we still want to have a clickable region outside of the trap though.

joschect · 2018-06-26T18:49:08Z

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

+                      )}
+                    {isCollapsed &&
+                      ariaDescribedBy && (
+                        <p id={ariaDescribedBy} className={classNames.ariaContainer}>


You can move this up to the other isCollapsed section so you don't need to evaluate twice. You might need to make it an array if it complains about needing a single parent but I don't believe you'll need to do that.

Fixed in latest commit

joschect · 2018-06-26T18:51:15Z

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

+        if (this.props.ariaAlertText) {
+          this._async.setTimeout(() => {
+            if (this.props.ariaAlertText && this._ariaAlertContainer.current) {
+              const alertText = document.createTextNode(this.props.ariaAlertText);


Why are you creating a new node instead of doing something like setState({ alertText: this.props.alertText}) and then in the container you can populate the alert text. That should work the same I believe.

Fixed in latest commit

joschect · 2018-06-26T18:53:27Z

packages/office-ui-fabric-react/src/components/TeachingBubble/TeachingBubbleContent.tsx

+        role={'dialog'}
+        tabIndex={-1}
+        aria-labelledby={ariaLabelledBy}
+        aria-describedby={ariaDescribedBy}


these don't appear in the dom if they're undefined, correct?

Correct, although they do appear in snapshot output

…xt state to Coachmark.

JasonGore · 2018-07-05T21:02:23Z

Spoke with Eddie and he said he'd address the merge conflict.

…c-react into coachmarkAccessibility

leddie24 · 2018-07-06T23:13:28Z

packages/office-ui-fabric-react/src/components/Coachmark/Coachmark.types.ts

+   */
+  ariaAlertText?: string;
+
+  /**


@JasonGore Any suggestions here? I'm not quite sure how make the interface for teachingBubbleRef look nicer.

JasonGore · 2018-07-06T23:20:11Z

packages/office-ui-fabric-react/src/components/Coachmark/Coachmark.types.ts

+
+  /**
+   * Ref for TeachingBubble
+   * @todo Fix interface here


What needs to be fixed about this interface?

JasonGore · 2018-07-06T23:21:35Z

packages/office-ui-fabric-react/src/components/Coachmark/Coachmark.types.ts

+  teachingBubbleRef?: {
+    (component: ITeachingBubble | null): void;
+    current: ITeachingBubble | null;
+    value: ITeachingBubble | null;


This seems like a weird flattened mixing of componentRef and and createRef, did you mean to do this?

Yes, not entirely sure what the right interface is. If I set it to teachingBubbleRef?: (component: ITeachingBubble | null) => void; then I don't have access to this.props.teachingBubbleRef.current in Coachmark.tsx (line 487). Do you know what's the correct way to implement this?

micahgodbolt · 2018-07-09T20:27:32Z

@JasonGore just ping me when you're ready for me to add my approval

JasonGore · 2018-07-09T20:32:33Z

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

        (): void => {
+          // Need setTimeout to trigger narrator
+          this._async.setTimeout(() => {
+            if (this.props.teachingBubbleRef && this.props.teachingBubbleRef) {


This is redundant

JasonGore · 2018-07-09T20:37:49Z

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

          this.forceUpdate();
        }

+        document.addEventListener('keydown', this._onKeyDown, true);


BaseComponent provides _events that should be used for these.. it automatically takes care of a lot of housekeeping for you. For example in KeytipLayer.base.tsx:

this._events.on(window, 'keydown', this._onKeyDown, true);

JasonGore

Just a couple of minor things that should probably be changed, but otherwise approved

…isteners to use _events

leddie24 · 2018-07-09T21:15:55Z

@micahgodbolt this should be good to go for your approval, made changes as requested from @JasonGore .

…5656) * Cherry pick Coachmark: Positioning fixes (#4995) e87289b * Cherry pick TeachingBubble/Coachmark: Fix TeachingBubble content wrapping to next line unnecessarily (#5121) 72bab3c * Cherrry pick Fix import paths to use relative paths in product code (#5338) e2076e1 * Cherry pick Coachmark accessibility #5155 7a36bb9 * Revert formatting back to 5.0 in Coachmark.tsx * Fix unnecessary ; and change " to ' * Fix merge conflicts for Coachmark.Basic.Example.tsx * " to ' * Fix imports for TeachingBubble.tsx * Update TeachinBubbleContent, update TeachingBubble snapshot * Update snapshot. Fix TeachingBubble.types * Fix import for RefObject * Fix some inadvertent formatting from prettier.

edwlmsft and others added 8 commits June 7, 2018 11:28

Add aria alert text

2b7eabf

Add keypress shortcut to open coachmark

eea962f

Have narrator announce TeachingBubbleContent text upon coachmark open…

ec0dad4

…ing.

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

f97afeb

…c-react into coachmarkAccessibility # Conflicts: # packages/office-ui-fabric-react/src/components/TeachingBubble/TeachingBubbleContent.tsx

Add aria-hidden to beak component

689018c

Add aria-hidden to ariaAlert container once Coachmark is expanded. Ch…

a4f5f14

…ange from FocusZone to FocusTrapZone.

Change back to FocusZone. Add ariaLabelled and ariaDescribedBy props …

6e9019a

…for Coachmark

Remove force focus on coachmark after mounting. Remove console logs

1576232

leddie24 requested review from cschlechty and joschect June 8, 2018 22:57

leddie24 requested a review from micahgodbolt as a code owner June 8, 2018 22:57

Only render elements if ariaLabelledBy and ariaDescribedBy props are …

17ac4d7

…set.

edwlmsft added 3 commits June 13, 2018 12:36

Update snapshot test for TeachingBubble

4fa9c4d

Add change file

752ef79

JasonGore reviewed Jun 18, 2018

View reviewed changes

edwlmsft added 3 commits June 18, 2018 14:42

Add onKeyDown, change from FocusZone to FocusTrapZone. Remove setTime…

e4c7b20

…out hacky solution for ARIA text; seems to be working fine now in narrator.

Merge branch 'coachmarkAccessibility' of https://github.com/leddie24/…

e70b32b

…office-ui-fabric-react into coachmarkAccessibility

Add escape key handler to TeachingBubbleContent

f2173de

cschlechty reviewed Jun 18, 2018

View reviewed changes

edwlmsft added 6 commits June 18, 2018 15:49

Remove onClick, change from h1 to p

f1e86c5

Add back in setTimeout for appending ariaAlertText.

344167f

Change from aria-hidden to role="presentation" for Beak component.

93811cf

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

32029ac

…c-react into coachmarkAccessibility

Fix inadvertent change to AppState.tsx file (checkout from master)

ef70ee4

Update Do's. Edit basic example ariaDescribedByText prop. Add change …

101854f

…file for fabric-website. Set default true for isPositionForced.

JasonGore mentioned this pull request Jun 25, 2018

[Accessibility] Callout is not readable by windows narrator when callout is not having any focusable element #1655

Closed

joschect reviewed Jun 26, 2018

View reviewed changes

Change import path of FocusTrapZone to use relative path. Add alertTe…

7774b0e

…xt state to Coachmark.

JasonGore assigned leddie24 Jul 5, 2018

edwlmsft added 5 commits July 6, 2018 15:15

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

3b1c254

…c-react into coachmarkAccessibility

Fix bad commits

94b7034

remove unused css import for Beak

413e38d

fix bad commit for package.json in dashboard-grid-layout

89b06a9

remove unused change file

e149c87

leddie24 commented Jul 6, 2018

View reviewed changes

JasonGore reviewed Jul 6, 2018

View reviewed changes

edwlmsft added 2 commits July 6, 2018 17:30

Fix teachingBubbleRef interface

989b8b7

Add line

4884b00

JasonGore reviewed Jul 9, 2018

View reviewed changes

JasonGore approved these changes Jul 9, 2018

View reviewed changes

Remove redundant check for this.props.teachingBubbleRef. Move event l…

b04c5b3

…isteners to use _events

micahgodbolt approved these changes Jul 9, 2018

View reviewed changes

leddie24 merged commit 7a36bb9 into microsoft:master Jul 9, 2018

leddie24 mentioned this pull request Jul 23, 2018

Cherry pick Coachmark accessibility and positioning fixes into 5.0 #5656

Merged

2 tasks

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

Conversation

leddie24 commented Jun 8, 2018 • edited by fabricteam 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

Microsoft Reviewers: Open in CodeFlow

Uh oh!

micahgodbolt commented Jun 12, 2018

Uh oh!

JasonGore left a comment • 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!

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!

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!

JasonGore Jun 18, 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!

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!

JasonGore Jun 18, 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!

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!

cschlechty Jun 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leddie24 Jun 19, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leddie24 commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leddie24 commented Jun 8, 2018 •

edited by fabricteam

Loading

JasonGore left a comment •

edited

Loading

JasonGore Jun 18, 2018 •

edited

Loading

JasonGore Jun 18, 2018 •

edited

Loading

cschlechty Jun 18, 2018 •

edited

Loading

leddie24 Jun 19, 2018 •

edited

Loading

leddie24 Jul 6, 2018 •

edited

Loading