922847 - Move downloads animations into their own element rather than in a stack inside the button

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

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

Attachment: Separate animation

I have a local patch for this that I've tested on OS X and seems to work (attached). I'll add styles for Linux and Windows tomorrow and do some more testing before asking for review. This fixes at least three separate Australis bugs, and from what I understood Marco was already thinking about doing this anyway.

I'm not 100% sure if this means we can get rid of the stack, separate binding, and all that thingymajigs. I suspect so, but I'm not 100%. Mike, got an opinion on that? If we could style the existing icon to look correct for when the progress bar is also visible, and just dynamically DOM-append the progress bar if it's not there, that probably takes care of things and we can ditch the whole overlay situation, but again, maybe I'm missing something. Regardless, that can be fodder for a followup bug as far as I'm concerned. Mike, does that sound plausible to you?

Comment 1

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

Attachment: Separate animation from download button,

This works on Windows as well. Passes the download mochitests on my windows machine, too, but I gave it a try run to be sure: https://tbpl.mozilla.org/?tree=Try&rev=e642f5419418

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 813055 [details] [diff] [review]
Separate animation from download button,

Review of attachment 813055 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Gijs,

-Mike

::: browser/components/downloads/content/indicator.js
@@ +331,3 @@
>      }
>  
> +    // If we're not positioned yet, do that now:

Please expand this comment to describe where we're positioning this element and why.

::: browser/themes/linux/downloads/indicator.css
@@ +8,5 @@
> +  min-height: 1px;
> +  min-width: 1px;
> +  height: 1px;
> +  margin-bottom: -1px;
> +  /* Makes the outermost animation container element positioned, so that its 

Nit - trailing whitespace

::: browser/themes/osx/downloads/indicator.css
@@ +13,5 @@
> +  min-height: 1px;
> +  min-width: 1px;
> +  height: 1px;
> +  margin-bottom: -1px;
> +  /* Makes the outermost animation container element positioned, so that its 

Nit - trailing whitespace

::: browser/themes/windows/downloads/indicator.css
@@ +8,5 @@
> +  min-height: 1px;
> +  min-width: 1px;
> +  height: 1px;
> +  margin-bottom: -1px;
> +  /* Makes the outermost animation container element positioned, so that its 

Nit - trailing whitespace

Comment 3

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

Landed with nits fixed, and this added because I suddenly realized that the current code would throw a JS error or do the wrong thing otherwise:

    // If the anchor is not there or its container is hidden, don't show
    // a notification
    let anchor = DownloadsButton._placeholder;
    if (!anchor || !isElementVisible(anchor.parentNode)) {
      return;
    }


D'oh!



https://hg.mozilla.org/integration/fx-team/rev/8a1d8044a4c8

Comment 4

[Matt Brubeck (:mbrubeck)]

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

Comment 5

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

https://hg.mozilla.org/projects/ux/rev/8a1d8044a4c8

Comment 6

[tabmix.onemen]

Many css rules with:

#TabsToolbar[tabsontop="false"]:last-child

broke by this bug since now TabsToolbar is not the last child

Comment 7

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

(In reply to onemen.one from comment #6)
> Many css rules with:
> 
> #TabsToolbar[tabsontop="false"]:last-child
> 
> broke by this bug since now TabsToolbar is not the last child

Are you talking about custom themes? I don't see any such rules in m-c offhand. I also don't understand what the ":last-child" is trying to solve. Why would you check for "last-child"?

Comment 8

[tabmix.onemen]

I'm using the default theme on window 7

Comment 9

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

(In reply to onemen.one from comment #8)
> I'm using the default theme on window 7

Filed bug 923843 for this issue.