987859 - Australis: Top of page invisible in DOM full screen when coming from restored window with the menu bar hidden

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Peter J. Sloetjes]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140314030202

Steps to reproduce:

I switched from a restored window to a full screen window, with the Firefox menu hidden.


Actual results:

The top of the page including the scroll bar was clipped off by about 12px.


Expected results:

The issue is not present when the menu is visible or when the window was previously maximized.

Comment 1

[:Gijs (out for now; he/him)]

(In reply to pjs.nl from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101
> Firefox/30.0 (Beta/Release)
> Build ID: 20140314030202
> 
> Steps to reproduce:
> 
> I switched from a restored window to a full screen window, with the Firefox
> menu hidden.

Which menu do you mean?

> Actual results:
> 
> The top of the page including the scroll bar was clipped off by about 12px.

Can you post a screenshot, please?

Comment 2

[:Gijs (out for now; he/him)]

(also, how did you switch to fullscreen mode? With F11, did the website do it, or did you click the button?)

Comment 3

[Peter J. Sloetjes]

I mean the Firefox Menu Bar (hidden via the toolbar context menu).
The issue occurs both when I press F11 and when I press the full screen button from the toolbar panel.
For an imaginary screenshot, think of any page in full screen with 12px clipped off at the top :)

Comment 4

[Alice0775 White]

(In reply to pjs.nl from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101
> Firefox/30.0 (Beta/Release)
> Build ID: 20140314030202


Build ID: 20140314030202 is too old!
Please test on latest build.

Comment 5

[Peter J. Sloetjes]

I now tested with the latest build in safe mode and the issue is still there.
I also noticed that it is not possible to toggle visibility of the navigator bar by moving to the top of the screen, which makes sense if the entire container box is shifted upwards, possibly by a negative top margin.

Comment 6

[:Gijs (out for now; he/him)]

I can't reproduce this issue, and I'm guessing Alice couldn't either...

What Windows 7 theme do you use? A screenshot would still be helpful...

Comment 7

[Peter J. Sloetjes]

The issue is also present for Windows basic and classic themes, is independent of the position of the task bar and occurs only for Australis versions of Firefox. It does NOT occur when switching from a maximized window, but only when switching from a restored window to full screen. I will play with some of the advanced visualization settings of Windows to see if I can make the issue go away.

P.S. I encountered the opposite issue with Australis versions when using my add-on Monitor Master, in which case the navigator container instead had too much margin at the top when the window was re-positioned before becoming visible during start-up, especially when start-up took long. (Note that this cannot be directly relevant to the current issue because it also occurs in safe mode.) It does however indicate that parts of the layout mechanism at least in some cases work with out-dated window state/size/position information, or that an additional layout step is needed after window state changes.

Comment 8

[:Gijs (out for now; he/him)]

(In reply to pjs.nl from comment #7)
> The issue is also present for Windows basic and classic themes, is
> independent of the position of the task bar and occurs only for Australis
> versions of Firefox. It does NOT occur when switching from a maximized
> window, but only when switching from a restored window to full screen. I
> will play with some of the advanced visualization settings of Windows to see
> if I can make the issue go away.
> 
> P.S. I encountered the opposite issue with Australis versions when using my
> add-on Monitor Master, in which case the navigator container instead had too
> much margin at the top when the window was re-positioned before becoming
> visible during start-up, especially when start-up took long. (Note that this
> cannot be directly relevant to the current issue because it also occurs in
> safe mode.) It does however indicate that parts of the layout mechanism at
> least in some cases work with out-dated window state/size/position
> information, or that an additional layout step is needed after window state
> changes.

Can you try a clean profile instead of safe mode? We persist some of the relevant values in localstore.rdf, and it's possible that your localstore.rdf still contains values that were influenced by your add-on, and that that's messing things up.

Comment 9

[Peter J. Sloetjes]

Well, I removed my localstore.rdf, to no avail. I guess the other conditions made a role for serialized data unlikely already. In my experience, XUL layout in response to window state changes was a bit unreliable on Windows even before Australis. Perhaps testing on Windows should be more extensive.

Comment 10

[:Gijs (out for now; he/him)]

(In reply to pjs.nl from comment #9)
> Well, I removed my localstore.rdf, to no avail. I guess the other conditions
> made a role for serialized data unlikely already. In my experience, XUL
> layout in response to window state changes was a bit unreliable on Windows
> even before Australis. Perhaps testing on Windows should be more extensive.

We do test extensively on Windows - in fact, I'd like to think that if this was easy to reproduce, we would have noticed before now... - I spent a while trying to reproduce this on both Windows 8 and Windows 7 and couldn't. There must be something we're missing. Are you using a non-default font or something other than a 100% DPI setting?

Comment 11

[Peter J. Sloetjes]

No, I don't, but I looked into my preferences and found that the issue is present only when browser.fullscreen.animateUp is set to a non-default value (0). Perhaps that makes it reproducible?

Comment 12

[Alice0775 White]

I can reproduce the problem if I set browser.fullscreen.animateUp=0.
And this problem started since Australis merge day(2013-11-18).

Comment 13

[:Gijs (out for now; he/him)]

This is probably P4 bordering on P5, considering most people won't have that pref set...

Comment 14

[Peter J. Sloetjes]

It shows that something is fundamentally wrong with the layout cascade. As noted above, this is not the only case where the layout ends up in an invalid state. In the past I have encountered exactly the same problem when implementing a toolbar customization add-on and when implementing a window border size option in Monitor Master. In addition, I have several extensions in which I would like to temporarily set the animateUp preference to 0, because the default animation is really annoying in cases where page layout is computationally expensive. Please fix this, because I don't want to annoy my users with unpredictable behavior.

Comment 15

[:Gijs (out for now; he/him)]

(In reply to pjs.nl from comment #14)
> It shows that something is fundamentally wrong with the layout cascade.

No. We mess with a bunch of these things "manually" in TabsInTitlebar handling code. It likely has little if anything to do with "the layout cascade". See:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4423

It's a lot more complicated than you seem to think, as there are a lot of cases that code needs to cover (Windows and OS X behave very differently, lightweight themes, different window sizemodes, different fullscreen modes (with/without toolbars, DOM fullscreen, Lion fullscreen on OS X, ...), different font sizes, different DPI settings, various things can turn off tabsintitlebar either temporarily or permanently, ...).

If you have time to investigate why disabling the animation makes a difference, that would be very much appreciated, but if not, please stop speculating. Thank you!

Comment 16

[Alice0775 White]

Regression window:
Good:
http://hg.mozilla.org/projects/ux/rev/fc287e8c97a1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130228 Firefox/22.0 ID:20130228040201

Bad:
http://hg.mozilla.org/projects/ux/rev/0e37f0b17eb7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130301 Firefox/22.0 ID:20130301040203

But, I cannot figure out pushlog...

Comment 17

[Peter J. Sloetjes]

The actual hiding occurs by setting a negative margin on gNavToolbox (line 302 of https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fullScreen.js).

When animating, this final step occurs after a CSS layout step, which removes 12px from the TabsToolbar margin-top (line 39/44 of https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css).

When not animating, it happens before this CSS layout step, which results in a too large negative margin being set.

Potential quick fixes are the following (the first is more robust):
1) Always animate, but only for 1ms when animateUp is 0, in order to delay the final step.
2) Manually subtract 12px from the negative margin when the OS is Windows and the menu is hidden.

Structurally, it would be best to move all rules to either CSS or JS.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to pjs.nl from comment #17)
> The actual hiding occurs by setting a negative margin on gNavToolbox (line
> 302 of
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> fullScreen.js).
> 
> When animating, this final step occurs after a CSS layout step, which
> removes 12px from the TabsToolbar margin-top (line 39/44 of
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/
> browser.css).
> 
> When not animating, it happens before this CSS layout step, which results in
> a too large negative margin being set.

Thanks, that's very helpful! :-)

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Not tracking (cf comment #13)

Comment 20

[Peter J. Sloetjes]

Well, part of the content area not being visible seems to me a much more important issue than nearly all  of the issues which are reported here by core Mozillians in endless notes-to-self.

Hiding this bug from tracking is the common cause of Firefox and XUL having many options of which usually only one combination actually works, which is the one that is to be found by the developer :( 

If animation was actually introduced to get around the a-synchronicity problem, then please remove the 'animateUp' preference and provide an 'animateUpDuration' preference that allows the animation time to be set to 1ms, because that would most likely fix this bug.

Another reason for cautioning against not tracking, analyzing and fixing this bug is that similar issues do arise during start-up. But I guess I'll have to file another bug for that.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

I am not hiding it. That just means that I won't block the 29 (or 30) release because of this bug.
I will be happy to uplift any patches for 29 (or later).

Comment 22

[Peter J. Sloetjes]

OK, that's fine, I will be happy, too. I guess I was getting a little irritated, because until now, in most cases in which I reported a bug, bug handling stopped after I had provided a screen shot and a test case, and I thought I had come a little further with this one by exposing the cause ...

Comment 23

[Peter J. Sloetjes]

This issue does affect all users after all, because it is also present when mozRequestFullScreen is called on an element (while the window is restored and the Firefox menu is hidden, regardless of the animateUp preference).

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Discussed about this bug on IRC with Gijs and it is actually important enough to be tracked.

Comment 25

[:Gijs (out for now; he/him)]

I'm going to need some help here, because I don't really know why the code works the way it currently does, and I don't see an easy way of fixing this issue (short of the 1ms animation suggested as fix (1) in comment 17, but that has issues, see below).

Jared/Dão, do you know why this uses mozrequestanimationframe and a 'manual' animation, rather than e.g. a CSS transition? And this code looks pretty fragile (e.g. what if in another window, I toggle a toolbar and thereby change the height of the toolbox) - can we animate the height of the box instead, and set height to 1px, and margin-top to -1px? (I'm assuming setting the height to 0 would re-create the issues from bug 430687)

Another alternative fix would be setting an attribute before entering fullscreen that forces the margins to their fullscreen value, so that we calculate the right height (but that'd still have the issue with multiple windows + toolbar visibility).

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

We can keep tracking this for FF30 to make sure it doesn't fall off the radar but as this is still considered something users would have to set a pref to experience, we shouldn't expect there to be widespread effects here.

Comment 27

[Peter J. Sloetjes]

No pref needs to be set, see Comment 23, and this design flaw should be removed a.s.a.p.

Comment 28

[Peter J. Sloetjes]

I noted that, when FTDeepDark 10beta8 is installed on Australis versions (a dark theme with square tabs), the top offset is correct for both ways to enter full screen. I think that this shows that this issue could be fixed without touching 'browser-fullScreen.js', that is, by fixing the Australis CSS (somewhere).

Still, a mistake in the CSS should not be allowed to produce an error in the top offset, so improving the JS would still be desirable, but that could be done in a later version.

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Peter J. Sloetjes from comment #27)
> No pref needs to be set, see Comment 23, and this design flaw should be
> removed a.s.a.p.

Do we have a specific bug that can be backed out to do this?  We're in the final weeks of beta and won't be taking anything speculative here so a backout, if low risk, is the only way to go in order to fix this for FF30.

Comment 31

[:Gijs (out for now; he/him)]

(In reply to Lukas Blakk [:lsblakk] from comment #30)
> (In reply to Peter J. Sloetjes from comment #27)
> > No pref needs to be set, see Comment 23, and this design flaw should be
> > removed a.s.a.p.
> 
> Do we have a specific bug that can be backed out to do this?  We're in the
> final weeks of beta and won't be taking anything speculative here so a
> backout, if low risk, is the only way to go in order to fix this for FF30.

No.

Comment 32

[:Gijs (out for now; he/him)]

Dolske, can we get this added to the next iteration's backlog so we can fix this at least for 31? :-\

Comment 33

[Peter J. Sloetjes]

The cause is that AFTER a line of JS sets the negative top margin on gNavToolbox in order to hide it, a CSS rule removes the 15px top margin of the TabsToolbar (not 12px as I estimated before) in response to an attribute change, thereby reducing the height of gNavToolbox by 15px.

To prevent a change in the height of gNavToolbox, one could change line 44 (and 39?) in https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css in one of two ways:
1) Add the 15px top margin that compensate for the menu to gNavToolbox instead of the TabsToolbar.
2) Do not compensate for the menu and draw the tabs in the title bar. This makes a lot of sense, but it requires some care for the visibility of the window buttons.

What more info could be needed? Some complete themes do get it right.

Comment 34

[Peter J. Sloetjes]

Setting a padding-top on the TitleBar instead of a margin-top on the TabsToolbar also seems to do the trick (see the FT DeepDark complete theme).

Comment 35

[Dão Gottwald [:dao]]

Attachment: patch

(In reply to Peter J. Sloetjes from comment #33)
> The cause is that AFTER a line of JS sets the negative top margin on
> gNavToolbox in order to hide it, a CSS rule removes the 15px top margin of
> the TabsToolbar (not 12px as I estimated before) in response to an attribute
> change, thereby reducing the height of gNavToolbox by 15px.

And the reason why the CSS removes the margin after the JS ran is that it uses the sizemode attribute, which gets updated too late (after the fullscreen event). We can workaround this with the inFullscreen attribute which is set by the code handling the fullscreen event.

Comment 36

[Dão Gottwald [:dao]]

Attachment: patch v2

Dropped the changes to the #toolbar-menubar rules ... the fullscreen code always collapses that toolbar, at which point its margin shouldn't apply anymore.

Comment 37

[:Gijs (out for now; he/him)]

Comment on attachment 8425407 [details] [diff] [review]
patch v2

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

This is low-risk/high-impact enough that I think we should push for beta uplift still.

Comment 38

[:Gijs (out for now; he/him)]

Comment on attachment 8425407 [details] [diff] [review]
patch v2

(and the flags don't work in splinter or I would have done it immediately)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: fullscreen sometimes misses the top of the window on Windows
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): pretty much no risk; this is optimizing a CSS selector that applies later in the "go into fullscreen" process to apply earlier in the "go into fullscreen" process
String or IDL/UUID changes made by this patch: none

Comment 39

[:Gijs (out for now; he/him)]

Marco, seems we're doing this this iteration. :-)

(Dão, I'm picking a points value semi-at-random, please adjust it if you disagree)

Comment 40

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #39)
> Marco, seems we're doing this this iteration. :-)
> 
> (Dão, I'm picking a points value semi-at-random, please adjust it if you
> disagree)

It wasn't quite as trivial as it may seem. The reason why I was so quick to explain what's going on after Peter's comments is that I had already spent time on this trying to figure out what went wrong and what regressed this (my first bet was bug 813802).

Comment 41

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/7b0d9bec2497

Comment 42

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #40)
> (In reply to :Gijs Kruitbosch from comment #39)
> > Marco, seems we're doing this this iteration. :-)
> > 
> > (Dão, I'm picking a points value semi-at-random, please adjust it if you
> > disagree)
> 
> It wasn't quite as trivial as it may seem. The reason why I was so quick to
> explain what's going on after Peter's comments is that I had already spent
> time on this trying to figure out what went wrong and what regressed this
> (my first bet was bug 813802).

Great! Thanks for the explanation; at least I also feel less stupid now. (I'd not looked in detail yet.) :-)

Comment 43

[Marco Mucci [:MarcoM]]

Added to Iteration 32.2

Comment 44

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by: will make sure this gets into Thursday's Beta.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b0d9bec2497

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d71874cd9efa
https://hg.mozilla.org/releases/mozilla-beta/rev/2925e9a0a33d

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/2925e9a0a33d

Comment 48

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0

Verified as fixed on latest Nightly, build ID: 20140522030204.
Tried several times by switching to full screen with both F11 and using the full screen button from the menu panel methods.

I will change the Status flag as soon as I'll verify this fix on Aurora and Beta.

Comment 49

[Dão Gottwald [:dao]]

(In reply to Cornel Ionce [QA] from comment #48)
> Verified as fixed on latest Nightly, build ID: 20140522030204.
> Tried several times by switching to full screen with both F11 and using the
> full screen button from the menu panel methods.

You need to use DOM fullscreen mode, for instance from the testcase in this bug's URL field.

Comment 50

[Peter J. Sloetjes]

Good to see that you guys found an intelligent solution.

Please also look into a few other bugs that aren't regressions (sorry for stealing your attention):

When upgrading bootstrapped add-ons, XUL icons are not updated
	https://bugzilla.mozilla.org/show_bug.cgi?id=968541
Top-level panels cannot be dragged when parent window is in full screen.
	https://bugzilla.mozilla.org/show_bug.cgi?id=978500
-moz-element doesn't work when <base> tag is present
	https://bugzilla.mozilla.org/show_bug.cgi?id=776363
Browser crash when using drawImage on too large canvas.
	https://bugzilla.mozilla.org/show_bug.cgi?id=702150
Canvas globalAlpha does not apply to drawWindow.
	https://bugzilla.mozilla.org/show_bug.cgi?id=913999

And, last but not least:

Asm.js code fails to link in SDK addon
	https://bugzilla.mozilla.org/show_bug.cgi?id=958972

Now solving that last one would actually be a major breakthrough :)

Comment 51

[Catalin Varga [QA][:VarCat]]

Verified as fixed using 2 scenarios the one that involves changing the pref and restoring a session before going fullscreen and the testcase from the URL.
Environment used:

FF 30.0b7
Build Id: 20140522105902

FF 31 Aurora
Build Id:20140522004003 

FF 32 Nightly
Build Id: 20140522030204

Os: Windows 7 x64