473847 - <progressmeter> + -moz-appearance:none + large |max| = wonkyness

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P1)

Description

[Justin Dolske [:Dolske]]

Attachment: Testcase

Found while switching videocontrols over to use a <progressmeter>.

When incrementing the value of the progressmeter, I found that the bar was jumping around randomly. This seems to require using -moz-appearance:none and using a large |max| attribute on the progressmeter. Without either condition, the bar works normally.

Looks like a layout bug, because resizing the window (without changing the progressmeter's value) also makes it jump around as it resizes.

Testcase attached, it increments a red progress bar in 10% increments every 500ms. You should see a progression from 0% to 100%, instead it jumps around. If |max| is < ~10,000, it seems to work though!

Comment 1

[Justin Dolske [:Dolske]]

Also happens on Linux, even without the -moz-appearance:none (in fact, deleting the whole <html:style/> block in the testcase).

Comment 2

[Justin Dolske [:Dolske]]

Windows is weird too; the bar doesn't jump around randomly, but seems to just immediately peg at 100%. It's even broken on a static XUL page with just:
<progressmeter value="500" max="1000"/>

Comment 3

[Neil Deakin]

Attachment: fix

The issue here is that multiplying the flex and the sizeRemaining overflows. Instead, we divide first. Might not be the best solution, but it fixes this progressmeter issue.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why not cast to PRInt64 instead, and then back to PRInt32?

Comment 5

[Neil Deakin]

Attachment: yes, that makes more sense

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 357711 [details] [diff] [review]
yes, that makes more sense


r+sr=dbaron, although I much prefer construction-style casts in C++, i.e. replacing

>+            PRInt32 newSize = pref + (PRInt32)((PRInt64)sizeRemaining * flex / spacerConstantsRemaining);

with ... = pref + PRInt32(PRInt64(sizeRemaining) * flex / spacerConstantsRemaining);

etc.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking blocker is blocking!

Comment 8

[Justin Dolske [:Dolske]]

Is this ready to land on 1.9.1 too? I'd like to land the video scrubber tomorrow, which needs this. [I'd be happy to land and watch the tree for this patch too.]