Closed Bug 962620 Opened 6 years ago Closed 6 years ago

Don't set the font size for buttons in the menu panel to what it would be anyway with no font-size specified

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: dao, Assigned: dao)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Attached patch patchSplinter Review
font-size:1rem seems like a no-op here.
Attachment #8363703 - Flags: review?(gijskruitbosch+bugs)
(In reply to Dão Gottwald [:dao] from comment #0)
> Created attachment 8363703 [details] [diff] [review]
> patch
> 
> font-size:1rem seems like a no-op here.

This doesn't make sense to me. rem is supposed to refer to the font size of the root element; if other code sets the font size for anything in the parent chain of the label (including the label itself), this isn't a no-op.
(In reply to :Gijs Kruitbosch from comment #1)
> if other code sets the font size for anything in the
> parent chain of the label (including the label itself)

First of all, there's no code actually doing this. Secondly, if one did set the font size on one of the toolbar buttons' containers, you would actually expect this to affect the buttons, just like with toolbar buttons and other widgets elsewhere in the UI.
Comment on attachment 8363703 [details] [diff] [review]
patch

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

(In reply to Dão Gottwald [:dao] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > if other code sets the font size for anything in the
> > parent chain of the label (including the label itself)
> 
> Secondly, if one did set
> the font size on one of the toolbar buttons' containers, you would actually
> expect this to affect the buttons, just like with toolbar buttons and other
> widgets elsewhere in the UI.

I disagree, but I don't actually care right now. If you think this is better and tested this with a reasonable variety of stuff, land away.
Attachment #8363703 - Flags: review?(gijskruitbosch+bugs) → review+
The behavior of toolbar buttons and other widgets in other context is a fact. It's just not common that we set font-size:1rem for such things. There's also no reason why toolbar buttons in this panel should be different. So it's not clear to me what exactly you disagree with.
(In reply to Dão Gottwald [:dao] from comment #4)
> The behavior of toolbar buttons and other widgets in other context is a
> fact. It's just not common that we set font-size:1rem for such things.
> There's also no reason why toolbar buttons in this panel should be
> different. So it's not clear to me what exactly you disagree with.

The problem is, lots of add-ons currently make assumptions about where their buttons/items are shown and what the appropriate styling is meant to be. The results are often ugly. Ergo, not styling because the default styling is OK for us, and explicitly specifying that styling, are not equivalent, and other styles about such items (e.g. toolbar buttons) aren't necessarily 'more right' than the style we picked for these items. Several of these selectors are so low-specificity that they would be unlikely to override add-on styling, however, so I don't know how likely it is that this will be helpful in practice.

I just remembered: did you check what happens to the bookmarks toolbar button, both in and out of customize mode? IIRC it has its own font styles set (bookmarks themselves certainly do). Maybe it's fine, but it'd be good to check before landing this.
https://hg.mozilla.org/mozilla-central/rev/d607ee927c69
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.