Fix time-display.js to restore hidden label text for screen readers. Fixes #5135 #5157
Conversation
…ixes videojs#5135 Also, fix a typo in translations-needed.md, and add a space in the hidden label for live-display, so that it doesn't run together with the visible "LIVE" indication.
OwenEdwards
added
a11y
| @@ -5,7 +5,7 @@ | |||
| "Pause": "Pause", | |||
| "Replay": "Replay", | |||
| "Current Time": "Current Time", | |||
| "Duration Time": "Duration Time", | |||
There was a problem hiding this comment.
So, looking at the translations, most of them just translated 'Duration', so, I wonder whether we should just leave this as is? Or add Duration as a second option and then where we use it in the code we first try Duration localization and then Duration Time localization.
There was a problem hiding this comment.
"Duration Time" sounds like something poorly translated into English ;-)
There was a problem hiding this comment.
yup, makes sense, just thinking about how to reduce need for re-working all the translations.
There was a problem hiding this comment.
Could we just change it to "Duration" in this and all the other language files, and then note some of the translations may be incorrect? It seems like translations get fixed as much as they get added to?
There was a problem hiding this comment.
yeah, maybe that's just the best choice.
src/js/control-bar/live-display.js
Outdated
| @@ -42,7 +42,7 @@ class LiveDisplay extends Component { | |||
|
|
|||
| this.contentEl_ = Dom.createEl('div', { | |||
| className: 'vjs-live-display', | |||
| innerHTML: `<span class="vjs-control-text">${this.localize('Stream Type')}</span>${this.localize('LIVE')}` | |||
| innerHTML: `<span class="vjs-control-text">${this.localize('Stream Type')} </span>${this.localize('LIVE')}` | |||
There was a problem hiding this comment.
we should use the unicode representation (\u00a0) for xhtml safety #4825
| * | ||
| * @type {string} | ||
| * @private | ||
| */ | ||
| CurrentTimeDisplay.prototype.controlText_ = 'Current Time'; |
There was a problem hiding this comment.
I'm thinking we should leave the controlText_ property in case anyone is relying on it but internally ignore it and only use labelText_
There was a problem hiding this comment.
It's confusing, because these components don't extend Button or ClickableComponent which do have that concept of controlText_, and because these aren't controls, they're information displays.
It looks like controlText_ was added when the common time-display.js was created. Which version was that? If it wasn't 6.0, then it doesn't seem like anyone ought to be "relying on it", since it's private.
| @@ -47,6 +47,7 @@ class RemainingTimeDisplay extends TimeDisplay { | |||
| * @private | |||
| */ | |||
| formatTime_(time) { | |||
| // TODO: The "-" should be decorative, and not announced by a screen reader | |||
There was a problem hiding this comment.
this is because the screen reader would announce something like "remaining time minus 5:00" as it is currently?
There was a problem hiding this comment.
Exactly. The "-" is a visual convention, but the actual amount of time remaining is not negative. It's a small detail, which is why I didn't fix it now, but it ought to be fixed in the future.
There was a problem hiding this comment.
we should probably open an issue so we don't forget
| }); | ||
|
|
||
| this.contentEl_ = Dom.createEl('div', { | ||
| this.contentEl_ = Dom.createEl('span', { |
There was a problem hiding this comment.
this is probably fine to make, but I'd like to take a look at the output first.
There was a problem hiding this comment.
If we use a <div>, JAWS treats the content label and the time as two separate things, so you need to press the arrow key to move between them in Virtual Cursor mode. With a <span>, JAWS treats them as a single thing (hence the need to be careful about adding a space in certain places with spans).
There was a problem hiding this comment.
yeah, I don't see any issues with this change once trying it out.
…update translations-needed.md Note that some translations of "Duration" may need updating, where the translation of "Duration Time" is not equivalent.
|
@gkatsev is this what you were thinking as far as leaving the |
|
yup, thanks @OwenEdwards! |
Restore hidden label text for screen readers that describes what the button control does. Renames the Duration Time language item to Duration. Deprecate controlText_ property. Fix a typo in translations-needed.md. Add a space in the hidden label for live-display, so that it doesn't run together with the visible "LIVE" indication. Fixes #5135
Description
Fix time-display.js to restore hidden label text for screen readers. Fixes #5135
Specific Changes proposed
Also, fix a typo in translations-needed.md, and add a space in the hidden label for live-display, so that it doesn't run together with the visible "LIVE" indication.
Requirements Checklist