Closed Bug 922847 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P2][Australis:M9][fixed-in-ux])

Attachments

(1 file, 1 obsolete file)

Attached patch Separate animation (obsolete) — Splinter Review
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?
Flags: needinfo?(mconley)
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
Attachment #813055 - Flags: review?(mconley)
Attachment #812829 - Attachment is obsolete: true
Flags: needinfo?(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
Attachment #813055 - Flags: review?(mconley) → review+
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
Whiteboard: [Australis:P2][Australis:M?] → [Australis:P2][Australis:M?][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8a1d8044a4c8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][Australis:M?][fixed-in-fx-team] → [Australis:P2][Australis:M?]
Target Milestone: --- → Firefox 27
https://hg.mozilla.org/projects/ux/rev/8a1d8044a4c8
Whiteboard: [Australis:P2][Australis:M?] → [Australis:P2][Australis:M9][fixed-in-ux]
Blocks: 923525
Many css rules with:

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

broke by this bug since now TabsToolbar is not the last child
(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"?
I'm using the default theme on window 7
Depends on: 923843
(In reply to onemen.one from comment #8)
> I'm using the default theme on window 7

Filed bug 923843 for this issue.
Depends on: 943785
No longer depends on: 943785
You need to log in before you can comment on or make changes to this bug.