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: Add alternate text in an ARIA alert to the loading spinner. #4916

Merged
merged 3 commits into from Feb 22, 2018

Conversation

Projects
None yet
3 participants
@misteroneill
Member

misteroneill commented Jan 31, 2018

Partial fix for #4902

Verified in VoiceOver.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors
return super.createEl('div', {
className: 'vjs-loading-spinner',
dir: 'ltr'
});
}, {
role: 'alert'

This comment has been minimized.

@gkatsev

gkatsev Jan 31, 2018

Member

I'm not sure we want this, at least not yet.

This comment has been minimized.

@misteroneill

misteroneill Jan 31, 2018

Member

Yeah, me either. I wanted to see how it behaved.

}, {
role: 'alert'
}, [
dom.createEl('div', {

This comment has been minimized.

@gkatsev

gkatsev Jan 31, 2018

Member

I'm not seeing the innerHTML in the live example: https://deploy-preview-4916--videojs-docs.netlify.com/test-example/

Also, I think it would be better to use a span here as it also patches other vjs-control-text uses.

@@ -80,5 +80,6 @@
"Done": "Done",
"Caption Settings Dialog": "Caption Settings Dialog",
"Beginning of dialog window. Escape will cancel and close the window.": "Beginning of dialog window. Escape will cancel and close the window.",
"End of dialog window.": "End of dialog window."
"End of dialog window.": "End of dialog window.",
"{1} is loading.": "{1} is loading."

This comment has been minimized.

@gkatsev

gkatsev Jan 31, 2018

Member

can you regenerate grunt check-translations?

This comment has been minimized.

@misteroneill
@gkatsev

This comment has been minimized.

Member

gkatsev commented Jan 31, 2018

Also, this could definitely be considered a fix.

@misteroneill misteroneill changed the title from feat: Add alternate text in an ARIA alert to the loading spinner. to fix: Add alternate text in an ARIA alert to the loading spinner. Feb 1, 2018

@misteroneill misteroneill added the a11y label Feb 1, 2018

@misteroneill

This comment has been minimized.

Member

misteroneill commented Feb 2, 2018

This morning, we discussed the potential for role="alert" to be disruptive. One idea thrown around was to only add it when the player gains focus.

@BrandonOCasey

This comment has been minimized.

Contributor

BrandonOCasey commented Feb 8, 2018

I would think that we can add it if any part of the player is currently in focus, or the player gains focus

Remove role:alert to keep this a small change.
At some point, we would want to add the alert back in but only do so when focused on the player itself.
@gkatsev

LGTM. This is only the beginning.

@gkatsev gkatsev merged commit 50831e3 into videojs:master Feb 22, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@misteroneill misteroneill deleted the misteroneill:loading-text-alt branch Jun 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment