Last Comment Bug 783605 - DOM Full screen should disable social sidebar
: DOM Full screen should disable social sidebar
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
:
Mentors:
: 785165 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-17 10:38 PDT by Johnathan Nightingale [:johnath]
Modified: 2013-12-27 14:33 PST (History)
6 users (show)
jaws: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch v.0 (784 bytes, patch)
2012-09-24 17:37 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
alternate patch (4.78 KB, patch)
2012-10-02 15:42 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
alternate patch (4.67 KB, patch)
2012-10-03 16:13 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
jaws: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Review

Description Johnathan Nightingale [:johnath] 2012-08-17 10:38:17 PDT
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] 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) [on leave 5/16-7/16] 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) [on leave 5/16-7/16] 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) [on leave 5/16-7/16] 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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 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) [on leave 5/16-7/16] 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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-04 17:11:11 PDT
Pushed to try server:
https://tbpl.mozilla.org/?tree=Try&rev=7c7658690388
Comment 12 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 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.

Note You need to log in before you can comment on or make changes to this bug.