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 2 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 && data === 'not-fullscreen') {
Copy link
Member

Choose a reason for hiding this comment

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

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).

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 agree, we should explicitly check for false here.

this.isFullscreen(false);
}
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', 'not-fullscreen');
Copy link
Member

Choose a reason for hiding this comment

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

I think we said event data should be a hash. So { fullscreen: false }?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, { fullscreen: false } is good for me. I was debating what to do for it specifically but then just did the string to get the PR out.
The main question is whether we want to do it this way or is there a better way of doing it.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have any specific thoughts/concerns on it? I can't really think of a better way. It doesn't require the tech to know about the player, so that's good. I think we need to make sure this event doesn't bubble, and #2351 isn't finished yet. We might be seeing multiple fullscreen events at the player level right now.

Copy link
Member

Choose a reason for hiding this comment

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

We might want to name the data key isFullscreen, but I could got either way.

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to balance this, so that handleTechFullscreenChange is always setting player.isFullscreen, instead of just on exit?

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 am seeing two fullscreenchange events on the player currently on ios.
I'm not sure whether it's better to keep the isFS info on the player or whether it should go into techs. I guess since the player div is the one that goes into fullscreen, it kind of makes for it to live on the player and ios would just need to update the player of this. In which case, should we just make the in the handleTechFullscreenChange to be data && this.isFullscreen(data.fullscreen);?

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 just make the in the handleTechFullscreenChange to be data && this.isFullscreen(data.fullscreen);

Yeah, I think so.

});

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