Closed Bug 1497837 Opened 2 years ago Closed 2 years ago

Progressmeter in mode="undetermined" behaves very weird

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 --- fixed

People

(Reporter: Paenglab, Assigned: surkov)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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.
Flags: needinfo?(surkov.alexander)
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.
Frank, sounds like the same bug.
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)
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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Flags: needinfo?(wip.the.gruik)
Attachment #9016172 - Flags: review?(paolo.mozmail)
Duplicate of this bug: 1498075
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-
(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?
Attached patch patchSplinter Review
Attachment #9016545 - Flags: feedback?(richard.marti)
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+
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)
Duplicate of this bug: 1498848
(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)
Attached patch patchSplinter Review
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 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+
Duplicate of this bug: 1498678
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
https://hg.mozilla.org/mozilla-central/rev/ba4289e62b60
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
No longer blocks: 1499843
Duplicate of this bug: 1499843
Duplicate of this bug: 1499830
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.