1373299 - Pocket toolbar button icon is still 18x18 pixels

Status: VERIFIED FIXED

(Firefox :: Pocket, enhancement, P1)

Description

[:Gijs (he/him)]

I tried to reuse the pocket SVG icon somewhere in the menus (which it turns out we don't want to do anyway, so never mind) but then I noticed it was still 18x18 pixels. I thought we switched everything to 16x16 pixels - does this still need updating?

I don't know to what degree this has consequences in the toolbar (I don't see any off-hand?), though it might have some more in the overflow panel in terms of the icons looking centered when they're aligned vertically.

There's an icon in the icon folder that I'll attach here for completeness.

Comment 1

[:Gijs (he/him)]

Attachment: pocket-16.svg

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1373299 - Pocket icon should be 16x16 like the others.

Review commit: https://reviewboard.mozilla.org/r/151748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151748/

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Patch was simpler than trying to track down why we originally didn't bother (I remember discussions around pre-processing, pocket button being moved to URL bar anyway, etc).

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8880379 [details]
Bug 1373299 - Pocket icon should be 16x16 like the others.

https://reviewboard.mozilla.org/r/151748/#review156718

::: browser/extensions/pocket/skin/linux/pocket.css:7
(Diff revision 1)
>  
>  #nav-bar #pocket-button > .toolbarbutton-icon {
>  %ifndef MOZ_PHOTON_THEME
>    padding: 2px 6px;
>  %else
> -  padding: calc(var(--toolbarbutton-inner-padding) - 1px);
> +  padding: var(--toolbarbutton-inner-padding);

toolbarbuttons.inc.css sets the same padding, so this shouldn't be needed.

::: browser/extensions/pocket/skin/linux/pocket.css:12
(Diff revision 1)
> -  padding: calc(var(--toolbarbutton-inner-padding) - 1px);
> +  padding: var(--toolbarbutton-inner-padding);
>  %endif
>  }
>  
>  :-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button > .toolbarbutton-icon {
>      max-width: 18px;

What about this?

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8880379 [details]
Bug 1373299 - Pocket icon should be 16x16 like the others.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151748/diff/1-2/

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8880379 [details]
Bug 1373299 - Pocket icon should be 16x16 like the others.

https://reviewboard.mozilla.org/r/151748/#review156728

::: browser/extensions/pocket/skin/linux/pocket.css:5
(Diff revision 2)
>  @import url("chrome://pocket-shared/skin/pocket.css");
>  
>  #nav-bar #pocket-button > .toolbarbutton-icon {
>  %ifndef MOZ_PHOTON_THEME
>    padding: 2px 6px;

And this can go away too?

::: browser/extensions/pocket/skin/linux/pocket.css:11
(Diff revision 2)
>  %endif
>  }
>  
>  :-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button > .toolbarbutton-icon {
> +%ifndef MOZ_PHOTON_THEME
>      max-width: 18px;

The icon size doesn't depend on photon, does it?

::: browser/extensions/pocket/skin/linux/pocket.css:13
(Diff revision 2)
>  
>  :-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button > .toolbarbutton-icon {
> +%ifndef MOZ_PHOTON_THEME
>      max-width: 18px;
> +%else
> +    max-width: 16px;

This shouldn't be needed anymore either.

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8880379 [details]
Bug 1373299 - Pocket icon should be 16x16 like the others.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151748/diff/2-3/

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8880379 [details]
Bug 1373299 - Pocket icon should be 16x16 like the others.

https://reviewboard.mozilla.org/r/151748/#review156728

> The icon size doesn't depend on photon, does it?

Indeed it does not! Clumsy me!

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8880379 [details]
Bug 1373299 - Pocket icon should be 16x16 like the others.

https://reviewboard.mozilla.org/r/151748/#review156738

::: browser/extensions/pocket/skin/linux/pocket.css:6
(Diff revision 3)
> -}
> -
>  :-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button > .toolbarbutton-icon {
> -    max-width: 18px;
> +    max-width: 16px;
>      padding: 0;
>  }

What is this rule still good for? As I understand it, you can just remove it.

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8880379 [details]
Bug 1373299 - Pocket icon should be 16x16 like the others.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151748/diff/3-4/

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8880379 [details]
Bug 1373299 - Pocket icon should be 16x16 like the others.

https://reviewboard.mozilla.org/r/151748/#review156748

::: browser/extensions/pocket/jar.mn:15
(Diff revisions 3 - 4)
>  % skin pocket-shared classic/1.0 %skin/shared/
>    content/  (content/*)
>    skin/shared (skin/shared/*)
>  #ifdef XP_WIN
>    skin/windows/ (skin/windows/*.png)
> -  skin/windows/pocket.css (skin/windows/pocket.css)
> +  skin/windows/pocket.css (skin/shared/pocket.css)

Is there a better way to do this?

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8880379 [details]
Bug 1373299 - Pocket icon should be 16x16 like the others.

https://reviewboard.mozilla.org/r/151748/#review156752

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91950bf37618
Pocket icon should be 16x16 like the others. r=dao

Comment 14

[Mark Banner (:standard8)]

This has busted autoland (Windows & Linux) with issues like:

https://treeherder.mozilla.org/logviewer.html#?job_id=109780034&repo=autoland&lineNumber=17175

ERROR: The following duplicated files are not allowed:
browser/features/[e-mail]/chrome/skin/linux/pocket.css
browser/features/[e-mail]/chrome/skin/shared/pocket.css

Unfortunately there's no sheriffs around to backout at the moment.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for the build bustage:

https://hg.mozilla.org/integration/autoland/rev/659451f14ddd

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Excitingly, it only broke on non-Mac builds.

Comment 17

[Dão Gottwald [:dao]]

pocket.svg is now used in the location bar rather than for a toolbar button but the size is still wrong. Nihanth?

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

This should be fixed by the patch in bug 1387077.

Comment 19

[ovidiu boca[:Ovidiu]]

Attachment: height - pocket icon.png

I tested this on Mac OS X 10.10 with Nightly 57.0a1(2017-09-17) and I have 2 questions:

1. The color from the pocket icon from the attachment is darker than the one from the browser. 
2. I don't have 18x18 px dimensions, I have w= 16px and h= 14px, this are the expected dimensions? Please see the attachments.

Comment 20

[ovidiu boca[:Ovidiu]]

Attachment: width - pocket icon.png

Comment 21

[:Gijs (he/him)]

(In reply to ovidiu boca[:Ovidiu] from comment #19)
> Created attachment 8909279 [details]
> height - pocket icon.png
> 
> I tested this on Mac OS X 10.10 with Nightly 57.0a1(2017-09-17) and I have 2
> questions:
> 
> 1. The color from the pocket icon from the attachment is darker than the one
> from the browser. 
> 2. I don't have 18x18 px dimensions, I have w= 16px and h= 14px, this are
> the expected dimensions? Please see the attachments.

Yep, that's expected, thanks!