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.

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

Make the withNotices notices announced by assistive technologies #9443

Closed
wants to merge 7 commits into from

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Aug 29, 2018

Description

The notices created by withNotices convey important information to users but they're not announced by screen readers. This PR simply adds a role="alert" attribute to the notice wrapper. Reference: https://www.w3.org/TR/wai-aria-1.1/#alert

Alerts are assertive live regions and will be processed as such by assistive technologies.

So in this case, this is all it takes to make the notices text announced by screen readers.

To test, use a screen reader. On a mac you can use Safari and VoiceOver (Cmd+F5).
To make one of these notices appear, you can set background and text color on a paragraph in a way to make the contrast ratio notice appear.
Before this change, nothing is announced. After this change, VoiceOver automatically announces the notice text:

screen shot 2018-08-29 at 19 38 56

Other similar notices are used in the blocks, typically when dragging e media file within a block that accepts media. You can, for example, set "Offline" in your dev tools, drag an image on an Image block, and see the related norice appear (and be announced).

Fixes #9442

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Aug 29, 2018
@@ -21,7 +21,7 @@ function Notice( { className, status, children, onRemove = noop, isDismissible =
'is-dismissible': isDismissible,
} );
return (
<div className={ classNames }>
<div className={ classNames } role="alert">
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for this improvement @afercia! I did some tests, and it looks like we are applying the role="alert" to a div that also contains a dismiss icon button. So for example in the max upload error message we get this as the alert message:
image

Maybe we can apply the role to a div that does not contain the IconButton, so we avoid referencing the dismiss as part of the alert message.

(To easily replicate the problem we can limit the site upload limit by adding the following two lines to .htaccess file php_value upload_max_filesize 1M, php_value post_max_size 1M)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgefilipecosta thanks, yep noticed that and can replicate in an easier way (I'm on nginx - VVV, no .htaccess here 😉):

screen shot 2018-08-30 at 08 57 19

The problem is if the notice content is not a string, then there's no inner wrapper:

{ isString( children ) ? <div className="components-notice__content">{ children }</div> : children }

That leads me to the question: why components-notice__content shouldn't be rendered when the children is not a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah hm seems to me that's because originally the wrapper was a <p> element, so it made sense to avoid to wrap other elements in a paragraph:
d64de0f#diff-90222e8dfd0cf666132b45557dab1ba9R23

but now that the wrapper is a div, I think it can just be always rendered.

@afercia
Copy link
Contributor Author

afercia commented Aug 30, 2018

I've tested it extensively in Windows and unfortunately it doesn't work so well. The thing is, role="alert" must be set on the element that is added to the DOM. For example, in the contrast checker, it must be set on the outer wrapper editor-contrast-checker otherwise it won't work, especially in Firefox.

I can think of two options:

  • make sure the Notice is added to the DOM without wrappers and live with the dismiss button being announced
  • completely change approach and use speak from @wordpress/a11y, but I'm not sure it can be done easily

@afercia afercia force-pushed the update/with-notices-notices-role-alert branch from d97f1e3 to 7e9f6ca Compare August 30, 2018 10:57
@afercia
Copy link
Contributor Author

afercia commented Aug 30, 2018

Although support for role=alert has improved across browsers and assistive technologies, seems it doesn't work so well especially when components render (and re-render). I've changed approach and this is now a first pass to use speak for the audible messages. It also allows to pass only the message (and avoid the dismiss button) and even to pass a custom string for the message. If the approach looks good, the PR can be refined further.

Aside issue: I'd greatly appreciate some help to understand why the contrast-checker renders twice when changing a color. Actually, seems to me it's the PanelColorSettings that renders twice. This is important for the way a11y speak works, especially for Safari. It should render only once, and send a speak message only once at each button press. Happens only on the Paragraph block, doesn't happen on the Button block.

@afercia afercia force-pushed the update/with-notices-notices-role-alert branch from 52f63aa to 488c121 Compare August 30, 2018 16:05
@afercia
Copy link
Contributor Author

afercia commented Aug 31, 2018

Going to change the PR title, as it doesn't use a role=alert any longer.

@afercia afercia changed the title Add role alert to the withNotices notices Make the withNotices notices announced by assistive technologies Aug 31, 2018
@gziolo gziolo requested a review from aduth January 31, 2019 13:51
@aduth
Copy link
Member

aduth commented Feb 6, 2019

As a passing comment, the changes here as currently proposed would conflict with / duplicate the spoken messages triggered by notices created through the notices module.

I don't know that it's tracked anywhere, but I foresee that withNotices can (and should) be reimplemented to use the notices module, assigning some random/unique value as a scope using the notice module's context property.

https://wordpress.org/gutenberg/handbook/designers-developers/developers/data/data-core-notices/#createnotice

Previously: #9617

@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

@afercia do you plan to bring this PR up to date and update to take comments from @aduth into account? For the time being, I mark this PR as Stale to make triaging and reviewing easier. Feel free to replace it with In Progress when you refresh PR.

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 7, 2019
@afercia
Copy link
Contributor Author

afercia commented Feb 16, 2019

the changes here as currently proposed would conflict with / duplicate the spoken messages triggered by notices created through the notices module

Timing issues 🙂This PR was made a few days before the notices module one. I'd agree consolidating in an more abstracted implementation is the best way to go.

@gziolo @aduth no objections in closing this PR: honestly, I wouldn't know where to start to re-implement withNotices using the notices module. That would need someone with better knowledge than me. Please let me know if I should open a new issue.

However, quick question: is the notices module able to handle also "in place" notices besides the ones at the top of the editor? We'd need something to announce notices that appear within other components, for example:

the contrast checker:

screenshot 2019-02-16 at 16 05 04

notices when the apiFetch response is invalid / errors:

screenshot 2019-02-16 at 16 05 53

allowed file types:

screenshot 2019-02-16 at 16 22 18

etc. etc.

Also, some notices (e.g. the template validation one) appear on page load. We should make sure the speak() message works well or if it would need a brief delay:

screenshot 2019-02-16 at 13 50 48

@gziolo
Copy link
Member

gziolo commented Feb 17, 2019

However, quick question: is the notices module able to handle also "in place" notices besides the ones at the top of the editor? We'd need something to announce notices that appear within other components, for example:

Yes. This is possible as @aduth explained:

I don't know that it's tracked anywhere, but I foresee that withNotices can (and should) be reimplemented to use the notices module, assigning some random/unique value as a scope using the notice module's context property.

However, this needs to be implemented this way. I think it would solve also a related issue with multiple notices showing up in those components. I think I saw PRs trying to fix it the other day.

Saying that I will close this PR and copy my note to the parent issue to give a direction for someone who would like to fix it. Thanks for working on that PR 💯

@gziolo gziolo closed this Feb 17, 2019
@gziolo gziolo deleted the update/with-notices-notices-role-alert branch February 17, 2019 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants