Closed Bug 618096 Opened 9 years ago Closed 9 years ago

Use 16x16 for extension toolbar icons on Mac

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set

Tracking

()

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

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
This brings the Mac theme in sync with the Windows theme so extension authors can use the same image for both themes. For more background discussion see bug 616472.
Attachment #496627 - Flags: review?(dao)
Comment on attachment 496627 [details] [diff] [review]
v1

>-%define primaryToolbarButtons ...
>+%define primaryToolbarButtons ..., #alltabs-button, #tabview-button

There's probably code in winstripe that needs an update for this...

> .toolbarbutton-1 > .toolbarbutton-icon,
> .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> #restore-button > .toolbarbutton-icon {
>   padding: 0;
>-  height: 20px;
>-  width: 20px;
>+  height: 16px;
>+  width: 16px;
>+}

This seems wrong for #restore-button.

Does the reduced size need to be compensated with a margin?
Attachment #496627 - Flags: review?(dao) → review-
(In reply to comment #1)
> Comment on attachment 496627 [details] [diff] [review]
> v1
> 
> >-%define primaryToolbarButtons ...
> >+%define primaryToolbarButtons ..., #alltabs-button, #tabview-button
> 
> There's probably code in winstripe that needs an update for this...

Oh, because of the different margin? Hmm. Can you fix that for me?

> > .toolbarbutton-1 > .toolbarbutton-icon,
> > .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> > #restore-button > .toolbarbutton-icon {
> >   padding: 0;
> >-  height: 20px;
> >-  width: 20px;
> >+  height: 16px;
> >+  width: 16px;
> >+}
> 
> This seems wrong for #restore-button.

Indeed.

> Does the reduced size need to be compensated with a margin?

First I thought it wouldn't make a difference since the button size is fixed and the icons are centered, but that's actually not true. They don't have a fixed width, only a min-width, and for example toolbarbutton[type="menu"]s are wider, so there it does make a difference.
Attached patch v2 (obsolete) — Splinter Review
Attachment #496627 - Attachment is obsolete: true
Attachment #496814 - Flags: review?(dao)
Comment on attachment 496814 [details] [diff] [review]
v2

> .toolbarbutton-1 > .toolbarbutton-icon,
> .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> #restore-button > .toolbarbutton-icon {
>   padding: 0;
>-  height: 20px;
>-  width: 20px;
>+  margin: 2px;
>+  height: 16px;
>+  width: 16px;
>+}

Looks like you should remove #restore-button > .toolbarbutton-icon here.
Including it there is the shortest way of applying padding: 0 to it. But I can set the padding separately if you'd prefer that.
Was there a good reason for that padding to exist in the first place?
Attached patch v3 (obsolete) — Splinter Review
Nope. Removing that padding lets us remove a few more padding resets.

Unrelated to the padding issue, I also noticed that the !important for margin:0 necessitated another !important for the margin on the boorkmarks button icon (for the case where it's in the bookmarks toolbar).
Attachment #496814 - Attachment is obsolete: true
Attachment #497236 - Flags: review?(dao)
Attachment #496814 - Flags: review?(dao)
Comment on attachment 497236 [details] [diff] [review]
v3

> .toolbarbutton-1 > .toolbarbutton-icon,

I wonder whether we should just add :not(@primaryToolbarButtons@) here. Any idea whether that would perform differently?

>-.toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>-#restore-button > .toolbarbutton-icon {
>-  padding: 0;
>-  height: 20px;
>-  width: 20px;
>+.toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
>+  margin: 2px;
>+  height: 16px;
>+  width: 16px;
>+}
>+
>+/* Most default icons are 20*20px (even in small mode) due to a built-in glow.
>+   Using 'auto' will pick the correct size based on the image region. */
>+:-moz-any(@primaryToolbarButtons@) > .toolbarbutton-icon {
>+  margin: 0 !important;
>+  width: auto !important;
>+  height: auto !important;
> }
Attached patch v4 (obsolete) — Splinter Review
I don't know, but my gut feeling says that it probably won't make a difference performance-wise. I think we should go for it.
Attachment #497236 - Attachment is obsolete: true
Attachment #497246 - Flags: review?(dao)
Attachment #497236 - Flags: review?(dao)
Comment on attachment 497246 [details] [diff] [review]
v4

>+.toolbarbutton-1:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-icon,
>+.toolbarbutton-1:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-menubutton-button > .toolbarbutton-icon {

The second :not() shouldn't be needed, as there's no built in toolbarbutton-1 with type="menu-button".
Attachment #497246 - Flags: review?(dao) → review+
Attached patch v5 (obsolete) — Splinter Review
Requesting approval2.0 for making extension developers' lives easier.
Attachment #497251 - Flags: approval2.0?
Attached patch for checkinSplinter Review
I think the blocking+ status from bug 616472 can be applied to this patch.
Dão, can you check whether winstripe needs any changes?
Attachment #497246 - Attachment is obsolete: true
Attachment #497251 - Attachment is obsolete: true
Attachment #497251 - Flags: approval2.0?
blocking2.0: --- → ?
Not within the next few days...
As long as bug 616472 blocks, so does this...
blocking2.0: ? → betaN+
http://hg.mozilla.org/mozilla-central/rev/c0b9c60dcb3e

Winstripe: http://hg.mozilla.org/mozilla-central/rev/2b749142bf27
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b9
Blocks: 626382
Duplicate of this bug: 599535
You need to log in before you can comment on or make changes to this bug.