-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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 fieldsets and labels to text-track-settings #3054
Conversation
<option value="1">Opaque</option> | ||
<option value="0.8" selected>80% Opaque</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is adding another bg opacity necessary? Would it be better to change the value for semi-transparent
instead?
The FCC rules for this call for opaque
, semi-transparent
, and transparent
options.
@OwenEdwards does WCAG or a11y have anything to say about these values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gkatsev this is an FCC requirement (FCC 12-9), not a WCAG requirement. Specifically, it calls for:
... at least four settings, opaque (100% opacity), semi-transparent (at 75% or 25% opacity), and transparent (0% opacity)
for background
and for window
, and three for font
(hmm, actually transparent font is essentially what we need for description tracks! That gives me an idea!)
So yes, we need to add another semi-transparent
. Whether we need to change the values (to 0.75 and 0.25) is something to discuss, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote to change the current semi-transparent from 50% to 75% and not add another option.
Oh, @marguinbc, you dropped some functionality here I think (right, @gkatsev?). It wasn't clear, but the |
Hmm, @marguinbc @gkatsev I realize now that this logic around the |
@OwenEdwards well, there is the |
What my intent was is since we have a 'selected' for each field, these sort of act as the default. So, I modified setDefault to use the same values. I also think from an accessibility standpoint, it makes sense for the selected value to be the currently set one and for screen readers to be able to read, foreground color, white... Doing so I did not account for the case where the user changed the default, saved them, went in to make another change, and decided NOT to apply that change after all. Reset/default would wipe out their previous changes and Done would apply the most recent changes. Not sure how best to resolve that instance. |
Closing this one as #3281 supercedes it. Thanks for starting the ball rolling on this one @marguinbc! |
Multiple structural changes for accessibility
Remaining tasks:
Properly label opacity fields for colors combo boxes
Fix one test I had problems with in 'should restore default settings'