1230638 - move pocket toolbar icon to pocket addon

Status: RESOLVED FIXED

(Firefox :: Pocket, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Waiting to land bug 1215694 before handling this to avoid potential bitrot on the image patch.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: pocketicon

copies pocket icon to addon.
- images created by copying originals and croping to only have pocket icon
- duplicated chrome.manifest overrides to maintain skin appearance
- have not visually inspected all variants (I just don't have them)
- not removing pocket icon from browser skin yet (will followup with 2nd patch/bug)

Comment 2

[:Gijs (he/him)]

Comment on attachment 8707228 [details] [diff] [review]
pocketicon

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

::: browser/extensions/pocket/jar.mn
@@ +17,5 @@
> +% override chrome://pocket/skin/[e-mail]          chrome://pocket/skin/[e-mail]         os=WINNT osversion=6.1
> +% override chrome://pocket/skin/menuPanel-small.png       chrome://pocket/skin/menuPanel-small-aero.png      os=WINNT osversion=6
> +% override chrome://pocket/skin/menuPanel-small.png       chrome://pocket/skin/menuPanel-small-aero.png      os=WINNT osversion=6.1
> +% override chrome://pocket/skin/[e-mail]    chrome://pocket/skin/[e-mail]   os=WINNT osversion=6
> +% override chrome://pocket/skin/[e-mail]    chrome://pocket/skin/[e-mail]   os=WINNT osversion=6.1

This looks like copy-pasta soup - you don't need/use menuPanel-small.png, right?

Conversely, you added lunaSilver.png, but there seems to be no override for it.

::: browser/extensions/pocket/skin/osx/pocket.css
@@ +34,5 @@
>  }
>  
>  @media (min-resolution: 1.1dppx) {
>    #pocket-button:hover:active:not([disabled="true"]):not([cui-areatype="menu-panel"]) {
> +    -moz-image-region: rect(36px, 18px, 72px, 0);

This is wrong, isn't it? Should be 2x the values in the previous selector.

::: browser/extensions/pocket/skin/shared/pocket.css
@@ +14,5 @@
>  }
>  
>  #pocket-button {
> +  list-style-image: url("chrome://pocket/skin/Toolbar.png");
> +    -moz-image-region: rect(0, 18px, 18px, 0);

Nit: indentation, here and elsewhere

Comment 3

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #2)

> Conversely, you added lunaSilver.png, but there seems to be no override for
> it.

it's added via css (but I still missed that)

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: MozReview Request: Bug 1230638 move pocket toolbar/menu icons into addon, r?gijs

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

Comment 5

[:Gijs (he/him)]

Comment on attachment 8708027 [details]
MozReview Request: Bug 1230638 move pocket toolbar/menu icons into addon, r?gijs

https://reviewboard.mozilla.org/r/30945/#review27789

So, I'm sorry for not realizing this the first time, but... can you crop the images to use 16x16 and 32x32px for Toolbar.png, and use those coordinate multipliers? Because the icon is clearly being rendered inside 16px, and there's 1px of empty space on all sides, but because we're sizing the 18x18 pixels to 16x16, it looks squashed.

Finally, can you use a tool like ImageOptim.app ? On these files, the savings are on average some 69% (!).

::: browser/extensions/pocket/skin/shared/pocket.css:23
(Diff revision 1)
> +  -moz-image-region: rect(0, 18px, 18px, 0);

You shouldn't need to re-set the moz-image-region here, right?

Comment 6

[Shane Caraveo (:mixedpuppy)]

I thought these were all supposed to be 18px.  The are cropped out of the existing files at the same size, and toolbarbuttons.inc.css uses 18px offsets for everything.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: pocketiconsize.png

also, it does seem the icons are smaller than they are supposed to be, I'm not clear why, was going to look into that.

Comment 8

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> Created attachment 8708124 [details]
> pocketiconsize.png
> 
> also, it does seem the icons are smaller than they are supposed to be, I'm
> not clear why, was going to look into that.

It's late here, but see comments on bug 1233701. If that's not clear, I can clarify more in the morning.

Comment 9

[Shane Caraveo (:mixedpuppy)]

While normal are 16px with padding in the image, inverted icons are actually 18px.  Does this also affect @2x icons?

Comment 10

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> While normal are 16px with padding in the image, inverted icons are actually
> 18px.  Does this also affect @2x icons?

Yes.

If the inverted icons are too big, then the other option is to override the styles for #pocket-button, but that's going to be "interesting". That or get the inverted icons resized by UX. :-(

Comment 11

[Shane Caraveo (:mixedpuppy)]

Simply adding the css below fixes the problem so far as I can see.  Works in toolbar, overflow, doesn't affect palette or menu.

#pocket-button[cui-areatype="toolbar"] > .toolbarbutton-icon {
  max-width: 18px;
  margin: 0;
}

Is there something I'm missing?

Comment 12

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> Simply adding the css below fixes the problem so far as I can see.  Works in
> toolbar, overflow, doesn't affect palette or menu.
> 
> #pocket-button[cui-areatype="toolbar"] > .toolbarbutton-icon {
>   max-width: 18px;
>   margin: 0;
> }
> 
> Is there something I'm missing?

Did you test cross-platform?

Off the top of my head, I think the padding will still be off with this CSS.

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #12)

> > Is there something I'm missing?
> 
> Did you test cross-platform?
> 
> Off the top of my head, I think the padding will still be off with this CSS.

tested only on osx at this point, padding is fine.  I was just wondering if I'm missing some bit of complexity based on your "interesting" comment 10.

Comment 14

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> 
> > > Is there something I'm missing?
> > 
> > Did you test cross-platform?
> > 
> > Off the top of my head, I think the padding will still be off with this CSS.
> 
> tested only on osx at this point, padding is fine.

It's Windows and Linux where the max-width includes a sizable amount of padding so that you can click "outside" of the border of the button and still hit it. That is where this will break. See e.g. https://dxr.mozilla.org/mozilla-central/rev/b67316254602a63bf4e568198a5c7d3288a9db27/browser/themes/linux/browser.css#605-609 .

>  I was just wondering if
> I'm missing some bit of complexity based on your "interesting" comment 10.

Making sure that this works correctly in all of the OSes and devedition, and irrespective of the button's placement etc. is tricky. Hard to verify when writing the patch, hard to review.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8708027 [details]
MozReview Request: Bug 1230638 move pocket toolbar/menu icons into addon, r?gijs

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

Comment 16

[Shane Caraveo (:mixedpuppy)]

Verified on win/lin/osx but I don't have the screen for @2x

Comment 17

[:Gijs (he/him)]

Comment on attachment 8708027 [details]
MozReview Request: Bug 1230638 move pocket toolbar/menu icons into addon, r?gijs

https://reviewboard.mozilla.org/r/30945/#review28645

Tested for hidpi on OSX and Windows, looks OK to me.

FWIW, you can test the hidpi case by just setting the layout.css.devPixelsPerPx pref to 2.0 on OSX and something like 1.25 on Windows. :-)

::: browser/extensions/pocket/skin/windows/pocket.css:3
(Diff revisions 1 - 2)
> +#nav-bar #pocket-button > .toolbarbutton-icon,
> +#nav-bar #pocket-button > .toolbarbutton-badge-stack,
> +#nav-bar #pocket-button > .toolbarbutton-menubutton-button > .toolbarbutton-icon {

The first 1 of 3

::: browser/extensions/pocket/skin/windows/pocket.css:9
(Diff revisions 1 - 2)
> +:-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button > .toolbarbutton-icon,
> +:-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button >
> +#pocket-button:-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon {

And the first 2 of 3

::: browser/extensions/pocket/skin/linux/pocket.css:3
(Diff revision 2)
> +#nav-bar #pocket-button > .toolbarbutton-icon,
> +#nav-bar #pocket-button > .toolbarbutton-badge-stack,
> +#nav-bar #pocket-button > .toolbarbutton-menubutton-button > .toolbarbutton-icon {

You only need the first of these three selectors.

::: browser/extensions/pocket/skin/linux/pocket.css:9
(Diff revision 2)
> +:-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button > .toolbarbutton-icon,
> +:-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button >
> +#pocket-button:-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon {

The first 2 of the 3...

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/197bf80d62f8

Comment 19

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/ab602ecfad88 - it's a fair bet that Linux32 debug and ASan are our slowest two platforms, and they said https://treeherder.mozilla.org/logviewer.html#?job_id=6785463&repo=fx-team 2-of-10 times on Linux32 debug and https://treeherder.mozilla.org/logviewer.html#?job_id=6786378&repo=fx-team 8-of-10 times on ASan.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/434c20fe100a

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/434c20fe100a

Comment 22

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> INCOMPLETE

Comments:
STR: 
It is not recommend to mess with user privacy and choice by giving private access to Pocket addon.

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
pocket addon instead of icon.

Actual Results: 
As expected