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: current time and duration display accessibility with VoiceOver #5653

Merged
merged 3 commits into from
Dec 11, 2018

Conversation

alex-barstow
Copy link
Contributor

@alex-barstow alex-barstow commented Dec 5, 2018

Description

The desired behavior when navigating any of the TimeDisplay components (current time, duration, remaining time) with the arrow keys with a screen reader is for the singular parent div to receive focus, and the reader should announce the text of each child span in order, ex. "Current Time 0:17" and "Duration 1:10". This is how it works when using JAWS. However, with VoiceOver each individual child span element receives focus and the contents are announced separately:

Screenshot of 1st span - control text
Screenshot of 2nd span - numerical time

According to the ARIA spec (see here and example 8), there is no implicit role for span elements, so the contents should be exposed but the elements themselves should be invisible to screen readers. In other words, <span> Sample Content </span> should be the same as <span role="presentation"> Sample Content </span>. But this is not the case with VoiceOver. As far as I can tell, the desired behavior is only achieved with VoiceOver if each child span is explicitly made presentational, either by assigning each of them the presentation role, or by assigning the parent div a role that makes its children presentational (although there doesn't seem to be a role whose semantics fit the purpose of that div)

The first ARIA doc link above shows the Mac OS X Accessibility Protocol mapping for span elements is to the group role. I don't know enough about accessibility API mapping to confidently draw conclusions about root cause, but this seems like a possible explanation. Pinging @gkatsev and @OwenEdwards for your expertise on this.

Proposed Additions

  • add role="presentation" to each of the two span elements inside TimeDisplay divs
  • add aria-hidden="true to the TimeDivider (tangential improvement)
  • fix inaccurate description of the TimeDisplay component (tangential improvement)

// this element and its contents can be hidden from assistive techs since
// it is made extraneous by the announcement of the control text
// for the current time and duration displays
innerHTML: '<div aria-hidden="true"><span>/</span></div>'
Copy link
Member

Choose a reason for hiding this comment

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

rather than it being added at this level, we should make the main time-divider div be aria-hidden, super.createEl may accent a 3rd argument object for properties, if not, you can use Dom.createEl directly and return that.

@OwenEdwards
Copy link
Member

I've got to say that I don't like this at all.

There is no "standard" for how screen readers implement their Virtual Cursor mode (moving through all DOM content with the arrow keys, rather than just moving through active elements with Tab/Shift-Tab), and for a long time VoiceOver has treated <span>s as a "breath break" where JAWS has not. Trying to get those two screen readers to behave in the same way is a can of worms that we don't want to open; the screen reader behavior should be consistent with what it does everywhere else on the web, quirks and all, because the users know those quirks. That user may use both JAWS and VoiceOver, but they may not; in either case, they know how each one behaves much better than non-users do.

This PR is also using ARIA in a non-standard way that may get flagged by conformance checking and accessibility checking tools. According to https://www.w3.org/TR/html-aria/, the role of a <div> and the role of a <span> are identical; it's just that VoiceOver treats them differently in its Virtual Cursor mode, which it's entirely free to do. It's definitely inconvenient that adding spans for all kinds of purposes in HTML causes VoiceOver to break up what was previously a single "breath" into multiple "breaths", but I'm not comfortable with this hack to get around it.

@alex-barstow
Copy link
Contributor Author

alex-barstow commented Dec 7, 2018

@OwenEdwards Makes sense to me. I was tasked with investigating a workaround, but I'm perfectly content leaving things as is and calling it an expected VoiceOver behavior. A couple follow-up questions: 1) Is it still reasonable to hide the TimeDivider "/" from readers, or do you think that should remain visible as well? 2) What specifically in this PR might get flagged by conformance-checking tools? Do you mean that adding a presentation role to a span element is inherently non-standard? This states that a span can be given any role, and I found an additional example (8) that states it is "redundant in some implementations", but I took that to mean it isn't explicitly forbidden, there may be valid use cases for it, and at worst it is superfluous. Again, I have zero investment in this PR, I'm just asking for curiosity's sake since this is a new area for me. Thanks!

@OwenEdwards
Copy link
Member

@alex-barstow I try to keep this work separate from my "day job", but I talked it over with some of my smartest colleagues, and I'm going to admit that I was wrong. In general there are real issues about applying ARIA roles to HTML elements where the role matches the implicit role of the element, and I thought that this was true in this case. I now understand that it's not - a <span> does not have an implicit role, rather than having an implicit role of presentation. So you're right - this isn't "non-conforming", and that example (8) does specifically use the construct and have that note.

So I change my review, with my appologies. And yes, I do think that hiding the "/" from screen readers makes sense, in the same way that I recommended in #5168 that the "-" be hidden from screen readers; as you say, the additional information for screen reader users makes these visual elements redundant.

Copy link
Member

@OwenEdwards OwenEdwards left a comment

Choose a reason for hiding this comment

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

One suggestion for a change in a comment, otherwise LGTM.

});

this.contentEl_ = Dom.createEl('span', {
className: `${className}-display`
}, {
// tell screen readers not to automatically read the time as it changes
'aria-live': 'off'
'aria-live': 'off',
// span elements should have no implicit role semantics, but make this
Copy link
Member

@OwenEdwards OwenEdwards Dec 7, 2018

Choose a reason for hiding this comment

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

You could change this comment to "span elements have no implicit role, but some screen readers (notably VoiceOver) treat them as a break between items in the DOM when using arrow keys (or left-to-right swipes on iOS) to read contents of a page. Using role='presentation' causes VoiceOver to NOT treat this span as a break."

@gkatsev gkatsev added the patch This PR can be added to a patch release. label Dec 10, 2018
@alex-barstow
Copy link
Contributor Author

Tested with JAWS, VoiceOver, and NVDA -- I'm seeing the correct behavior with all three

@OwenEdwards
Copy link
Member

We might want to make similar changes elsewhere, including in #5655.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Thanks, Alex! 👍

@gkatsev gkatsev merged commit 8932611 into videojs:master Dec 11, 2018
@gkatsev gkatsev added the a11y This item might affect the accessibility of the player label Dec 11, 2018
@alex-barstow alex-barstow deleted the time-display-a11y-fixes branch December 11, 2018 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y This item might affect the accessibility of the player patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants