Progressmeter in mode="undetermined" behaves very weird

RESOLVED FIXED in Firefox 64

Status

()

defect
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: Paenglab, Assigned: surkov)

Tracking

({regression})

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 unaffected, firefox64 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

7 months ago
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.

Updated

7 months ago
Flags: needinfo?(surkov.alexander)

Comment 1

7 months ago
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.
Reporter

Comment 2

7 months ago
Frank, sounds like the same bug.
Assignee

Comment 3

7 months ago
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?
Flags: needinfo?(surkov.alexander) → needinfo?(wip.the.gruik)
Assignee

Comment 4

7 months ago
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.
Assignee

Comment 5

7 months ago
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Flags: needinfo?(wip.the.gruik)
Attachment #9016172 - Flags: review?(paolo.mozmail)

Updated

7 months ago
Duplicate of this bug: 1498075
Reporter

Comment 7

7 months ago
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.
Attachment #9016172 - Flags: feedback-
Assignee

Comment 8

7 months ago
(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?
Assignee

Comment 9

7 months ago
Posted patch patchSplinter Review
Attachment #9016545 - Flags: feedback?(richard.marti)
Reporter

Comment 10

7 months ago
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.
Attachment #9016545 - Flags: feedback?(richard.marti) → feedback+

Comment 11

7 months ago
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
Flags: needinfo?(surkov.alexander)
Reporter

Updated

7 months ago
Duplicate of this bug: 1498848
Assignee

Comment 13

7 months ago
(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.
Flags: needinfo?(surkov.alexander)
Assignee

Comment 14

7 months ago
Posted patch patchSplinter Review
Assignee

Comment 15

7 months ago
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.
Attachment #9017077 - Flags: review?(paolo.mozmail)

Comment 16

7 months ago
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.
Attachment #9017077 - Flags: review?(paolo.mozmail) → review+
Assignee

Updated

7 months ago
Duplicate of this bug: 1498678

Comment 18

7 months ago
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4289e62b60
Progressmeter in mode=undetermined behaves very weird, r=paolo, f=paenglab

Comment 19

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba4289e62b60
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee

Updated

7 months ago
No longer blocks: 1499843
Duplicate of this bug: 1499843

Updated

7 months ago
Duplicate of this bug: 1499830
Assignee

Updated

7 months ago
Attachment #9016172 - Attachment is obsolete: true
Attachment #9016172 - Flags: review?(paolo.mozmail)
You need to log in before you can comment on or make changes to this bug.