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

ADD aria attributes for better accessibility... #1198

Merged
merged 6 commits into from Jun 27, 2016
Merged

ADD aria attributes for better accessibility... #1198

merged 6 commits into from Jun 27, 2016

Conversation

francoismassart
Copy link
Contributor

@francoismassart francoismassart commented May 18, 2016

Changes proposed in this pull request:

Make the JS version of jwplayer compatible with Voice Over via aria-label attributes

Fixes #1183
JW7-2628

-Labels can be translated via the config
-aria-label
-aria-hidden (when needed)
-role
-tabindex

-Labels can be translated via the config
-aria-label
-aria-hidden (when needed)
-role
-tabindex
@jwplayer-robot
Copy link

Can one of the admins verify this patch?

@robwalch robwalch self-assigned this May 18, 2016
@robwalch
Copy link
Contributor

Thanks @francoismassart! I'm reviewing with the team and determining which release this can make it in to.

@francoismassart
Copy link
Contributor Author

No problem if you edit my changes but one thing that matter to us is to be able to translate the aria-label's. Our sites are in French, so are our blind users...
Thank you for the feedback.

@robwalch
Copy link
Contributor

Yes. We would have liked to figure out internationalization for the player before the labels but hopefully this will get things going in that direction.

I'd like to make the setup option multi-purpose and use simpler names that don't match the CSS classes and allow us to use dot syntax. So for config Defaults it would look more like:

localization: {
  play: 'Play',
  playback: 'Start playback',
  pause: 'Pause',
  volume: 'Volume',
  prev: 'Previous',
  next: 'Next',
  cast: 'Chromecast',
  fullscreen: 'Fullscreen',
  playlist: 'Playlist',
  hd: 'Quality',
  cc: 'Closed captions',
  audioTracks: 'Audio tracks', // need to convert camelCase to hyphens
  replay: 'Replay',
  buffer: 'Loading'
}

This should still allow you to provide localization strings for the aria tags en Français ;)

@francoismassart
Copy link
Contributor Author

Please add also the "live status" translation which is currently harcoded to "Live broadcast" ;-)
Or I can do another PR once you merged the aria labels.

@francoismassart
Copy link
Contributor Author

@robwalch do you have any feedback on this PR?
Thank you

@robwalch
Copy link
Contributor

robwalch commented Jun 3, 2016

Yes. I'll inline my comment from above.

'jw-icon-audio-tracks': 'Audio tracks',
'jw-icon-replay': 'Replay',
'jw-icon-buffer': 'Loading'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use simpler names that don't match the CSS classes and allow for dot syntax. This aria block should look like:

localization: {
  play: 'Play',
  playback: 'Start playback',
  pause: 'Pause',
  volume: 'Volume',
  prev: 'Previous',
  next: 'Next',
  cast: 'Chromecast',
  fullscreen: 'Fullscreen',
  playlist: 'Playlist',
  hd: 'Quality',
  cc: 'Closed captions',
  audioTracks: 'Audio tracks', // need to convert camelCase to hyphens
  replay: 'Replay',
  buffer: 'Loading'
}

@robwalch
Copy link
Contributor

robwalch commented Jun 3, 2016

The main thing is to simplify the names in the aria block. Also please include information in the description regarding browser support and testing. Which operating systems and browsers did you test this on, and what is the best way for our team to validate and test these accessibility features?

Thanks for going the extra mile with role and aria-hidden. We'll need to evaluate how these and the especially the tabindex changes impact the player on it's own and within existing sites.

Import changes from jwplayer/jwplayer:master
+ ADD accessibilty.md with plenty of documentation and notes
+ `config.aria` to `config.localization` ...
+ renaming properties of `config.localization`
+ ADD new translations for `more`, `liveBroadcast` & `loadingAd`
+ TESTING providing `<meta charset="utf-8">` allowing the usage of accents inside the html file
+ TESTING using custom `aria-label` values set via `config.localization` in French
+ FIX for assigning the role attribute valur in `controlbar.js`
Importing latest change fro the `master` of `jwplayer/jwplayer`
@francoismassart
Copy link
Contributor Author

Hey @robwalch about the tabindex usage, it should not cause any issue.
I added more explainations inside the accessibility.md

tabindex="0" means "please allow this html node to be accessed via the tab key (even if it is a <div>), but don't change the order, just use the same order as the markup"

tabindex="-1" means "please ignore this html node when the user uses the tab key"

tabindex="1", tabindex="2", etc. (any other value than "0" or "-1") should never be used because it messes with the natural order defined by the markup and it'll quickly become nearly impossible to manage and can create unexpected tab jumps...

You can read more about this here:
Don’t Use Tabindex Greater than 0

As you will read inside the accessibility.md, there is still other issues to fix for improving the accessibility in JWPlayer but this would be already nice to have.

Can you merge or provide additional feedback ?

Cheers

this.el.setAttribute('role', 'button');
this.el.setAttribute('aria-label', ariaText);
}
if (ariaShown === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

more succinct to write as if (ariaShown)

@robwalch
Copy link
Contributor

Thank you @francoismassart. My team needs to demo the new features to our product manager to get it our roadmap.

I'd like us to set the milestone of 7.5 for this, but it might have to wait until later. I'll keep you posted.

@johnBartos johnBartos self-assigned this Jun 14, 2016
@francoismassart
Copy link
Contributor Author

@robwalch, I can make a quick screencast demonstrating the before/after user experience if you want.
Just let me know :^)

@robwalch robwalch added this to the v7.5.0 milestone Jun 22, 2016
@dannyfinks
Copy link
Contributor

test this please

@robwalch robwalch merged commit feaec89 into jwplayer:master Jun 27, 2016
hussam-i-am pushed a commit that referenced this pull request Jun 28, 2016
…st/vtt-unit

* 'master' of https://github.com/jwplayer/jwplayer: (21 commits)
  Setup 608 Captions in mixin (#1274)
  Add subtitles tracks to CC menu JW7-2641
  Set default line value to -1 instead of 'auto' so captions render in the same line in Safari and Chrome. Revert view changes to captions layer positioning
  Add sideloaded tracks using video method JW7-2484
  Sort levels for Flash in flash.js JW7-2607
  Style focused elements using hover colors JW7-2628
  Improvements from feedback
  Split VTT.js into: vttparser, vttcue (polyfill), vttregion (polyfill), vttrenderer. Updated mixin and renderer to load files async only when needed.
  Remove log messages
  Cleanup code and improve comments
  Convert flash 608 cues to VTTCues for rendering with VTT.js
  Working on consistent loading of tracks across providers
  Always apply captions styles in renderer
  Only set track mode when rendering natively. Add polyfill to captions renderer
  Port all track loading functionality to the tracks mixin
  Leverage VTT.js for non-native captions rendering JW7-1657
  ADD aria attributes for better accessibility... (#1198) JW7-2628
  Fixes #1149: Remove bower dependencies
  Emit meta events for cue changes in Safari JW7-2637
  Try to play the next item in adpod on error
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants