Bug 63861 - value DOM property returns 1 for indeterminate progress bars
Summary: value DOM property returns 1 for indeterminate progress bars
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dominic Cooney
URL: http://jsfiddle.net/leaverou/p3nHQ/
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-02 11:37 PDT by Lea Verou
Modified: 2011-07-06 06:25 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.87 KB, patch)
2011-07-03 20:58 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (5.82 KB, patch)
2011-07-03 22:19 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (5.82 KB, patch)
2011-07-03 22:26 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (4.35 KB, patch)
2011-07-03 22:45 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lea Verou 2011-07-02 11:37:18 PDT
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.
Comment 1 Dominic Cooney 2011-07-03 20:58:53 PDT
Created attachment 99592 [details]
Patch
Comment 2 Kent Tamura 2011-07-03 22:07:18 PDT
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.
Comment 3 Dominic Cooney 2011-07-03 22:19:55 PDT
Created attachment 99595 [details]
Patch
Comment 4 Dominic Cooney 2011-07-03 22:22:09 PDT
(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."
Comment 5 Dominic Cooney 2011-07-03 22:25:09 PDT
Comment on attachment 99595 [details]
Patch

Oops, forgot to use fastHasAttribute.
Comment 6 Dominic Cooney 2011-07-03 22:26:26 PDT
Created attachment 99596 [details]
Patch
Comment 7 Kent Tamura 2011-07-03 22:35:53 PDT
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?
Comment 8 Dominic Cooney 2011-07-03 22:45:54 PDT
Created attachment 99599 [details]
Patch
Comment 9 Kent Tamura 2011-07-03 22:47:47 PDT
Comment on attachment 99599 [details]
Patch

ok
Comment 10 Dominic Cooney 2011-07-03 22:48:49 PDT
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.
Comment 11 WebKit Review Bot 2011-07-03 22:57:06 PDT
Comment on attachment 99599 [details]
Patch

Clearing flags on attachment: 99599

Committed r90351: <http://trac.webkit.org/changeset/90351>
Comment 12 WebKit Review Bot 2011-07-03 22:57:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Yael 2011-07-06 05:47:00 PDT
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.
Comment 14 Dominic Cooney 2011-07-06 06:25:20 PDT
I have filed <http://www.w3.org/Bugs/Public/show_bug.cgi?id=13160> for the spec issue.