Closed
Bug 783605
Opened 13 years ago
Closed 13 years ago
DOM Full screen should disable social sidebar
Categories
(Firefox Graveyard :: SocialAPI, defect)
Tracking
(firefox17+ fixed)
RESOLVED
FIXED
Firefox 18
People
(Reporter: johnath, Assigned: mixedpuppy)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
4.67 KB,
patch
|
jaws
:
review+
Gavin
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
I think we need to fix this for v1.
OS: Mac OS X → All
Target Milestone: --- → Firefox 17
Assignee | ||
Updated•13 years ago
|
Whiteboard: [Fx17]
Comment 3•13 years ago
|
||
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)
Comment 4•13 years ago
|
||
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).
Updated•13 years ago
|
Assignee: nobody → dolske
Status: NEW → ASSIGNED
tracking-firefox17:
--- → ?
Hardware: x86 → All
Whiteboard: [Fx17]
Target Milestone: Firefox 17 → ---
Version: unspecified → 17 Branch
Updated•13 years ago
|
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
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.
Assignee | ||
Comment 7•13 years ago
|
||
updated with feedback
Attachment #667213 -
Attachment is obsolete: true
Attachment #667213 -
Flags: feedback?(jaws)
Attachment #667723 -
Flags: review?(jaws)
Updated•13 years ago
|
Attachment #667723 -
Flags: review?(jaws) → review+
Updated•13 years ago
|
Assignee: dolske → mixedpuppy
Comment 8•13 years ago
|
||
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•13 years ago
|
||
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.
Updated•13 years ago
|
Summary: Full screen should disable social sidebar → DOM Full screen should disable social sidebar
Assignee | ||
Comment 10•13 years ago
|
||
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?
Updated•13 years ago
|
Attachment #664297 -
Attachment is obsolete: true
Attachment #664297 -
Flags: feedback?(gavin.sharp)
Comment 11•13 years ago
|
||
Pushed to try server:
https://tbpl.mozilla.org/?tree=Try&rev=7c7658690388
Comment 12•13 years ago
|
||
Flags: in-testsuite+
Comment 13•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Updated•13 years ago
|
Attachment #667723 -
Flags: approval-mozilla-aurora+
Comment 14•13 years ago
|
||
status-firefox17:
--- → fixed
Comment 15•13 years ago
|
||
No QA verification since this has in-testsuite coverage.
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•