Mook (songbird dead account) Assignee
	Description • 16 years ago

<progressmeter> currently (when determined) has possible values of 0 to 100 in increments of 1.  For a very wide progressmeter, this means the value can be coarse.  I think it would be useful to add a max= attribute (a positive integer) that can be used to determine the total possible value, defaulting to 100 (so there is no behaviour change).

I was thinking of having a custom min= as well, but that gets complicated (needing to bound both values correctly, needing to shift values) when the (XUL widget) user can do that easily.

	Mook (songbird dead account) Assignee
	Comment 1 • 16 years ago

Patch for Mac (it just has to be special!) and everybody-else.  Includes reftest, but I'm not sure if I did that part right.  (It compares 50/100 against 99/198)

	Neil Deakin
	Comment 2 • 16 years ago

Comment on attachment 327920 [details] [diff] [review]
add max= attribute to <xul:progressbar>

>diff --git a/layout/xul/base/src/nsProgressMeterFrame.cpp b/layout/xul/base/src/nsProgressMeterFrame.cpp
>-    if (flex < 0) flex = 0;
>-    if (flex > 100) flex = 100;
>+    PRInt32 maxFlex = maxValue.ToInteger(&error);
>+    if (maxValue.IsEmpty()) {
>+      maxFlex = 100;

Seems like this should default to 100 if there was an error parsing.

>+      <property name="max"
>+                onget="return this.getAttribute('max') || '100';"
>+                onset="this.setAttribute('max', Math.max(val, 1));"/>

Similarly here.

Ideally, setting the max to something will also adjust the value to the maximum if it is higher.

	Stephen Lau
	Comment 3 • 16 years ago

Hi Neil - I've updated Mook's patch so it applies against mozilla-central tip, and tried to take into account your comments for defaulting to 100 in the event of a parse error.
Mind reviewing this again?

	Neil Deakin
	Comment 4 • 16 years ago

Comment on attachment 340452 [details] [diff] [review]
Updated patch for tip & for enndeakin's comments


>+      <property name="max"
>+                onget="return this.getAttribute('max') || '100';"
>+                onset="if (isNaN(val)) this.setAttribute('max', 100); else this.setAttribute('max', Math.max(val, 1)); if (this.getAttribute('value') > val) this.setAttribute('value', Math.max(val, 1));"/>

This is quite complicated. It should go in a <setter> block, or at least be better spaced out. Also, a setter needs to return 'val' as well. Updating the value should be a matter of just setting this.value = this.value i would think. Something like the following perhaps?

onset="this.setAttribute("max", isNaN(val) ? 100 : Math.max(val, 1);
       this.value = this.value;
       return val;"

	Stephen Lau
	Comment 5 • 16 years ago

Thanks, I've updated to continue using the onset with your cleaner suggestion.  (Didn't occur to me that simply doing this.value = this.value would invoke the getter's logic for capping it at the max)

	Serge Gautherie (:sgautherie)
	Comment 6 • 16 years ago

Comment on attachment 344702 [details] [diff] [review]
updated patch with better onset

>--- a/layout/xul/base/src/nsProgressMeterFrame.cpp
>+++ b/layout/xul/base/src/nsProgressMeterFrame.cpp
>@@ -129,15 +129,27 @@ nsProgressMeterFrame::AttributeChanged(P
>+    if (!NS_SUCCEEDED(error) || maxValue.IsEmpty()) {

s/!NS_SUCCEEDED/NS_FAILED/ !?

	Stephen Lau
	Comment 7 • 16 years ago

Good idea Serge, here's a new patch that uses NS_FAILED instead of NS_SUCCEEDED. I assume this doesn't need new r/sr since it's so trivial.

	(no longer active)
	Comment 8 • 16 years ago

Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/0c2c2c895e5d>

	Dave Townsend [:mossop]
	Comment 9 • 16 years ago

Pushed a bustage fix for this: http://hg.mozilla.org/mozilla-central/rev/b9dd40cbf22d

	Dave Townsend [:mossop]
	Comment 10 • 16 years ago

Oops, missed a bit: http://hg.mozilla.org/mozilla-central/rev/5fdc78311d6b

	Stephen Lau
	Comment 11 • 16 years ago

augh, sorry bout that. Thanks Mossop!

	Markus Stange [:mstange]
	Comment 12 • 15 years ago

It looks like the added reftest hasn't ever been run because toolkit/content/tests/reftests/reftest.list isn't included in layout/reftests/reftest.list.

	David Baron :dbaron:
	Comment 13 • 10 years ago

(In reply to Markus Stange [:mstange] from comment #12)
> It looks like the added reftest hasn't ever been run because
> toolkit/content/tests/reftests/reftest.list isn't included in
> layout/reftests/reftest.list.

I'll fix that in bug 989688.