STR: (fresh profile) 1. Click pocket icon in URL bar. EXPECTED RESULTS: Pocket icon is supposed to play a "pulse" animation, getting slightly larger. ACTUAL RESULTS: This animation happens, but the animated icon is slightly shifted to the right with respect to the normal icon. And since they're superimposed, this makes the icon look broken and extra-wide. See attached screenshot, taken at the end of the animation -- the pocket icon is noticeably wider than it should be in this screenshot, because the animation is mis-positioned.

Initial (slightly broad) regression range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a63fec8ffe9176b814817bcb7661aa24e229831c&tochange=56676baf58b7fbd5aba272a9d06596e16ede48d4 Looks like a regression from bug 1389740. (Also worth mentioning: this animation itself was also only reimplemented very recently, in bug 1387077. But it did work correctly after that landed, until the regression range linked above, though.)

I'm guessing this position mismatch is related to the slight icon-position-change that is shown in the before/after screenshot animations in bug 1389740 comment 16 -- e.g. this one: https://screenshots.mattn.ca/comparisons/mozilla-central/e594b178728bc6a51fc81bffcb1d6f0a7071cd33/mozilla-central/3bfcbdf5c6c381d5a8febb5c209e27a69fb89f9b/linux64/controlCenter_03_noLWT_http.png

Also: the regressing csets landed on inbound, so I tried to run mozregression with "--repo inbound" to validate a tighter regression range than comment 1, but the animation doesn't play at all for the builds period around when those csets landed on that repo. So this bug can't be effectively regression-tested on inbound. I think the Pocket-animation-reimplementation (bug 1387077) landed on Autoland (with correct icon placement) at around the same time that bug 1389740 landed on inbound -- and bug 1389740 happens to impact the icon placement of the animated icon but not the un-animated icon (or vice versa) so it breaks the placement. Anyway -- that makes it easy to see how this regression was introduced, and I'm hoping this is as easy as a position-adjustment somewhere...

Is the bookmarks star animation similarly offset?

It is, yeah! The star-filling-in animation, as well as the "already bookmarked" filled-in-star static image, are *slightly to the right* of the unbookmarked not-filled-in-star static image.

I am using 57.0a1 (2017-08-14) (64-bit). Tested two different linux machines (one running Ubuntu 17.04, one Ubuntu 17.10 prerelease -- both with gnome-shell AKA gnome3 as the desktop session, if it matters)

Using the star icon animation, I was able to narrow this to the following single commit: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=76ba8c64110f81ba31a295c7adec76abcf0f3279&tochange=26e09dd7734bea1683d1d2b747220494f05908a8 > 26e09dd7734b Dão Gottwald — Bug 1389740 - Replace urlbar-icon margin with padding. r=gijs

Comment on attachment 8897365 [details] Bug 1390182 - Update horizontal position of the star and pocket animations. https://reviewboard.mozilla.org/r/168678/#review174116 r=me if you make the following change to 4px. ::: browser/extensions/pocket/skin/shared/pocket.css:68 (Diff revision 2) > max-height: 16px; > } > > #pocket-button-box > #pocket-animatable-box { > - /* Match the 6px margin-inline-start of .urlbar-icon plus 1px for internal padding in the animation sprite. */ > - margin-inline-start: 7px; > + /* Add 1px for internal padding in the animation sprite. */ > + margin-inline-start: 1px; In testing this patch I had to change this to 4px. It still needs to match the margin of the adjacent .urlbar-icon, and it has what appears to be 2px of internal padding in the non-enlarged state.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12) > Comment on attachment 8897365 [details] > Bug 1390182 - Update horizontal position of the star and pocket animations. > > https://reviewboard.mozilla.org/r/168678/#review174116 > > r=me if you make the following change to 4px. > > ::: browser/extensions/pocket/skin/shared/pocket.css:68 > (Diff revision 2) > > max-height: 16px; > > } > > > > #pocket-button-box > #pocket-animatable-box { > > - /* Match the 6px margin-inline-start of .urlbar-icon plus 1px for internal padding in the animation sprite. */ > > - margin-inline-start: 7px; > > + /* Add 1px for internal padding in the animation sprite. */ > > + margin-inline-start: 1px; > > In testing this patch I had to change this to 4px. It still needs to match > the margin of the adjacent .urlbar-icon, .urlbar-icon shouldn't have margin anymore. > and it has what appears to be 2px > of internal padding in the non-enlarged state. I don't understand what this means...

(In reply to Dão Gottwald [::dao] from comment #13) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12) > > In testing this patch I had to change this to 4px. It still needs to match > > the margin of the adjacent .urlbar-icon, > > .urlbar-icon shouldn't have margin anymore. Sorry, I should have said that it needs to match the padding of the adjacent .urlbar-icon. > > and it has what appears to be 2px > > of internal padding in the non-enlarged state. > > I don't understand what this means... The animation goes from the normal pocket icon, to growing four pixels wider (two on each horizontal size). So setting the margin-inline-start to 4px is correct, because 4px + the 2px that are internal to the start state of the animation equals the 6px of padding-inline-start on the adjacent .urlbar-icon.

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bb8c8f5b23d4 Update horizontal position of the star and pocket animations. r=jaws

#pocket-button-box > #pocket-animatable-box { - /* Match the 6px margin-inline-start of .urlbar-icon plus 1px for internal padding in the animation sprite. */ - margin-inline-start: 7px; + /* .urlbar-icon has width 28px. Each frame is 20px wide. Set margin-inline-start + to be half the difference, 4px. */ + margin-inline-start: 4px; } This makes sense. I don't know why I didn't think of this, but it's good that it matches the same approach as was used for the star-button.

Verified FIXED in linux Nightly 57.0a1 (2017-08-17) (64-bit) -- tested star and pocket icon, neither move anymore.

(In reply to Daniel Holbert [:dholbert] (mostly AFK Nov 15-26) from comment #21) > Verified FIXED in linux Nightly 57.0a1 (2017-08-17) (64-bit) -- tested star > and pocket icon, neither move anymore. One wrinkle -- I just noticed that the *bottom edge* of the star icon does move a bit (it's ~2px lower for the filled-in star as compared to the unfilled star). This happens even in the Nightly that I mentioned in this^^ comment. I filed bug 1417192 on that remaining (subtle) issue.