1386029 - Pocket icon arrow shifts to the left side

Status: VERIFIED FIXED

(Firefox :: Pocket, defect, P1)

Description

[Amy Lee [:amylee] UX]

When saving the Pocket, the icon arrow shifts slightly to the right in the active pink state. This seems to only occur on Windows 10 Nightly Compact Dark theme. 

See video attachment https://cl.ly/0Q0n070H2r1n

Comment 1

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

Attachment: Screen recording 2017-07-31 at 05.05.45 PM.mp4

Comment 3

[Amy Lee [:amylee] UX]

Attachment: Comparision.png

Comment 4

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

Looking at the SVG file that is being used, chrome://pocket-shared/skin/pocket-animation.svg, there is no horizontal shifting of the arrow within the pocket badge. However we are noticing that it shifts during the animation.

@jwatt, could this be due to a rounding issue in SVG code on HiDPI machines? I can reproduce it 100% on Win10 HiDPI with the compact dark theme.

Comment 5

[Jonathan Watt [:jwatt]]

Where is the animation for pocket-animation.svg? I see an animation for library-pocket-animation.svg, but not for pocket-animation.svg.

Comment 6

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

The SVG is at chrome://pocket-shared/skin/pocket-animation.svg

In the tree it's located at /browser/extensions/pocket/skin/shared/pocket-animation.svg

The CSS for the animation is defined at http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/browser/extensions/pocket/skin/shared/pocket.css#20-27,38-96

Comment 7

[Jonathan Watt [:jwatt]]

Thanks.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Looking at the SVG file that is being used,
> chrome://pocket-shared/skin/pocket-animation.svg, there is no horizontal
> shifting of the arrow within the pocket badge. However we are noticing that
> it shifts during the animation.

That doesn't seem to be correct.

In pocket-animation.svg the path data is the same for each "frame". However, in each frame there is a transform that is applied to the entire icon including the allow. It looks something like this:

  transform="translate(1.749 1.092) scale(1.02438)"

The Y argument that is passed to translate() changes with each subsequent frame taking on the following values:

1.092
1.092
1.86
1.862
1.86
1.817
1.705
1.552
1.387
1.238
1.132
1.092
1.092
1.092

So the change in Y from frame 2 to 3 is 0.77 px (the maximum step change). Say your HiDPI display results in us using 2 screen pixels per CSS px, we'll scale the change to 1.54 screen pixels. It seems like that could account for the noticeable step change between two consecutive frames during the animation.

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #7)
> in each frame there is a transform that is applied to the entire icon
> including the allow. It looks something like this:

Actually the transform is applied *only* to the arrow. (My point remains unchanged though.)

Comment 9

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

Attachment: Bug 1386029 - Change the X translation of the Pocket inner arrow to use a number that works better with DPI scaling so as not to end up with less than half a pixel and shift the icon.

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

Comment 10

[Jonathan Watt [:jwatt]]

BTW pocket-animation.svg has a lot of unnecessary overhead.

 * all the filters are identical - we could just use one

 * there's no need to put the <filter> in a <defs>

 * in fact it looks like the filter is just about coloring - it
   could be replaced by passing in a context-stroke value as a second
   color which would render much faster and with less CPU work than a
   filter

 * all the <svg> (other than the root) and <g> elements are
   unnecessary

 * the 'transform' attributes could be applied to the path data
   statically to avoid having to transform all the paths at runtime
   which is more expensive

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #10)
>  * all the <svg> (other than the root) and <g> elements are
>    unnecessary

To be clear the 'x' attributes on the inner-<svg> elements would need to be propagated into the path data.

Comment 12

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8900515 [details]
Bug 1386029 - Change the X translation of the Pocket inner arrow to use a number that works better with DPI scaling so as not to end up with less than half a pixel and shift the icon.

https://reviewboard.mozilla.org/r/171918/#review177128

Same comments as jwatt on the svg, some of that SVGO should be able to do, though removing the extra svg elements and moving the "x" attribute for each frame down to the right element would need a new SVGO plugin. Regardless, that should have its own bug, changes here LGTM.

Comment 13

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

Filed bug 1393421 to clean up the SVG.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39b92731bbdf
Change the X translation of the Pocket inner arrow to use a number that works better with DPI scaling so as not to end up with less than half a pixel and shift the icon. r=sfoster

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39b92731bbdf

Comment 16

[Stefan [:StefanG_QA]]

User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build   ID    20170905100117

This bug fix is Verified with latest Nightly 57.0a1.