pass non-fs state info up to the player from tech, split fullscreen and fullwindow styles #2357
Conversation
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 51d4e38 (Please note that this is a fully automated comment.) |
gkatsev
changed the title
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 77b3195 (Please note that this is a fully automated comment.) |
|
@pam retry |
| @@ -86,7 +86,7 @@ body.vjs-full-window { | |||
| /* Fix for IE6 full-window. http://www.cssplay.co.uk/layouts/fixed.html */ | |||
| overflow-y: auto; | |||
| } | |||
| .video-js.vjs-fullscreen { | |||
| .vjs-full-window .video-js.vjs-fullscreen { | |||
There was a problem hiding this comment.
Do the top/bottom/left/right need to be here or can they stay in the non-full-window vjs-fullscreen?
There was a problem hiding this comment.
I guess just moving the position:fixed will be less invasive; though, it seems like the top/bottom/etc are for the full-window and not really for fullscreen, so, it's more correct to keep them here.
However, I tried this out on ios, android, firefox, chrome, safari, IE11, and IE8 and they all work fine.
There was a problem hiding this comment.
Yeah, I guess it's position relative otherwise, so that's ok with me.
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 77b3195 (Please note that this is a fully automated comment.) |
| @@ -975,7 +975,10 @@ class Player extends Component { | |||
| * | |||
| * @method handleTechFullscreenChange | |||
| */ | |||
| handleTechFullscreenChange() { | |||
| handleTechFullscreenChange(event, data) { | |||
| if (data && data === 'not-fullscreen') { | |||
There was a problem hiding this comment.
Given the conversation below, just a reminder that this should become data && data.fullscreen === false (assume we want to check for an explicit false here and not just undefined).
There was a problem hiding this comment.
I agree, we should explicitly check for false here.
|
Updated |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 01b16b1 (Please note that this is a fully automated comment.) |
|
Nice. Looks good to me. |
| @@ -321,7 +321,7 @@ class Html5 extends Tech { | |||
| if ('webkitDisplayingFullscreen' in video) { | |||
| this.one('webkitbeginfullscreen', function() { | |||
| this.one('webkitendfullscreen', function() { | |||
| this.trigger('fullscreenchange'); | |||
| this.trigger('fullscreenchange', { isFullscreen: false }); | |||
| }); | |||
|
|
|||
| this.trigger('fullscreenchange'); | |||
There was a problem hiding this comment.
Should we set isFullscreen: true here?
There was a problem hiding this comment.
I can add it for symmetry, but seems to work fine.
There was a problem hiding this comment.
Thanks for adding it. I don't really understand why it was working otherwise, but feels better to be consistent.
There was a problem hiding this comment.
Probably because the player was still getting the fullscreenchange and up until this point everything is similar to the normal, expected, spec behavior. Just that exiting the fullscreen doesn't get set correctly.
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 0b8cb00 (Please note that this is a fully automated comment.) |
|
@pam retry |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 0b8cb00 (Please note that this is a fully automated comment.) |
Fixes #2350
Fixes #2346