Closed Bug 783605 Opened 8 years ago Closed 8 years ago
DOM Full screen should disable social sidebar
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
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.
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).
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.
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.
updated with feedback
Attachment #667723 - Flags: review?(jaws) → review+
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?
Pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=7c7658690388
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Attachment #667723 - Flags: approval-mozilla-aurora+
No QA verification since this has in-testsuite coverage.
You need to log in before you can comment on or make changes to this bug.