Closed Bug 547419 Opened 10 years ago Closed 9 years ago

Extension's small icons are stretched to 18 pixels

Categories

(Firefox :: Theme, defect)

All
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b4
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: u88484, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

Attached image 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.
Duplicate of this bug: 547725
Blocks: 546098
No longer blocks: 545842
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #430035 - Flags: review?(rflint)
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.
Attachment #430035 - Flags: review?(rflint)
I'd be in favour of such a class, too.
This doesn't work for the home button, as its class name is persisted.
Then maybe iconsource="browser-default"? Or change the home button behaviour in a way that doesn't need persisted classes?
Attached image 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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #442701 - Flags: review?(gavin.sharp)
blocking2.0: --- → ?
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)
Attached patch patch v3Splinter Review
I never really liked the extra attribute...

This patch uses :-moz-any, which seems better than the chained :not()s from the first patch.
Attachment #430035 - Attachment is obsolete: true
Attachment #442701 - Attachment is obsolete: true
Attachment #462799 - Flags: review?(gavin.sharp)
Attachment #442701 - Flags: review?(gavin.sharp)
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?)
(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.
(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;
 }
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?
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.)
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.
You should probably file a new bug titled "get rid of large icons". This one is about small icons.
(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?
(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 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?
Attachment #462799 - Flags: review?(gavin.sharp) → review+
blocking2.0: ? → betaN+
(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.
http://hg.mozilla.org/mozilla-central/rev/39b49a70c729
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
OS: All → Windows XP
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b4
(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
Depends on: 631020
No longer depends on: 631020
You need to log in before you can comment on or make changes to this bug.