Library icon animation is covered behind active tab when saving to Pocket or adding a bookmark

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
a year ago
10 months ago

People

(Reporter: amylee, Assigned: jaws)

Tracking

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-animation])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8890878 [details]
pocket animation covered by tab.mov

When saving to Pocket, the Pocket icon animation is being covered by an active tab. Please see video for reference.
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [reserve-photon-animation]
Component: Pocket → Theme
Summary: Pocket icon animation is covered behind active tab when saving to Pocket → Library icon animation is covered behind active tab when saving to Pocket or adding a bookmark
Blocks: 1352063
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.
Flags: needinfo?(shorlander)
(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.
Flags: needinfo?(shorlander)
(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.
(Assignee)

Updated

11 months ago
Assignee: nobody → jaws
Status: NEW → ASSIGNED

Updated

11 months ago
Iteration: --- → 57.1 - Aug 15
Priority: P3 → P1
Comment hidden (mozreview-request)
Note that this patch applies on top of the patch from bug 1387077.

Updated

11 months ago
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
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).

Updated

11 months ago
Flags: qe-verify? → qe-verify+
QA Contact: stefan.georgiev
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...
Attachment #8896049 - Flags: review?(dao+bmo) → review?(gijskruitbosch+bugs)

Comment 9

11 months ago
mozreview-review
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?
Attachment #8896049 - Flags: review?(gijskruitbosch+bugs)

Comment 10

11 months ago
Here's the rebase I used to test, in case it's of interest: https://pastebin.mozilla.org/9030318
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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 hidden (mozreview-request)

Comment 15

11 months ago
mozreview-review
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.
Attachment #8896049 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Depends on: 1393565

Comment 19

11 months ago
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

Comment 20

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5a5c38a0add
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Updated

11 months ago
Depends on: 1393677

Comment 21

11 months ago
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
QA Whiteboard: [bugday-20170823]

Updated

11 months ago
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

11 months ago
Depends on: 1394588

Updated

11 months ago
Depends on: 1397236
Depends on: 1399651
(Assignee)

Updated

10 months ago
Depends on: 1403550
You need to log in before you can comment on or make changes to this bug.