Bug 44886

Summary:

Reflected attribute input.size wraps on negative values (Chrome), or returns them (Safari)

Product:

WebKit

Reporter:

Aryeh Gregor <ayg>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Minor

CC:

ap, darin, rasamassen, tabatkins, webkit.review.bot, xqb748

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Attachments:

Description	Flags
Patch	darin: review-
Patch	darin: review-, darin: commit-queue-
Patch	none
Patch	darin: review+, darin: commit-queue-
Patch	none

Aryeh Gregor

Reported 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?

Attachments
Patch (4.68 KB, patch) 2011-09-23 22:22 PDT, Antaryami Pandia	darin: review-	Details Formatted Diff Diff
Patch (5.05 KB, patch) 2011-09-25 01:12 PDT, Antaryami Pandia	darin: review- darin: commit-queue-	Details Formatted Diff Diff
Patch (5.54 KB, patch) 2011-09-26 22:34 PDT, Antaryami Pandia	no flags	Details Formatted Diff Diff
Patch (7.27 KB, patch) 2011-09-26 22:44 PDT, Antaryami Pandia	darin: review+ darin: commit-queue-	Details Formatted Diff Diff
Patch (7.37 KB, patch) 2011-09-27 22:40 PDT, Antaryami Pandia	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

rasamassen

Comment 1 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.

Aryeh Gregor

Comment 2 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.

rasamassen

Comment 3 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.

Aryeh Gregor

Comment 4 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.

Antaryami Pandia

Comment 5 2011-09-23 22:22:02 PDT

Created attachment 108576 [details] Patch Proposed patch.

Darin Adler

Comment 6 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.

Darin Adler

Comment 7 2011-09-24 12:17:00 PDT

Antaryami Pandia

Comment 8 2011-09-25 01:12:13 PDT

Created attachment 108602 [details] Patch Modified patch with Darin's review comments.

Darin Adler

Comment 9 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.

Antaryami Pandia

Comment 10 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>

Darin Adler

Comment 11 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.

Antaryami Pandia

Comment 12 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.

Antaryami Pandia

Comment 13 2011-09-26 22:34:35 PDT

Created attachment 108788 [details] Patch Patch after incorporating Darin's review comments.

Antaryami Pandia

Comment 14 2011-09-26 22:44:35 PDT

Created attachment 108789 [details] Patch Modified Patch with review comments.

Darin Adler

Comment 15 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.

Antaryami Pandia

Comment 16 2011-09-27 22:40:25 PDT

Created attachment 108965 [details] Patch Patch with updated test case.

Darin Adler

Comment 17 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.

WebKit Review Bot

Comment 18 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>

WebKit Review Bot

Comment 19 2011-09-28 08:51:30 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.