948956 - Download-in-progress (indeterminate) is non obvious when server gives no content-length

Status: NEW

(Firefox :: Downloads Panel, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

STR:

1) Enter this link "http://download.documentfoundation.org/libreoffice/stable/4.1.3/win/x86/LibreOffice_4.1.3_Win_x86.msi"
2) Observe the Firefox UI, notable the download panel arrow, then open the downloads panel.

It seems that this is a download server that doesn't give a content-length in advance. Unfortunately, our UI gives absolutely no indication that a download is in progress in that case (it will only notify when finished, so a user could erroneously assume that his download didn't start. In any case it is not possible to see that a download is in progress from the main UI, the panel would have to be explicitly opened.

On downloads with a content-length, this is not an issue because the download arrow panel will be replaced by a progress bar with a moving time indication.

Comment 2

[Manish Goregaokar [:manishearth]]

Attachment: Patch - Working infinite downloads indicator; linux only. Restructured indicator-updating code

Attachment moved from bug 970032 comment 7:


Unpolished patch.

I've only done the theming for linux, if you try this on a different OS the downloads indicator will become huge. (Simple matter of copying the styles over, but I'd rather get all changes done before doing that)

I also restructured the code a bit, in the past changes to  `percentComplete` in indicator.js were used to trigger changes in the displayed state of the downloads indicator. Since `percentComplete=-1` applies to virus scans and indeterminate downloads, as well as a lack of downloads, I separated the "change indicator state" code from the "change progressbar state".

I added a couple more symbols that get exported from DownloadsIndicatorData to the view (hasActiveDownloads,isUndetermined). I hope that's OK, it seems to be created as a generic API which addons may use as well (which is why I didn't simply change the behavior of `percentComplete` so that it can be -2 when there are indeterminate downloads).


This patch doesn't yet respond on pause events; but neither does the progressbar in the downloads popup. That might be a separate bug.

Comment 3

[Manish Goregaokar [:manishearth]]

Infinite download testcase: http://manishearth.anapnea.net/test.php. This will keep sending characters continuously to you, so best to terminate the connection before you use up too much bandwidth.

Comment 4

[Manish Goregaokar [:manishearth]]

(To expand on the last section of comment 2 above, I am talking about indeterminate downloads and scanning downloads only. The progressbar stops properly if you pause a regular download.)

Comment 5

[Manish Goregaokar [:manishearth]]

Attachment: Patch with linux-only theming.

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: Patch with linux-only theming.

Forgot to qref.

Comment 7

[:Paolo Amadini]

OK, so this displays a moving bar similar to the indeterminate indicator on a panel item. The effect is nice, but I wonder if the movement can be distracting in the long run. In fact, normally we don't have a lot of movement in the indicator area while downloads are in progress, intentionally, so we might need a more subtle animation.

Can you attach a representative screenshot and ask shorlander for ui-review on it?

Comment 8

[Manish Goregaokar [:manishearth]]

Attachment: Proposed UI

This is the proposed UI for indeterminate downloads.

@shorlander, would this be too distracting for longer downloads? If so, is there a better way of showing an "indeterminate" download that is less distracting?

Comment 9

[Manish Goregaokar [:manishearth]]

(In reply to :Paolo Amadini from comment #7)
> In fact, normally we don't have a lot of
> movement in the indicator area while downloads are in progress,
> intentionally, so we might need a more subtle animation.

Note that most subtler animations will probably require the rewrite of the progressbar code to include the new type of progressbar. (undetermined progressbars are already used elsewhere, so we can't break that)

Comment 10

[Manish Goregaokar [:manishearth]]

Just a note, with the patch if you want to trigger the UI without downloading anything, try

    let v=DownloadsCommon.getIndicatorData(window)._views[0];v.hasDownloads=true;
    //I think this bit is async, so wait a second before entering the next part
    v.hasActiveDownloads=true;v.percentComplete=-1;v.isUndetermined=true;

Comment 11

[:Paolo Amadini]

Comment on attachment 8377826 [details] [diff] [review]
Patch with linux-only theming.

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

This patch is doing two different things.

The first is the rationalization of the percentComplete property by removing the usage of the "-1" value. This is worth doing, but is separated from the main concern of this patch. It should be done by splitting the "percentComplete" property in two new properties named "hasProgress" and "progress", using the Downloads.jsm model. If you're interested in doing this, feel free to file a separate bug!

The second is the actual introduction of the "undetermined progress" mode, that should be kept in this bug. I'm doing an initial code review here while we determine the right animation to use.

::: browser/components/downloads/content/indicator.js
@@ +246,5 @@
>      // Reset the view properties, so that a neutral indicator is displayed if we
>      // are visible only temporarily as an anchor.
>      this.counter = "";
>      this.percentComplete = 0;
> +    this.isUndetermined=false;

In this patch, we should just check whether percentComplete is -1, instead of using this new property.

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +1486,5 @@
>        DownloadsCommon.summarizeDownloads(this._activeDataItems());
>  
>      // Determine if the indicator should be shown or get attention.
>      this._hasDownloads = (this._itemCount > 0);
> +    this._hasActiveDownloads = summary.numDownloading>0;

Unfortunately, we already used the word "active" to indicate just the presence of a download. We should use a name for the new property that is consistent with other code, for example "isDownloading" might work.

Feel free to ask for feedback again on the next iteration of the patch.

Comment 12

[:Paolo Amadini]

(In reply to Manish Goregaokar [:manishearth] from comment #9)
> Note that most subtler animations will probably require the rewrite of the
> progressbar code to include the new type of progressbar. (undetermined
> progressbars are already used elsewhere, so we can't break that)

Or we can just use a similarly styled element, or even better set the value "0" or "100" on our custom progressbar and modify our styling to represent the undetermined case.

Comment 13

[Manish Goregaokar [:manishearth]]

(In reply to :Paolo Amadini from comment #11)

> The first is the rationalization of the percentComplete property by removing
> the usage of the "-1" value. This is worth doing, but is separated from the
> main concern of this patch. It should be done by splitting the
> "percentComplete" property in two new properties named "hasProgress" and
> "progress", using the Downloads.jsm model. If you're interested in doing
> this, feel free to file a separate bug!

I haven't removed usage of "-1" (the code is still very dependent on it, in fact), I've just made it not affect the "progress" attribute of the indicator (which otherwise deactivates the indicator completely).


Basically, percentComplete, as used internally, is set to -1 whenever there is no percentage to be shown. This can happen when there isn't a download, or downloads are indeterminate/scanning, or some other cases which I can't remember (will look into this later today) 


In the older code, the downloads indicator was just turning itself off when percentComplete was -1; which was a problem, since this includes when the download is indeterminate. So I would either have to turn it on again, or I would have to include some checks in both hasDownloads() and percentComplete().  The former results in a duplicate animation, the latter results in really strange code with a lot of redundant-looking (but not redundant) ifs since I have to check the indeterminate state everywhere.

So I ended up just making percentComplete mean exactly what it says, with no interconnection with the activation of the indicator. In the past this wasn't necessary as percentComplete=-1 could mean both "there is no progress to show" and "there is no need for a progressmeter", this bug makes these two meanings separate, so I separated the code accordingly. 

So AFAICT the changes to percentComplete

> The second is the actual introduction of the "undetermined progress" mode,
> that should be kept in this bug. I'm doing an initial code review here while
> we determine the right animation to use.
> 
> ::: browser/components/downloads/content/indicator.js
> @@ +246,5 @@
> >      // Reset the view properties, so that a neutral indicator is displayed if we
> >      // are visible only temporarily as an anchor.
> >      this.counter = "";
> >      this.percentComplete = 0;
> > +    this.isUndetermined=false;
> 
> In this patch, we should just check whether percentComplete is -1, instead
> of using this new property.

percentComplete=-1 isn't just used for indeterminate downloads, as mentioned above. It also means "there is no progress to be shown".



> ::: browser/components/downloads/src/DownloadsCommon.jsm
> @@ +1486,5 @@
> >        DownloadsCommon.summarizeDownloads(this._activeDataItems());
> >  
> >      // Determine if the indicator should be shown or get attention.
> >      this._hasDownloads = (this._itemCount > 0);
> > +    this._hasActiveDownloads = summary.numDownloading>0;
> 
> Unfortunately, we already used the word "active" to indicate just the
> presence of a download. We should use a name for the new property that is
> consistent with other code, for example "isDownloading" might work.
> 

isDownloading sounds good :)

Comment 14

[Manish Goregaokar [:manishearth]]

(In reply to :Paolo Amadini from comment #12)
> (In reply to Manish Goregaokar [:manishearth] from comment #9)
> > Note that most subtler animations will probably require the rewrite of the
> > progressbar code to include the new type of progressbar. (undetermined
> > progressbars are already used elsewhere, so we can't break that)
> 
> Or we can just use a similarly styled element, or even better set the value
> "0" or "100" on our custom progressbar and modify our styling to represent
> the undetermined case.

That works, and should be easy enough.

Can't think of any less flashy way to show indeterminate progress, though, aside from using the same UI with a different color for the bar (just a CSS change away, too).

Hopefully shorlander can figure this out :)

Comment 15

[:Paolo Amadini]

(In reply to Manish Goregaokar [:manishearth] from comment #13)
> percentComplete=-1 isn't just used for indeterminate downloads, as mentioned
> above. It also means "there is no progress to be shown".

The "there is no progress to be shown" state is now incarnated by isDownloading being false, thus I guess you can just move the progress bar visibility check from the percentComplete setter to the isDownloading setter.

> In the older code, the downloads indicator was just turning itself off when
> percentComplete was -1; which was a problem, since this includes when the
> download is indeterminate. So I would either have to turn it on again, or I
> would have to include some checks in both hasDownloads() and
> percentComplete().  The former results in a duplicate animation, the latter
> results in really strange code with a lot of redundant-looking (but not
> redundant) ifs since I have to check the indeterminate state everywhere.

I think that eventually things will be simpler than what you're suggesting, but I might be wrong. As a proof of concept, can you try to write a separate patch that only adds the isDownloading property, with minimal CSS changes or none at all? When isDownloading is true but percentComplete is -1, it should show a progress bar with zero progress.

This will not be the final patch, but can help in identifying any blocker.

Comment 16

[Manish Goregaokar [:manishearth]]

Attachment: Rough proof of concept patch without isUndetermined

Alright, removed the isUndetermined bits and moved the functionality into the other two setters (There's some duplication here since we need to check for indeterminate in case either is changed)

The CSS is necessary; on the default styling the undetermined progressbar grows rapidly (vertically), which breaks the toolbar.


Also, I just noticed: Both patches give an error on the *first* indeterminate download you try, that `this.indicator` is null (the lines from `set hasActiveDownloads()` ).

We'll probably want to use an _ensureOperational callback here -- I'll look into that and renaming things once we decide which patch to use.

Comment 17

[:Paolo Amadini]

Comment on attachment 8380398 [details] [diff] [review]
Rough proof of concept patch without isUndetermined

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

::: browser/components/downloads/content/indicator.js
@@ +433,5 @@
> +        this._indicatorProgress.setAttribute("mode","determined");
> +      }else{
> +        if(this._hasActiveDownloads){
> +          this._indicatorProgress.setAttribute("mode","undetermined");
> +        }

You can remove the this._hasActiveDownloads check here...

@@ +444,5 @@
> +    if(this._hasActiveDownloads != aValue){
> +      this._hasActiveDownloads = aValue;
> +      if(aValue){
> +        this.indicator.setAttribute("progress", "true");
> +        this._indicatorProgress.setAttribute("mode",this._percentComplete < 0 ?"undetermined":"determined");

...and the setAttribute call here. The patch should work in the same way.

Also, if you remove the "mode" setter in precentComplete, you should be able to remove all the CSS as well. This is what we should do in case we go for a static undetermined indicator.

Actually, you might already try to create a patch that just sets a different, custom attribute, used by the CSS to select a lighter background for the progress bar.

Comment 18

[:Paolo Amadini]

Comment on attachment 8377826 [details] [diff] [review]
Patch with linux-only theming.

We should continue working on the other patch, given its simplicity.

Comment 19

[Manish Goregaokar [:manishearth]]

(In reply to :Paolo Amadini from comment #17)
> Comment on attachment 8380398 [details] [diff] [review]
> ::: browser/components/downloads/content/indicator.js
> @@ +433,5 @@
> > +        this._indicatorProgress.setAttribute("mode","determined");
> > +      }else{
> > +        if(this._hasActiveDownloads){
> > +          this._indicatorProgress.setAttribute("mode","undetermined");
> > +        }
> 
> You can remove the this._hasActiveDownloads check here...


IIRC without that check, there is a chance that it will try to set the attribute for a nonexistent indicatorProgress, causing a JS error. 

Remember, percentComplete=-1 *also* means "there are no downloads" (which sets the progress attribute to false), so we can have a case where there is no indicator progressbar, but we're still trying to set its mode.


> @@ +444,5 @@
> > +    if(this._hasActiveDownloads != aValue){
> > +      this._hasActiveDownloads = aValue;
> > +      if(aValue){
> > +        this.indicator.setAttribute("progress", "true");
> > +        this._indicatorProgress.setAttribute("mode",this._percentComplete < 0 ?"undetermined":"determined");
> 
> ...and the setAttribute call here. The patch should work in the same way.

Uh, it won't, since *someone* has to call the setAttribute, and in this patch that's the hasActiveDownloads call. The whole point of hasActiveDownloads is to separate the "activate indicator" bit from the "set current progress of indicator" bit.


> Also, if you remove the "mode" setter in precentComplete, you should be able
> to remove all the CSS as well. This is what we should do in case we go for a
> static undetermined indicator.

Yeah, we can do that, but I'd wait on shorlander's comment


> Actually, you might already try to create a patch that just sets a
> different, custom attribute, used by the CSS to select a lighter background
> for the progress bar.

Comment 20

[:Paolo Amadini]

(In reply to Manish Goregaokar [:manishearth] from comment #19)
> IIRC without that check, there is a chance that it will try to set the
> attribute for a nonexistent indicatorProgress, causing a JS error. 

The existence of the element should be independent from its visibility. We can set the attribute on an invisible element, that is not an issue at all.
 
> > @@ +444,5 @@
> > > +    if(this._hasActiveDownloads != aValue){
> > > +      this._hasActiveDownloads = aValue;
> > > +      if(aValue){
> > > +        this.indicator.setAttribute("progress", "true");
> > > +        this._indicatorProgress.setAttribute("mode",this._percentComplete < 0 ?"undetermined":"determined");
> > 
> > ...and the setAttribute call here. The patch should work in the same way.
> 
> Uh, it won't, since *someone* has to call the setAttribute, and in this
> patch that's the hasActiveDownloads call. The whole point of
> hasActiveDownloads is to separate the "activate indicator" bit from the "set
> current progress of indicator" bit.

I'm referring to the _indicatorProgress.setAttribute, not the other one, sorry if it was unclear. From my reading of the patch, the same setAttribute call is done by percentComplete.

Comment 21

[Manish Goregaokar [:manishearth]]

(In reply to :Paolo Amadini from comment #20)

> The existence of the element should be independent from its visibility. We
> can set the attribute on an invisible element, that is not an issue at all.

Uh, but we can't set the attribute on a nonexisting element. The downloads code currently creates the progress bar only when necessary.
  
 
> I'm referring to the _indicatorProgress.setAttribute, not the other one,
> sorry if it was unclear. From my reading of the patch, the same setAttribute
> call is done by percentComplete.

Ah, but that will be called only if percentComplete changes. If I start a new indeterminate download, then percentComplete stays -1 and that method is never triggered. That's why the check needs to be there in both methods; because the methods only trigger on a value change, and the undetermined property might need to be set if the state goes from having downloads with content length to having downloads without, as well as if a new undetermined download is added when there aren't any others.

(This was why I had the isUndetermined property, it cleanly separates out the undetermined code instead of having it in multiple places)

Comment 22

[:Paolo Amadini]

(In reply to Manish Goregaokar [:manishearth] from comment #21)
> Uh, but we can't set the attribute on a nonexisting element. The downloads
> code currently creates the progress bar only when necessary.

I checked if something in the meantime, but it still seems to me that all the elements are created at the same time (see the "_operational" guard at the start of each setter).

When the view is loaded, it forces a refresh of all the properties, so that all the setters are always invoked:

http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/indicator.js#276

If each setter is responsible for one attribute, the situation should always be consistent.

Comment 23

[Manish Goregaokar [:manishearth]]

After debugging it, yes, the property check is redundant.

Though if we're sticking to the mode=undetermined thing, this might end up with unnecessary JS being executed for an invisible element since the undetermined indicator is dynamic.

To summarize, the changes that need to be made are:

 - rename hasActiveDownloads
 - remove redundant mode change
 - have a callback to _ensureOperational in isDownloading*
 - modify CSS depending on shorlander's input


*We can't just do what percentComplete does and simply return out if the indicator is not operational, since hasActiveDownloads isn't called as often as percentComplete is. If percentComplete fails, there always is the chance of working the next time the percentage is updated, for hasActiveDownloads this would break until the next update of variable state, which might take a while.

Comment 24

[Manish Goregaokar [:manishearth]]

Attachment: Remove redundancy, rename to isUndetermined, add _ensureOperational callbacks

Comment 25

[:Paolo Amadini]

(In reply to Manish Goregaokar [:manishearth] from comment #23)
> *We can't just do what percentComplete does and simply return out if the
> indicator is not operational, since hasActiveDownloads isn't called as often
> as percentComplete is. If percentComplete fails, there always is the chance
> of working the next time the percentage is updated, for hasActiveDownloads
> this would break until the next update of variable state, which might take a
> while.

I don't think this is true. When the view goes from non-operational to operational, all the setters are called again, exactly to avoid unnecessarily complicated logic in the setters themselves.

The reason why all the setters except hasDownloads do nothing on a non-operational view is that the elements should not be loaded at all, to improve startup performance.

Comment 27

[Manish Goregaokar [:manishearth]]

Comment on attachment 8379331 [details]
Proposed UI

Changing ui-r to phlsa as discussed in IRC.

Comment 28

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8379331 [details]
Proposed UI

I think that this works really well!
The graphics needs some tweaking in order to match different platforms.
needinfo to shorlander because chances are he already has something in his pocket there :)

Comment 29

[Manish Goregaokar [:manishearth]]

(In reply to Philipp Sackl [:phlsa] from comment #28)
> Comment on attachment 8379331 [details]
> Proposed UI
> 
> I think that this works really well!
> The graphics needs some tweaking in order to match different platforms.
> needinfo to shorlander because chances are he already has something in his
> pocket there :)

Thanks! I myself am unable to test on anything other than Linux (have Windows, but no time to set up the full environment).

The linux theming *should* work for the others, but I'm not sure. We basically have to separate the themes for [mode=undetermined] and [mode=determined] because the standard behavior of determinate progressbars completely breaks the entire toolbar when used here.

Comment 32

[Matthew N. [:MattN]]

Manish, any update? If you are waiting on shorlander, have you tried asking for the info in #ux?

Comment 33

[Manish Goregaokar [:manishearth]]

I recall doing so in the past, will do once more. I basically need someone to help with the OSX/Windows styling -- I'll probably get jdm to check on OSX, not sure what to do about Windows.

I'll make a tentative patch tomorrow and test it out.

Comment 34

[Manish Goregaokar [:manishearth]]

Attachment: (Patch 1) Rebased patch, linux only

Comment 35

[Manish Goregaokar [:manishearth]]

Attachment: (Patch 2) Tentative OSX and Windows theming

Comment 36

[Manish Goregaokar [:manishearth]]

Testing instructions:
The download indicator should be in the main toolbar

Visit manishearth.anapnea.net/test.php and accept the download. After a short delay, the download indicator should have a tiny progressbar as shown in attachment #8379331 [details] , perhaps of a different color.

Cancel the download and delete any files left behind, it's an infinite download and generates large files.

If you don't want to use the php test page, you can test it locally by:

1. Run `nc -l 8000` (netcat)
2. Visit localhost:8000 in firefox
3. Paste the following into netcat (without the lines)
------------------
HTTP/1.1 200 OK
Server: nginx
Date: Fri, 17 Oct 2014 05:14:45 GMT
Content-Type: text/html
Content-Disposition: attachment; filename=test.txt
Connection: close

This is some content
This is more content

----------

A download prompt should appear in firefox. Continue with the steps as mentioned previously.

Comment 37

[Josh Matthews [:jdm]]

I see a full download indicator for the duration of the download on OS X instead of the travelling one visible in the attachment.

Comment 38

[Matthew N. [:MattN]]

(In reply to Josh Matthews [:jdm] from comment #37)
> I see a full download indicator for the duration of the download on OS X
> instead of the travelling one visible in the attachment.

That could be because the class="plain" sets `-moz-appearance: none` and `progressmeter-undetermined` binding is only applied on Linux:
https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css?rev=341484e03a4a&mark=28-30#28

I don't know whether that small progressmeter is supposed to use native animations or not. Paolo would probably know.

Comment 39

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #38)
> I don't know whether that small progressmeter is supposed to use native
> animations or not. Paolo would probably know.

As far as I can tell, the small progress bar may use platform colors but not other types of native styling or animation.

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 42

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 5 duplicates.
:mak, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 43

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Comment 44

[ed]

(In reply to Manish Goregaokar [:manishearth] from comment #8)

This is the proposed UI for indeterminate downloads.

@shorlander, would this be too distracting for longer downloads? If so, is
there a better way of showing an "indeterminate" download that is less
distracting?

The speed is what makes it distracting. But I think there’s a bigger problem: it’s unclear whether we have an indefinite size, or whether we are waiting for a server to respond.

As someone on a measured rate internet connection where a big download could eat up all my credit, it’s important for me and others in my position to know quickly whether we are dealing with an object of indefinite size or not.