801055 - When Background Theme applied, Downloads toolbar button is strangely tall.

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

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

Attachment: With background theme applied

STR:

1)  In a Nightly, go to https://www.getpersonas.com/en-US/
2)  Hover your mouse over some themes.

What happens?

The Downloads button gets strangely tall. (Also, for dark background themes, the icon disappears. This is being taken care of in bug 779223).

Seen screenshots.

What's expected?

The button should persist its height, regardless of the background theme.

Comment 1

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

Attachment: No background theme, for reference

Comment 2

[Marco Bonardo [:mak]]

highly visible in primary ui :(

Comment 3

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

Attachment: Patch v1

This was caused by the stack having a min-height of 20px.

This was different from the winstripe / gnomestripe themes, which have a min-height of 18px. There was still a perceptible difference in height with 18px, so I just removed the min-height entirely.

I'm not entirely sure what purpose the min-height served...I did some testing without it, and I'm not seeing any difference (except that the buttons are the same height with and without a background theme enabled). So that's my solution, until further notice.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 671977 [details] [diff] [review]
Patch v1

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

So, 2 things:
- not sure how this acts on lwtheme, unless there is a non-lwtheme rule that overrides the min-height
- if we remove it from here, makes sense to remove it from win and gnome as well

Comment 5

[Marco Bonardo [:mak]]

I suppose you may check with DOMi wich rule is applied here

Comment 6

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

Comment on attachment 671977 [details] [diff] [review]
Patch v1

Ok, I think I've narrowed down the problem some - removing the toolbarbutton-icon class from the downloads-indicator-anchor seems to fix things.

Digging into why exactly that is...

Comment 7

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

Attachment: Patch v2

Ah hah - figured it out. It wasn't a misbehaving rule - it was a missing rule that was being applied to all other default toolbar buttons.

Comment 8

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

(To make reviewing easier, I just added #downloads-indicator to the list)

Comment 9

[Marco Bonardo [:mak]]

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

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

this makes a lot of sense, shame on me for having missed this in the original review

Comment 10

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

(In reply to Marco Bonardo [:mak] from comment #9)
> ... shame on me for having missed this in the
> original review

No worries - omissions are often the hardest defects to detect.

Landed on inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/8bfca415b643

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Simona Badau, Desktop QA]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121113030658

Verified  as fixed on the latest Nightly - the Downloads button does not change  dimension when a background theme is applied.