When saving to Pocket, the Pocket icon animation is being covered by an active tab. Please see video for reference.

From bug 1384341, this is happening because the selected tab has a higher z-index than the nav-bar, which is because of the nav-bar shadow/highlight that overlaps the selected tab. Shorlander, would you be OK with temporarily disabling the shadow/highlight when these two animations are in progress? See https://bugzilla.mozilla.org/show_bug.cgi?id=1384341#c10 for more details.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1) > From bug 1384341, this is happening because the selected tab has a higher > z-index than the nav-bar, which is because of the nav-bar shadow/highlight > that overlaps the selected tab. > > Shorlander, would you be OK with temporarily disabling the shadow/highlight > when these two animations are in progress? > > See https://bugzilla.mozilla.org/show_bug.cgi?id=1384341#c10 for more > details. I might need to see it in action, but this seems like something that could be worse than the problem it's solving. The tab will only occasionally cover the animation but toggling the shadow / border will be a persistent problem and could be pretty annoying and broken looking.

(In reply to Stephen Horlander [:shorlander] from comment #2) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1) > > From bug 1384341, this is happening because the selected tab has a higher > > z-index than the nav-bar, which is because of the nav-bar shadow/highlight > > that overlaps the selected tab. > > > > Shorlander, would you be OK with temporarily disabling the shadow/highlight > > when these two animations are in progress? > > > > See https://bugzilla.mozilla.org/show_bug.cgi?id=1384341#c10 for more > > details. > > I might need to see it in action, but this seems like something that could > be worse than the problem it's solving. > > The tab will only occasionally cover the animation but toggling the shadow / > border will be a persistent problem and could be pretty annoying and broken > looking. I agree, let's not do that.

Thinking more about this, we may need to move the animation to a different place in the DOM so that it is not restricted by this.

Note that this patch applies on top of the patch from bug 1387077.

review ping? I have confirmed that this patch continues to work on latest m-c with the patch from bug 1390182 applied (I figured it would be worth mentioning even though 1390182 doesn't do anything to the library button).

Comment on attachment 8896049 [details] Bug 1384953 - Run the library and pocket animations outside of the navigation-toolbar to workaround z-index issues with the selected tab. Redirecting review to Gijs...

Comment on attachment 8896049 [details] Bug 1384953 - Run the library and pocket animations outside of the navigation-toolbar to workaround z-index issues with the selected tab. https://reviewboard.mozilla.org/r/167316/#review176324 This would have been r+ but I see a "jump" at the end of the bookmarks/pocket animation where the library icon seems to move up. win10, 175%dpi ::: browser/base/content/browser-places.js:82 (Diff revision 1) > > // nsIDOMEventListener > handleEvent(aEvent) { > switch (aEvent.type) { > case "animationend": { > let libraryButton = document.getElementById("library-button"); Nit: this can move into the else if block now. ::: browser/base/content/browser-places.js:88 (Diff revision 1) > + let animatableBox = document.getElementById("library-animatable-box"); > if (aEvent.animationName.startsWith("library-bookmark-animation")) { > - libraryButton.setAttribute("fade", "true"); > + animatableBox.setAttribute("fade", "true"); > } else if (aEvent.animationName == "library-bookmark-fade") { > - libraryButton.removeEventListener("animationend", this); > + animatableBox.removeEventListener("animationend", this); > libraryButton.removeAttribute("animate"); Can you add a comment to explain why we still need to do this (ie to un-fill the 'normal' icon during the animation) ::: browser/base/content/browser-places.js:91 (Diff revision 1) > + let toolbox = document.getElementById("navigator-toolbox"); > + toolbox.removeAttribute("animate"); Nit: gNavToolbox ::: browser/base/content/browser-places.js:153 (Diff revision 1) > Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled") && > (libraryButton = document.getElementById("library-button")) && > libraryButton.getAttribute("cui-areatype") != "menu-panel" && > libraryButton.getAttribute("overflowedItem") != "true" && > libraryButton.closest("#nav-bar")) { > - BrowserUtils.setToolbarButtonHeightProperty(libraryButton); > + let toolbox = document.getElementById("navigator-toolbox"); Nit: gNavToolbox ::: browser/base/content/browser-places.js:169 (Diff revision 1) > + if (navBar.hasAttribute("brighttext")) { > + animatableBox.setAttribute("brighttext", "true"); > + } else { > + animatableBox.removeAttribute("brighttext"); > + } > + toolbox.appendChild(animatableBox); Why do we need to do this? ::: browser/extensions/pocket/skin/shared/pocket.css:167 (Diff revision 1) > /* Since .toolbarbutton-icon uses a different width than the animatable box, > - we need to set a padding relative to the difference in widths. */ > - margin-inline-start: calc((16px + 2 * var(--toolbarbutton-inner-padding) - 22px) / 2); > + we need to set a margin relative to the difference in widths. > + margin-left is used here even in RTL because the item is positioned using `left` */ > + margin-left: calc((16px + 2 * var(--toolbarbutton-inner-padding) - 22px) / 2); This feels like it should just be a single calc() to get the left: just like we do for top:. Was there a reason not to do that here?

Here's the rebase I used to test, in case it's of interest: https://pastebin.mozilla.org/9030318

(In reply to :Gijs from comment #9) > This would have been r+ but I see a "jump" at the end of the > bookmarks/pocket animation where the library icon seems to move up. win10, > 175%dpi Testing this out with various different DPIs, this seems similar to bug 1386029 where we had to adjust some of the internal values of the SVG due to pixel rounding issues at different DPIs. I'd like to fix this in a separate bug since it may require deeper investigation and is not visible at 100% DPI or 200% DPI.

Comment on attachment 8896049 [details] Bug 1384953 - Run the library and pocket animations outside of the navigation-toolbar to workaround z-index issues with the selected tab. https://reviewboard.mozilla.org/r/167316/#review177538 Please file a followup for the deduplication of CSS + JS and another one for the rounding / dpi issue (I don't have time to check this right now, but it sounds like you haven't tried to fix it so it's probably still happening. :-) ::: browser/base/content/browser-places.js:152 (Diff revision 3) > Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled") && > (libraryButton = document.getElementById("library-button")) && > libraryButton.getAttribute("cui-areatype") != "menu-panel" && > libraryButton.getAttribute("overflowedItem") != "true" && > libraryButton.closest("#nav-bar")) { > - BrowserUtils.setToolbarButtonHeightProperty(libraryButton); > + let animatableBox = document.getElementById("library-animatable-box"); Can we factor all this duplicated code out to a utility function on (for want of a better place) BookmarkingUI? Like maybe `triggerLibraryAnimation(animation, animationendListener)`, where `animation` is one of `bookmark` or `pocket`, and then invoke that from the two callsite? It looks like the fade animation stuff could also be shared. ::: browser/extensions/pocket/bootstrap.js:238 (Diff revision 3) > + let toolbox = doc.getElementById("navigator-toolbox"); > + toolbox.removeAttribute("animate"); or event.view.gNavToolbox, I guess? ::: browser/themes/shared/toolbarbutton-icons.inc.css:464 (Diff revision 3) > - > #library-button[animate="bookmark"] > .toolbarbutton-icon { > fill: transparent; > } > > -#library-button[animate="bookmark"] > .toolbarbutton-animatable-box { > +.toolbarbutton-animatable-box[animate="bookmark"] { It looks like most of the styles in this block could be shared too.

(In reply to :Gijs from comment #15) > Please file a followup for the deduplication of CSS + JS and another one for > the rounding / dpi issue (I don't have time to check this right now, but it > sounds like you haven't tried to fix it so it's probably still happening. :-) Right, I confirmed that I can see it, and that fixing it won't be so straightforward as adding a missing 1px in one of our front-end calculations. Thanks for the r+, I'll file those bugs.

Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b5a5c38a0add Run the library and pocket animations outside of the navigation-toolbar to workaround z-index issues with the selected tab. r=gijs

I have reproduced this bug according to (2017-07-27) Fixing bug is verified on Latest Nightly-- Build ID :20170824100243 User Agent :Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 It seems ok now on Latest Nightly. Tested OS-- windows7 32bit