Fullscreen updates for iOS

[gkatsev]

#1504 but against stable.

[heff]

In #1504 when I was asking about use cases I was actually meaning the entire vjs.IS_IOS block. Is there a usecase behind checking isFullscreen in enterFullScreen mode?

[gkatsev]

I think it's good to do it just in case. It's theoretically possible that the fullscreen gets cancelled for whatever reason. Also, this way, it mimics the code that uses the proper fullscreenchange event.

[heff]

It's theoretically possible that the fullscreen gets cancelled for whatever reason.

But if we didn't change anything when it entered fullscreen does it matter? The thing that we've relied on up to this point is that when enterFullScreen is used, Video.js essentially doesn't know about it. The controls stay the same and the vjs-fullscreen class isn't added, which is fine because they wouldn't do anything anyway since you can't see the player. Then when you exitFullScreen everything is exactly how it should be because we never changed anything (unless I'm missing something).

Consistency may be a good enough reason to add this, though you might be able to make a case that they're different enough modes that they don't need to work the same, since in one mode we have no control over the UI. What were you trying to do when you discovered this issue?

[gkatsev]

The problem was that on iOS, even after exiting fullscreen, isFullscreen method was returning true.
I think the way it's currently written is good since the endfullscreen event won't get registered unless we actually succeed in entering fullscreen. Though, now that I think about it, it should probably be changed to be one instead of on or deregistered within the handler.

[heff]

Ah, I get it now. So this could also potentially be fixed by moving the isFullScreen(true) into the specific fullscreen-type blocks, excluding it from the enterFullScreen block so that isFullScreen is never true in that case. However if we're already reporting true in that case we probably shouldn't change that.

In that case I might go back on my previous comment of removing the fullscreenchange events. If we're going to mimic requestfullscreen with enterfullscreen we should probably do it fully.

Could this run into the same issue that #1476 is trying to solve for? I'm not sure that one's quite valid yet.

[heff]

This line could be simplified to

this.one('webkitbeginfullscreen', function(e) {

Same with line 217

[gkatsev]

Right.

[gkatsev]

I think changing from on to one might be a better way of making sure that the handlers are removed rather than what is in #1476. Though, they probably would need to be cleaned up on dispose anyway, i.e., if the player is disposed when the player is in fullscreen.

setting isFullscreen to true is working just fine. The problem was that when you exited fullscreen (on iOS, at least), isFullscreen was still reporting true.

Maybe the real solution is to change isFullscreen to check whatever is needed to determine whether we are in fullscreen as opposed to setting it from the requestFullscreen handlers?

[heff]

Do we need to check video['webkitDisplayingFullscreen'] or can we just assume that it's true since it's in webkitbeginfullscreen?

[gkatsev]

We may be able to assume it's true. This is again to mimic the other piece of code.

[heff]

Yeah, I think you're right about the isFullScreen change. Want to take a quick pass at it? If it's going to be complicated or a significant change we can pull this in first then make that change.

[gkatsev]

I'll take a look.

[gkatsev]

I think changing the isFullscreen function might need to be moved to another issue. The fullscreen code relies on the current implementation too much.

a POC is below:

function isFullscreen() {
  var isFS,
      fsApi = vjs.browser.fullscreenAPI,
      thisEl = this.el(),
      techEl = this.tech.el(),
      fsEl;

  // if we are spec compliant and there is no fullscreen element, return false
  // otherwise, return true if the fullscreen element is either our player div or the tech element
  if (fsApi) {
    fsEl = document[fsApi.fullscreenElement]
    return fsEl || fsEl === thisEl || fsEl === techEl;
  }

  // if we are not spec compliant, then either the player el is in fullscreen
  // or the tech el is in fullscreen.
  isFS = isFS || thisEl['webkitDisplayingFullscreen'];
  isFS = isFS || techEl['webkitDisplayingFullscreen'];;
  return isFS;
}

[heff]

Would it be possible to use feature detection here instead of UA? Maybe check for this.el_['onwebkitbeginfullscreen']?

[gkatsev]

I think we can test by checking: 'webkitDisplayingFullscreen' in video. onwebkitbeginfullscreen doesn't exist on the element.