Closed Bug 536891 Opened 14 years ago Closed 14 years ago

Negative maxlength should be treated the same as unspecified maxlength

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: ayg, Assigned: mounir)

References

()

Details

(Keywords: dev-doc-complete, html5)

Attachments

(1 file, 3 obsolete files)

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
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.2Splinter 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: 14 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.