The default bug view has changed. See this FAQ.

DOM Full screen should disable social sidebar

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: johnath, Assigned: mixedpuppy)

Tracking

17 Branch
Firefox 18
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
I think we need to fix this for v1.
OS: Mac OS X → All
Target Milestone: --- → Firefox 17

Updated

5 years ago
Duplicate of this bug: 785165
(Assignee)

Updated

5 years ago
Whiteboard: [Fx17]
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.
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
tracking-firefox17: --- → ?
Hardware: x86 → All
Whiteboard: [Fx17]
Target Milestone: Firefox 17 → ---
Version: unspecified → 17 Branch

Updated

5 years ago
tracking-firefox17: ? → +
(Assignee)

Comment 5

5 years ago
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.
Attachment #667213 - Flags: feedback?(jaws)
(Assignee)

Comment 6

5 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

5 years ago
Created attachment 667723 [details] [diff] [review]
alternate patch

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
(Assignee)

Comment 10

5 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?
Attachment #664297 - Attachment is obsolete: true
Attachment #664297 - Flags: feedback?(gavin.sharp)
Pushed to try server:
https://tbpl.mozilla.org/?tree=Try&rev=7c7658690388
https://hg.mozilla.org/integration/mozilla-inbound/rev/a05bad13ec39
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/a05bad13ec39
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Attachment #667723 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/49457c91c7c7
status-firefox17: --- → fixed
Whiteboard: [qa-]
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.