1612104 - Part of the download start animation is missing / cut off

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[najaro]

Attachment: download animation 57.gif

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Start downloading a file

Actual results:

No start animation was played

Expected results:

There's should be start animation like in previous versions.
I don't know when this stops working but I downloaded firefox version 57 and this animation is present.

Also checked fresh installation of firefox 72.0.2 with same result - no animation.

I attach gif of what this animation looks like in firefox 57.

Comment 1

[najaro]

Ignore User Agent. I'm currently on version 72.0.2. I don't know why it says 68.

Comment 2

[najaro]

Problem with no animation also occurs on firefox 64.0.2 and latest nightly 74.0a1 (2020-01-29).

Comment 3

[najaro]

60.0 ESR - no animation either.

Comment 4

[Alice0775 White]

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a124f4901430f6db74cfc7fe3b07957a1c691b40&tochange=252a8528c5ab4affc2742e7ccfe4ef4aa5ee42f8

Comment 5

[najaro]

This isn't probably the best place for this question but how is it possible that no one has noticed this for 2 years! :O

Comment 6

[xayide5381]

I confirm this error and its regression range

Comment 7

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

In bug 1376519 this was changed to be a "dip" animation (the arrow moves down a few pixels and back up when the download starts), and I still see that on nightly (tested on both macOS and Windows 10) when e.g. downloading an ubuntu iso. In other words, I can't reproduce this bug as reported - though the animation has changed since 57.

I'm confused because the change to the animation doesn't line up with the regression window from comment #4 - and the regression window does not have super obvious other suspects (nothing relating to downloads directly). Alice, do you not see the animation I described above (different from the one attached in comment #0)? Can you provide more specific steps to reproduce? Does it maybe only happen if the button is not already visible in the toolbar, or is there some other condition to reproducing this?

Comment 8

[Alice0775 White]

Attachment: download animation on good build

Comment 9

[Alice0775 White]

Attachment: download animation on bad build

Comment 10

[Alice0775 White]

Attachment: download animation on Nightly74

Comment 11

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

OK. I don't think we consider the "bad" build a regression, as that was an intentional change from bug 1376519.

That new animation should still appear though, so the behaviour in the 74 video is still wrong. Can you reproduce that reliably, and if so, can you find a regression from the new animation to no animation at all? It may be helpful to use something like https://www.thinkbroadband.com/download rather than nightly ftp downloads, so the downloads take longer to complete and there is no race with the "download finished" animation (which overrides the "start" animation).

Comment 12

[Alice0775 White]

Attachment: Download animation from thinkbroadband on Nightly74.0a1.

Comment 13

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

OK, given that there is an animation in the latest video, and it's just that the animation is different from the one in 57, I think this is working as designed. If there is something I'm missing here (does it never work for some websites?) then please clarify and reopen.

Comment 15

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

(In reply to najaro from comment #14)

If you're wondering why we don't fix some bugs, the namecalling and insults would be a good place to start.

Dan, seems like your fix for bug 1410561 did not in fact preserve behaviour. Any chance you could take a look?

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to :Gijs (he/him) from comment #15)

Dan, seems like your fix for bug 1410561 did not in fact preserve behaviour. Any chance you could take a look?

I don't really have cycles to investigate thoroughly today -- I think my patch in bug 1410561 was just taking some frontend-CSS code-comments at their word when they were saying that the min/max-width usage there had just been to workaround an assertion failure (which had then become fixed).

If that patch caused problems, we might as well just revert it (and adjust the explanatory comment). I can do that if needed, or feel free to do it, if you like.

Comment 17

[Daniel Holbert [:dholbert]]

OK, I did investigate a bit, using devtools to see where things are & how big they are.

What's happening here is roughly as follows:

• We have the following markup:

<hbox id="downloads-animation-container">
<vbox id="downloads-notification-anchor">

• The outer element (the hbox) has height:1px:
  https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/browser/themes/shared/downloads/indicator.inc.css#158

• The inner element is the one whose styling was changed in bug 1410561 (replacing min-height & max-height with just height)

• Due to XUL default min-size behavior (/me waves hands, I don't know XUL super well), the inner vbox allows itself to shrink as-needed (awayu from its specified 98px, all the way down to its parent's 1px size) by default, unless it has a specified min-height value.

• So: for its height:98px to actually have an effect, it also needs min-height:98px (or else the 1px constraint from the parent "wins" and squashes it down).

So, the animation isn't showing up because it's taking place in a 1px-tall box, rather than in a 98px-tall box.

Comment 18

[Daniel Holbert [:dholbert]]

Auditing all of the rules that were changed in bug 1410561, in https://hg.mozilla.org/mozilla-central/rev/a59e00e80ba4
(1) pocket.css:

#pocket-button-box[animate="true"] > #pocket-animatable-box,
#pocket-button > .toolbarbutton-animatable-box {

This change doesn't make a difference at this point, because (a) there is no element called "pocket-animatable-box" that I can find (in my browser DOM & in a searchfox search), and (b) #pocket-button has no descendants -- it's an SVG <image>.

So I think this CSS rule might just be dead code?

(2) indicator.inc.css:
That's the CSS involved with the bug reported here, with the download icon animation. Needs to have min-height restored; see comment 17.

(3) toolbarbutton-icons.inc.css:
This change to this file doesn't make a difference at this point, because the container for the affected elements has a reasonable height (it's the browser toolbar itself), so it's not compressing them down to their min-height values.

(4) urlbar-searchbar.inc.css
This change to this file doesn't make a difference at this point, because the container for the affected elements has a reasonable height (it's the urlbar itself), so it's not compressing them down to their min-height values.

So I think the damage here is limited just to the "download" animation, and it's specifically fallout from the fact that we throw that animated element into a 1px-tall container for some reason.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: Bug 1612104: Give #downloads-notification-anchor a min-height so that the download-start animation is visible. r?Gijs

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: screencast after patch is applied

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb9fe456168c
Give #downloads-notification-anchor a min-height so that the download-start animation is visible. r=mak

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/bb9fe456168c