Closed Bug 1270495 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: arni2033, Assigned: jaws)

References

Details

Attachments

(1 file, 1 obsolete file)

>>>   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.
Screenshot:  attachment 8744644 [details]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
(the network i'm connected to has blocked port 22 so no mozreview)
Attachment #8749290 - Flags: review?(gijskruitbosch+bugs)
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) ?
Attachment #8749290 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8749290 - Flags: review?(gijskruitbosch+bugs)
(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...
Attached patch Patch v2Splinter Review
Yeah that's true. Okay, I've updated the patch. Thanks for the feedback and ideas.
Attachment #8749290 - Attachment is obsolete: true
Attachment #8749290 - Flags: review?(gijskruitbosch+bugs)
Attachment #8749351 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8749351 [details] [diff] [review]
Patch v2

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

r=me
Attachment #8749351 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/57a2b1d3941f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.