Native controls updates #2499
Native controls updates #2499
Conversation
Fixes videojs#2365. Also, wrap the trigger in a setTimeout. This is because otherwise, the player will never be able to hear this event since it happens in the constructor of the tech and the player adds listeners after the tech has been created.
| @@ -501,6 +501,7 @@ class Player extends Component { | |||
|
|
|||
| // Grab tech-specific options from player options and add source and parent element to use. | |||
| var techOptions = assign({ | |||
| 'nativeControlsForTouch': this.options_.nativeControlsForTouch, | |||
There was a problem hiding this comment.
pass the option to the tech
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 1af6089 (Please note that this is a fully automated comment.) |
|
lgtm!
|
|
Could you add a description for this issue for people who come back to it later? |
|
Yep, I'll definitely update the description. Trying to figure out the best way to force the native controls on by default only on the native android browsers (because they don't work right with our controls). |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 28509f4 (Please note that this is a fully automated comment.) |
| .video-js.vjs-error .vjs-control-bar { | ||
| .video-js.video-js.video-js.vjs-controls-disabled .vjs-control-bar, | ||
| .video-js.video-js.video-js.vjs-using-native-controls .vjs-control-bar, | ||
| .video-js.video-js.video-js.vjs-error .vjs-control-bar { |
There was a problem hiding this comment.
this is necessarily because otherwise, the .vjs-no-flex selector below ends up being more specific than this and sets the control bar's display to table. This avoids using !important or really strong css selectors.
There was a problem hiding this comment.
I get the limitation here, but I don't think we should say this is an ok thing to do in the stylesheet. It's gonna result in uglier and uglier selectors, and I think we can do better.
- I'd prefer
importanthere over a triple class name. It is after all important that the controls be hidden when the controls are disabled. - We could try lowering some of the selector strengths and using strategic ordering over increasing class names.
- We could hide the controls in additional ways to
display, so you have to do more than just display:table to override it.
There was a problem hiding this comment.
Another option to fix this specific issue would be to add a .vjs-flex class. Since that's the default behavior it could basically be a noop with no specific styles associated with it (because they're the default), but it would solve specificity problems in situations like this.
|
@heff updated. |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 693ae2e (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: d9c0263 (Please note that this is a fully automated comment.) |
Previously, native controls were enabled by firing an event from the tech to the player. However, the player only bound events on the tech after the tech may have fired the event. Instead, the player will ask the tech whether controls are enabled or not. If the controls are enabled, it means we should be using native controls.
Also in this PR: when on iphones or native android browser, we should be using native controls.