1497837 - Progressmeter in mode="undetermined" behaves very weird

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Richard Marti (:Paenglab)]

On TB we have a progressmeter in the statusbar. When it goes to undetermined over a full cycle of the animation, the progressbar grows when it starts again at the left.

STR: To simulate it, go to Prefs/Privacy&Security/use a master password.
Open the DevTools Inspector and navigate to the progressmeter.
Change now the mode from "determined" to "undetermined".
See what happens to the dialog.

I can imagine that this also affects the download dialog when the download size isn't determined.

Comment 1

[Franck (Wip)]

There is a similar visual artifact in Firefox Nightly with the download progress meter.
When resuming a paused download, a green progress bar appears outside the meter on its left, just during the transition.

I suppose it is the same bug.
If not, just tell me, I'll fill a new bug.

Comment 2

[Richard Marti (:Paenglab)]

Frank, sounds like the same bug.

Comment 3

[alexander :surkov (:asurkov)]

It appears this is a layout issue. When .progress-bar width is set, then .progress-remainder slack width may be also changed:

"before: stack w: 583, stack h: 12, new width: 145"
"after: stack w: 583, stack h: 12, w: 145, l: 434.0253614749996"
"before: stack w: 583, stack h: 12, new width: 145"
"after: stack w: 584, stack h: 12, w: 145, l: 439.6953142374998" -> stack's width was changed for no apparent reason
"before: stack w: 584, stack h: 12, new width: 146"
"after: stack w: 595, stack h: 12, w: 146, l: 449.13812939500025"
"before: stack w: 595, stack h: 12, new width: 148"
"after: stack w: 615, stack h: 12, w: 148, l: 467.32973542999935"

and then after a while, geometry normalizes to go crazy again.

Also when I repeat STR and the progressmeter starts dancing, I select a .progress-bar spacer in dev inspector, then the progressmeter behavior normalizes.

I guess we could try to workaround it right in CE, but wonder if anyone around (or from layout team?) is curious to take a look at the issue?

Comment 4

[alexander :surkov (:asurkov)]

Apparently, progressmeter[mode="undertermined"] was used on linux only. So rolling this back will fix this bug on win and os x I think. I'm curious though, if this bug is reproducible on linux.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 7

[Richard Marti (:Paenglab)]

Comment on attachment 9016172 [details] [diff] [review]
patch

On Linux there is the same effect with growing to the right.

And on Windows it shows now 100% in print preview when waiting.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Richard Marti (:Paenglab) from comment #7)
> Comment on attachment 9016172 [details] [diff] [review]
> patch
> 
> On Linux there is the same effect with growing to the right.

I wonder whether win/osx progressmeter implementation can be shared with linux as well.

> And on Windows it shows now 100% in print preview when waiting.

is it different from bug 1498140?

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 9016545 [details] [diff] [review]
patch

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

It still shows 100% (completely filled) when undetermined on Windows in printProgress.xul. When I simulate the undetermined on a other progressmeter it's empty. In printProgress.xul the progress-bar gets a flex="1" on the other progressmeters not. Maybe this is a other bug. So if the empty progressmeter is intended and the printProgress is a different bug, then f+.

I can't reproduce the fallback to 0% on end. Maybe my PC is too fast.

::: toolkit/content/widgets/progressmeter.js
@@ -147,5 @@
> -      window.requestAnimationFrame(nextStep);
> -    };
> -
> -    window.requestAnimationFrame(nextStep);
> -  }

The closing bracket shouldn't be removed.

Comment 11

[:Paolo Amadini]

By looking at the latest patch, it seems the JavaScript implementation of the animation that we have for XUL progressmeter may be unnecessary. Given that it turned out that, to verify this Custom Element loading and rendering, we may have to test all our uses in detail, I'm wondering if we should change strategy here, and just apply the latest suggestion in bug 1428869 and migrate everything to "html:progress" with no Custom Element, assuming that works?

https://searchfox.org/mozilla-central/search?q=%3Cprogressmeter

This is something we need to do anyways, and may allow us to remove the XUL frame implementation and a few other platform pieces that depend on the XBL binding structure:

https://searchfox.org/mozilla-central/source/layout/xul/nsProgressMeterFrame.cpp

Comment 13

[alexander :surkov (:asurkov)]

(In reply to :Paolo Amadini from comment #11)
> By looking at the latest patch, it seems the JavaScript implementation of
> the animation that we have for XUL progressmeter may be unnecessary. Given
> that it turned out that, to verify this Custom Element loading and
> rendering, we may have to test all our uses in detail, I'm wondering if we
> should change strategy here, and just apply the latest suggestion in bug
> 1428869 and migrate everything to "html:progress" with no Custom Element,
> assuming that works?

I thought about this given not large amount of progressmeters in sources, but thought that fixing regressions should be an easier way. I can give it a try though in spite of bug 1498140, which I sort of feel lost about.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 9017077 [details] [diff] [review]
patch

agreed, conversion to HTML:progress looks tempting, but it takes time (I tried this approach in bug 1498140, it's doable but meticulous work). Given ongoing release timeframe, it's worth to consider this patch.

Comment 16

[:Paolo Amadini]

Comment on attachment 9017077 [details] [diff] [review]
patch

I still think we should try to land bug 1428869 for this release, since there's no point in handling potential regressions and repeat the testing in two different releases in a row.

However, I see no harm in landing this patch as well in the meantime. If it causes more regressions, we'll fix them by converting to the HTML progress element.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4289e62b60
Progressmeter in mode=undetermined behaves very weird, r=paolo, f=paenglab

Comment 19

[Cosmin Sabou [:CosminS]]

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