Fullscreen API should restore sidebar if it was open before entering fullscreen

RESOLVED FIXED in Firefox 36

Status

()

Firefox
Theme
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Steve Scott (pxbugz), Assigned: eoger, Mentored)

Tracking

({dev-doc-needed})

Trunk
Firefox 36
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 affected, firefox35 affected)

Details

(Whiteboard: [testday-20120203][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120102 Firefox/12.0a1
Build ID: 20120102031055

Steps to reproduce:

Loaded current nightly with clean profile
Turned on HTML5 on Youtube
Opened Bookmarks Sidebar
Pressed fullscreen button on HTML5 video (also works with testcase from bug 701992 )
pressed fullscreen button to exit fullscreen (also occurs pressing ESC)


Actual results:

Bookmarks sidebar remained closed


Expected results:

Bookmarks sidebar should have been restored to the open state, as before entering fullscreen
Component: General → Untriaged
QA Contact: general → untriaged
Component: Untriaged → General
QA Contact: untriaged → general
(Reporter)

Updated

6 years ago
Blocks: 545812
I can reproduce this on ubuntu with beta and nightly

Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (X11; Linux x86_64; rv:13.0a1) Gecko/20120203 Firefox/13.0a1
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [testday-20120203]
Duplicate of this bug: 727928

Updated

6 years ago
Blocks: 701992

Updated

6 years ago
Duplicate of this bug: 775595

Comment 4

6 years ago
Yes, wrong section, I know, but this also happens in Facebook when we choose to see an image on Full Screen mode.
If you guys can redirect me to the right direction or make a bug out of this, it would be helpful.
(In reply to megarig_2000 from comment #4)
> Yes, wrong section, I know, but this also happens in Facebook when we choose
> to see an image on Full Screen mode.
> If you guys can redirect me to the right direction or make a bug out of
> this, it would be helpful.

I'm not sure what you mean. Do you mean that if Firefox's sidebars are open before viewing an image as fullscreen on Facebook, then the sidebars are not visible after exiting fullscreen? If so, then that is this bug.

Comment 6

6 years ago
(In reply to Chris Pearce (:cpearce) from comment #5)
> I'm not sure what you mean. Do you mean that if Firefox's sidebars are open
> before viewing an image as fullscreen on Facebook, then the sidebars are not
> visible after exiting fullscreen? If so, then that is this bug.

As far as I can tell, everything that sends Firefox into fullscreen mode using the Fullscreen API doesn't restore the sidebar (Bookmarks, History). Another example is HTML5 video. So in order to reproduce, you can enable the sidebar, go to a YouTube video which is available as HTML5, play it, go to fullscreen, and back again. The sidebar will not be restored.

Comment 7

6 years ago
(In reply to Chris Pearce (:cpearce) from comment #5) 
> I'm not sure what you mean. Do you mean that if Firefox's sidebars are open
> before viewing an image as fullscreen on Facebook, then the sidebars are not
> visible after exiting fullscreen? If so, then that is this bug.

This is a bug report topic for YouTube, so I ask someone to make a new bug report for Facebook alone, unless they are okay just keeping both sites on this topic, since they are related.

Yes, when I return from Full-Screen the bookmarks sidebar is still closed when I leave that mode, it closes the sidebar as I go into Full-Screen mode in the beginning.

Comment 8

5 years ago
Any updates on this one?
I've not had time to work on this. Not sure when I will.

Updated

5 years ago
Duplicate of this bug: 835049

Updated

5 years ago
Duplicate of this bug: 865946

Comment 12

4 years ago
While this is not a bug new to the Australis interface, could this be something that can be fixed along with the other Australis bugs ... and maybe make it block bug 870032?
Component: General → Theme
Summary: Fullscreen API should restore sidebar if it was open before entering fullscreen → (autralis) Fullscreen API should restore sidebar if it was open before entering fullscreen

Updated

4 years ago
Summary: (autralis) Fullscreen API should restore sidebar if it was open before entering fullscreen → (australis) Fullscreen API should restore sidebar if it was open before entering fullscreen

Updated

4 years ago
Summary: (australis) Fullscreen API should restore sidebar if it was open before entering fullscreen → Fullscreen API should restore sidebar if it was open before entering fullscreen
Just an FYI, with the add-on OmniSidebar version 1.3.1 (currently in beta) the browser will do this automatically: the sidebar will be hidden when in HTML5 fullscreen mode and will be restored to its previous state when exiting it.

Comment 14

4 years ago
(In reply to Luís Miguel [:Quicksaver] from comment #13)
> Just an FYI, with the add-on OmniSidebar version 1.3.1 (currently in beta)
> the browser will do this automatically: the sidebar will be hidden when in
> HTML5 fullscreen mode and will be restored to its previous state when
> exiting it.

I completely understand but do we have to rely on a third-party add-on to do something that the Full-screen feature shouldn't, close the side-bar. It could at least do like Flash and pop-out in fullscreen, leaving the browser in-tact int he background. I honor your findings, though, yet no word on progress for this thing is making me lose hope. It should have been fixed a long time ago, this is absurd.

Updated

4 years ago
Duplicate of this bug: 1047445

Updated

4 years ago
Duplicate of this bug: 1062068
status-firefox34: --- → affected
status-firefox35: --- → affected
Any progress with this bug? We've got some reports of this issue on the support forum, as the HTML5 player becomes default on YouTube.
I think I know a straight-forward fix for this.

When entering DOM Fullscreen, we need to record whether or not the sidebar was open in the toggle: method inside browser-fullscreen.js. Then call toggleSidebar(). On exiting, check if we had the sidebar open, and if so, call toggleSidebar() again.

Currently we only call toggleSidebar() once, when entering DOM Fullscreen (in the enterDomFullscreen method). I suspect we'll want to move that logic into toggle, and condition it on whether or not document.mozFullscreen is true (so as to not hide the sidebar when entering browser fullscreen).

I think I can mentor this.
Mentor: mconley
Whiteboard: [testday-20120203] → [testday-20120203][lang=js]
Why not just set [moz-collapsed="true"] on the sidebar like it already does for the toolbars? It won't (or shouldn't) interfere with the sidebar function, it will keep its state after removing it/exiting fullscreen, and seems so much simpler anyway.
Ah, yes! That is simpler. Thank you Quicksaver, I like that solution much more.
Setting bug 1087564 in the see also field, in a way to sort of categorize both these bugs as "ways to improve DOM fullscreen behavior".
See Also: → bug 1087564
(Assignee)

Comment 22

4 years ago
Created attachment 8513976 [details] [diff] [review]
714675.patch

Here's a patch using the moz-collapsed technique Luís recommended and it's working great.
However I'm not satisfied with my implementation of this solution: the sidebar is restored in cleanup().

The cleanest way imo would be to introduce a new MozExitedDomFullscreen message so we could create a "proper" exitDomFullscreen function similar to enterDomFullscreen.
I tried to simulate this behaviour with the document.mozFullscreen property, but unfortunately it is already false when handleEvent() is called.
Attachment #8513976 - Flags: review?(mconley)
(In reply to Edouard Oger from comment #22)
> Created attachment 8513976 [details] [diff] [review]
> 714675.patch
> 
> Here's a patch using the moz-collapsed technique Luís recommended and it's
> working great.

This makes it impossible to manually re-show the sidebar while you're in fullscreen mode.
(In reply to Dão Gottwald [:dao] from comment #23)
> This makes it impossible to manually re-show the sidebar while you're in
> fullscreen mode.

I thought that was the point. Or do you mean it should be able to toggle off DOM fullscreen when you for example hit Ctrl+B?
(In reply to Luís Miguel [:Quicksaver] from comment #24)
> (In reply to Dão Gottwald [:dao] from comment #23)
> > This makes it impossible to manually re-show the sidebar while you're in
> > fullscreen mode.
> 
> I thought that was the point.

No, neither this bug's summary nor comment 0 imply this.

> Or do you mean it should be able to toggle off
> DOM fullscreen when you for example hit Ctrl+B?

No.
(In reply to Dão Gottwald [:dao] from comment #25)
> (In reply to Luís Miguel [:Quicksaver] from comment #24)
> > (In reply to Dão Gottwald [:dao] from comment #23)
> > > This makes it impossible to manually re-show the sidebar while you're in
> > > fullscreen mode.
> > 
> > I thought that was the point.
> 
> No, neither this bug's summary nor comment 0 imply this.

Point Dao. Still, the current behavior itself implies this. The sidebar is closed when entering DOM fullscreen for a reason, I think if it was meant to be accessible while in there, it wouldn't need to be closed in the first place... At least there also wasn't much discussion in bug 701992 about if that was actually desirable or not.

I also don't see the point in "fixing" this bug, only to open another one called something like "implement correct sidebar/UI behavior in DOM fullscreen" and have that override this bug or even null it altogether. In my opinion that'd be just spreading the same issue over several places unnecessarily.

To simplify then I suggest:
- Treat this bug as "sidebar behavior in DOM fullscreen", and not just for the specific situations described throughout (DOM fullscreen is the same regardless of website anyway, so at least the browser UI behavior should also always be the same), and change the summary accordingly.
- Answer the question "Should the sidebar be accessible/usable while in DOM fullscreen?"
-- If no (my vote), proceed from the already submitted patch, as that takes care of everything already.
-- If yes, simply don't even close the sidebar when entering DOM fullscreen; this renders the initial report mute anyway.
Thanks Quicksaver,

I agree that I don't fully understand why we'd want to open the sidebar while in DOM Fullscreen anyway. However, I suspect that Dao was concerned about the "other" fullscreen case[1], which I believe[2] will also be affected by this in the way Dao describes, which is undesirable.

So likely we want to special-case this sidebar behaviour for DOM Fullscreen.

[1]: Browser fullscreen. This feature is usually activated by going to Menu Button > Fullscreen.
[2]: I haven't tested this patch yet - I'm in the middle of a rebuild, but I'll apply and confirm once the build is done.
(Assignee)

Comment 28

4 years ago
The "browser fullscreen" is not affected by this patch. The sidebar stays on screen if you hit F11.
Yes, my build completed, and I noticed that myself. So now I'm utterly confused.

Dao - are you saying that sidebars are still supposed to be toggleable while in DOM Fullscreen?
Flags: needinfo?(dao)
If that patch doesn't mess with manual fullscreen mode, that's fine. However, rather than hiding and showing the sidebar with JS, it might be better to just set an attribute on the window element (e.g. inDOMFullscreen, to complement the inFullscreen attribute), and use that to hide the sidebar pretty much like this:
http://hg.mozilla.org/mozilla-central/annotate/675913ddbb55/browser/base/content/browser.css#l259
Flags: needinfo?(dao)
(Assignee)

Comment 31

4 years ago
I agree with your solution, it's clean. However we still need to differentiate from code which fullscreen mode (XUL or DOM) we exited to remove the new inFullscreen attribute on elements (I proposed a solution in comment 22).
Couldn't we just remove those attributes wholesale upon exiting?
(In reply to Dão Gottwald [:dao] from comment #30)
> However, rather than hiding and showing the sidebar with JS, it might be
> better to just set an attribute on the window element (e.g. inDOMFullscreen,
> to complement the inFullscreen attribute), and use that to hide the sidebar
> pretty much like this:
> http://hg.mozilla.org/mozilla-central/annotate/675913ddbb55/browser/base/
> content/browser.css#l259

To be completely honest, I don't agree. Setting the attribute in the sidebar itself avoids the need to check which fulscreen mode we are exiting, and if removing the attribute "wholesale" as Mike suggested, it's the same thing in the end. And also this seems a bit contradictory to the similar logic used for hiding the toolbars (even though this applies to browser fullscreen): http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullScreen.js#564
(In reply to Luís Miguel [:Quicksaver] from comment #33)
> And also this seems a bit contradictory to the similar logic used
> for hiding the toolbars

I.e. if "set attribute in toolbars but preserve its state" logic, then "set attribute in sidebar but preserve its state" logic

But perhaps that's me and my overly consistency-driven-border-on-OCD brain :)
So, I'm starting to lose track, but I'm trying to piece it together.

Dao is advocating an approach that uses JS to set an attribute on the Window if we've entered DOM Fullscreen, and is removed from the window when we exit fullscreen (DOM or XUL fullscreen - in the latter case, we'll be removing an attribute that is not there, a noop). With some CSS, we can hide the sidebar if that attribute is set on the window.

I think Quicksaver is still advocating direct manipulation of the collapsed state of the sidebar with JS. Am I wrong on that?

This feels like it's getting muddy, and I'd like to converge if at all possible - especially with  Edouard primed to take this one out.
Flags: needinfo?(quicksaver)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #35)
> I think Quicksaver is still advocating direct manipulation of the collapsed
> state of the sidebar with JS. Am I wrong on that?

In short, yes. I think it's the simplest approach: moz-collapsed is already in the stylesheets precisely for fullscreen usage, so use it. I do agree that setting an attribute on the window would be better practice overall (as in for everything; not just sidebar but toolbars and anything else), as it would allow for a higher degree of versatility in the end if needed eventually, but not *just* for the sidebar and its direct intended behavior, which is this bug.

You could make the point that this bug would be the start of this trend, and for example open another bug to change the toolbars code to do something similar. I am opposed to keeping two different methods of handling DOM fullscreen (window attribute for sidebar vs individual node attribute for toolbars) in an indefinite term though! I don't think that would be beneficial for anyone now or in the future.

BTW personally, it doesn't really make a difference to me (and my add-ons) as I can work with it either way. I'm just giving my opinion, I never meant for things to get muddy like you said, I'm sorry about that. :)
Flags: needinfo?(quicksaver)
On retrospect, the toolbars' code isn't specific for DOM fullscreen, it also applies for browser fullscreen. So the consistency I've been speaking of doesn't really hold up... In that sense, Dao's suggestion is the way to go. Converge on window attribute it seems. :)
Comment on attachment 8513976 [details] [diff] [review]
714675.patch

Ok, so the approach we're thinking of going for instead is this:

1) When enterDomFullscreen is called, set an attribute on the window inDOMFullscreen="true".
2) Add some CSS to hide the sidebar when that attribute is set to true on the window
3) Remove the attribute in toggle.

That's probably the shortest path to success here.
Attachment #8513976 - Flags: review?(mconley) → review-
(Assignee)

Comment 39

4 years ago
Created attachment 8514476 [details] [diff] [review]
714675_2.patch
Attachment #8513976 - Attachment is obsolete: true
Attachment #8514476 - Flags: review?(mconley)
(In reply to Edouard Oger [:eoger] from comment #39)
> Created attachment 8514476 [details] [diff] [review]
> 714675_2.patch

I think the attribute should be physically removed, just like inFullscreen is removed, rather than setting it with a "false" value. Besides cleanliness, I was once told (in a bug forever lost in the vast cosmos of our existence...) that removeAttribute is more optimized than setAttribute for noop stuff like this.
(Assignee)

Comment 41

4 years ago
Created attachment 8514499 [details] [diff] [review]
714675_3.patch

Good catch, here it is corrected.
Attachment #8514476 - Attachment is obsolete: true
Attachment #8514476 - Flags: review?(mconley)
Attachment #8514499 - Flags: review?(mconley)
Comment on attachment 8514499 [details] [diff] [review]
714675_3.patch

Review of attachment 8514499 [details] [diff] [review]:
-----------------------------------------------------------------

I like it! I've pushed this patch to try:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ce57719bb141

If / when this comes back green, I'll push this into the tree. Thanks so much Edouard!

Feel like hacking on another one? Bug 982856 could use some love...
Attachment #8514499 - Flags: review?(mconley) → review+
https://hg.mozilla.org/integration/fx-team/rev/3aef39ca3919
Assignee: nobody → edouard.oger
Keywords: checkin-needed
Whiteboard: [testday-20120203][lang=js] → [testday-20120203][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3aef39ca3919
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [testday-20120203][lang=js][fixed-in-fx-team] → [testday-20120203][lang=js]
Target Milestone: --- → Firefox 36

Comment 46

4 years ago
The new window property should be documented somewhere, I guess.
Keywords: dev-doc-needed
QA Whiteboard: [good first verify]

Updated

3 years ago
Duplicate of this bug: 1133240

Comment 48

3 years ago
This bug is still present.  I'm using FF 41.0.1 on Linux.  I had the sidebar open with vertical tabs, as usual, and happened to watch a video in fullscreen mode.  When I exited fullscreen mode, that FF window auto-hides the sidebar now.  I have to hover over the left area to make the sidebar appear.  It's intensely irritating.

The only way I found to "fix" the session was to open a new window, and drag all the sets of tabs over to the new window, leaving the "autohide-crippled window" behind.
(In reply to Luke Kendall from comment #48)
> I had the sidebar open with vertical tabs

I don't believe you're actually looking at the sidebar itself (the thing that this bug fixed) but at another thing entirely, perhaps an add-on like TreeStyleTabs? If so, that is an issue you should take to the add-on's developers, I'm sure they will appreciate you bringing it to their attention.

Comment 50

3 years ago
It is TreeStyleTabs, yes - thanks, I will.  Sorry for the erroneous report.
You need to log in before you can comment on or make changes to this bug.