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

Fix: Publish date not announced in voice over; Redundant duplicate labels; #15381

Merged
merged 2 commits into from
Jul 8, 2019

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented May 1, 2019

Fix: #11747

This PR fixes #11747 by removing a duplicate label.
It also addresses another problem. Given that we have a Publish label labeling the button, the publish date is never announced for users of assistive technologies. Only the word publish is announced, this PR addresses that problem.
I'm using an aria-label for the button because, besides the date normally shown in the button, I also want assistive technologies to announce 'Click to change'. I'm not sure if announcing 'Click to change' is required but it seemed helpful.

How has this been tested?

I used voice over and verified the select date is now announced when the button gets focus.

Screenshots

May-01-2019 23-01-17

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Code Quality Issues or PRs that relate to code quality Needs Accessibility Feedback Need input from accessibility labels May 1, 2019
@jorgefilipecosta jorgefilipecosta requested review from afercia and gziolo May 2, 2019 10:04
@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label May 2, 2019
@afercia
Copy link
Contributor

afercia commented May 2, 2019

@jorgefilipecosta thanks for looking into this!

Reminder:

  • when testing with VoiceOver, please use Safari. Testing with Chrome + VoiceOver may be misleading, as some things might be announced (or not announced) in unexpected ways
  • consider that screen reader users navigate content mostly by arrowing (with VoiceOver that is Control-Option-Right (or Left) Arrow) rather than by tabbing

Chrome + VoiceOver before:

before Chrome

Safari + VoiceOver before:

before Safari

Technically, multiple labels are valid and as you can see Safari+VoiceOver announce both labels. The fact it doesn't work with Chrome is very likely a Chrome bug.

Safari + VoiceOver after the patch:

Screenshot 2019-05-02 at 15 04 25

and, after a brief pause, the aria-describedby is announced:

Screenshot 2019-05-02 at 15 08 00

  • I'm not sure using an aria-label="Mar 19, 2019 12:05 am Click to change" to add Click to change overriding the button visible text that already says Mar 19, 2019 12:05 am can help so much. This control is a button: it's implied that can be clicked to do something.
  • The aria-describedby targets what the real label should be, but it can be missed as it's announced only after a brief pause. In any case "Publish" shouldn't be the description.
  • Pre-existing issue: aria-polite used this way is buggy, will open a separate issue

The real, fundamental problem with these "Publish" and "Visibility" buttons is that their text represents the current value. Instead, it should be the available action: the "what" the buttons are about.

This was initially reported on #470, created on April 2017. Two years ago. Still pending a solution.

I don't want to repeat all the considerations made on #470 but, to summarize: controls should never be labelled with the current value or state. Labels are meant to give controls their accessible name, that's meant to give users the necessary information on what a control does.

Imagine a "first name" input field: would anyone ever label the input field with their first name? (see screenshot below):

label value

I guess nobody would 🙂 However, that's basically what these two buttons do: they're labelled with the current value, which is totally incorrect from a web standards / semantics / accessibility perspective.

For now, I'd suggest to change this PR and make the Publish button the same as the Visibility button: they should work the same and also get the same improvements. Please remove the aria-label, aria-describedby, the id on the span. I'd then recommend to give #470 a higher priority and come to a better solution.

Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

Please see previous comment.

@afercia afercia removed the Needs Accessibility Feedback Need input from accessibility label May 9, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the fix/publish-not-annouced-in-voice-over branch 3 times, most recently from 5808ab3 to 0d1b38d Compare May 28, 2019 09:01
@jorgefilipecosta
Copy link
Member Author

Thank you for all the context provided @afercia.

Please remove the aria-label, aria-describedby, the id on the span.

I followed the suggestion and applied these changes, this control is now equal to the visibility control and the same improvements could be applied to but controls.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/publish-not-annouced-in-voice-over branch from 0d1b38d to 86e2dde Compare July 1, 2019 20:00
@jorgefilipecosta
Copy link
Member Author

This PR was rebased, I guess it is ready to merge.

@afercia
Copy link
Contributor

afercia commented Jul 8, 2019

@jorgefilipecosta thanks! Looks good to me, as it's a simplification and at least the two buttons are now the same.

I've removed aria-live="polite" from the publish date button, as it was triggering an announcement at any change in the date picker. Instead, I'd like to suggest to use a speak() message when users confirm the new date: this could be added, together with other improvements, in #470.

Please do feel free to merge at your leisure (if no objections)!

@jorgefilipecosta jorgefilipecosta force-pushed the fix/publish-not-annouced-in-voice-over branch from 4eeaa7e to f4e380f Compare July 8, 2019 16:34
@jorgefilipecosta jorgefilipecosta merged commit e7807e3 into master Jul 8, 2019
@jorgefilipecosta jorgefilipecosta deleted the fix/publish-not-annouced-in-voice-over branch July 8, 2019 17:09
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). Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PostSchedule redundant label element
3 participants