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

Remove unnecessary ARIA role on the Control Bar. Fixes #5134 #5154

Merged
merged 2 commits into from
May 11, 2018

Conversation

OwenEdwards
Copy link
Member

@OwenEdwards OwenEdwards commented May 8, 2018

Description

Remove unnecessary ARIA role on the Control Bar. Fixes #5134

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 (Firefox+NVDA, IE+JAWS)
  • Reviewed by Two Core Contributors

@OwenEdwards OwenEdwards requested a review from gkatsev May 8, 2018 19:45
@OwenEdwards OwenEdwards added the a11y This item might affect the accessibility of the player label May 8, 2018
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.

We'll want to port this to master later, right?

@@ -40,9 +40,6 @@ class ControlBar extends Component {
className: 'vjs-control-bar',
dir: 'ltr'
}, {
Copy link
Member

Choose a reason for hiding this comment

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

we can just get rid of this object entirely, if we aren't using it.

@gkatsev gkatsev added the 6.x label May 8, 2018
@gkatsev
Copy link
Member

gkatsev commented May 8, 2018

So you think that the control bar should just not have a role? I guess it makes sense since you never interact with the control bar itself.

@OwenEdwards
Copy link
Member Author

@gkatsev I did some testing, and there just doesn't seem to be any point in indicating the Control Bar as a separate object - it's all just part of the video player. Perhaps in the future, if there's content which isn't part of the control bar that needs to be clearly distinguishable from the control bar, then it would make sense, but right now you don't interact with the control bar and it doesn't convey any concept of grouping. For example, is it semantically important that the BPB isn't part of the control bar? I don't think it is.

To answer your other question, yes - this should also be ported to master; I wasn't sure if it was better to start there and back-post, or vice versa. Let me know if you want me to switch it.

@gkatsev
Copy link
Member

gkatsev commented May 9, 2018

Makes sense.

Either way is fine. We can easily port for master when merged.

@gkatsev gkatsev merged commit 9607712 into videojs:6.x May 11, 2018
gkatsev pushed a commit that referenced this pull request May 11, 2018
There doesn't seem to be any point in indicating the Control Bar as a separate object - it's all just part of the video player. Perhaps in the future, if there's content which isn't part of the control bar that needs to be clearly distinguishable from the control bar, then it would make sense, but right now you don't interact with the control bar and it doesn't convey any concept of grouping.

Fixes #5134
@OwenEdwards OwenEdwards deleted the fix/remove-aria-role-on-controlbar branch May 14, 2018 16:22
@OwenEdwards OwenEdwards restored the fix/remove-aria-role-on-controlbar branch May 14, 2018 16:31
@OwenEdwards OwenEdwards deleted the fix/remove-aria-role-on-controlbar branch May 14, 2018 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.x a11y This item might affect the accessibility of the player confirmed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants