Closed Bug 783605 Opened 8 years ago Closed 8 years ago

DOM Full screen should disable social sidebar

Categories

(Firefox Graveyard :: SocialAPI, defect)

17 Branch
defect
Not set

Tracking

(firefox17+ fixed)

RESOLVED FIXED
Firefox 18
Tracking Status
firefox17 + fixed

People

(Reporter: johnath, Assigned: mixedpuppy)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

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.
I think we need to fix this for v1.
OS: Mac OS X → All
Target Milestone: --- → Firefox 17
Duplicate of this bug: 785165
Whiteboard: [Fx17]
Attached patch Patch v.0 (obsolete) — Splinter Review
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.
Attachment #664297 - Flags: feedback?(gavin.sharp)
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).
Assignee: nobody → dolske
Status: NEW → ASSIGNED
Hardware: x86 → All
Whiteboard: [Fx17]
Target Milestone: Firefox 17 → ---
Version: unspecified → 17 Branch
Attached patch alternate patch (obsolete) — Splinter Review
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.
Attachment #667213 - Flags: feedback?(jaws)
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.
Attached patch alternate patchSplinter Review
updated with feedback
Attachment #667213 - Attachment is obsolete: true
Attachment #667213 - Flags: feedback?(jaws)
Attachment #667723 - Flags: review?(jaws)
Attachment #667723 - Flags: review?(jaws) → review+
Assignee: dolske → mixedpuppy
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).
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.
Summary: Full screen should disable social sidebar → DOM Full screen should disable social sidebar
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?
Attachment #664297 - Attachment is obsolete: true
Attachment #664297 - Flags: feedback?(gavin.sharp)
https://hg.mozilla.org/mozilla-central/rev/a05bad13ec39
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Attachment #667723 - Flags: approval-mozilla-aurora+
Whiteboard: [qa-]
No QA verification since this has in-testsuite coverage.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.