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: aria-labelledby attribute has an extra space #4708

Merged
merged 1 commit into from
Oct 31, 2017

Conversation

knilob
Copy link
Contributor

@knilob knilob commented Oct 31, 2017

Description

The aria-labelledby attribute on the fontPercent, edgeStyle, and
fontFamily select options includes an extra space since there is
no ledgendId variable being set on the createElFont_() method.

Fixes #4688

Specific Changes proposed

This fix adds a check to see if the legendId value is set or not inside
the createElSelect_() method. This should keep the extra space
from appearing on the select tags created by the createElFont_()
method.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

The aria-labelledby attribute on the fontPercent, edgeStyle, and
fontFamily select options inlcudes an extra space since there is
no ledgendId variable being set on the createElFont_() method. This
fix adds a check to see if the legendId value is set or not inside
the createElSelect_() method. This should keep the extra space
from appearing on the select tags created by the createElFont_()
method.

Fixes videojs#4688
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

@gkatsev gkatsev merged commit 855adf3 into videojs:master Oct 31, 2017
@gkatsev gkatsev added the a11y This item might affect the accessibility of the player label Oct 31, 2017
@OwenEdwards
Copy link
Member

@knilob did you see any symptom of this extra space? Was a screen reader (JAWS, NVDA, VoiceOver, TalkBack) having issues with it? I'm just wondering because, like CSS classes, aria-labelledby takes a space-separated list of values (in this case IDs), so it really shouldn't have any impact. If it does, it'd be worth knowing exactly what and where. Thanks!

@gkatsev
Copy link
Member

gkatsev commented Oct 31, 2017

@OwenEdwards see #4688, seems like it's being flagged by the WAVE tool from webaim

@gkatsev
Copy link
Member

gkatsev commented Oct 31, 2017

Also, looks like we have a bug in the generation of the aria-labelledby for the font families since the values themselves have spaces when we create the idea for it, it ends up messing up. For example, we end up with an id captions-font-family-vid1_component_423-Proportional Sans-Serif instead of something like captions-font-family-vid1_component_423-Proportional-Sans-Serif and then when we reference it it's wrong.

@OwenEdwards
Copy link
Member

@gkatsev seems like a component ought to have a general-purpose toID() method (or maybe it should be part of DOM) that only allows valid ID labels to be created; anything that's not a-z, A-Z, 0-9, - or _ gets converted (technically a period is allowed too, but it gets tricky for CSS selectors).

I'm not convinced that having a space at the start or end of aria-labelledby (or aria-describedby) is invalid, but if WAVE is flagging it, it seems like any time an aria-labelledby is created it could just be trim()'ed.

@gkatsev
Copy link
Member

gkatsev commented Oct 31, 2017

Yeah, I don't think this causes issues and making the code remove it isn't a big burden. trim() is probably better and now that I noticed we need a slightly better refactor, we probably should switch to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y This item might affect the accessibility of the player
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants