907787 - Australis: toolbar overflow button should be hidden by default

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

(see also: bug 902857 comment 18)
As per summary. Right now it becomes invisible when the toolbar gets its XBL constructor called. That leads to extra reflow work. A quick local tpaint test on my win7 machine shows the following numbers:

Before: 126.06, 125.05, 125.68 (avg: 125.60)
After: 124.56, 124.57, 124.35 (avg: 124.49)

(this is without PGO, on a fast desktop machine)

Comment 1

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

Attachment: Hide the overflow button by default

Comment 2

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

https://hg.mozilla.org/projects/ux/rev/2512d523d5fc

Comment 3

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

This caused a bunch of mochitest-1/2/3 bustage. Attempt to fix it pushed as https://hg.mozilla.org/projects/ux/rev/8349a50ded27

Comment 4

[Dão Gottwald [:dao]]

Can you please get that "bustage fix" reviewed, since it makes the selector more expensive and is far from obvious? Why is #main-window:not([chromehidden~="toolbar"]) needed now when it wasn't needed before?

Comment 5

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

(In reply to Dão Gottwald [:dao] from comment #4)
> Can you please get that "bustage fix" reviewed, since it makes the selector
> more expensive and is far from obvious? Why is
> #main-window:not([chromehidden~="toolbar"]) needed now when it wasn't needed
> before?

It was discussed with Jared on IRC at the time. The simple explanation is: before, the button defaulted to visible, except on an overflowable toolbar that was not overflowing. That meant it was hidden on popup windows opened with toolbar=no. In the current incarnation, it defaults to invisible, except on overflowable toolbars that do overflow. That meant it showed up on popup windows with toolbar=no. That enlarged the size of the popup and made it overlay the URL bar. That's bad. The change to the selector fixes this. If we could make a similar change to the selector in less expensive fashion, please let me know, but I don't know of one.

Another alternative could be not running the overflowable toolbar constructor for a window that's got toolbar=no, but I'm afraid that that may cause other problems, like not having it be run even if toolbars are shown at a later point.

Comment 6

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > Can you please get that "bustage fix" reviewed, since it makes the selector
> > more expensive and is far from obvious? Why is
> > #main-window:not([chromehidden~="toolbar"]) needed now when it wasn't needed
> > before?
> 
> It was discussed with Jared on IRC at the time. The simple explanation is:
> before, the button defaulted to visible, except on an overflowable toolbar
> that was not overflowing. That meant it was hidden on popup windows opened
> with toolbar=no.

So in popups, the nav bar was overflowable but didn't overflow...

> In the current incarnation, it defaults to invisible,
> except on overflowable toolbars that do overflow. That meant it showed up on
> popup windows with toolbar=no.

And now it is overflowable and does overflow? Why?

Comment 7

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

(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > (In reply to Dão Gottwald [:dao] from comment #4)
> > > Can you please get that "bustage fix" reviewed, since it makes the selector
> > > more expensive and is far from obvious? Why is
> > > #main-window:not([chromehidden~="toolbar"]) needed now when it wasn't needed
> > > before?
> > 
> > It was discussed with Jared on IRC at the time. The simple explanation is:
> > before, the button defaulted to visible, except on an overflowable toolbar
> > that was not overflowing. That meant it was hidden on popup windows opened
> > with toolbar=no.
> 
> So in popups, the nav bar was overflowable but didn't overflow...

Good question! I went back and checked.

So: No, I'm sorry, my explanation was incomplete.

Obviously, the button defaulted to visible because nothing hid it by default. So it would show unless something else made it invisible.

Which was *either* the toolbar not overflowing (see the patch on this bug, which changed that selector, so you can see how it worked before), *or* our generic chromehidden selector for chromeclass-toolbar-additional (xul.css line 50 on my win7 machine, according to DOMI) which hides all additional toolbar stuff.

Right now, the display: -moz-box selector has higher specificity than the display: none selector for the chromehidden case, so I adjusted the display: -moz-box selector to take the chromehidden case into account. If there's some more obvious/other way to do that, I'd be glad to hear it. I can also add a comment to explain why this fix/hack is there, if you like - but I'll take suggestions for the wording. :-)

The overflowable toolbar is and was overflowing both before and after this patch. I could be convinced that that's something we should fix for popup windows with toolbar=no, but that's definitely a separate bug.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 793530 [details] [diff] [review]
Hide the overflow button by default

You shouldn't set display:-moz-box at all and only set display:none in the cases where you want it by reversing the logic of this selector:

>+toolbar[overflowable][overflowing]:not([customizing]) > .overflow-button {

Comment 9

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

Attachment: followup: improve CSS logic used to hide the button,

Like this?

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 793953 [details] [diff] [review]
followup: improve CSS logic used to hide the button,

To exactly mirror the selector, you'd have to add toolbar:not([overflowable]) > .overflow-button. Whether it's actually needed, I don't know. Depends on whether we can and up with [overflowing] without [overflowable].

Comment 11

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

(In reply to Dão Gottwald [:dao] from comment #10)
> Depends on whether we can and up with [overflowing]
> without [overflowable].

Not through the current code. It would only be set from the overflowable toolbar code in CustomizableUI, and that cleans up after itself (including removing "overflowing" if it was) if it ever stops being an overflowable toolbar - which AFAIK currently never happens except when the window closes.

I've tested this patch locally (including on popup windows) and have landed it:

https://hg.mozilla.org/projects/ux/rev/5b0ab793bc7c

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/2512d523d5fc
https://hg.mozilla.org/mozilla-central/rev/8349a50ded27
https://hg.mozilla.org/mozilla-central/rev/5b0ab793bc7c