882910 - toolbarbutton's icon doesn't have a max height

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Attachment: Addon icon 32x32 added as toolbarbutton icon

This is partially related to bug Bug 879163.

In Jetpack we want to provide an API where the add-on developer can set a different set of icons for the add-on (both for navbar and panel area, plus the high density display); but we want to leave also the capability to set just an icon where the icon is scaled (e.g. set just 16x16 pixel icon will result in a "pixelate" icon on panel).

Unfortunately, xul:image on toolbarbutton doesn't have a max height, it means that setting a 32x32 image as icon will results in 16x32 icon, where the icon will exceeds the edge of the button – see the attachment.

At the moment I'm registering a stylesheet at runtime, but it should be fixed properly on Firefox side.

Comment 1

[Justin Dolske [:Dolske]]

P2 given a hacky workaround, I guess.

Comment 2

[:Gijs (he/him)]

I don't think we should fix this on the browser side, mostly because after the failed attempt to provide a fallback icon, and the reduction in fallback rules etc. done in bug 875488, I don't think we should be adding extra rules "just for safety".

After talking with ZER0, it seems the bug shown in the screenshot is because the button's width was constrained, not the button's icon's width. Given the following alternative sample code:


let b = document.createElement("toolbarbutton");
// On OS X, this is a retina image that is 32x32px:
b.setAttribute("style", "list-style-image: url(chrome://browser/skin/tabbrowser/[e-mail])")

document.getElementById("nav-bar").appendChild(b);

// Now the image is too big

document.getAnonymousElementByAttribute(b, "class", "toolbarbutton-icon").setAttribute("style", "width: 16px")

// Now the image is correctly sized; if that was deemed better, one could also use 'height' rather than 'width' in the style attribute's value.


I don't understand why this isn't an adequate solution for the SDK, and why it'd (therefore) be necessary for browser CSS to have additional "preventative" styles.

Comment 3

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

A solution like that is looking an hack – like, in fact, it is – and I honestly don't think should be adopted in the SDK or add-on code.

The problem with this solution is that SDK is not "agnostic" anymore about concrete implementation of the button, and we rely on the fact that the height (or the width) of a button in the toolbar is, let's say, 16px. But is not: in fact, if I remember now the icons in Australis have a different size (18px maybe? Stephen told to me): so we hard coding a value relying on that, but if this value is changing – because we change the button or toolbar's height on Firefox or it depends by some Operating System - it would break again, as in fact, it break when the new icons where introduced.

In my opinion is much less hacky implement that on Firefox side, especially because can be implemented just in CSS where in SDK we have to do that in JavaScript, plus in that way the SDK is agnostic about that size, and if it will be change we don't risk any breaking.

Comment 4

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

I will post some other concrete use case once I will have chat with UX and I will have a better idea myself about how the different widths for button have to work.

Comment 5

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #3)
> A solution like that is looking an hack – like, in fact, it is – and I
> honestly don't think should be adopted in the SDK or add-on code.

We use the same technique to restrict all the normal icons in retina mode - we just use CSS to set the toolbar's icon's sizes, e.g.:

http://hg.mozilla.org/projects/ux/file/1ba44a63743d/browser/themes/osx/browser.css#l146 (which happens to be a 13x13px icon)

I don't understand why you think that's "hacky".

I was assuming the actual value was dynamic (as the SDK can get any kind of icon, right?) and essentially half of the image's actual height/width, to get the right size for retina displays, irrespective of what size the add-on author has picked. Certainly the description of this bug in comment #0 is all about retina, and this bug is filed as being specific to OS X.

> The problem with this solution is that SDK is not "agnostic" anymore about
> concrete implementation of the button, and we rely on the fact that the
> height (or the width) of a button in the toolbar is, let's say, 16px.

These are two separate issues. The toolbarbutton-icon class as part of the toolbar button implementation has, to my knowledge, been around for a very very very very very long time (I suspect at least since hg@1, but likely significantly longer).

The actual size, as I already said, I assumed is dependent on the author's provided icon.

> But is
> not: in fact, if I remember now the icons in Australis have a different size
> (18px maybe? Stephen told to me): so we hard coding a value relying on that,
> but if this value is changing – because we change the button or toolbar's
> height on Firefox or it depends by some Operating System - it would break
> again, as in fact, it break when the new icons where introduced.

See above. I picked 16px here because the image, as noted, is 32x32 (as is your example in comment #0 !), so to get it to display well on retina, the height/width would have to be 16px. Anything other than that and it'll look bad. In fact, we ourselves can't know what the right size is - indeed, right now it should be 18px, but if the author supplies a 32x32 pixel image, sizing it to 16x18 or something is just going to look horrible. It should be done dynamically based on the size of the original icon and respecting its aspect ratio.

> In my opinion is much less hacky implement that on Firefox side, especially
> because can be implemented just in CSS where in SDK we have to do that in
> JavaScript, plus in that way the SDK is agnostic about that size, and if it
> will be change we don't risk any breaking.

As noted, if this depends on add-on-supplied icons, we should be using their sizes to have the toolbar buttons' icons be the right size, too.

Generally, I think this bug is conflating two issues. You filed it originally trying to fix icons on retina, and now it seems you also want to have a preventative CSS rule so add-on authors can't accidentally (or on purpose) have icons which are too big.

I don't think these are the same problem, or that they should be fixed the same way. I do think the retina issue should be fixed on the SDK side, by setting the icon width/height in the same way that we're doing it for browser icons. We can't do that in the browser code because we don't know what size the icons you're getting will be.

I also think that, if you then Math.max()'d whatever value you got dynamically together with some sensible limit, you could prevent SDK add-ons presenting overly large icons from creating problems. Therefore, that seems like the most sensible solution to me here.

Comment 6

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #4)
> I will post some other concrete use case once I will have chat with UX and I
> will have a better idea myself about how the different widths for button
> have to work.

Matteo, did you have this conversation and/or (given comment #5), would it be OK not to do anything here on the browser side?

Comment 7

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

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

> Matteo, did you have this conversation and/or (given comment #5), would it
> be OK not to do anything here on the browser side?

So, at the moment during the devtools work week we decided with UX to do not allow non-square button, at least in the 1st iteration. Therefore we can ignore this bug, and work on it again if/when we decide to allow non-square button again.

Comment 8

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> 
> > Matteo, did you have this conversation and/or (given comment #5), would it
> > be OK not to do anything here on the browser side?
> 
> So, at the moment during the devtools work week we decided with UX to do not
> allow non-square button, at least in the 1st iteration. Therefore we can
> ignore this bug, and work on it again if/when we decide to allow non-square
> button again.

Great. Minusing for Australis so we stop tracking it for the initial release, then. :-)

Comment 9

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

Unfortunately this issue stab in the back again! :(

It's a major issue on HiDPI display, and I didn't find a workaround that it works, see comment: https://bugzilla.mozilla.org/show_bug.cgi?id=961613#c1

I honestly think it has to be fixed on CSS side, with or without a sdk specific class.
I can also write that patch, if you want!

Comment 10

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #9)
> It's a major issue on HiDPI display, and I didn't find a workaround that it
> works, see comment: https://bugzilla.mozilla.org/show_bug.cgi?id=961613#c1

Ugh. Is there a bug tracking the SDK having its own style sheets? That would be the sensible thing to do here.

> I honestly think it has to be fixed on CSS side, with or without a sdk
> specific class.
> I can also write that patch, if you want!

Please do. I'll be happy to review.

It really should be SDK-specific though, see: http://mxr.mozilla.org/mozilla-central/search?string=sdkstylewidget .

Comment 11

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

Attachment: Add max-height for SDK button

Not sure about the selectors, probably the `:not` one could be avoided just using 32px for `toolbarbutton[sdk-button] > .toolbarbutton-icon` and 18px for `toolbarbutton[sdk-button][cui-areatype="toolbar"] > .toolbarbutton-icon` that has more specificity?
I also check only for the presence of the attribute but not the value, as it's done in other part of the same CSS, because we could use in the near future this attribute to style sdk button with badge, if any similar problem arise (maybe in that case the attribute could be name `sdk-button-type`?)

Comment 12

[:Gijs (he/him)]

Comment on attachment 8363592 [details] [diff] [review]
Add max-height for SDK button

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

r=me with these adjustments

::: browser/base/content/browser.css
@@ +248,2 @@
>    display: none;
>  }

Please put both of these rules after the socialmarks badged toolbarbutton rules: 

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#740

@@ +248,4 @@
>    display: none;
>  }
>  
> +toolbarbutton[sdk-button][cui-areatype="toolbar"] > .toolbarbutton-icon {

I'd prefer a boolean, sdk-button="true". The badge icon isn't going to be inside the .toolbarbutton-icon, so that shouldn't cause any problems. Badge type buttons would likely need their own XBL binding in order to even insert the extra badge icon, so I don't think we should worry about that now.

@@ +251,5 @@
> +toolbarbutton[sdk-button][cui-areatype="toolbar"] > .toolbarbutton-icon {
> +  max-height: 18px;
> +}
> +
> +toolbarbutton[sdk-button]:not([cui-areatype="toolbar"]) > .toolbarbutton-icon {

Put this rule first and lose the :not.

Comment 13

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

Attachment: Add max-height for SDK button

Comment 14

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

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1359

Comment 15

[:Gijs (he/him)]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9dc09dfe412b

Comment 17

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a6e844dedd7035bc9d0454fe6859bb131bca3236
Bug 882910 - toolbarbutton's icon doesn't have a max height

- Added `sdk-button` attribute to buttons created by SDK, in order to apply the style properly

https://github.com/mozilla/addon-sdk/commit/d3a2a23169a35cb9907e013874fc6e73b5f6b477
Merge pull request #1359 from ZER0/max-height/882910

Bug 882910 - toolbarbutton's icon doesn't have a max height

Comment 18

[Sören Hentzschel]

Hi,

is this really fixed?

I did a 'git pull' and verified that I have the latest version (the line "node.setAttribute('sdk-button', 'true');" is in the file "lib/sdk/ui/button/view.js" of my local version of the sdk). Then I tested 'cfx run' and 'cfx xpi'. But the icon is still too big on my HiDPI display. I opened the browser toolbox (great tool!) and there is no "sdk-button" attribute on my icon. With this attribute the icon looks great, but the attribute is missing.

Comment 19

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

(In reply to Sören Hentzschel from comment #18)
> I did a 'git pull' and verified that I have the latest version (the line
> "node.setAttribute('sdk-button', 'true');" is in the file
> "lib/sdk/ui/button/view.js" of my local version of the sdk). Then I tested
> 'cfx run' and 'cfx xpi'.

Did you use the option `-o` to override the browser's module? Otherwise it will use the add-on sdk shipped in the browser, and they aren't there yet.

Comment 20

[Sören Hentzschel]

It works with the -o option, thank you! :)