Closed Bug 1230638 Opened 6 years ago Closed 6 years ago

move pocket toolbar icon to pocket addon

Categories

(Firefox :: Pocket, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(2 files, 1 obsolete file)

Waiting to land bug 1215694 before handling this to avoid potential bitrot on the image patch.
Depends on: 1236014
No longer depends on: 1236014
Attached patch pocketicon (obsolete) — Splinter Review
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)
Assignee: nobody → mixedpuppy
Attachment #8707228 - Flags: review?(gijskruitbosch+bugs)
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/menuPanel@2x.png          chrome://pocket/skin/menuPanel-aero@2x.png         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/menuPanel-small@2x.png    chrome://pocket/skin/menuPanel-small-aero@2x.png   os=WINNT osversion=6
> +% override chrome://pocket/skin/menuPanel-small@2x.png    chrome://pocket/skin/menuPanel-small-aero@2x.png   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
Attachment #8707228 - Flags: review?(gijskruitbosch+bugs) → review-
(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)
Attachment #8707228 - Attachment is obsolete: true
Attachment #8708027 - Flags: review?(gijskruitbosch+bugs)
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?
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.
Flags: needinfo?(gijskruitbosch+bugs)
Attached image 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.
(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.
Flags: needinfo?(gijskruitbosch+bugs)
While normal are 16px with padding in the image, inverted icons are actually 18px.  Does this also affect @2x icons?
Flags: needinfo?(gijskruitbosch+bugs)
(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. :-(
Flags: needinfo?(gijskruitbosch+bugs)
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?
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
(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 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/
Attachment #8708027 - Attachment description: MozReview Request: Bug 1230638 move pocket toolbar/menu icons into addon, r?Gijs → MozReview Request: Bug 1230638 move pocket toolbar/menu icons into addon, r?gijs
Attachment #8708027 - Flags: review?(gijskruitbosch+bugs)
Verified on win/lin/osx but I don't have the screen for @2x
Attachment #8708027 - Flags: review?(gijskruitbosch+bugs) → review+
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...
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.
https://hg.mozilla.org/mozilla-central/rev/434c20fe100a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[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
Depends on: 1302744
You need to log in before you can comment on or make changes to this bug.