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(subs-caps-button): captions items should hide icon from SR #4158

Merged
merged 3 commits into from
Mar 7, 2017

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Mar 2, 2017

We want to visually show that captions items in the subs-caps menu are captions vs subtitles using the captions icon. However, we want to hide this from screen readers. Moreover, we can't display things visually that isn't also conveyed to screen readers.
To do so, I created a Subs Caps Menu Item which adds an icon placeholder for the icon and a localized text of captions for screen readers.

@OwenEdwards can you review this please? Thanks!

@gkatsev gkatsev force-pushed the subs-caps-sr-icon branch from 01ece2e to c59689d Compare March 2, 2017 22:56
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.

The SubCaps menu doesn't render properly with IE11 - I don't know if this was true before this PR, but without it rendering properly it's difficult to review it with JAWS.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 3, 2017

Hm... I'll take a look.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 3, 2017

@OwenEdwards are you using npm start? I've noticed that sometimes you have to kick the styles into gear with it by saving a scss file. It seems to be working for me after I did that.

@OwenEdwards
Copy link
Member

I'm seeing this consistently with IE11 when I build this locally and then load combined-tracks.html.example from my server with Ctrl-F5:

2017-03-03_16-19-08

But testing with JAWS+IE, NVDA+Firefox and Safari+VoiceOver on macOS, this change works as expected - so this is just a visual issue with IE, otherwise it's good.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 5, 2017

Awesome, thanks @OwenEdwards. I'll have to look at it some more in IE11. Maybe I'll try the combined-tracks example page directly as well, might be configured differently than my sandbox page at this point.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 6, 2017

Oh, was just able to reproduce what you're seeing @OwenEdwards. I'll investigate.

@@ -10,7 +10,7 @@
width: 4em;
@include flex(none);

& .vjs-icon-placeholder:before {
& > .vjs-icon-placeholder:before {
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes it so that only direct descendants of vjs-control gets these defaults for icon-placeholder.
In IE11, with the subs-caps menu, the icon-placeholder gets this font-size and the font-size from below applied in that order, so, the menu ends up overflowing and disappearing or looking weird.

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.

LGTM.

@gkatsev gkatsev merged commit 2ee133f into master Mar 7, 2017
@gkatsev gkatsev deleted the subs-caps-sr-icon branch March 7, 2017 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants