967115 - Fix SDK toolbarbutton icon styling to not mess with the other buttons

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[:Gijs (he/him)]

Because this is not OK: http://i.imgur.com/kIz1umP.png

The current max-height is 16px. Buttons should be square. The only issue the SDK had, as far as I could tell, was hidpi icons. Logically, it follows we should be OK fixing the width to 16px on OSX hidpi, and leaving everything else alone.

Matteo, does that sound like a plan, or have I missed something?

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

I think you've missed the point of bug 882910, that was the original bug, and bug 948582.

In addition, I feel like assuming only OS X will have retina display could be dangerous; Windows 8.1 supports HiDPI screen as well, AFAIK.

Comment 2

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #1)
> I think you've missed the point of bug 882910, that was the original bug,
> and bug 948582.
> 
> In addition, I feel like assuming only OS X will have retina display could
> be dangerous; Windows 8.1 supports HiDPI screen as well, AFAIK.

Just telling me "you've missed the point" doesn't really help. Why does what I suggest not work?

At the moment, we don't do anything special for Windows 8.1 HiDPI, icon or size-wise. I hope the SDK doesn't either. Either way, outside of the scope of this bug.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

I just linked the bugs with the whole discussion instead of reporting entirely here, I would suggest to read them – it will be also ensure that I do not miss any points in copying.

In short, you're suggesting to set max-height only for OS X, but that doesn't help because in other OS with HiDPI we'll have the problem explained here: https://bugzilla.mozilla.org/show_bug.cgi?id=951317#c4.
I don't think is outside the scope of this bug, I guess in the CSS for firefox's button you just set the icon depends by the dpi of the screen with media query; so it will work nice both in OS X and Windows 8.1.

If you read the comment above you can understand the issue we have and why max-height was necessary in case of HiDPI display – in short, we set 32x32 or 36x36 icon in the toolbar in case of HiDPI, we calculate the proper size based on the density and the area where the button has to be placed, but if the DOM element doesn't have a constraint in size, it would be applied doubling the icon. If you read the bug it would be probably more clear.

Plus, we have also bug 948582, that I linked above. If you take a look, you will see how set large icon will breaks the UI. That shouldn't happens. Especially because by API the user is allowed to add just one single icon, instead of a set of icons, and therefore that icon should fit all the area:

    ActionButton({
      icon: './addon64x64.png',
      id: 'my-button',
      label: 'My Button'
    });

I we have max-height only on OS X, only on OS X that icon will be scaled properly, where on Linux and Windows will "break" the UI, as described in the bug.

So, to sum up, I don't think is acceptable set max-height only on OS X.

Comment 4

[:Gijs (he/him)]

The discussion is spread over 3 different bugs and revolves around what the bugs claim are 3 different problems, for which you want 1 solution. I don't think it's too much to ask for a summary if I already provided a solution and you don't give any indication of why that's not the right one.

First off, I would propose your API is broken if you let people insert arbitrary content and don't constrain it. Putting a 64x64 icon in the toolbar is a dumb thing to do unless we get 4ddpi screens. We don't (yet).

Second, Windows has many DPI settings available, not just 'normal' and 'hidpi', which is part of why the main browser theme hasn't done anything about them. I would strongly suggest that you don't, either.

Third, setting the width of the icon to 16/18/32/whatever will also affect the height. The difference is that we will set a fixed width instead of a max-width.

We can't use max-height because it causes XUL layout issues.

You still haven't explained why setting width: 16px doesn't work. Did you actually try it with any of the problematic cases?

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

The bugs are related to the same issue; I thought I summarize well in the comment, please let me know what is still unclear.

The API is not broken, this is what devs expected and we tried to match their expectations. The new API was also designed to simplify the porting from google chrome extensions – a lot of devs found hard that – and larger icons are also scaled to fit. Either way, I don't think this is the right place where discuss about that.

About the second point: we do not doing anything "special" between "normal" and "hidpi": but we set the icon based on the density. So even if the density of the display is not "hidpi" it should be fine as well, as soon as there is a constraint on the DOM node. And that's the bug we're trying to fix. 

About the third point: again, doing something only for OS X it's not acceptable for the points already explained – if I wasn't clear, I will try to do be more about them –, it doesn't matter if it's height, or width.
So, if now you are you suggesting to set the `width` (not the `max-width`) to all the Operating System, my only concern is the same I already mentioned to you the first time in IRC: compared to `max-width`, set `width` to 18 will scales the 16x16 icons and makes them ugly. At least, that was the issue I had on OS X.


Did you try to play with `-moz-box-sizing` to see if there was any difference and if it fixes the bug?

Personally, if there is really now other way to fix this issue than have a fix width of 18px, then we can say that 16x16 icons are not longer supported; However because 16x16 were a common icon's size until now, and you still find icons assets with that size, I would like to avoid that. It's not my call in that case however, it's better if jeff or dave gaves their thoughts on that.

But definitely shouldn't be on "OS X" only, because the reason explained above (1. high density display not only on OS X; 2. larger icon as only asset).

Comment 6

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #5)
> So, if now you are you suggesting to set the `width` (not the `max-width`)
> to all the Operating System, my only concern is the same I already mentioned
> to you the first time in IRC: compared to `max-width`, set `width` to 18
> will scales the 16x16 icons and makes them ugly. At least, that was the
> issue I had on OS X.

I think we should set the width to *16* px, as I said in comment #0. This was already the effect that the max-height of 16px was having (well, either that or squashing them to be 18/32/64x16px, which would be worse). As I also said in comment 0. I don't understand why you think this will have a negative effect of any kind.

Comment 7

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #5)
> But definitely shouldn't be on "OS X" only, because the reason explained
> above (1. high density display not only on OS X; 2. larger icon as only
> asset).

As I've repeated several times now, current Firefox doesn't do anything for hidpi on non-OSX. As I've also explained, the situation on Windows is much more complicated than on OSX. Do you actually handle 1.5ddpi? 1.25? How about arbitrary custom values that the user can select from the Windows preferences?

The larger icon as the only asset case, as I've said, that doesn't make sense to me from an API perspective. It also wasn't raised in bug 882910, as far as I can tell. Nor in bug 961613. It came up in bug 948582 but I am not CC'd there and it wasn't involved in the discussion in bug 961613 or bug 882910, where you reopened the discussion by saying that this was only problematic because of hipdi.

I don't understand why the SDK can't require the right icon sizes to be provided, but either way, as far as I can tell the 16px width CSS setting would resolve this issue, too.

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to :Gijs Kruitbosch from comment #6)

> I think we should set the width to *16* px, as I said in comment #0.

Sorry, I think I got confused by the discussion on IRC; the part where you said that the current icon are 18px but are not really 18px.

I think it's not clear to me that part, why the SDK icon will be 16px where the Australis icons are 18px.
What I want to be sure is that the SDK will provides the same look & feel of "native" (sorry for the term) button / icon. So, no smaller icon or button without shadow or whatever; so that also "native" icon could be reused in case.

> This
> was already the effect that the max-height of 16px was having

The max-height currently is not 16px in fact, is 18px:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#757

> I don't understand why you think this will have a
> negative effect of any kind.

I don't understand why the native UI icons and the SDK icons should have a different size and what is the impact to have 16px instead of 18px (and if there is no impact, why the icons Australis are 18px instead of 16x); it's probably clear to you but not to me.

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to :Gijs Kruitbosch from comment #7)

> Do you actually handle 1.5ddpi? 1.25? How
> about arbitrary custom values that the user can select from the Windows
> preferences?

We handled them; of course if the API we're using reports the right value. If Firefox's API says the density display is 1ddpi even if we're in 1.5ddpi of course we do not handle that. But I would say, in that case, is a bug on platform side. If the value is reported properly, we choose the icon of the proper size, from the list provided by the dev.

> The larger icon as the only asset case, as I've said, that doesn't make
> sense to me from an API perspective.

It's hard makes an API that makes everyone happy, I know that. I tried to explain why this was required by specs in first place. So far, is not something we're going to change.

> It also wasn't raised in bug 882910, as
> far as I can tell. Nor in bug 961613. It came up in bug 948582 but I am not
> CC'd there and it wasn't involved in the discussion in bug 961613 or bug
> 882910, where you reopened the discussion by saying that this was only
> problematic because of hipdi.

It wasn't mentioned because was part of the specification from the beginning, probably. It's not something we added after.
 
> I don't understand why the SDK can't require the right icon sizes to be
> provided, but either way, as far as I can tell the 16px width CSS setting
> would resolve this issue, too.

Yes, that's why the max-height fix was made in first place: add a size constraint on the DOM element solve all the issue. So, I'm not against that, I want just be sure that we do that for all the platforms, and I'd like to understand what are the differences in having SDK buttons 16x16 where native UI are 18x18.

Comment 10

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #6)
> 
> > I think we should set the width to *16* px, as I said in comment #0.
> 
> Sorry, I think I got confused by the discussion on IRC; the part where you
> said that the current icon are 18px but are not really 18px.
> 
> I think it's not clear to me that part, why the SDK icon will be 16px where
> the Australis icons are 18px.
> What I want to be sure is that the SDK will provides the same look & feel of
> "native" (sorry for the term) button / icon. So, no smaller icon or button
> without shadow or whatever; so that also "native" icon could be reused in
> case.

http://hg.mozilla.org/mozilla-central/annotate/71740219ddf9/browser/themes/windows/browser.css#l517

We style all the non-primary toolbar buttons with 1px extra padding on all sides (ours have 2px 6px, non-primary ones have 3px 7px), meaning the effective size of things stays the same. As I tried to explain to you on IRC the other day, the effective size of the icon is 16x16; we're only using the extra pixels for dropshadows and stuff. There shouldn't be any obvious difference in perceived size between the primary browser toolbar buttons and those of add-ons.

This is a big help with add-on compatibility because now add-on authors can continue to use 16px icons which is a much more convenient size (and one they mostly already used).

I don't think we should special-case the SDK here as compared to other add-ons.

> > This
> > was already the effect that the max-height of 16px was having
> 
> The max-height currently is not 16px in fact, is 18px:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> css#757

Huh. My mistake; I shouldn't have let that get through review in that state. It should have been 16px from the start.

Comment 11

[:Gijs (he/him)]

So the problem on Windows with both the current situation, and, in fact, also with applying the width: 16px there is that we add a lot of padding. In XUL, padding and border are included in width/height and so you end up being very sad because there's 14px of padding going on, which means your icon is invisible and things go pear-shaped.

I disagree with the requirement for Windows and/or only providing stupidly-large icons, but it seems we're now stuck with it, which means that either the rule we use on Windows should change the box model we use for SDK buttons, or it should take the padding and border into account (and probably have liberal comments both there and in the place where we set the padding, so that we don't forget to update the other site if we change anything).

Comment 12

[Blake Winton (:bwinton) (:☕️)]

To fix the problem I'm seeing when I converted the SnoozeTabs button from a Widget into an ActionButton, it looks like we need to explicitly set the width to 18px when it's in one of the toolbars, otherwise the xul:image.toolbarbutton-icon will have the width of the retina image, but unscaled, so will appear twice as large as it should.

Gijs, why isn't that just picking up the default button styling?
browser/themes/osx/browser.css:1005 says:
  :-moz-any(@primaryToolbarButtons@) > .toolbarbutton-icon,
  :-moz-any(@primaryToolbarButtons@) > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
    width: 18px;
  }
but I can't think of why we would restrict it to only those buttons…

Comment 13

[:Gijs (he/him)]

(In reply to Blake Winton (:bwinton) from comment #12)
> Gijs, why isn't that just picking up the default button styling?
> browser/themes/osx/browser.css:1005 says:
>   :-moz-any(@primaryToolbarButtons@) > .toolbarbutton-icon,
>   :-moz-any(@primaryToolbarButtons@) > .toolbarbutton-menubutton-button >
> .toolbarbutton-icon {
>     width: 18px;
>   }
> but I can't think of why we would restrict it to only those buttons…

Because what if add-ons wanted to have a button that was something else than 18px wide? In particular, if they wanted 16px because that's what they've always done?

We expect add-ons to set the right width for retina - after all, most don't even provide a different icon, and won't need to set the width - if they do add retina icons, they can also set the width in CSS.

It is unfortunate that the SDK still doesn't have a way for add-on writers to do CSS easily, nor does it have a dedicated bit of CSS separate from the browser's own CSS files.

Comment 14

[Dave Townsend [:mossop]]

Per IRC Gijs is going to implement the fixed width on all platforms

Comment 15

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #13)
> It is unfortunate that the SDK still doesn't have a way for add-on writers
> to do CSS easily, nor does it have a dedicated bit of CSS separate from the
> browser's own CSS files.

There is of course nothing stopping us creating a SDK specific stylesheet for the UI, I'm not sure what benefit it would serve though.

Comment 16

[:Gijs (he/him)]

(In reply to Dave Townsend (:Mossop) from comment #15)
> (In reply to :Gijs Kruitbosch from comment #13)
> > It is unfortunate that the SDK still doesn't have a way for add-on writers
> > to do CSS easily, nor does it have a dedicated bit of CSS separate from the
> > browser's own CSS files.
> 
> There is of course nothing stopping us creating a SDK specific stylesheet
> for the UI, I'm not sure what benefit it would serve though.

Repository/patch pain. We now need to patch two different places in two different repos in order to get issues fixed. That's a little sadfaces. :-)

Plus, you can iterate faster if you don't need to do separate patches for hg that go "oh, and we need this bit of CSS landed too", plus you have coordination issues with landings and uplift, ...

Comment 17

[:Gijs (he/him)]

Attachment: fix styling of SDK toolbar buttons in Australis,

This seems to fix the issues. I added non-SDK-specific fixes in two places. One, the default margins on OS X were applying to non-menubutton addon buttons in the panel, and two, the padding on Linux didn't match that on Windows. I tested with the whimsy button, both in the panel and the toolbar, and things look good to me.

Comment 18

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

Comment on attachment 8372538 [details] [diff] [review]
fix styling of SDK toolbar buttons in Australis,

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

nice to see the max-width/max-height rules get axed.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8372538 [details] [diff] [review]
fix styling of SDK toolbar buttons in Australis,

remote:   https://hg.mozilla.org/integration/fx-team/rev/aa65dca3dbff

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/aa65dca3dbff

Comment 21

[:Gijs (he/him)]

Comment on attachment 8372538 [details] [diff] [review]
fix styling of SDK toolbar buttons in Australis,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 882910 / Australis
User impact if declined: Windows and Linux users with SDK (new-style, ui-)buttons will have all the buttons in their navbar looking wonky. Users that move it to the panel (on any platform) will have it looking more wonky still.
Testing completed (on m-c, etc.): locally, on m-c
Risk to taking this patch (and alternatives if risky): low, only CSS changes, can't look much more wonky than this...
String or IDL/UUID changes made by this patch: none

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/14190c246d09

Comment 23

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed on Windows 7 64bi, Ubuntu 12.04 32bit, Mac OS X 10.9 and Windows 8.1 64bit (MS Pro 2 - hiDPI):
- Fx 29 beta 8 (build ID: 20140414143035)
- Latest Aurora (build ID: 20140415004003)
- Latest Nightly (build ID: 20140415030203).