Spec TBD

Placing into reserve backlog for now.

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?

(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 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).

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.

(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...

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

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 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 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 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 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.

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d4b7fc921b88 Save to Pocket should have an associated animation. r=Gijs

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.

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

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

Backout by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7aba372846c8 Backed out changeset d4b7fc921b88 for assertions in test_display_mode.html a=backout

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8c927b51ca41 Save to Pocket should have an associated animation. r=Gijs

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

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

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

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

This is verified as fixed.

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