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

[REVIEW] Let keyboard shortcut seek be announced by accessibility #3193

Merged
merged 7 commits into from Dec 6, 2018

Conversation

Projects
None yet
6 participants
@pajong
Member

pajong commented Nov 30, 2018

This PR will...

  • Add visually hidden div to the player
  • Add seek time update to the visually hidden div
  • Make the seek time update aria-live="assertive" to have assistive technology announce every change

Why is this Pull Request needed?

  • We want assistive technologies to announce time change from user interactive.
  • Assistive technologies does not announce changes to hidden elements, so we are adding a "fake" hidden element (from: https://www.w3.org/WAI/tutorials/forms/labels/)

Are there any points in the code the reviewer needs to double check?

This may not be an ideal solution. If you have suggestions on how to do this better, please let me know.

We are adding jw-hidden-accessibility to jw-wrapper, because the user can seek with keyboard without having focus on the controlbar. If we add the div to the controlbar instead of jw-wrapper, the changes are not announced when controlbar is not showing.

Are there any Pull Requests open in other repos which need to be merged with this?

Addresses Issue(s):

JW8-2467

@pajong pajong requested review from robwalch, radium-v and karimMourra Nov 30, 2018

@johnBartos

This comment has been minimized.

Member

johnBartos commented Nov 30, 2018

Warnings
⚠️

🗿 Set a milestone. It should be the ticket's fix version in JIRA.

Generated by 🚫 dangerJS

@jwplayer-robot

This comment has been minimized.

jwplayer-robot commented Nov 30, 2018

MULTI Build for commit 834984e passed.
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

@karimMourra

THis is an awesome POC but it seems excessive to pass _playerContainer down 3 layers just to get the hidden element. I'll think of an alternative

super('jw-slider-time', 'horizontal');
this._model = _model;
this._api = _api;
this.accessibilityContainer = _playerContainer.querySelector('.jw-hidden-accessibility');

This comment has been minimized.

@robwalch

robwalch Dec 4, 2018

Member

Pass in accessibilityContainer not _playerContainer on setup. There is no use for _playerContainer other than this query which should be done externally.

this.timeUpdateKeeper = document.createElement('p');
setAttribute(this.timeUpdateKeeper, 'aria-live', 'assertive');
this.accessibilityContainer.appendChild(this.timeUpdateKeeper);

This comment has been minimized.

@robwalch

robwalch Dec 4, 2018

Member

This should be added to the player template.

@@ -298,6 +305,7 @@ class TimeSlider extends Slider {
} else {
ariaText = `${timeFormat(position)} of ${timeFormat(duration)}`;
}
this.timeUpdateKeeper.innerHTML = ariaText;

This comment has been minimized.

@robwalch

robwalch Dec 4, 2018

Member

Use textContent instead of innerHTML when there is no HTML that needs to be parsed. This is a plain time string - no HTML.

@@ -121,7 +121,7 @@ export default class Controls {
}
// Controlbar
const controlbar = this.controlbar = new Controlbar(api, model);
const controlbar = this.controlbar = new Controlbar(api, model, this.playerContainer);

This comment has been minimized.

@robwalch

robwalch Dec 4, 2018

Member

Pass in the new element on setup.

@jwplayer-robot

This comment has been minimized.

jwplayer-robot commented Dec 4, 2018

MULTI Build for commit ba4b6fb passed.
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

@pajong pajong dismissed stale reviews from robwalch and karimMourra Dec 4, 2018

addressed all comments

super('jw-slider-time', 'horizontal');
this._model = _model;
this._api = _api;
this.accessibilityContainer = _accessibilityContainer;

This comment has been minimized.

@karimMourra

karimMourra Dec 4, 2018

Contributor

no need to keep a reference to this. accessibilityContainer

@jwplayer-robot

This comment has been minimized.

jwplayer-robot commented Dec 5, 2018

MULTI Build for commit a21c301 passed.
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

super('jw-slider-time', 'horizontal');
this._model = _model;
this._api = _api;
this.timeUpdateKeeper = _accessibilityContainer.querySelector('.jw-time-update');

This comment has been minimized.

@vseventer

vseventer Dec 5, 2018

Collaborator

Pass the .jw-time-update element to the constructor rather than the entire _accessibilityContainer.

addressed

@jwplayer-robot

This comment has been minimized.

jwplayer-robot commented Dec 5, 2018

MULTI Build for commit eadb4ca passed.
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

@robwalch

Almost there!

@@ -121,7 +122,7 @@ export default class Controls {
}
// Controlbar
const controlbar = this.controlbar = new Controlbar(api, model);
const controlbar = this.controlbar = new Controlbar(api, model, this.accessibilityContainer);

This comment has been minimized.

@robwalch

robwalch Dec 6, 2018

Member

We don't need this.accessibilityContainer as a public property on controls.

container = document.createElement('div');
container.appendChild(settingsButton);
container.appendChild(spacer);
controlBar = new ControlBar({}, model);
controlBar = new ControlBar({}, model, playerContainer);

This comment has been minimized.

@robwalch

robwalch Dec 6, 2018

Member

not what we pass anymore

@jwplayer-robot

This comment has been minimized.

jwplayer-robot commented Dec 6, 2018

MULTI Build for commit 3eebee8 passed.
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

@karimMourra karimMourra merged commit bf58362 into master Dec 6, 2018

4 checks passed

Danger ⚠️ Danger found some issues. Don't worry, everything is fixable.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
jw7-pr-multi-opensource Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment