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

Accessibility: Announce notices to assertive technologies #4192

Merged
merged 1 commit into from Jan 2, 2018

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Dec 28, 2017

closes #3690

This PR uses the a11y lib to announce the publish flow notices to assertive technologies.
I was wondering if I should do this systematically for all notices (in a generic way) but decided not to because the assertive message could defer from the notice's content.

@youknowriad youknowriad requested a review from afercia Dec 28, 2017

@youknowriad youknowriad self-assigned this Dec 28, 2017

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Dec 28, 2017

Member

I think a generic way would be preferable as by default we would always announce notices.

the assertive message could defer from the notice's content.

One option the overcome that problem in a generic approach would be to add an option called spokenMessage to createNotice options. When that option is present we would use that property instead of the normal notice message for the spoken text.

Member

jorgefilipecosta commented Dec 28, 2017

I think a generic way would be preferable as by default we would always announce notices.

the assertive message could defer from the notice's content.

One option the overcome that problem in a generic approach would be to add an option called spokenMessage to createNotice options. When that option is present we would use that property instead of the normal notice message for the spoken text.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 28, 2017

Contributor

One option the overcome that problem in a generic approach would be to add an option called spokenMessage to createNotice options. When that option is present we would use that property instead of the normal notice message for the spoken text

I thought about this, but if the developer is unaware of the behavior, this might produce "wrong" messages (html inside etc...). Thoughts @afercia

Contributor

youknowriad commented Dec 28, 2017

One option the overcome that problem in a generic approach would be to add an option called spokenMessage to createNotice options. When that option is present we would use that property instead of the normal notice message for the spoken text

I thought about this, but if the developer is unaware of the behavior, this might produce "wrong" messages (html inside etc...). Thoughts @afercia

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Dec 28, 2017

Contributor

Hm as far as I know, speak should already strip out HTML?
Ideally, the string for an audible message should be carefully crafted, for example it wouldn't make much sense to reuse "Post published! View post". Even stripping out the link, View post wouldn't make much sense. The message could simply be Post published. I'd tend to agree with @jorgefilipecosta: add a specific string for the audible messages, fallback to the notice string, educate developers, and document everything.

Contributor

afercia commented Dec 28, 2017

Hm as far as I know, speak should already strip out HTML?
Ideally, the string for an audible message should be carefully crafted, for example it wouldn't make much sense to reuse "Post published! View post". Even stripping out the link, View post wouldn't make much sense. The message could simply be Post published. I'd tend to agree with @jorgefilipecosta: add a specific string for the audible messages, fallback to the notice string, educate developers, and document everything.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 28, 2017

Contributor

PR updated with the generic approach.

Contributor

youknowriad commented Dec 28, 2017

PR updated with the generic approach.

@jorgefilipecosta

From the code point view changes look good to me 👍

@youknowriad youknowriad merged commit 617edcb into master Jan 2, 2018

3 checks passed

codecov/project 40.42% (+1.25%) compared to edf17df
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/announce-notices branch Jan 2, 2018

@@ -346,6 +346,7 @@ describe( 'effects', () => {
id: 'SAVE_POST_NOTICE_ID',
isDismissible: true,
status: 'success',
spokenMessage: 'Post published!',

This comment has been minimized.

@aduth

aduth Jan 3, 2018

Member

These should be localized.

@aduth

aduth Jan 3, 2018

Member

These should be localized.

This comment has been minimized.

@youknowriad

youknowriad Jan 4, 2018

Contributor

Actually, these are only tests. The message is localized :)

@youknowriad

youknowriad Jan 4, 2018

Contributor

Actually, these are only tests. The message is localized :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment