Closed Bug 1386029 Opened 3 years ago Closed 3 years ago

Pocket icon arrow shifts to the left side

Categories

(Firefox :: Pocket, defect, P1)

56 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.2 - Aug 29
Tracking Status
firefox57 --- fixed

People

(Reporter: amylee, Assigned: jaws)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(3 files)

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
Blocks: 1355922
Whiteboard: [photon-animation][triage]
Flags: qe-verify+
Priority: -- → P3
QA Contact: jwilliams
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
Priority: P3 → P4
Duplicate of this bug: 1391786
Summary: Pocket icon arrow shifts right on highlight state → Pocket icon arrow shifts to the side (left or right) on highlight state in Windows 10 compact dark
Summary: Pocket icon arrow shifts to the side (left or right) on highlight state in Windows 10 compact dark → Pocket icon arrow shifts to the side (left or right) on highlight state
Summary: Pocket icon arrow shifts to the side (left or right) on highlight state → Pocket icon arrow shifts to the left side
Attached image Comparision.png
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.
Flags: needinfo?(jwatt)
Where is the animation for pocket-animation.svg? I see an animation for library-pocket-animation.svg, but not for pocket-animation.svg.
Flags: needinfo?(jwatt) → needinfo?(jaws)
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
Flags: needinfo?(jaws)
Flags: needinfo?(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.
Flags: needinfo?(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.)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
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
(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.
Iteration: --- → 57.2 - Aug 29
Priority: P4 → P1
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.
Attachment #8900515 - Flags: review?(sfoster) → review+
Component: Toolbars and Customization → Pocket
Filed bug 1393421 to clean up the SVG.
Depends on: 1393421
Pushed by jwein@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/39b92731bbdf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
QA Contact: jwilliams → stefan.georgiev
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.