Closed
Bug 618096
Opened 14 years ago
Closed 13 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)
6.29 KB,
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•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c0b9c60dcb3e Winstripe: http://hg.mozilla.org/mozilla-central/rev/2b749142bf27
Status: ASSIGNED → RESOLVED
Closed: 13 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
•