Closed Bug 990533 Opened 6 years ago Closed 6 years ago

Home button icon looks stretched when placed in the Bookmarks toolbar

Categories

(Firefox :: Theme, defect)

31 Branch
All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- verified
firefox30 --- verified
firefox31 --- verified

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Keywords: regression, Whiteboard: [Australis:P3+][good first verify])

Attachments

(1 file, 1 obsolete file)

Screenshot follows, see Summary; it's rather straightforward.

The icon of the Home button, when placed in the Bookmarks toolbar is 12x12px.
Currently, it's stretched to 16px.
Attachment #8399924 - Flags: review?(mak77)
Comment on attachment 8399924 [details] [diff] [review]
Patch v1: size Home button icon to 12px

Review of attachment 8399924 [details] [diff] [review]:
-----------------------------------------------------------------

why is this 12x12? this image is only used on the bookmarks toolbar on osx, doesn't make sense for it to be 12px if other icons on the toolbar are 16px...
imo we should either:
1. get new graphic asset from Stephen (or UX in general)
2. just reuse the home graphic from Toolbar.png, like we do on Windows

I'm more prone towards 2

also notice the not here http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#102 (I don't recall what image we use on Linux, please check?)
Attachment #8399924 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #2)
> Comment on attachment 8399924 [details] [diff] [review]
> Patch v1: size Home button icon to 12px
> 
> Review of attachment 8399924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> why is this 12x12? this image is only used on the bookmarks toolbar on osx,
> doesn't make sense for it to be 12px if other icons on the toolbar are
> 16px...
> imo we should either:
> 1. get new graphic asset from Stephen (or UX in general)
> 2. just reuse the home graphic from Toolbar.png, like we do on Windows
> 
> I'm more prone towards 2

Yes, we should use the Home icon from Toolbar.png; no reason to duplicate it.
Alrighty, this looks good to me... how about you?
Attachment #8399924 - Attachment is obsolete: true
Attachment #8400550 - Flags: review?(mak77)
Comment on attachment 8400550 [details] [diff] [review]
Patch v2: use correct toolbar icon for the Home button

Review of attachment 8400550 [details] [diff] [review]:
-----------------------------------------------------------------

r=me provided you try to keep this consistent with the windows theme. That one just shrinks to 16px and thus doesn't seem to need special padding, see comments below)

::: browser/themes/osx/browser.css
@@ +229,5 @@
>    margin-top: 2px;
>    -moz-margin-start: 3px;
>  }
>  
> +.bookmark-item:not(#home-button) > .toolbarbutton-icon,

when I pointed this out, was cause in the previous patch it may have been enough to do this, to avoid having a stretched icon... But now if we now use the 18px icon on the toolbar, the old rule makes more sense as it was.
I'd say to stay consistent with windows theme for now, is resizes to 16x16, so basically just undo this change. We will attack this in future, once we decide to make the bookmarks toolbar look as any other toolbar.

@@ +1510,5 @@
>  #home-button.bookmark-item {
> +  /* We need to use !important here, because other selectors that apply padding
> +     are more specific. */
> +  padding-top: 0 !important;
> +  padding-bottom: 0 !important;

is this still needed after resizing the icon to 16px? doesn't look like other themes are doing this.
Attachment #8400550 - Flags: review?(mak77) → review+
Marco, I think scaling down the image to be 16x16px looks quite horrible: http://note.io/1oxddz2. OSX (icons) is very unforgiving that way.

Care to reconsider NOT accepting the patch as-is?
Flags: needinfo?(mak77)
ugh, I guess it's the specific icon design, it doesn't look that horrible on Windows... ok, let's keep it as-is
Flags: needinfo?(mak77)
Thanks!

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/b4d998e8fb8e
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b4d998e8fb8e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 31
Comment on attachment 8400550 [details] [diff] [review]
Patch v2: use correct toolbar icon for the Home button

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis.
User impact if declined: When the Home button is placed on the Bookmarks toolbar, its current icon will look stretched. Or ugly. This patch changes the icon to the one also shown when placed on the nav-bar, which makes it more consistent and doesn't stretch anymore.
Testing completed (on m-c, etc.): landed on m-c.
Risk to taking this patch (and alternatives if risky): minor.
String or IDL/UUID changes made by this patch: n/a.
Attachment #8400550 - Flags: approval-mozilla-beta?
Attachment #8400550 - Flags: approval-mozilla-aurora?
Attachment #8400550 - Flags: approval-mozilla-beta?
Attachment #8400550 - Flags: approval-mozilla-beta+
Attachment #8400550 - Flags: approval-mozilla-aurora?
Attachment #8400550 - Flags: approval-mozilla-aurora+
Please be very careful when adding a :not() to a rule as it increases the specificity by the selectors inside the negation. In this case it's a huge change to the specificity since there was an ID selector inside. This broke the icon of all special folders.
Whiteboard: [Australis:P3+] → [Australis:P3+][Do not uplift until the regression dependencies are fixed]
Whiteboard: [Australis:P3+][Do not uplift until the regression dependencies are fixed] → [Australis:P3+]
Whiteboard: [Australis:P3+] → [Australis:P3+][good first verify]
I don't experience this in Beta, everything looks great, no stretching or anything wrong with the icon.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.