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.

Sign up for GitHub

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

Jump to bottom

Make Safari + VoiceOver announce the screen-reader-text within buttons in the correct order #13221

Merged
merged 2 commits into from
Jan 16, 2019
Merged

Make Safari + VoiceOver announce the screen-reader-text within buttons in the correct order #13221

merged 2 commits into from
Jan 16, 2019

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Jan 7, 2019

Description

Fixes a Safari + VoiceOver bug where the screen-reader-text within buttons is announced not respecting the source order. For more details, please see https://core.trac.wordpress.org/ticket/42006

  • Adds height: auto to screen reader text within buttons

Fixes #13219

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jan 7, 2019
aduth
aduth reviewed Jan 7, 2019
View reviewed changes
packages/components/src/button/style.scss
@@ -223,6 +223,11 @@
color: color(theme(outlines) shade(25%));
}
}

// Fixes a Safari+VoiceOver bug, where the screen reader text is announced not respecting the source order.
Copy link
Member

Choose a reason for hiding this comment

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

Could we include a reference link as part of the inline comment, perhaps to the Trac ticket or some of the debugging from the related HTML5 Boilerplate discussion, as without it, it'd be more difficult to revisit as part of future maintenance should the bug become resolved in Safari, or a better alternative proposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@youknowriad youknowriad requested a review from aduth January 9, 2019 15:04
@youknowriad youknowriad added this to the 4.9 (Gutenberg) milestone Jan 9, 2019
@aduth
Copy link
Member

aduth commented Jan 9, 2019

Testing instructions:

  1. Navigate to Posts > Add New
  2. Insert a title and/or content
  3. Activate macOS Voiceover via Cmd+F5
  4. Move focus to Preview button (may require Advanced settings configuration in Safari to navigate to Preview link by Tab, otherwise Option+Tab)
  5. Voiceover should read "link, Preview (Opens in a new tab)"

@aduth
Copy link
Member

aduth commented Jan 9, 2019

This appears to solve the issue for me in Safari, but the problem also exists (and is not fixed by these changes) for Voiceover in Chrome. Is that expected?

Is this intended to serve as a temporary fix, with a more durable one planned to be applied for all .screen-reader-text ? It's not clear to me why this should only impact buttons, or at least specifically the .component-button and not all button/link elements.

@afercia
Copy link
Contributor Author

afercia commented Jan 10, 2019

A more durable fix would be Apple fixing this bug 🙂

height: auto works for buttons because they have a height. Inline links or text don't have a height. Several approaches were tested on the HTML5 boilerplate issue https://github.com/h5bp/html5-boilerplate/issues/1985 but none of them works reliably.

If there are more places where height: auto can work I'd be happy to apply this fix there too. I just can't think of other components where we can do it reliably.

Re: Chrome, I wouldn't be too concerned, as many other things don't work so well with VoiceOver + Chrome and users know the best combo is Safari + VoiceOver. The reason why it doesn't work on Chrome is the inconsistent implementation of absolutely positioned flex items across browsers. In Chrome, display: inline-flex on the button does affect the position of the screen reader text, see the blue vertical line in the screenshot below:

screenshot 2019-01-10 at 08 59 52

  • it is taken out from the flow (and this is correct)
  • however, it's placed before the content, and this doesn't seem correct to me: in absence of left/right/top/bottom values, an absolutely positioned element should stay in its original position

The only way I can think of to fix this is by applying justify-content: flex-end on the button but I guess we don't want to touch the buttons styling. Anyways, I'd consider it a minor issue as we don't support the Chrome + VoiceOver combo.

Worth reminding the same fix is going to land in core, see https://core.trac.wordpress.org/ticket/42006

@afercia afercia merged commit 626d618 into master Jan 16, 2019
@afercia afercia deleted the fix/buttons-screen-reader-text-voiceover-order branch January 16, 2019 15:26
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@afercia @youknowriad
Make Safari + VoiceOver announce the screen-reader-text within button…
a729d83 
…s in the correct order (#13221)

* Add height auto to screen reader text within buttons.

* Add reference links to the inline comment.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@afercia @youknowriad
Make Safari + VoiceOver announce the screen-reader-text within button…
455348e 
…s in the correct order (#13221)

* Add height auto to screen reader text within buttons.

* Add reference links to the inline comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth approved these changes

Assignees
No one assigned
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Milestone
4.9 (Gutenberg)
Development

Successfully merging this pull request may close these issues.
3 participants
@afercia @aduth @youknowriad