1270495 - Long buttons placed in overflow panel have increased height. Their sub-buttons have extra indent on top

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160504043118
STR:
1. Move zoom level toolbarbutton  [- | 100% | +]  to the end of toolbar
2. Resize the window so that overflow button appeared in toolbar, and the button from Step 1 was
   the only button in overflow panel
3. Open overflow panel
4. Hover mouse over "100%" sub-button

AR:  The button itself has increased height. 2nd and 3rd sub-buttons have some indent on top
ER:  Either A or A+B
 A) Long buttons in overflow panel should have normal height, without extra indents in sub-buttons
 B) Long buttons themselves should have additional indent above, as was planned in bug 1242271
   (btw I somehow forgot that it originally was my suggestion...)

Note:  It was broken in bug 1242271, but isn't marked as "regression", because
       it's more like one glitch was replaced with another one.

Comment 1

[arni2033]

Screenshot:  attachment 8744644 [details]

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

(the network i'm connected to has blocked port 22 so no mozreview)

Comment 3

[:Gijs (he/him)]

Comment on attachment 8749290 [details] [diff] [review]
Patch

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

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +1067,5 @@
>  }
>  
>  panelview .toolbarbutton-1,
> +.widget-overflow-list > .toolbarbutton-1:not(:first-child),
> +.widget-overflow-list > .toolbaritem-combined-buttons:not(:first-child) {

Shouldn't the second item just be toolbaritem:not(:first-child) ? Does that work (might not, because specificity) ?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8749290 [details] [diff] [review]
Patch

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

We could? But I don't see what that would gain us, besides breaking consistency with the rest of panelUIOverlay.inc.css.

Comment 5

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Comment on attachment 8749290 [details] [diff] [review]
> Patch
> 
> Review of attachment 8749290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We could? But I don't see what that would gain us, besides breaking
> consistency with the rest of panelUIOverlay.inc.css.

Well, it would work for other toolbaritems, notably those created by add-ons. If an add-on does:

<toolbaritem><toolbarbutton class="toolbarbutton-1"/></toolbaritem>

and expects it to get treated like a single toolbaritem in the panel menu and the toolbar, that will work everywhere except here. The toolbarbutton-combined stuff also won't match the search bar...

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Yeah that's true. Okay, I've updated the patch. Thanks for the feedback and ideas.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8749351 [details] [diff] [review]
Patch v2

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

r=me

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/57a2b1d3941f

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57a2b1d3941f