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.

Sign up for GitHub

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

Jump to bottom

pass non-fs state info up to the player from tech, split fullscreen and fullwindow styles #2357

Closed
wants to merge 4 commits into from
Closed

pass non-fs state info up to the player from tech, split fullscreen and fullwindow styles #2357

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51d4e38
pass non-fs state info up to the player from tech
gkatsev Jul 15, 2015
77b3195
split out full-window and fullscreen styles.
gkatsev Jul 15, 2015
01b16b1
Switch to object with prop isFullscreen. Change isFullscreen always i…
gkatsev Jul 15, 2015
0b8cb00
Also signal entering fs
gkatsev Jul 16, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/css/components/_layout.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,16 @@ 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 {
Copy link
Member

Choose a reason for hiding this comment

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

Do the top/bottom/left/right need to be here or can they stay in the non-full-window vjs-fullscreen?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess it's position relative otherwise, so that's ok with me.

position: fixed;
overflow: hidden;
z-index: 1000;
left: 0;
top: 0;
bottom: 0;
right: 0;
}
.video-js.vjs-fullscreen {
width: 100% !important;
height: 100% !important;
/* Undo any aspect ratio padding for fluid layouts */
Expand Down
5 changes: 4 additions & 1 deletion src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,7 +975,10 @@ class Player extends Component {
*
* @method handleTechFullscreenChange
*/
handleTechFullscreenChange() {
handleTechFullscreenChange(event, data) {
if (data) {
this.isFullscreen(data.isFullscreen);
}
this.trigger('fullscreenchange');
}

Expand Down
2 changes: 1 addition & 1 deletion src/js/tech/html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

Should we set isFullscreen: true here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add it for symmetry, but seems to work fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding it. I don't really understand why it was working otherwise, but feels better to be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Expand Down