Closed Bug 536891 Opened 16 years ago Closed 15 years ago

Negative maxlength should be treated the same as unspecified maxlength

Categories

(Core :: DOM: Core & HTML, enhancement)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a3

People

(Reporter: ayg, Assigned: mounir)

References

(
URL
)

Details

(Keywords: dev-doc-complete, html5)

Attachments

(1 file, 3 obsolete files)

Patch v0.2.2
8.29 KB, patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/532.6 (KHTML, like Gecko) Chrome/4.0.266.0 Safari/532.6 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091227 Minefield/3.7a1pre From bug 535043 comment 1: Note: as far as I can tell, maxlength="-1" is equivalent to maxlength="0" in Gecko, including on inputs. This is a bug per HTML5 AFAICT. maxlength is to be parsed as a nonnegative integer, and an error is the same as not specifying the attribute; and the algorithm for parsing a nonnegative integer implies that the presence of "-" is an error: <http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#attr-fe-maxlength> <http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-non-negative-integers> Test case: data:text/html,<!doctype html><body> <input maxlength=-2><input> <script> var inputs = document.getElementsByTagName('input'); document.write('<p>First input has maxLength ' + inputs[0].maxLength + ' while second is ' + inputs[1].maxLength + '; both should be -1'); </script> On Minefield I get 0 and -1. On WebKit, both maxLengths evaluate to 524288 (Chrome 4.0.266.0). Opera 10.10 has -2 and -1 respectively. IE8 has -2 and 2147483647. WebKit and Opera both permit typing into the input with maxlength="-2", which Firefox does not (forgot to try on IE) -- Firefox really treats it the same as a maxlength of 0. Reproducible: Always
Marking as blocking bug 535043 since I think this is what you're supposed to do with followups, please correct it if I'm wrong.
Blocks: 535043
Assignee: nobody → mounir.lamouri
URL: http://dev.w3.org/html5/spec/infrastr...
Severity: minor → enhancement
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: html5
Version: unspecified → Trunk
Attached patch Patch v0.1 (obsolete) — — Splinter Review
This patch is adding a new way to parse non negative int values so maxLength='-X' is see as maxLength='-1' ($URL explains why it has to be done this way). This new parsing way follows rules from the specs [1]. [1] http://dev.w3.org/html5/spec/infrastructure.html#rules-for-parsing-non-negative-integers Olli, I am asking a review as your reviewed the initial patch, bug 535043.
Attachment #428244 - Flags: review?(Olli.Pettay)
Comment on attachment 428244 [details] [diff] [review] Patch v0.1 >diff -r beb0ffcd0694 content/base/src/nsAttrValue.cpp >--- a/content/base/src/nsAttrValue.cpp Mon Feb 22 16:18:17 2010 +0300 >+++ b/content/base/src/nsAttrValue.cpp Mon Feb 22 18:30:21 2010 +0100 >@@ -1085,16 +1085,39 @@ nsAttrValue::ParseIntWithBounds(const ns > PRInt32 val = PR_MAX(originalVal, aMin); > val = PR_MIN(val, aMax); > strict = strict && (originalVal == val); > SetIntValueAndType(val, eInteger, strict ? nsnull : &aString); > > return PR_TRUE; > } > >+PRBool >+nsAttrValue::ParseNonNegativeIntValue(const nsAString& aString) >+{ >+ ResetIfSet(); >+ >+ PRInt32 ec; >+ PRBool strict; >+ PRInt32 originalVal = StringToInteger(aString, &strict, &ec); >+ if (NS_FAILED(ec)) { >+ return PR_FALSE; >+ } >+ >+ PRInt32 val = originalVal; >+ if (val < 0) { >+ val = -1; // -1 means the value was not correct >+ } >+ val = PR_MIN(val, PR_INT32_MAX); I don't understand this. How could val be bigger than PR_INT32_MAX? I guess you want PRInt32 val = originalVal; val = PR_MAX(val, -1); >+ * Parse a string value into a non-negative integer. >+ * This method follows the rules for parsing non-negative integer from: >+ * http://dev.w3.org/html5/spec/infrastructure.html#rules-for-parsing-non-negative-integers This doesn't explain that in which way the error handling works in this particular implementation. You need to say something about -1. >+ * >+ * @param aString the string to parse >+ * @return whether the value could be parsed >+ */ >+ PRBool ParseNonNegativeIntValue(const nsAString& aString); >+ >+ /** >diff -r beb0ffcd0694 content/html/content/test/test_bug536891.html >--- /dev/null Thu Jan 01 00:00:00 1970 +0000 >+++ b/content/html/content/test/test_bug536891.html Mon Feb 22 18:30:21 2010 +0100 >@@ -0,0 +1,45 @@ >+<!DOCTYPE HTML> >+<html> >+<!-- >+https://bugzilla.mozilla.org/show_bug.cgi?id=536891 >+--> >+<head> >+ <title>Test for Bug 536891</title> >+ <script type="application/javascript" src="/MochiKit/packed.js"></script> >+ <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> >+ <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> >+</head> >+<body> >+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=536891">Mozilla Bug 536891</a> >+<p id="display"></p> >+<div id="content" style="display: none"> >+<textarea id="t" maxlength="-2"></textarea> >+<input id="i" type="text" maxlength="-2"> >+<input id="p" type="password" maxlength="-2"> >+</div> >+<pre id="test"> >+<script type="application/javascript"> >+ >+/** Test for Bug 536891 **/ >+ >+function checkNegativeMaxLength(element) >+{ >+ /* maxLength is set to -2 initially in the document, see above */ >+ is(element.maxLength, -1, "negative maxLength should be considered invalid and represented as -1"); >+ >+ element.setAttribute('maxLength', -15); >+ is(element.maxLength, -1, "negative maxLength is not processed correctly when set dynamically"); >+ >+ element.maxLength = -10; >+ is(element.maxLength, -1, "negative maxLength is not processed correctly when set dynamically from the DOM"); >+} Add checks for values 0, PRInt32 min/max and check that .getAttribute still returns the right value.
Attachment #428244 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #3) > >+PRBool > >+nsAttrValue::ParseNonNegativeIntValue(const nsAString& aString) > >+{ > >+ ResetIfSet(); > >+ > >+ PRInt32 ec; > >+ PRBool strict; > >+ PRInt32 originalVal = StringToInteger(aString, &strict, &ec); > >+ if (NS_FAILED(ec)) { > >+ return PR_FALSE; > >+ } > >+ > >+ PRInt32 val = originalVal; > >+ if (val < 0) { > >+ val = -1; // -1 means the value was not correct > >+ } > >+ val = PR_MIN(val, PR_INT32_MAX); > I don't understand this. How could val be bigger than > PR_INT32_MAX? Let's remove that. > I guess you want > PRInt32 val = originalVal; > val = PR_MAX(val, -1); I was thinking the if statement was more readable. > >+ * Parse a string value into a non-negative integer. > >+ * This method follows the rules for parsing non-negative integer from: > >+ * http://dev.w3.org/html5/spec/infrastructure.html#rules-for-parsing-non-negative-integers > This doesn't explain that in which way the error handling works in this > particular implementation. You need to say something about -1. Ok. > >diff -r beb0ffcd0694 content/html/content/test/test_bug536891.html > >--- /dev/null Thu Jan 01 00:00:00 1970 +0000 > >+++ b/content/html/content/test/test_bug536891.html Mon Feb 22 18:30:21 2010 +0100 > >@@ -0,0 +1,45 @@ > >+<!DOCTYPE HTML> > >+<html> > >+<!-- > >+https://bugzilla.mozilla.org/show_bug.cgi?id=536891 > >+--> > >+<head> > >+ <title>Test for Bug 536891</title> > >+ <script type="application/javascript" src="/MochiKit/packed.js"></script> > >+ <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > >+ <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > >+</head> > >+<body> > >+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=536891">Mozilla Bug 536891</a> > >+<p id="display"></p> > >+<div id="content" style="display: none"> > >+<textarea id="t" maxlength="-2"></textarea> > >+<input id="i" type="text" maxlength="-2"> > >+<input id="p" type="password" maxlength="-2"> > >+</div> > >+<pre id="test"> > >+<script type="application/javascript"> > >+ > >+/** Test for Bug 536891 **/ > >+ > >+function checkNegativeMaxLength(element) > >+{ > >+ /* maxLength is set to -2 initially in the document, see above */ > >+ is(element.maxLength, -1, "negative maxLength should be considered invalid and represented as -1"); > >+ > >+ element.setAttribute('maxLength', -15); > >+ is(element.maxLength, -1, "negative maxLength is not processed correctly when set dynamically"); > >+ > >+ element.maxLength = -10; > >+ is(element.maxLength, -1, "negative maxLength is not processed correctly when set dynamically from the DOM"); > >+} > Add checks for values 0, PRInt32 min/max and check that .getAttribute still > returns the > right value. Ok.
Attached patch Patch v0.2 (obsolete) — — Splinter Review
This patch is fixing every issues mentioned previously.
Attachment #428244 - Attachment is obsolete: true
Attachment #428435 - Flags: review?(Olli.Pettay)
Comment on attachment 428435 [details] [diff] [review] Patch v0.2 >+function checkNegativeMaxLength(element) >+{ >+ /* maxLength is set to -2 initially in the document, see above */ >+ is(element.maxLength, -1, "negative maxLength should be considered invalid and represented as -1"); >+ >+ element.setAttribute('maxLength', -15); >+ is(element.maxLength, -1, "negative maxLength is not processed correctly when set dynamically"); >+ is(element.getAttribute('maxLength'), -15, "maxLength attribute doesn't return the correct value"); >+ >+ element.setAttribute('maxLength', 0); >+ is(element.maxLength, 0, "negative maxLength is not processed correctly"); >+ element.setAttribute('maxLength', 2147483647); /* PR_INT32_MAX */ >+ is(element.maxLength, 2147483647, "negative maxLength is not processed correctly"); >+ element.setAttribute('maxLength', -2147483648); /* PR_INT32_MIN */ >+ is(element.maxLength, -1, "negative maxLength is not processed correctly"); Could you add yet some test for element.setAttribute("maxLength", "non-numerical-value"); (No need to ask re-review)
Attachment #428435 - Flags: review?(Olli.Pettay) → review+
Blocks: 344614
Attached patch Patch v0.2.1 (obsolete) — — Splinter Review
The test has been added in this patch. jst, may you sr this patch ?
Attachment #428435 - Attachment is obsolete: true
Attachment #428734 - Flags: superreview?
Attachment #428734 - Flags: superreview? → superreview?(jst)
Keywords: dev-doc-needed
Attachment #428734 - Flags: superreview?(jst) → superreview+
Marking checkin-needed with r=Olli.Pettay, sr=jst
Keywords: checkin-needed
this patch has bitrotted
Keywords: checkin-needed
Should apply with some fuzz
Keywords: checkin-needed
Well, I actually tried importing it...
Attached patch Patch v0.2.2 — — Splinter Review
I've just generated a new patch. It should be easier to apply.
Attachment #428734 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/850c6580ce66
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.3a3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: