547419 - Extension's small icons are stretched to 18 pixels

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[u88484]

Attachment: Screenshot

When extensions have code for small icons ( toolbar[iconsize="small"] ) and the the toolbar icons are set to small icons mode, the icons which used to be a standard 16 pixel are now stretched to 18 pixels.  Removing the extensions code for small icons seems to fix this issue by resizing the large icons from 24 pixel to 18 pixels.

See screenshot, the undo closed tabs button icon is actually 24 pixels but has been resized by Firefox to 18 pixels.  When I had the small icon mode by icon was stretched, fuzzy, and looked like the ABP icon does in the screenshot.

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 430035 [details] [diff] [review]
patch

I'd prefer we create a new class for the default palette items instead of having a chain of :nots. Something like 'browser-default' or even 'non-addon' to make its use clear to extension authors.

Comment 4

[Markus Stange [:mstange]]

I'd be in favour of such a class, too.

Comment 5

[Dão Gottwald [:dao]]

This doesn't work for the home button, as its class name is persisted.

Comment 6

[Markus Stange [:mstange]]

Then maybe iconsource="browser-default"? Or change the home button behaviour in a way that doesn't need persisted classes?

Comment 7

[u88484]

Attachment: Comparison screenshot

The button on the left is what I drag and dropped from the toolbars.  The button on the right is the one that my overlay automatically adds to the toolbar.  Compare the two icons and noticed how stretched and fuzzy the icon on the right is even though both are using the same image.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 9

[Mike Kaply [:mkaply]]

This regresses previous small icon behavior. While small icons were suggested to be 16x16, in reality they could be any width.

So a 32x16 icon displayed properly. 

This change forces all icons in a 16x16 box.

Is width and height necessary at all? Won't it use the height of the image (which in most cases is 16x16)

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v3

I never really liked the extra attribute...

This patch uses :-moz-any, which seems better than the chained :not()s from the first patch.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Can you explain what the patch does, and why? Assume I don't understand what causes this bug!

(Is mkaply's concern in comment 9 addressed by the latest patch?)

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> Can you explain what the patch does, and why? Assume I don't understand what
> causes this bug!

The code at the top of the patch causes this bug.

> (Is mkaply's concern in comment 9 addressed by the latest patch?)

No. Comment 9 is only remotely related to this bug. Comment 0 describes pretty well what this bug is about and what the patch addresses.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> (In reply to comment #11)
> > Can you explain what the patch does, and why? Assume I don't understand what
> > causes this bug!
> 
> The code at the top of the patch causes this bug.

This:

 .toolbarbutton-menubutton-button > .toolbarbutton-icon,
 .toolbarbutton-1 > .toolbarbutton-icon {
   -moz-margin-end: 0;
   width: 18px;
   height: 18px;
 }

Comment 14

[Mike Kaply [:mkaply]]

Wouldn't it be possible to only force the built in icons to 18x18 and leave other icons alone?

Why are large icons being constrained to 18x18?

Comment 15

[Dão Gottwald [:dao]]

We don't want arbitrarily sized toolbarbuttons with the new button style, so we enforce the size. Don't use the toolbarbutton-1 class if you don't want the size constraint. (This will drop the new button style as well.)

Comment 16

[Mike Kaply [:mkaply]]

Then please just get rid of large icons completely. Please.

It is stupid to downsize a 24x24 icon to 18x18 for "large" mode when there is a 16x16 icon available.

The large icons in the new theme are simply useless. They just adjust padding and margin.

Comment 17

[Dão Gottwald [:dao]]

You should probably file a new bug titled "get rid of large icons". This one is about small icons.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #13)
> > The code at the top of the patch causes this bug.

That explains the cause, but not what the patch is doing. I can understand overriding the 18/18px rules for toolbar[iconsize="small"] icons, making them 16x16... but why does it then also override that for all the default buttons, making them width/height: auto?

Comment 19

[Dão Gottwald [:dao]]

(In reply to comment #18)
> I can understand
> overriding the 18/18px rules for toolbar[iconsize="small"] icons, making them
> 16x16... but why does it then also override that for all the default buttons,
> making them width/height: auto?

Default icons have a built-in glow, so they are 18x18 (even in small mode) -- except for the large back icon, which is why I'm using auto rather than 18px. This will choose the right size based on the image region.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 462799 [details] [diff] [review]
patch v3

Can you put an explanation like the one in comment 19 as a comment above the -moz-any block?

Should we add a comment in browser.xul making sure that people update this style when adding a new default button?

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> Can you put an explanation like the one in comment 19 as a comment above the
> -moz-any block?

Done locally.

> Should we add a comment in browser.xul making sure that people update this
> style when adding a new default button?

I think we'll want to centralize the id list in browser/themes/shared.inc or so, and add a reference to that in browser.xul. I'll fill a follow-up.

Comment 22

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/39b49a70c729

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #21)
> I think we'll want to centralize the id list in browser/themes/shared.inc or
> so, and add a reference to that in browser.xul.

Filed as bug 585007

Comment 24

[Markus Stange [:mstange]]

Bug 618096 ports this to pinstripe.