1436334 - The fullscreen button is not fully functional on Bloomberg videos

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

[Affected versions]:
- 60.0a1 (2018-02-06) 
- 59.0b7 build1 (20180205211730)
- 58.0.2 build1 (20180206200532)

[Affected platforms]:
- Windows 7 x64
- Windows 10 x64
- macOS 10.13	
- Ubuntu 14.04 x64	

[Steps to reproduce]:
1. Launch Firefox. 
2. Access https://www.bloomberg.com/video and play a random video (or choose a random article that contains video content - e.g. https://www.bloomberg.com/news/articles/2018-02-06/stocks-in-asia-face-rally-as-u-s-equities-recover-markets-wrap and wait for the video to be rendered).
3. Hover the video controls and enter the fullscreen mode by clicking the corresponding button.
4. Exit the fullscreen mode by using the same button.

[Expected result]:
- The fullscreen button is available and fully functional in both modes: fullscreen and normal view.

[Actual result]:
- The fullscreen button is not functional when already in fullscreen mode.
- The user is only able to exit the fullscreen mode by using the ESC key. 

[Regression range]:
- This is not a recent regression, since it's reproducible all the way back to Firefox 49.0.

[Additional notes]:
- It seems that the inline videos from the main pages (e.g. https://www.bloomberg.com/europe) are not affected.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

I can reproduce it, but investigating a bit of the code, I failed to find anything interesting...

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

In the bplayer.js, I put a break point in the function for 'requestFullScreen', and one in the function for 'addFullScreenObserver', but the second break point is never reached, but the fullscreen observer is still correctly created. And I also put a break point in the function for 'onFullScreenChange' which is supposed to trigger when fullscreen change event is dispatched.

It is unclear to me whether addFullScreenObserver correctly add the event handler, and if it does, why it doesn't reach the handler.

(The object tree looks rather obscure to me, and I have no idea how to debug it at all...)

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Anyway, this is unlikely a layout issue. It is more likely a Fullscreen API issue which should belong to DOM. It would be great if someone can tell me what to do for the situation mentioned in comment 2.

Comment 4

[Anne (:annevk)]

Still breaks in current Nightly. bz, care to have a look or find someone?

Comment 5

[Boris Zbarsky [:bzbarsky]]

In bplayer.js we have this bit:

  {
    key: 'requestFullScreen',
    value: function () {
      void 0 === this.fullScreenObserverIndex && (this.fullScreenObserverIndex = t.addFullScreenObserver(this)),
        d.default.requestFullScreen(this.el_)
    }
  },

When I click the fullscreen button, we land in this code, this.fullScreenObserverIndex is undefined, so we call t.addFullScreenObserver(this).  That lands us in:

  {
    key: 'addFullScreenObserver',
    value: function (e) {
      if (d.default.isSupported()) return t.fullScreenObserversCount++, t.fullScreenObservers || (t.fullScreenObservers = [
      ], document.addEventListener(d.default.fullscreenchange,
      t.onFullScreenChange)), t.fullScreenObservers.push(e) - 1
    }
  },

and we test true for d.default.isSupported().  At this point, d.default.fullscreenchange is "fullscreenchange" and t.onFullScreenChange is a function, ok so far.... but I never see this function get called.  I guess that's what comment 2 said too.

I'll poke around a bit to see what might be going on there.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Well, our devtools are broken enough (e.g. see bug 1449825) that I can't trust anything they tell me.

I did verify that we fire the fullscreenchange event, and that there are handlers for it, and that these handlers touch document.fullscreen for the Bloomberg document and that we return "true" when they do that.

I spent some time poking around bplayer.js, but at first glance nothing there is obviously wrong... :(

It would be really awesome if someone managed to produce a slightly more reduced testcase here, or if we had contacts at Bloomberg who might know what's going on.  Mike, do we have any of the latter?

Comment 7

[Mike Taylor [:miketaylr]]

It's interesting that both https://www.bloomberg.com/live and https://www.bloomberg.com/news/articles/2018-02-06/stocks-in-asia-face-rally-as-u-s-equities-recover-markets-wrap have the same version of bplayer.js, but only the non-live URL reproduces the error.

And yeah... I can't get any breakpoints to do anything useful in devtools... :(

Dennis, can you try to take a look?
In the meantime, do we have contacts with Bloomberg Adam?

Comment 8

[Dennis Schubert [:denschub]]

Gnarv. This took me way longer than it should have, Bloomberg is doing a good job at obfuscating their JS in a way that makes debugging really hard. In addition, the fullscreen API is really picky about when it wants to grant access or not. If you ask for fullscreen after you hit a breakpoint, it figures it might no longer be a user interaction, and thus prevents fullscreen toggling altogether. Anyway, enough rambling.

They have this little piece of code that gets executed every time the user clicks on the fullscreen button:

> {
>   key: "toggleFullScreen",
>   value: function() {
>     this.playlistView.isFullScreen
>       ? this.playlistView.exitFullScreen()
>       : this.playlistView.requestFullScreen();
>   }
> }

And as it turns out, `this.playlistView.isFullScreen` is false all the time in Firefox, even if the player is in fullscreen mode. Figuring out where this property gets set is a fun activity, but ultimately, it's their `onFullScreenChange` handler. In there, they have this snippet (this code has been reformatted and some variables renamed to make it easier to understand, but one should be able to find the original source when looking for `key: "onFullScreenChange"` in a somewhat prettified bplayer.js):

> if (!(fullScreenChangeEvent.target !== fullscreenObserver.el_ && fullScreenChangeEvent.target)) {
>   fullscreenObserver.isFullScreen = n;
>   fullscreenObserver.trigger(v.default.FULL_SCREEN_CHANGE, {
>     fullscreen : n
>   });
> }

`fullscreenObserver.el_` is always a reference to the <div> containing the video player. No surprises here. `fullScreenChangeEvent.target` is the interesting part: In Firefox, it is a reference to the HTMLDocument, while in Chrome, it references the <div> element. So in Firefox, the if statement is never true, and thus we never set the proper fullscreen state.

Now, Chrome only dispatches a prefixed webkitfullscreenchange event, while we dispatch a mozfullscreenchange event in release versions and have the unprefixed versions only enabled in prerelease versions (see bug 1269276), so I am not surprised there are differences here.

A year ago, the Fullscreen API was changed to have the fullscreenchange event being dispatched at the element first, see https://github.com/whatwg/fullscreen/issues/89, so we should do that as well. Turns out we already have a bug for that, bug 1375319. It is marked as a blocker for enabling the unprefixed fullscreen API, but we still might want to have an early look at that if it turns out to break sites. However, I am not sure if sites actually depend on us dispatching on the HTMLDocument first, so take that suggestion with a grain of salt.

Now... I assume they do this check to handle cases where there might be more than one video player on a site and they don't want to end up with more than one player thinking they're in full screen. Sadly, there is no easy workaround to suggest here, besides not checking the event target at all. As far as I can tell, they would be fine if they simply set the isFullScreen attribute regardless of what player actually is in full screen. The only logic that actually depends on the fullscreen state is the button itself.

To find a solution here, it is probably best to do some outreach and talk to the developers about possible implications of this change, as it is easy for me to miss something when working with this kind of obfuscated source.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Sounds like we should fix bug 1375319 in some higher priority.

Comment 10

[Adam Stevenson [:adamopenweb]]

Mike, yes we do. Will send an email and copy in you and Dennis.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Dennis, thank you!  That is some awesome detective work.

Comment 12

[Adam Stevenson [:adamopenweb]]

First contact has moved on. Have another now and reaching out again.

Comment 13

[Adam Stevenson [:adamopenweb]]

Our contact passed this along to the video team, to get addressed.

Comment 14

[Adam Stevenson [:adamopenweb]]

This issue appears to be fixed. I'm able to exit full screen by clicking the button. Tested in Firefox 67 nightly on MacOS 10.13.