1174431 - DOM Fullscreen makes the Bookmarks bar disappear

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Osmose [:osmose, :mkelly]]

Tested on a fresh profile on Developer Edition 40.0a2 (2015-06-13), OS X 10.10.3.

STR:

1. Ensure the Bookmarks bar is visible.
2. Visit http://codepen.io/anon/pen/mJwNba.
3. Click the button to enter fullscreen mode.
4. Hit Esc to exit fullscreen mode (or decline the request or whatever).

Expected: The Bookmarks bar is still visible below the URL bar.

Actual: The Bookmarks bar has disappeared from the UI. The View menu still shows it as enabled, and disabling/re-enabling it does not make it appear again, nor does it show up in the customization view.

This affects Youtube, Vimeo, etc. and can be reproduced on them as well.

Comment 2

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

This bug is a regression from bug 1168397 in Nightly 41 that was uplifted to Aurora 40.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

This bug actually have another behavior which affects all users who even don't use bookmark toolbar: the context menu of the window is still in fullscreen mode, and restart Firefox makes the window maximized.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 6

[:Gijs (he/him)]

Comment on attachment 8624114 [details] [diff] [review]
patch

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

Considering Dão reviewed all the patches that changed all this code, he seems like a better reviewer. One note below already.

::: browser/base/content/browser-fullScreen.js
@@ -598,5 @@
> -    // For Lion fullscreen, all fullscreen controls are hidden, don't
> -    // bother to touch them. If we don't stop here, the following code
> -    // could cause the native fullscreen button be shown unexpectedly.
> -    // See bug 1165570.
> -    if (this.useLionFullScreen) {

Why are you reverting this?

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8624114 [details] [diff] [review]
> patch
> 
> Review of attachment 8624114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Considering Dão reviewed all the patches that changed all this code, he
> seems like a better reviewer. One note below already.
> 
> ::: browser/base/content/browser-fullScreen.js
> @@ -598,5 @@
> > -    // For Lion fullscreen, all fullscreen controls are hidden, don't
> > -    // bother to touch them. If we don't stop here, the following code
> > -    // could cause the native fullscreen button be shown unexpectedly.
> > -    // See bug 1165570.
> > -    if (this.useLionFullScreen) {
> 
> Why are you reverting this?

Because the only callsite of this function has the exactly the same guard now.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8624114 [details] [diff] [review]
patch

This patch seems completely wrong to me. We absolutely should hide toolbars for DOM fullscreen mode, Lion or not. If I understand comment 0 correctly, the bug is that the bookmarks toolbar doesn't reappear when exiting DOM FS.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8624162 [details] [diff] [review]
patch

Maybe I'm missing something, but I don't see how this patch is better than the previous one regarding my review comment on that...

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 8624162 [details] [diff] [review]
> patch
> 
> Maybe I'm missing something, but I don't see how this patch is better than
> the previous one regarding my review comment on that...

Yes, they are effectively the same thing. But I describe more detailedly about what the code does in this patch.

So in DOM fullscreen, we have hidden the whole toolbar, so we don't need to bother what should be shown or hidden on the toolbar at all.

Originally, hidding non-fullscreen toolbar code is called when entering fullscreen, but is not called when leaving fullscreen on OS X. This patch fixes this via making both path identical in all platforms.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> So in DOM fullscreen, we have hidden the whole toolbar, so we don't need to
> bother what should be shown or hidden on the toolbar at all.

I see. But why would we avoid hiding toolbars for DOM FS only on OS X Lion and later rather than all OSes? This doesn't seem to make much sense and contributed to me being confused by the patch.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #11)
> > (In reply to Dão Gottwald [:dao] from comment #10)
> > So in DOM fullscreen, we have hidden the whole toolbar, so we don't need to
> > bother what should be shown or hidden on the toolbar at all.
> 
> I see. But why would we avoid hiding toolbars for DOM FS only on OS X Lion
> and later rather than all OSes? This doesn't seem to make much sense and
> contributed to me being confused by the patch.

We hide toolbars on all platforms *including* OS X Lion for DOM Fullscreen via calling hideNavToolbox().

We just do not hide non-fullscreen toolbar, which has already been invisible on DOM Fullscreen, again on OS X Lion like other platforms.

The reason is that, doing so makes code path more complicated. If we need to hide that in addition to hiding the whole toolbar, we need to distinguish between exiting fullscreen mode and exiting DOM fullscreen. If we want to distinguish that, then, we need to backup the state to check, because events may not be helpful enough in that case. ("fullscreen" event is triggered earlier than "MozDOMFullscreen:Exited" event)

Comment 14

[Dão Gottwald [:dao]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #13)
> We just do not hide non-fullscreen toolbar, which has already been invisible
> on DOM Fullscreen, again on OS X Lion like other platforms.

I'm not sure how to read this sentence. Are you saying that (with your patch applied) we'd avoid hiding toolbars on all OSes or that we'd make the whole toolbox invisible on all OSes and on top of that avoid hiding toolbars on OS X Lion? The latter is how I read the patch, and it's confusing.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #13)
> > We just do not hide non-fullscreen toolbar, which has already been invisible
> > on DOM Fullscreen, again on OS X Lion like other platforms.
> 
> I'm not sure how to read this sentence. Are you saying that (with your patch
> applied) we'd avoid hiding toolbars on all OSes or that we'd make the whole
> toolbox invisible on all OSes and on top of that avoid hiding toolbars on OS
> X Lion? The latter is how I read the patch, and it's confusing.

On platforms other than OS X (as well as OS X without the patch), when enter DOM fullscreen, we do:
1) hide non-fullscreen toolbar, then
2) hide the whole toolbar

On platforms other than OS X, when leave DOM fullscreen, we do:
3) show non-fullscreen toolbar, then
4) show the whole toolbar

But on OS X without the patch, when leave DOM fullscreen, we skip 3).

So what this patch does is, making 1) also be skipped on OS X.

Behavior on other platforms should be unchanged.

Comment 16

[Dão Gottwald [:dao]]

We should do 1) and 3) on all platforms or nowhere for DOM FS; preferably nowhere. There should be no special treatment for OS X here. If the events aren't useful or not in the right order to do the right thing here, we should fix that.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Dão Gottwald [:dao] from comment #16)
> We should do 1) and 3) on all platforms or nowhere for DOM FS; preferably
> nowhere. There should be no special treatment for OS X here. If the events
> aren't useful or not in the right order to do the right thing here, we
> should fix that.

Fixing that is much more complicated than fixing this bug in this way. And even if we decide to do so, it is also much more dangerous to uplift.

We have skipped 1) and 3) on OS X for fullscreen mode as a special case, why we cannot skip them for DOM fullscreen, especially given it has no visible difference?

Actually, I would prefer we backout bug 1168397 so that we don't treat anything special on OS X, but you all did not agree with that.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

The original behavior before all my changes, is:
* for DOM fullscreen, it prefectly runs all four steps on all platforms; but
* for fullscreen mode, it removes 1) but leave 3) on OS X.

We can restore to that behavior, but I consider asymmetric calls potentially more dangerous than removing a pair of corresponding operations.

Comment 19

[Dão Gottwald [:dao]]

I think my patch in bug 1176233 should fix this bug by replacing the faulty JS spaghetti code with simpler CSS.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Fixed by bug 1176233

Comment 23

[Kate Glazko]

Is this something that you would want to uplift to 40? Tracking because regression.

Comment 24

[Chris Peterson [:cpeterson]]

Xidorn says this bug was fixed by bug 1176233, which Dão has asked to uplift to Beta 40.

Comment 25

[Petruta Horea [:phorea]]

Verified as fixed using Aurora 41.0a2 (2015-07-08) under Ubuntu 14.04 x32, Mac OS X 10.9.5 and Windows 10 Pro x64 (Build 10162) on a Microsoft Surface Pro 2.

Comment 26

[Kate Glazko]

Setting status flags to reflect fix in bug 1176233.

Comment 28

[Petruta Horea [:phorea]]

Verified on Firefox 42 beta 7 during testday (https://public.etherpad-mozilla.org/p/mozqabd-testday-16102015).