1355922 - Save to Pocket should have an associated animation

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

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

Spec TBD

Comment 1

[Justin Dolske [:Dolske]]

Placing into reserve backlog for now.

Comment 2

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

Attachment: Bug 1355922 - Save to Pocket should have an associated animation.

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

Comment 3

[:Gijs (out for now; he/him)]

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

https://reviewboard.mozilla.org/r/154058/#review159300

::: browser/components/customizableui/CustomizableUI.jsm:1400
(Diff revision 1)
> +        box.classList.add("toolbarbutton-animatable-box");
> +        let image = aDocument.createElement("image");
> +        image.classList.add("toolbarbutton-animatable-image");
> +        box.appendChild(image);
> +        node.appendChild(box);
> +        node.setAttribute("animationsenabled", "true");

I don't really know why, but when I trigger this animation with the library button in the permanent part of the overflow panel, the library button's icon disappears. If I then move it back to the toolbar, the icon is red and doesn't change / go back to its usual self.

::: browser/extensions/pocket/content/main.js:93
(Diff revision 1)
> +
> +        if (_lastAddSucceeded) {

This seems like it's too late. Why don't we do this while the panel is still up? Waiting for the panel to disappear makes it seem like the animation is just late. Maybe we should not show the panel in this case, especially if the save was not triggered using the pocket button itself?

::: browser/extensions/pocket/content/main.js:95
(Diff revision 1)
> +            var libraryButton = document.getElementById("library-button");
> +            if (!libraryButton) {

There's CSS and an image for a pocket button animation, but no code?

::: browser/extensions/pocket/content/main.js:102
(Diff revision 1)
> +            libraryButton.setAttribute("animate", "pocket");
> +            libraryButton.addEventListener("transitionend", function onTransformTransitionEnd(event) {

What happens if the user bookmarks *and* pockets? How do we resolve conflicts between these animations?

::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:5
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="810" height="36">
> +  <svg width="18" height="36" viewBox="0 0 18 36" preserveAspectRatio="xMidYMid meet" x="0" y="0">

What's the preserveaspect-ratio stuff here, do we really need it? It looks like generally, this SVG hasn't gone through whatever we normally put things through, in that it has a bunch of stuff that seems unused (e.g. opacity=1 attributes, fill-opacity=1 attributes, unreferenced paths, etc.)

Separately, at the end of this animation, on OS X, there's a visible 'jump' as we switch back to displaying the normal icon. The final state of the animation and the icon we normally display don't match.

::: browser/extensions/pocket/skin/shared/pocket.css:32
(Diff revision 1)
> +#pocket-button[open="true"][animationsenabled] > .toolbarbutton-icon {
> +  fill: transparent;

Is this just hiding the icon, or something? Why not just use visibility: hidden ? The library button seems to set a transition for this, but I don't see one here.

::: browser/extensions/pocket/skin/shared/pocket.css:85
(Diff revision 1)
> +  width: 18px; /* Width of each frame within the SVG sprite */
> +  height: 36px; /* Height of each frame within the SVG sprite */
> +}
> +
> +#library-button[animate="pocket"] > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image {
> +  height: 36px; /* Height of each frame within the SVG sprite */

Why do we need to set height on both this box and its parent?

Comment 4

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

(In reply to :Gijs from comment #3)
> Comment on attachment 8883104 [details]
> Bug 1355922 - Save to Pocket should have an associated animation.
> 
> https://reviewboard.mozilla.org/r/154058/#review159300
> 
> ::: browser/components/customizableui/CustomizableUI.jsm:1400
> (Diff revision 1)
> > +        box.classList.add("toolbarbutton-animatable-box");
> > +        let image = aDocument.createElement("image");
> > +        image.classList.add("toolbarbutton-animatable-image");
> > +        box.appendChild(image);
> > +        node.appendChild(box);
> > +        node.setAttribute("animationsenabled", "true");
> 
> I don't really know why, but when I trigger this animation with the library
> button in the permanent part of the overflow panel, the library button's
> icon disappears. If I then move it back to the toolbar, the icon is red and
> doesn't change / go back to its usual self.

I imagine this is broken because we don't get the first transitionend event listener in this case since the panel is hidden? I'll have to test this case.

> ::: browser/extensions/pocket/content/main.js:93
> (Diff revision 1)
> > +
> > +        if (_lastAddSucceeded) {
> 
> This seems like it's too late. Why don't we do this while the panel is still
> up? Waiting for the panel to disappear makes it seem like the animation is
> just late. Maybe we should not show the panel in this case, especially if
> the save was not triggered using the pocket button itself?

This is by design of the spec. UX wants the library animation to happen once the panel hides.

> ::: browser/extensions/pocket/content/main.js:95
> (Diff revision 1)
> > +            var libraryButton = document.getElementById("library-button");
> > +            if (!libraryButton) {
> 
> There's CSS and an image for a pocket button animation, but no code?

The pocket button animation is triggered off of [open="true"] being set on the button when the panel opens, so we don't need to add any code for it.

> ::: browser/extensions/pocket/content/main.js:102
> (Diff revision 1)
> > +            libraryButton.setAttribute("animate", "pocket");
> > +            libraryButton.addEventListener("transitionend", function onTransformTransitionEnd(event) {
> 
> What happens if the user bookmarks *and* pockets? How do we resolve
> conflicts between these animations?

Both the bookmark and pocket animation on the library button are triggered when their respective panel closes. We can make the last one that closes cancel the previous animation and begin playing their own animation. The animation implemented in this bug partially handles this already, by using the value of [animate="pocket"] to trigger the pocket one, and then that would switch to [animate="bookmark"] (or [animate="bookmarks"]) when the bookmarks panel closes.

This patch however doesn't handle removing event listeners in the event that there is a switch. I could handle that here or in the bookmarks implementation. I think it will be easier to handle it in the bookmarks implementation since at that point we will know what needs to be cleaned up.

> ::: browser/extensions/pocket/skin/shared/library-pocket-animation.svg:5
> (Diff revision 1)
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +<svg xmlns="http://www.w3.org/2000/svg" width="810" height="36">
> > +  <svg width="18" height="36" viewBox="0 0 18 36" preserveAspectRatio="xMidYMid meet" x="0" y="0">
> 
> What's the preserveaspect-ratio stuff here, do we really need it? It looks
> like generally, this SVG hasn't gone through whatever we normally put things
> through, in that it has a bunch of stuff that seems unused (e.g. opacity=1
> attributes, fill-opacity=1 attributes, unreferenced paths, etc.)

I had cleaned this up using SVGO as well as some manual other changes. The opacity=1 is/was sometimes inside of an opacity=0 block which is why it is/was necessary. I can take another pass through it and see what else can be removed.

> Separately, at the end of this animation, on OS X, there's a visible 'jump'
> as we switch back to displaying the normal icon. The final state of the
> animation and the icon we normally display don't match.

This is a known issue and UX is looking at correcting the sprite. I don't think we should hold up the whole patch for this though.

> ::: browser/extensions/pocket/skin/shared/pocket.css:32
> (Diff revision 1)
> > +#pocket-button[open="true"][animationsenabled] > .toolbarbutton-icon {
> > +  fill: transparent;
> 
> Is this just hiding the icon, or something? Why not just use visibility:
> hidden ? The library button seems to set a transition for this, but I don't
> see one here.

This is to hide the icon but preserve the intrinsic size, which is needed to keep the size of the button uniform. The transition on the library-button is only defined on the .toolbarbutton-animatable-image and is used to transition the fill color on the SVG filmstrip.

> ::: browser/extensions/pocket/skin/shared/pocket.css:85
> (Diff revision 1)
> > +  width: 18px; /* Width of each frame within the SVG sprite */
> > +  height: 36px; /* Height of each frame within the SVG sprite */
> > +}
> > +
> > +#library-button[animate="pocket"] > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image {
> > +  height: 36px; /* Height of each frame within the SVG sprite */
> 
> Why do we need to set height on both this box and its parent?

Without the height set on the parent, the box collapses and no animation is seen. This is because the .toolbarbutton-animatable-image has position:fixed which takes it out of the flow of the document.

Comment 5

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

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154058/diff/1-2/

Comment 6

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

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154058/diff/2-3/

Comment 7

[:Gijs (out for now; he/him)]

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

https://reviewboard.mozilla.org/r/154058/#review160316

::: browser/components/customizableui/CustomizableUI.jsm:1391
(Diff revision 3)
> +      if (aWidget.animatable &&
> +          Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled") &&
> +          AppConstants.MOZ_PHOTON_ANIMATIONS) {
> +        let box = aDocument.createElement("box");

This stuff makes me kind of sad... I guess we will need to move this to the in-urlbar icon when we get that? Is there any point in having generic CUI support for this? Pocket could just do this itself in the onCreated callabck...

::: browser/extensions/pocket/content/main.js:98
(Diff revision 3)
> +
> +        if (_lastAddSucceeded) {
> +            var libraryButton = document.getElementById("library-button");
> +            if (!Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled") ||
> +                !libraryButton ||
> +                libraryButton.getAttribute("cui-areatype") == "menu-panel") {

This doesn't check if the button is in the dynamic portion of the overflow panel.

::: browser/extensions/pocket/skin/shared/pocket.css:48
(Diff revision 3)
> +}
> +
> +#pocket-button > .toolbarbutton-animatable-box {
> +  position: fixed;
> +  overflow: hidden;
> +  margin-top: -8px;

Where does this magic number come from, and will we need to update it when we move this into the urlbar?

::: browser/extensions/pocket/skin/shared/pocket.css:62
(Diff revision 3)
> +  height: 16px; /* Height of each frame within the SVG sprite */
> +}
> +
> +#pocket-button[open="true"][animationsenabled][cui-areatype="toolbar"] > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image {
> +  animation-name: pocket-animation;
> +  animation-timing-function: steps(11);

So, I could be missing something, but it looks to me like there's 12 steps - 12 images in the sprite? We discussed this a bit on IRC, I'm still confused about how this is supposed to play out, but I'm going to submit this review because I have to head out soon, and there's other feedback that I guess makes sense to get to you sooner rather than later.

::: browser/extensions/pocket/skin/shared/pocket.css:63
(Diff revision 3)
> +}
> +
> +#pocket-button[open="true"][animationsenabled][cui-areatype="toolbar"] > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image {
> +  animation-name: pocket-animation;
> +  animation-timing-function: steps(11);
> +  animation-duration: 200ms;

This also seems to be designed for 12, not 11 - 200ms = 1/5s, and at 60fps that means 60/5 = 12 frames...

::: browser/extensions/pocket/skin/shared/pocket.css:66
(Diff revision 3)
> +  background-image: url("chrome://pocket-shared/skin/pocket-animation.svg"),
> +                    url("chrome://pocket-shared/skin/library-pocket-animation.svg");

I'm confused - presumably the preloading would work just as well when added to the library button itself? Why do we need to add it here? Does that mean there's still a white flicker if the pocket button isn't in the toolbar?

::: browser/extensions/pocket/skin/shared/pocket.css:99
(Diff revision 3)
> +  /* -26px looks right, don't know why this specific number is needed */
> +  margin-top: -26px;

I don't think this works. :-(

Specifically, this is margin-aligned from the top to match the icon, given the size of the toolbar button (against which it's positioned).

The toolbar buttons' height is unpredictable. The navbar uses -moz-box-align:stretch, and the buttons are therefore about 38px for me on Windows and Linux. My understanding is that the URL bar still uses relative font sizes, so if a user changed their Windows font size, the size of the button changes.

Likewise, the size of the button changes if you move the button to the tabstrip or bookmarks bar, which makes the animation look bad, too.

If we can't predict the offset we need here, I would suggest checking the relative offset of the icon with the domwindowutils' lazy bounds, or using a stack and CSS styling to match the inner padding that the icon is getting (kind of like what we do for badged buttons).

Comment 8

[:Gijs (out for now; he/him)]

In addition to the code feedback, I see either a small shadow or blurring artifacts around the red pocket thing in the pocket animation itself (not the library one). But in the middle of the animation, the sides of the pocket icon hit the bounding box of the animation frame, which means the blurring/shadow disappears and that different sides finish animating at different times. This makes the left/right sides look cut off, and because of the difference in when the animations finish, it makes the animation look less smooth.

Separately, when I actually run the animation, the upward/downward movement of the arrow on the icon itself is jumpy - it feels like it'd be better off just staying in place.

Normally, we could just swap out different icons in a followup, but it feels like because of the tying-together of the size of the animation and the implementation, as well as the post-processing necessary on the bits we get out of illustrator or whatever is being used to design these animations, that that might be trickier here, so ideally I think this should be addressed before landing.

Comment 9

[:Gijs (out for now; he/him)]

(In reply to :Gijs from comment #8)
> In addition to the code feedback, I see either a small shadow or blurring
> artifacts

Err, I meant anti-aliasing artifacts. It's Friday...

Comment 10

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

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154058/diff/3-4/

Comment 11

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

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154058/diff/4-5/

Comment 12

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

Try push, https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a02a539fda09cbaa2c48671b54b6ee6d838540b

Comment 13

[:Gijs (out for now; he/him)]

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

https://reviewboard.mozilla.org/r/154058/#review164104

::: browser/extensions/pocket/content/main.js:104
(Diff revision 5)
> +                if ((animationevent.animationName == "library-pocket-animation" ||
> +                     animationevent.animationName == "library-pocket-animation-rtl") &&
> +                    animationevent.target.className == "toolbarbutton-animatable-image") {

Nit: `animationevent` should be just `ev` or if we want to keep the long name, `animationEvent`.

Further, I think we should just `animationEvent.animationName.startsWith("library-pocket-animation")`.

Finally, do we really need the target check if we're already checking the animation name?

::: browser/extensions/pocket/content/main.js:109
(Diff revision 5)
> +                if ((animationevent.animationName == "library-pocket-animation" ||
> +                     animationevent.animationName == "library-pocket-animation-rtl") &&
> +                    animationevent.target.className == "toolbarbutton-animatable-image") {
> +                    libraryButton.removeEventListener("animationend", onAnimationEnd);
> +                    libraryButton.setAttribute("fade", "true");
> +                    libraryButton.addEventListener("transitionend", function onTransitionEnd(transitionevent) {

Again, event name nits from me.

::: browser/extensions/pocket/content/main.js:112
(Diff revision 5)
> +                    libraryButton.removeEventListener("animationend", onAnimationEnd);
> +                    libraryButton.setAttribute("fade", "true");
> +                    libraryButton.addEventListener("transitionend", function onTransitionEnd(transitionevent) {
> +                        if (transitionevent.propertyName == "fill") {
> +                            libraryButton.removeEventListener("transitionend", onTransitionEnd);
> +                            libraryButton.removeAttribute("animate");

I seem to be exceptionally good at making this break, but I haven't been able to figure out why. The button ends up looking normal in my browser while in the navbar, but somehow the animate/fade attributes remain present and this causes issues if the button moves somewhere else. I would expect the animationend/transitionend events to always fire, but it seems they don't. This seems to happen if the button is on another toolbar, though I'm not really sure why/how. I did find some reproducible steps which are a bit more obscure:

1. ensure the library button is first in line to end up in the overflow panel
2. pocket something
3. while the library animation runs, make the window narrower so the library button overflows

This causes all of the animation states to display on the library button in the overflow panel, which uh, doesn't look good. I first noticed this when trying to test making the font size bigger (like in bug 1381993) which caused everything to overflow, and apparently at that point the button was already stuck in a state where the animate/fade attribute was still present and so things broke.

I expect we should make some of the styling specific to the button being in the toolbar and non-overflowed, and that perhaps we should add some kind of 'safety' settimeout that removes the attributes if we can't rely on the events (which I'm not sure about).

::: browser/extensions/pocket/skin/shared/pocket.css:50
(Diff revision 5)
> +  }
> +}
> +
> +#pocket-button > .toolbarbutton-animatable-box {
> +  position: absolute;
> +  overflow: hidden;

When I put the button on the tabstrip, the animatable box becomes 240px wide and I see all the frames of the animation at once, so this doesn't seem to work correctly right now. The button also looks lopsided there when the animation is not playing (too much space on the right-hand-side, tested on OS X).

I tried moving the library button to the bookmarks toolbar, and that seems to have the same issue there.

Comment 14

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

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154058/diff/5-6/

Comment 15

[:Gijs (out for now; he/him)]

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

https://reviewboard.mozilla.org/r/154058/#review164392

Clearing for now because I have questions / suggestions. However, this looks like it's on the right track (though I haven't tested it again).

::: browser/extensions/pocket/bootstrap.js:131
(Diff revisions 5 - 6)
>      },
>      onCreated(node) {
> +      let area = CustomizableUI.getPlacementOfWidget("pocket-button").area;
> +      this.buildAnimationSubtree(node, area);
> +    },
> +    onWidgetAdded(widgetId, area) {

I don't think this gets called for reset / undo reset. It also won't be called for when the node overflows / gets removed from overflow (that bit might be OK, but I wanted to flag it up).

Rather than doing this, could we just display: none the child by default, and display: -moz-box it for the #nav-bar descendant non-overflow case? That seems like it'd be simpler.

::: browser/extensions/pocket/content/main.js:101
(Diff revisions 5 - 6)
>              if (!Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled") ||
>                  !libraryButton ||
>                  libraryButton.getAttribute("cui-areatype") == "menu-panel" ||
> -                libraryButton.getAttribute("overflowedItem") == "true") {
> +                libraryButton.getAttribute("overflowedItem") == "true" ||
> +                !libraryButton.closest("toolbar") ||
> +                libraryButton.closest("toolbar").getAttribute("id") != "nav-bar") {

Nit: toolbar.id

::: toolkit/modules/BrowserUtils.jsm:382
(Diff revision 6)
> +   *
> +   */
> +  setToolbarButtonHeightProperty(element) {
> +    let window = element.ownerGlobal;
> +    let dwu = window.getInterface(Ci.nsIDOMWindowUtils);
> +    let toolbarItem = element.closest(".customization-target > " + element.localName);

Under what circumstances does this not select the element itself? Do we really need the `.customization-target` bit?

::: toolkit/modules/BrowserUtils.jsm:392
(Diff revision 6)
> +    if (!toolbar) {
> +      return;
> +    }
> +    let bounds = dwu.getBoundsWithoutFlushing(toolbarItem);
> +    if (!bounds.height) {
> +      bounds = toolbarItem.getBoundingClientRect();

Oof. Do we really need this fallback (given sync layout flush etc.)? When does this get hit?

Without knowing when/where we need this, I would prefer to just set the height to 0, which presumably will prevent the animation from (visibly) displaying, which seems non-terrible.

Comment 16

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

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154058/diff/6-7/

Comment 17

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

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

https://reviewboard.mozilla.org/r/154058/#review164392

> Under what circumstances does this not select the element itself? Do we really need the `.customization-target` bit?

I've changed this in the patch to get a reference to the #urlbar-container if it is inside of it. This works the same as before, but is now more straightforward and covers the only case that it was designed for. I hope it is clearer now.

> Oof. Do we really need this fallback (given sync layout flush etc.)? When does this get hit?
> 
> Without knowing when/where we need this, I would prefer to just set the height to 0, which presumably will prevent the animation from (visibly) displaying, which seems non-terrible.

The bounds were 0-height when calculating the height of the pocket-button during the [open=true] animation. Calculating this prior to the setting of [open=true] fixes this, which is why I implemented onBeforeCommand in bug 1382667 and am now using it in this patch. In any fashion, we needed to calculate the height of the button *before* doing the animation, so even without the 0-height bounds, the onBeforeCommand is still necessary.

Comment 18

[:Gijs (out for now; he/him)]

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

https://reviewboard.mozilla.org/r/154058/#review164926

Ship it! (well, with these comments addressed)

::: browser/extensions/pocket/skin/shared/pocket.css:131
(Diff revisions 6 - 7)
>  
> +:-moz-any(#pocket-button, #library-button) > .toolbarbutton-animatable-box {
> +  display: none;
> +}
> +
> +#nav-bar :-moz-any(#pocket-button, #library-button) > .toolbarbutton-animatable-box {

Do we need to do the same for the reload/stop button?

::: browser/components/customizableui/CustomizableUI.jsm:2423
(Diff revision 7)
> +    if (typeof aData.onBeforeCommand == "function") {
> +      widget.onBeforeCommand = aData.onBeforeCommand;
> +    }

Oh, uh, I guess this is supposed to go in the patch for bug 1382667?

Comment 19

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

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154058/diff/7-8/

Comment 20

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

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

https://reviewboard.mozilla.org/r/154058/#review164926

> Do we need to do the same for the reload/stop button?

Yes, I filed bug 1382898 because I think that work should be separate from this bug.

> Oh, uh, I guess this is supposed to go in the patch for bug 1382667?

Thanks for catching this. I guess I screwed something up when rolling up my patches.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4b7fc921b88
Save to Pocket should have an associated animation. r=Gijs

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This appears to have caused a permafailing assertion or two in stylo tests: https://treeherder.mozilla.org/logviewer.html#?job_id=116282524&repo=autoland

https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=7b16c219d018fb48f72c64bb7a9c225b23624e15&noautoclassify&filter-searchStr=fd080c04ea39645fa5c2e78279eda4f46ed5ac04&group_state=expanded&tochange=ac12978e5346e24defd9042b721d590d11e31d44

These jobs are tier-2, so no immediate backout, but it would be good to fix these.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Actually, this busted non-stylo jobs that are tier-1: https://treeherder.mozilla.org/logviewer.html#?job_id=116218579&repo=autoland

Backing this out.

Comment 24

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

Thanks, looks like I have to do the work-around from bug 1379332.

Comment 25

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7aba372846c8
Backed out changeset d4b7fc921b88 for assertions in test_display_mode.html a=backout

Comment 26

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

Comment on attachment 8883104 [details]
Bug 1355922 - Save to Pocket should have an associated animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154058/diff/8-9/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c927b51ca41
Save to Pocket should have an associated animation. r=Gijs

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8c927b51ca41

Comment 29

[Azmina]

I have seen this feature "Save to Pocket should have an associated animation" has been implemented with Latest Nightly 56.0a1 on Windows 8.1 (64 Bit)

Build ID : 20170801100311
User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 30

[Justin [:JW_SoftvisionQA]]

I am not seeing the animation on the current nightly:

20170810100255
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Here is a video of my findings:

https://jwilliams-softvision.tinytake.com/sf/MTg2NDQ3Ml81OTk3MzMy

Comment 31

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

Yeah, the Pocket animation is currently broken and is on file as bug 1387077. I have a patch there that should land soon.

Comment 32

[Justin [:JW_SoftvisionQA]]

ok got it. I will make a note and inform Stefan.

Comment 33

[Abe - QA (:Abe_LV)]

This is verified as fixed.

Comment 34

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

Based on comment 33 I will change the tracking flag for Firefox56 to verified.