Comment 1 Asa Dotzler [:asa] 2012-09-07 13:23:14 PDT

I think we need to fix this for v1.

Comment 2 Asa Dotzler [:asa] 2012-09-07 13:23:34 PDT

*** Bug 785165 has been marked as a duplicate of this bug. ***

Comment 3 Justin Dolske [:Dolske] 2012-09-24 17:37:42 PDT

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: gavin@gavinsharp.com] 2012-09-24 17:50:15 PDT

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) 2012-10-02 15:42:07 PDT

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) 2012-10-02 15:43:04 PDT

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) 2012-10-03 16:13:24 PDT

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

updated with feedback

Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-03 16:41:55 PDT

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) 2012-10-03 16:59:10 PDT

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) 2012-10-03 17:15:02 PDT

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) 2012-10-04 17:11:11 PDT

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

Comment 12 Jared Wein [:jaws] (please needinfo? me) 2012-10-04 20:41:30 PDT

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

Comment 13 Ed Morley [:emorley] 2012-10-05 03:56:50 PDT

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

Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-05 07:37:25 PDT

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

Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-17 16:45:01 PDT

No QA verification since this has in-testsuite coverage.