962855 - [Australis] Scroll bar shown in panelUI with default set on Windows, Linux, Mac

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alice0775 White]

Attachment: screenshot

Steps To Reproduce:
0. Start Firefox with newly created clean profile
1. Click Hamburger

Actual Results:
Scroll bar shown in panelUI 

Regression window(fx)
Good:
http://hg.mozilla.org/integration/fx-team/rev/cdca987f43ce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140121091707
Bad:
http://hg.mozilla.org/integration/fx-team/rev/868c2aeb6e55
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140121110336
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=cdca987f43ce&tochange=868c2aeb6e55

Regressed by:
868c2aeb6e55	Gijs Kruitbosch — Bug 941109 - use closemenu instead of noautoclose attribute in Australis menupanel, r=mconley

Comment 1

[B.J. Herbison]

Attachment: Linux menu with scroll bar.

I see the scroll bar on Ubuntu on the first time I open the menu after starting Nightly. I tried with a clean profile, started Nightly three times, and opened the menu several times. The scroll bar appeared only on the first load for each restart.

Comment 2

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #0)
> Regressed by:
> 868c2aeb6e55	Gijs Kruitbosch — Bug 941109 - use closemenu instead of
> noautoclose attribute in Australis menupanel, r=mconley

This doesn't make any sense either. That attribute (and that patch) should make 0 difference on how stuff is laid out.

Anyway, as this is now tracking 29, it should probably be P2 rather than P4.

Comment 3

[Alice0775 White]

So, attempt with default set

Regression window(fx)
Good:
http://hg.mozilla.org/integration/fx-team/rev/6038ead525b2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140121055156
Bad:
http://hg.mozilla.org/integration/fx-team/rev/f59607697038
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140121060257
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=6038ead525b2&tochange=a6dd7dcee301
Regressed by:
	f59607697038	Stephen Horlander — Bug 900428 - Increase Australis Menu Panel Label Font-Size (part of the patch worked out by Gijs), r=jaws

Comment 4

[:Gijs (he/him)]

I can even repro on mac. Investigating what's going on here. See also bug 963365.

Comment 5

[:Gijs (he/him)]

So the weird thing I found out is... At least on mac, if you fix the font-size of the edit and zoom controls to 10px, the issue goes away (haven't had time to try other font-sizes and need to run right now). I don't understand why. I logged a bunch of sizes, and it seems the scrollHeight of the content pane is basically reported 1px too small. After closing and reopening the panel, it is correct. The difference is 1px. scrollTopMax is also 0. Really not sure what's going on here yet...

Comment 6

[:Gijs (he/him)]

So higher font sizes (say, 20px) cause the difference (1px for the 11px default font size) to be higher, ie, more scrolling is possible. Still not sure what causes this.

Comment 7

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

Remember when I said that panelUI.xml wasn't to blame? At the very least, it's involved - the _setMaxHeight is setting a height on the mainview that is 1px lower than what it sets it at during the second appearance of the panel.

Comment 8

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

Preeeeetty weird stuff here.

I've put a breakpoint in _setMaxHeight right at the point it sets the height value on the main view (see the epic tale in bug 861703 for why we're doing that), and the boxObject for the PanelUI-mainView is apparently 411px height (following the chain of panelmultiview.boxObject.children[0].boxObject.height).

HOWEVER

Looking at the element through another path tells another story... panelmultiview.children[0].boxObject.height is 412px.

panelmultiview.boxObject.children[0] and panelmultiview.children[0] are both PanelUI-mainView... so you'd think their boxObject heights would match up here.

But nope.

Wtf?

Comment 9

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

I smell rounding and XUL layout funk.

Comment 10

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #8)
> Preeeeetty weird stuff here.
> 
> I've put a breakpoint in _setMaxHeight right at the point it sets the height
> value on the main view (see the epic tale in bug 861703 for why we're doing
> that), and the boxObject for the PanelUI-mainView is apparently 411px height
> (following the chain of
> panelmultiview.boxObject.children[0].boxObject.height).
> 
> HOWEVER
> 
> Looking at the element through another path tells another story...
> panelmultiview.children[0].boxObject.height is 412px.
> 
> panelmultiview.boxObject.children[0] and panelmultiview.children[0] are both
> PanelUI-mainView... so you'd think their boxObject heights would match up
> here.
> 
> But nope.
> 
> Wtf?

I'm confused. panelmultiview (|this| in _setMaxHeight, right?) doesn't have the mainview as a kid anymore when I hit that breakpoint, it's moved it into its set of containers, which are anonymous and therefore not accessible through this tree. What did I miss?

(In reply to Mike Conley (:mconley) from comment #9)
> I smell rounding and XUL layout funk.

Layout, yes, rounding... no, I think? It's strange that it can be more than 1px if you increase the font size of the cut/copy/paste/zoom buttons.

Comment 11

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

Note that hard-coding the height of the zoom and edit controls seems to eliminate this bug.

Comment 12

[:Gijs (he/him)]

Attachment: fix scrollbar appearing in default set on Australis,

Indeed. This seems to work, too, which is good because we kind of need to keep this working with other font-sizes, even if the calc() is a little stupid, it seems to work well enough...

Comment 13

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

Comment on attachment 8366177 [details] [diff] [review]
fix scrollbar appearing in default set on Australis,

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

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +510,5 @@
>       of 3px. */
>    min-width: calc(@menuPanelWidth@ / 3 - 2px);
>    max-width: calc(@menuPanelWidth@ / 3 - 2px);
> +  /* We'd prefer to use height: auto here but it leads to layout bugs in the panel. Cope: */
> +  /* 1.2em for line height + 2 * .5em padding + margin on the label (2 * 2px) */

I wonder if we'll be bitten someday by this again if / when we change the margin and paddings on the label. Is it ridiculous to put these numbers into constants? Or is there a better way to avoid a future footgun?

@@ +512,5 @@
>    max-width: calc(@menuPanelWidth@ / 3 - 2px);
> +  /* We'd prefer to use height: auto here but it leads to layout bugs in the panel. Cope: */
> +  /* 1.2em for line height + 2 * .5em padding + margin on the label (2 * 2px) */
> +  height: calc(2.2em + 4px);
> +  /*height: auto; */

This line needs to go.

Comment 14

[:Gijs (he/him)]

Attachment: fix scrollbar appearing in default set on Australis,

Comment 15

[:Gijs (he/him)]

Comment on attachment 8366244 [details] [diff] [review]
fix scrollbar appearing in default set on Australis,

r+ from mconley via IRC, because the 1.2em isn't defined anywhere, the .5em padding is defined in the same rule, and the margin is defined elsewhere completely because it's the default for toolbarbuttons.

remote:   https://hg.mozilla.org/integration/fx-team/rev/44e247018701

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44e247018701

Comment 17

[:Gijs (he/him)]

This regressed again in the last day or so. :-(

Comment 18

[:Gijs (he/him)]

AFAICT this was fixed again by bug 965541.

https://hg.mozilla.org/mozilla-central/rev/8c00a78919c3

Comment 19

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

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0

The scrollbar is no longer displayed in Firefox 29 beat 1 - build ID: 20140318013849.
Marking issue verified.