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

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Steve Scott (pxbugz)]

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

Comment 1

[Leonard Camacho [:lcamacho]]

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

Comment 4

[megarig_2000]

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.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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

[Markus Popp]

(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

[megarig_2000]

(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

[Markus Popp]

Any updates on this one?

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

I've not had time to work on this. Not sure when I will.

Comment 12

[Markus Popp]

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?

Comment 13

[Luís Miguel [:quicksaver] (not reading bugmail)]

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

[megarig_2000]

(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.

Comment 17

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

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.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 19

[Luís Miguel [:quicksaver] (not reading bugmail)]

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.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Ah, yes! That is simpler. Thank you Quicksaver, I like that solution much more.

Comment 21

[Luís Miguel [:quicksaver] (not reading bugmail)]

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".

Comment 22

[Edouard Oger [:eoger]]

Attachment: 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.

Comment 23

[Dão Gottwald [:dao]]

(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.

Comment 24

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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?

Comment 25

[Dão Gottwald [:dao]]

(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.

Comment 26

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 28

[Edouard Oger [:eoger]]

The "browser fullscreen" is not affected by this patch. The sidebar stays on screen if you hit F11.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 30

[Dão Gottwald [: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

Comment 31

[Edouard Oger [:eoger]]

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).

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Couldn't we just remove those attributes wholesale upon exiting?

Comment 33

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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

Comment 34

[Luís Miguel [:quicksaver] (not reading bugmail)]

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

Comment 35

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 36

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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. :)

Comment 37

[Luís Miguel [:quicksaver] (not reading bugmail)]

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 38

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 39

[Edouard Oger [:eoger]]

Attachment: 714675_2.patch

Comment 40

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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.

Comment 41

[Edouard Oger [:eoger]]

Attachment: 714675_3.patch

Good catch, here it is corrected.

Comment 42

[Mike Conley (: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...

Comment 43

[Mike Conley (:mconley) (:⚙️)]

Try looks good.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/3aef39ca3919

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3aef39ca3919

Comment 46

[Florian Bender]

The new window property should be documented somewhere, I guess.

Comment 48

[Luke Kendall]

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.

Comment 49

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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

[Luke Kendall]

It is TreeStyleTabs, yes - thanks, I will.  Sorry for the erroneous report.