Closed
Bug 618096
Opened 14 years ago
Closed 14 years ago
Use 16x16 for extension toolbar icons on Mac
Categories
(Firefox :: Theme, defect)
Tracking
()
RESOLVED
FIXED
Firefox 4.0b9
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: mstange, Assigned: mstange)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Details | Diff | 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 1•14 years ago
|
||
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-
Assignee | ||
Comment 2•14 years ago
|
||
(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.
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #496627 -
Attachment is obsolete: true
Attachment #496814 -
Flags: review?(dao)
Comment 4•14 years ago
|
||
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.
Assignee | ||
Comment 5•14 years ago
|
||
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.
Comment 6•14 years ago
|
||
Was there a good reason for that padding to exist in the first place?
Assignee | ||
Comment 7•14 years ago
|
||
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 8•14 years ago
|
||
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;
> }
Assignee | ||
Comment 9•14 years ago
|
||
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 10•14 years ago
|
||
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+
Assignee | ||
Comment 11•14 years ago
|
||
Requesting approval2.0 for making extension developers' lives easier.
Attachment #497251 -
Flags: approval2.0?
Assignee | ||
Comment 12•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Comment 13•14 years ago
|
||
Not within the next few days...
Comment 15•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c0b9c60dcb3e
Winstripe: http://hg.mozilla.org/mozilla-central/rev/2b749142bf27
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b9
You need to log in
before you can comment on or make changes to this bug.
Description
•