825908 - HTMLProgressElement should not be in the indeterminate state when value is specified but isn't a number according to the specs

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Mounir Lamouri (:mounir)]

I do not believe we really should do that but Presto, Trident and Webkit do that quite consistently. It seems that we are very likely going to do that sooner or later.

To rephrase the summary, the following snippets should have progress.position = 0 instead of progress.position = -1:
<progress value=''></progress>
<progress value='foobar'></progress>

However, the following one should have progress.position = -1 (as we currently do):
<progress></progress>

In my opinion, setting value to NaN should make the value being ignored, thus the progress element should be indeterminate. However, latest versions of Internet Explorer, Chrome and Opera all implement what the spec say and Hixie refused to change them.

See http://www.w3.org/Bugs/Public/show_bug.cgi?id=11937

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: update HTMLProgressElement to return 0.0 instead of -1.0 for non-number values

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 697498 [details] [diff] [review]
update HTMLProgressElement to return 0.0 instead of -1.0 for non-number values

This seems to be sufficient to pass the current tests we have.  Do we need to add tests for NaN and friends?

Comment 3

[Mounir Lamouri (:mounir)]

Comment on attachment 697498 [details] [diff] [review]
update HTMLProgressElement to return 0.0 instead of -1.0 for non-number values

Review of attachment 697498 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should change IsIndeterminate() too.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Mounir Lamouri (:mounir) from comment #3)
> I think we should change IsIndeterminate() too.

Change it how?  Requiring that the check for eDoubleValue be done in callers or something else?

Comment 5

[Mounir Lamouri (:mounir)]

IsIndeterminate should be something like that:
  const nsAttrValue* attrValue = mAttrsAndChildren.GetAttr(nsGkAtoms::value);
  if (!attrValue) {
    return false;
  }
  return attrValue->Type() != nsAttrValue::eDoubleValue;

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Mounir Lamouri (:mounir) from comment #5)
> IsIndeterminate should be something like that:
>   const nsAttrValue* attrValue = mAttrsAndChildren.GetAttr(nsGkAtoms::value);
>   if (!attrValue) {
>     return false;
>   }
>   return attrValue->Type() != nsAttrValue::eDoubleValue;

Hm.  With this additional modification, I am seeing:

TEST-UNEXPECTED-FAIL | unknown test url | <progress> indeterminate state should be true - got false, expected true

when setting value to null and max to 1.0, which I think indicates that the test needs to be changed.  Fine.  But the previous test, which sets value to null and max to null, succeeds, expecting the progress element to be in an indeterminate state.  I don't see the reason for the difference.

Comment 7

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.