move pocket toolbar icon to pocket addon

RESOLVED FIXED in Firefox 46

Status

()

Firefox
Pocket
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

2 years ago
Depends on: 1236014
(Assignee)

Updated

2 years ago
No longer depends on: 1236014
(Assignee)

Comment 1

2 years ago
Created attachment 8707228 [details] [diff] [review]
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)
Assignee: nobody → mixedpuppy
Attachment #8707228 - Flags: review?(gijskruitbosch+bugs)

Comment 2

2 years ago
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-
(Assignee)

Comment 3

2 years ago
(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)
(Assignee)

Comment 4

2 years ago
Created attachment 8708027 [details]
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/
Attachment #8708027 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

2 years ago
Attachment #8707228 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8708027 - Flags: review?(gijskruitbosch+bugs)

Comment 5

2 years ago
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?
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 7

2 years ago
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.

Comment 8

2 years ago
(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)
(Assignee)

Comment 9

2 years ago
While normal are 16px with padding in the image, inverted icons are actually 18px.  Does this also affect @2x icons?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 10

2 years ago
(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)
(Assignee)

Comment 11

2 years ago
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)

Comment 12

2 years ago
(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)
(Assignee)

Comment 13

2 years ago
(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

2 years ago
(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.
(Assignee)

Comment 15

2 years ago
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)
(Assignee)

Comment 16

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

Updated

2 years ago
Attachment #8708027 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 17

2 years ago
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.

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/434c20fe100a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Comment 22

2 years ago
[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.