783605 - DOM Full screen should disable social sidebar

Status: RESOLVED FIXED

Description

[Johnathan Nightingale [:johnath]]

I think UI-initiated full screen browsing can probably leave the social sidebar in place, but DOM-initiated fullscreen (e.g. when playing a bananabread demo) should almost certainly hide it.

Comment 1

[Asa Dotzler [:asa]]

I think we need to fix this for v1.

Comment 3

[Justin Dolske [:Dolske]]

Created attachment 664297 [details] [diff] [review]
Patch v.0

This is the easy fix, but has a couple of concerns:

* Doesn't differentiate between DOM-fullscreen and browser-fullscreen (ie, F11)... Depending on how you're using F11 mode, you may or may not want the sidebar. We do keep the bookmarks sidebar around, fwiw.

* Should I worry about the display:none tearing down bindings and having something unexpectedly fail? I did a quick test with width/max-width set to zero, but that wasn't working.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Hmm, I wonder if this might mess up provider expectations (for the moment, the sidebar is only loaded when it is visible). A simpler solution might be to have the browser-fullscreen code call SocialSidebar.updateSidebar() and add a check there for full screen mode (both types? that's confusing).

Comment 5

[Shane Caraveo (:mixedpuppy)]

Created attachment 667213 [details] [diff] [review]
alternate patch

This hides the sidebar, without unloading the provider, when in dom fullscreen mode.  The sidebar remains visible (if it was) when in browser fullscreen mode.

Comment 6

[Shane Caraveo (:mixedpuppy)]

I also renamed SocialSidebar.enabled since a) it threw me off reading the code, and b) that was about whether the sidebar was toggled open or not.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Created attachment 667723 [details] [diff] [review]
alternate patch

updated with feedback

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 667723 [details] [diff] [review]
alternate patch

Is mozfullscreenchange the right event name? browser.js seems to use MozEnteredDomFullscreen. And should you check window.fullScreen in addition to mozFullScreen? As I understand it there are two separate "full screen mode"s (though I'm not quite clear on the difference).

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Gavin, mozfullscreenchange is fired when the dom fullscreen mode is entered/adjusted/exited. The other fullscreen mode is F11 full-screen, which IMO we should leave the sidebar visibility unchanged.

MozEnteredDomFullscreen implies that it only fires for entering fullscreen, and is only useful if you are displaying a fullscreen warning for security reasons.

Comment 10

[Shane Caraveo (:mixedpuppy)]

As for the flags: window.mozFullScreen, window.mozFullScreenElement and/or window.fullScreen.  MDN Documentation is not terribly clear:

mozFullScreen: whether document is in full screen mode
fullScreen: whether window is in full screen mode
mozFullScreenElement: element being presented in full screen mode

I *think* mozFullScreen is a flag for DOM full screen, whereas fullScreen is browser full screen, in any case using mozFullScreen is getting the correct results.

clear as mud?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to try server:
https://tbpl.mozilla.org/?tree=Try&rev=7c7658690388

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a05bad13ec39

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a05bad13ec39

Comment 14

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/49457c91c7c7

Comment 15

[Anthony Hughes (:ashughes) [GFX][QA][Mentor]]

No QA verification since this has in-testsuite coverage.