Bug 44886

Summary: Reflected attribute input.size wraps on negative values (Chrome), or returns them (Safari)
Product: WebKit Reporter: Aryeh Gregor <ayg>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Minor CC: ap, darin, rasamassen, tabatkins, webkit.review.bot, xqb748
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
darin: review-
Patch
darin: review-, darin: commit-queue-
Patch
none
Patch
darin: review+, darin: commit-queue-
Patch none

Description Aryeh Gregor 2010-08-30 12:24:43 PDT
Test case:

<!doctype html>
<script>
    var el = document.createElement("input");
    el.setAttribute("size", "-1");
    alert(el.size);
</script>

Chrome dev on Ubuntu alerts 4294967295, Safari 5 on XP alerts -1, IE8 alerts 20, Opera 10.60 alerts -1, recent Firefox nightly alerts 0.  The winner per the spec is Firefox:

"""
If a reflecting IDL attribute is an unsigned integer type (unsigned long) then, on getting, the content attribute must be parsed according to the rules for parsing non-negative integers, and if that is successful, and the value is in the range of the IDL attribute's type, the resulting value must be returned. If, on the other hand, it fails or returns an out of range value, or if the attribute is absent, the default value must be returned instead, or 0 if there is no default value.
"""
http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#reflecting-content-attributes-in-idl-attributes

There is no default value specified, so since parsing as a non-negative integer fails, it should return 0.  Both Chrome and Safari need to change here -- should I file separate bugs for both of them, or is this one fine by itself?
Comment 1 rasamassen 2011-05-26 17:59:46 PDT
IE9 is, as of this date, actually the only browser that seems to parse this correctly.  IE9 throws an "Invalid property value" error upon setting size = -1.  Per rule 7 of the "rules for parsing non-negative integers," this is the appropriate response.  Webkit should throw an error.  Only IE9 currently throws an error.

If that's a misreading, then Webkit should throw 20, but I think that's correct.
Comment 2 Aryeh Gregor 2011-05-27 08:15:51 PDT
(In reply to comment #1)
> IE9 is, as of this date, actually the only browser that seems to parse this correctly.  IE9 throws an "Invalid property value" error upon setting size = -1.  Per rule 7 of the "rules for parsing non-negative integers," this is the appropriate response.  Webkit should throw an error.  Only IE9 currently throws an error.

You mean that setAttribute() should throw?  setAttribute() never throws, if the first argument is a valid attribute name:

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#dom-element-setattribute

It will always set the content attribute to the given value, and getAttribute() will return exactly the string that was set (in this case, "-1").  The question is what the IDL attribute (element.size) should return.

Rule 7 of the rules for parsing non-negative integers says that it has to return an error, as you say.  This doesn't mean throw an exception.  The rules for reflection cited in comment #0 explain what to do in this case: "If, on the other hand, it fails or returns an out of range value, or if the attribute is absent, the default value must be returned instead, or 0 if there is no default value."

Whether to throw an exception when doing el.size = -1 is a separate issue.  I'm talking about parsing the content attribute here, whether it's set by setAttribute() or just <input size="-1"> (throwing in the latter case is of course impossible).


Actually, though, the spec has changed since I wrote comment #0.  See <http://www.w3.org/Bugs/Public/show_bug.cgi?id=10517>.  There's now a default value of 20:

"The size IDL attribute is limited to only non-negative numbers greater than zero and has a default value of 20."
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#attr-input-size

So the correct thing to alert per spec is now 20, not 0.  Firefox 5.0a2 now alerts 20, as does IE9.  Opera 11.10 still alerts -1.  The spec/IE/Firefox behavior makes the most sense and is probably the most compatible, so it makes sense to go with that.
Comment 3 rasamassen 2011-05-27 08:36:44 PDT
Being an beginning to intermediate javascript programmer, using jQuery 1.6 on the input element, the code $('input').prop('size','-1'); results in IE9 throwing an "Invalid property value" exception.  That is not the correct response?  I assumed IE was wrong until the spec said through an error.  If that's not correct, then a bug should be reported to IE.
Comment 4 Aryeh Gregor 2011-05-27 09:54:27 PDT
jQuery is a huge, complicated third-party library built on top of standard browser APIs.  As far as I understand it, not being a jQuery user, .prop() sets IDL attributes and .attr() sets content attributes.  This bug discusses *reading* an IDL attribute when the content attribute is -1.  .prop("size", -1) *sets* the IDL attribute, which is an entirely separate thing.

What this bug in talking about:

* Standard: input.setAttribute("size", "-1"); alert(input.size)
* jQuery: input.attr("size", "-1"); alert(input.prop("size"))

What you're talking about:

* Standard: input.size = -1
* jQuery: input.prop("size", -1)

These are separate things.  Behavior differences in the second case should probably be in a separate bug.
Comment 5 Antaryami Pandia 2011-09-23 22:22:02 PDT
Created attachment 108576 [details]
Patch

Proposed patch.
Comment 6 Darin Adler 2011-09-24 12:16:13 PDT
Comment on attachment 108576 [details]
Patch

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

Patch is generally good, but not quite right.

> Source/WebCore/html/HTMLInputElement.cpp:1017
>  int HTMLInputElement::size() const
>  {
> -    return m_size;
> +    if (m_size > 0)
> +        return m_size;
> +
> +    return defaultSize;
>  }

It’s not so great to have an m_size that is wrong rather than actually containing the size. This logic should instead go into HTMLInputElement::parseMappedAttribute in the code that sets m_size. That code could be further improved by only calling setNeedsLayoutAndPrefWidthsRecalc if m_size changes.

The alternative would be to remove m_size entirely and put all the parsing logic into HTMLInputElement::size. That would be good because it would make the object smaller. But it would make the optimization to call setNeedsLayoutAndPrefWidthsRecalc less often difficult.
Comment 7 Darin Adler 2011-09-24 12:17:00 PDT
(In reply to comment #1)
> IE9 is, as of this date, actually the only browser that seems to parse this correctly.  IE9 throws an "Invalid property value" error upon setting size = -1.  Per rule 7 of the "rules for parsing non-negative integers," this is the appropriate response.  Webkit should throw an error.  Only IE9 currently throws an error.

To change that we’d need to change the HTMLInputElement::setSize function. If a change is needed here, then we do need a separate function.
Comment 8 Antaryami Pandia 2011-09-25 01:12:13 PDT
Created attachment 108602 [details]
Patch

Modified patch with Darin's review comments.
Comment 9 Darin Adler 2011-09-25 18:54:00 PDT
Comment on attachment 108602 [details]
Patch

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

Thanks for taking this approach.

> Source/WebCore/html/HTMLInputElement.cpp:779
> +        if (!attr->isNull()) {
> +            int value = attr->value().toInt();
> +            m_size = value > 0 ? value : defaultSize;
> +        }

The way this is written, it means that if the attribute is removed, m_size will keep its old value. That’s wrong. We want m_size to get set to defaultSize in that case. Given that 0 turns into defaultSize, we can just remove the isNull check entirely, since toInt will return 0 in that case.

We should make sure we have a test case for the transition from an attribute to not having an attribute, since that test case would have revealed this mistake.
Comment 10 Antaryami Pandia 2011-09-25 21:21:52 PDT
(In reply to comment #9)
> (From update of attachment 108602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108602&action=review
> 
> Thanks for taking this approach.
> 
> > Source/WebCore/html/HTMLInputElement.cpp:779
> > +        if (!attr->isNull()) {
> > +            int value = attr->value().toInt();
> > +            m_size = value > 0 ? value : defaultSize;
> > +        }
> 
> The way this is written, it means that if the attribute is removed, m_size will keep its old value. That’s wrong. We want m_size to get set to defaultSize in that case. Given that 0 turns into defaultSize, we can just remove the isNull check entirely, since toInt will return 0 in that case.
> 
Yes, I should have done away with the if check.But the only thought behind that was, since m_size is initialized with defaultSize it will have this value.
I will remove the if check in the next patch.

> We should make sure we have a test case for the transition from an attribute to not having an attribute, since that test case would have revealed this mistake.

Are you talking of a test case which looks as :-

<!doctype html>
<script>
    var el = document.createElement("input");
    el.setAttribute("size", "-1");
    alert(el.size);

    el.removeAttribute("size");
    alert(el.size);
</script>

                 OR

<!doctype html>
<script>
    var el = document.createElement("input");
    alert(el.size);
</script>
Comment 11 Darin Adler 2011-09-26 09:31:01 PDT
(In reply to comment #10)
> Are you talking of a test case which looks as :-
> 
> <!doctype html>
> <script>
>     var el = document.createElement("input");
>     el.setAttribute("size", "-1");
>     alert(el.size);
> 
>     el.removeAttribute("size");
>     alert(el.size);
> </script>

Yes. A case roughly like the above, but the above case is fine. The bug will happen if the initial size is valid, so "10" instead of "-1".

With theist last patch we will get 10 for el.size but we should get 20. And the element will also be the wrong size.
Comment 12 Antaryami Pandia 2011-09-26 22:32:58 PDT
(In reply to comment #11)
> Yes. A case roughly like the above, but the above case is fine. The bug will happen if the initial size is valid, so "10" instead of "-1".
> 
> With theist last patch we will get 10 for el.size but we should get 20. And the element will also be the wrong size.

Thanks for the review.Will Upload a modified patch.
Comment 13 Antaryami Pandia 2011-09-26 22:34:35 PDT
Created attachment 108788 [details]
Patch

Patch after incorporating Darin's review comments.
Comment 14 Antaryami Pandia 2011-09-26 22:44:35 PDT
Created attachment 108789 [details]
Patch

Modified Patch with review comments.
Comment 15 Darin Adler 2011-09-27 09:39:58 PDT
Comment on attachment 108789 [details]
Patch

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

r=me but I would prefer that the new test be improved as I describe in the comments, so I have not set commit-queue+

> LayoutTests/fast/dom/HTMLInputElement/input-default-size.html:10
> +<script src="script-tests/input-default-size.js"></script>

This is no longer the preferred style for new tests. Instead we prefer to put the actual test inside the HTML file. The separate ".js" file structure is only useful for pure JavaScript tests, and even in that case, only useful in theory.

Also, the coverage in this test is too narrow. A while test just for this one case seems overkill. We should cover more transitions in size, to make sure we test all aspects of the code. Like size-attribute but with a focus on changing from previous values of the size attribute.
Comment 16 Antaryami Pandia 2011-09-27 22:40:25 PDT
Created attachment 108965 [details]
Patch

Patch with updated test case.
Comment 17 Darin Adler 2011-09-28 07:48:15 PDT
Comment on attachment 108965 [details]
Patch

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

> LayoutTests/fast/dom/HTMLInputElement/input-size-attribute.html:20
> +input.setAttribute("size", "-1");
> +shouldBe("input.size", "20");

There’s another way to write this that makes the test output clearer:

    shouldBe("input.setAttribute('size', '-1'); input.size", "20");

By putting the work inside the macro you can actually see it in the test output. We can always patch it to that new style after landing the initial version, though.
Comment 18 WebKit Review Bot 2011-09-28 08:51:25 PDT
Comment on attachment 108965 [details]
Patch

Clearing flags on attachment: 108965

Committed r96224: <http://trac.webkit.org/changeset/96224>
Comment 19 WebKit Review Bot 2011-09-28 08:51:30 PDT
All reviewed patches have been landed.  Closing bug.