Last Comment Bug 536891 - Negative maxlength should be treated the same as unspecified maxlength
: Negative maxlength should be treated the same as unspecified maxlength
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla1.9.3a3
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
http://dev.w3.org/html5/spec/infrastr...
Depends on:
Blocks: html5forms 535043
  Show dependency treegraph
 
Reported: 2009-12-27 12:49 PST by :Aryeh Gregor (working until September 2)
Modified: 2010-08-18 14:01 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0.1 (7.65 KB, patch)
2010-02-22 09:30 PST, Mounir Lamouri (:mounir)
bugs: review-
Details | Diff | Splinter Review
Patch v0.2 (8.14 KB, patch)
2010-02-23 06:08 PST, Mounir Lamouri (:mounir)
bugs: review+
Details | Diff | Splinter Review
Patch v0.2.1 (8.30 KB, patch)
2010-02-24 08:52 PST, Mounir Lamouri (:mounir)
jst: superreview+
Details | Diff | Splinter Review
Patch v0.2.2 (8.29 KB, patch)
2010-03-05 10:59 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description :Aryeh Gregor (working until September 2) 2009-12-27 12:49:47 PST
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
Comment 1 :Aryeh Gregor (working until September 2) 2009-12-27 12:51:01 PST
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.
Comment 2 Mounir Lamouri (:mounir) 2010-02-22 09:30:34 PST
Created attachment 428244 [details] [diff] [review]
Patch v0.1

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.
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-02-23 04:57:56 PST
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.
Comment 4 Mounir Lamouri (:mounir) 2010-02-23 05:11:07 PST
(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.
Comment 5 Mounir Lamouri (:mounir) 2010-02-23 06:08:00 PST
Created attachment 428435 [details] [diff] [review]
Patch v0.2

This patch is fixing every issues mentioned previously.
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-02-23 07:45:07 PST
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)
Comment 7 Mounir Lamouri (:mounir) 2010-02-24 08:52:57 PST
Created attachment 428734 [details] [diff] [review]
Patch v0.2.1

The test has been added in this patch.

jst, may you sr this patch ?
Comment 8 Mounir Lamouri (:mounir) 2010-03-02 15:04:57 PST
Marking checkin-needed with r=Olli.Pettay, sr=jst
Comment 9 Dão Gottwald [:dao] 2010-03-04 01:32:25 PST
this patch has bitrotted
Comment 10 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-03-04 02:16:42 PST
Should apply with some fuzz
Comment 11 Dão Gottwald [:dao] 2010-03-04 05:03:06 PST
Well, I actually tried importing it...
Comment 12 Mounir Lamouri (:mounir) 2010-03-05 10:59:03 PST
Created attachment 430670 [details] [diff] [review]
Patch v0.2.2

I've just generated a new patch. It should be easier to apply.
Comment 13 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-03-05 12:28:15 PST
http://hg.mozilla.org/mozilla-central/rev/850c6580ce66
Comment 14 Eric Shepherd [:sheppy] 2010-08-18 14:01:08 PDT
Updated the doc here:

https://developer.mozilla.org/en/HTML/Element/Input

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