Bug 63861 – value DOM property returns 1 for indeterminate progress bars

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Lea Verou]

It should return 0. The spec clearly states: "If the progress bar is an indeterminate progress bar, then the value IDL attribute, on getting, must return 0."

Opera does it correctly and returns 0.

[Dominic Cooney]

Created attachment 99592 [details]
Patch

[Kent Tamura]

Comment on attachment 99592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99592&action=review

> Source/WebCore/html/HTMLProgressElement.cpp:100
> +    bool ok = parseToDoubleForNumberType(getAttribute(valueAttr), &value);

You had better use fastGetAttribute().

> Source/WebCore/html/HTMLProgressElement.cpp:108
> +    if (value > maxValue)
> +        return maxValue;
> +
> +    return value;

No need to expand the original 1-line code to 4 lines.

> Source/WebCore/html/HTMLProgressElement.cpp:148
> +bool HTMLProgressElement::isDeterminate() const
>  {
> -    double currentPosition = position();
> -    return (HTMLProgressElement::IndeterminatePosition != currentPosition && HTMLProgressElement::InvalidPosition != currentPosition);
> +    return hasAttribute(valueAttr);

This is incorrect.
isDeterminate() should return true if fastHasAttribute(value) && the value is <= the maximum value.

[Dominic Cooney]

Created attachment 99595 [details]
Patch

[Dominic Cooney]

(In reply to comment #2)
> (From update of attachment 99592 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99592&action=review
> 
> > Source/WebCore/html/HTMLProgressElement.cpp:100
> > +    bool ok = parseToDoubleForNumberType(getAttribute(valueAttr), &value);
> 
> You had better use fastGetAttribute().

Done.

> > Source/WebCore/html/HTMLProgressElement.cpp:108
> > +    if (value > maxValue)
> > +        return maxValue;
> > +
> > +    return value;
> 
> No need to expand the original 1-line code to 4 lines.

Fixed. I guess calling max() twice is OK on an unlikely code path.

> > Source/WebCore/html/HTMLProgressElement.cpp:148
> > +bool HTMLProgressElement::isDeterminate() const
> >  {
> > -    double currentPosition = position();
> > -    return (HTMLProgressElement::IndeterminatePosition != currentPosition && HTMLProgressElement::InvalidPosition != currentPosition);
> > +    return hasAttribute(valueAttr);
> 
> This is incorrect.
> isDeterminate() should return true if fastHasAttribute(value) && the value is <= the maximum value.

No, it is correct. Determinate/indeterminate does not depend on the relationship between value and max. See <http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-progress-element>:

"If the value attribute is omitted, then the progress bar is an indeterminate progress bar. Otherwise, it is a determinate progress bar."

[Dominic Cooney]

Comment on attachment 99595 [details]
Patch

Oops, forgot to use fastHasAttribute.

[Dominic Cooney]

Created attachment 99596 [details]
Patch

[Kent Tamura]

Comment on attachment 99592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99592&action=review

>>> Source/WebCore/html/HTMLProgressElement.cpp:148
>>> +    return hasAttribute(valueAttr);
>> 
>> This is incorrect.
>> isDeterminate() should return true if fastHasAttribute(value) && the value is <= the maximum value.
> 
> No, it is correct. Determinate/indeterminate does not depend on the relationship between value and max. See <http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-progress-element>:
> 
> "If the value attribute is omitted, then the progress bar is an indeterminate progress bar. Otherwise, it is a determinate progress bar."

Oh, I see. I looked at the first paragraph of <progress>.

> The progress is either indeterminate, indicating that progress is being made but that it is not clear how much more work remains to be done before the task is complete (e.g. because the task is waiting for a remote host to respond), or the progress is a number in the range zero to a maximum, giving the fraction of work that has so far been completed.

This is a brief explanation of the concept.  The following sentences in the specification clearly says that it's determinate iff fastHasAttribute(valueAttr).

So, we have rendering bug about this. We should fix RenderProgress::isDeterminate() too, and fix or upadte rendering tests.
I think it should be a separated bug.  Can you remove the isDeterminate() change please?

[Dominic Cooney]

Created attachment 99599 [details]
Patch

[Kent Tamura]

Comment on attachment 99599 [details]
Patch

ok

[Dominic Cooney]

Thank you for many speedy reviews.

(In reply to comment #7)
> So, we have rendering bug about this. We should fix RenderProgress::isDeterminate() too, and fix or upadte rendering tests.
> I think it should be a separated bug.  Can you remove the isDeterminate() change please?

I have filed bug 63889 for updating isDeterminate and the rendering. I can work on that next.

[WebKit Review Bot]

Comment on attachment 99599 [details]
Patch

Clearing flags on attachment: 99599

Committed r90351: <http://trac.webkit.org/changeset/90351>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

It seems the spec has it both ways, and should be fixed.
In http://dev.w3.org/html5/spec/Overview.html chapter 4.10.16, just above "User agent requirements" it says:

The value and max attributes, when present, must have values that are valid floating point numbers. The value attribute, if present, must have a value equal to or greater than zero, and less than or equal to the value of the max attribute, if present, or 1.0, otherwise. The max attribute, if present, must have a value greater than zero.

[Dominic Cooney]

I have filed <http://www.w3.org/Bugs/Public/show_bug.cgi?id=13160> for the spec issue.