Pocket icon arrow shifts to the left side

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: amylee, Assigned: jaws)

Tracking

(Depends on: 1 bug)

56 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-animation], URL)

Attachments

(3 attachments)

(Reporter)

Description

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

Updated

2 years ago
Blocks: 1355922
Whiteboard: [photon-animation][triage]
Flags: qe-verify+
Priority: -- → P3
QA Contact: jwilliams
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
Priority: P3 → P4
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
(Reporter)

Updated

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

Updated

2 years ago
Summary: Pocket icon arrow shifts to the side (left or right) on highlight state → Pocket icon arrow shifts to the left side
(Reporter)

Comment 3

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

2 years ago
mozreview-review
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+

Comment 14

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

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39b92731bbdf
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
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.