-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
This adds touch and move listeners to the parent of volumeBar. #2638
Conversation
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: e29c47cb485c87560448212268526dc2636f943e (Please note that this is a fully automated comment.) |
@@ -22,6 +22,7 @@ class VolumeBar extends Slider { | |||
super(player, options); | |||
this.on(player, 'volumechange', this.updateARIAAttributes); | |||
player.ready(Fn.bind(this, this.updateARIAAttributes)); | |||
player.ready(() => this.on(this.el().parentNode, ['mousedown', 'touchdown'], this.handleMouseDown)); |
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.
Can we assume that this is safe for anything that parentNode could be, assuming that volume bar could be used independently of the volume menu?
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.
Not sure. I could also move that into the volume menu button instead. And it's not quite safe as it is because some tests are failing because of this.
Either way, this is not a blocker for 5.0.
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 sounds like we should move it into the parent component.
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.
Ok, I'll move it to the volume menu button.
This is useful because it allows us to more easily be able to interact with the volume bar. Instead of just being able to click or touch on the 4 pixel high bar, we can click or touch on the whole height of the container where the bar is located in.
e29c47c
to
c3def78
Compare
LGTM, though we'll have to be careful that we never allow volume menu buttons to be created without an associated volume bar. |
It gets created inside of |
I guess I should wrap them in |
we show the volumeBar on focus for keyboard accessibility when using the mouse, you don't want it to stay up, so, we blur it manually
// we show the volumeBar on focus for keyboard accessibility | ||
// when using the mouse, you don't want it to stay up, so, we blur it manually | ||
this.on('mouseout', function() { | ||
this.el().blur(); |
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.
mouseout could occur if they're using keyboard navigation but they also run their mouse over the volume bar.
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 think that may be ok. Or at least, something that we would need to live with.
Unless we want to (if we are even able to) know whether we entered into this via a keyboard or mouse and then only blur on mouseout if we entered with the mouse.
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 think I'm going to remove this and open a new issue for this to get a proper fix.
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.
Opened a new issue: #2918
LGTM |
This is useful because it allows us to more easily be able to interact with the volume bar. Instead of just being able to click or touch on the 4 pixel high bar, we can click or touch on the whole height of the container where the bar is located in.