Closed Bug 536891 Opened 14 years ago Closed 13 years ago

Negative maxlength should be treated the same as unspecified maxlength

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.1 14 years ago Mounir Lamouri (:mounir) 7.65 KB, patch	smaug : review-	Details \| Diff \| Splinter Review
Patch v0.2 14 years ago Mounir Lamouri (:mounir) 8.14 KB, patch	smaug : review+	Details \| Diff \| Splinter Review
Patch v0.2.1 14 years ago Mounir Lamouri (:mounir) 8.30 KB, patch	jst : superreview+	Details \| Diff \| Splinter Review
Patch v0.2.2 13 years ago Mounir Lamouri (:mounir) 8.29 KB, patch		Details \| Diff \| Splinter Review

Aryeh Gregor (:ayg) (no longer with Mozilla)

Reporter

Description

•

14 years ago

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

Aryeh Gregor (:ayg) (no longer with Mozilla)

Reporter

Comment 1

•

14 years ago

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

Mounir Lamouri (:mounir)

Assignee

Updated

•

14 years ago

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

Mounir Lamouri (:mounir)

Assignee

Comment 2

•

14 years ago

Attached patch Patch v0.1 (obsolete) — Details — 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)

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 3

•

14 years ago

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-

Mounir Lamouri (:mounir)

Assignee

Comment 4

•

14 years ago

(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.

Mounir Lamouri (:mounir)

Assignee

Comment 5

•

14 years ago

Attached patch Patch v0.2 (obsolete) — Details — Splinter Review

This patch is fixing every issues mentioned previously.

Attachment #428244 - Attachment is obsolete: true

Attachment #428435 - Flags: review?(Olli.Pettay)

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 6

•

14 years ago

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+

Mounir Lamouri (:mounir)

Assignee

Updated

•

14 years ago

Blocks: 344614

Mounir Lamouri (:mounir)

Assignee

Comment 7

•

14 years ago

Attached patch Patch v0.2.1 (obsolete) — Details — 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?

Mounir Lamouri (:mounir)

Assignee

Updated

•

14 years ago

Attachment #428734 - Flags: superreview? → superreview?(jst)

Mounir Lamouri (:mounir)

Assignee

Updated

•

14 years ago

Keywords: dev-doc-needed

Johnny Stenback (:jst)

Updated

•

14 years ago

Attachment #428734 - Flags: superreview?(jst) → superreview+

Mounir Lamouri (:mounir)

Assignee

Comment 8

•

14 years ago

Marking checkin-needed with r=Olli.Pettay, sr=jst

Keywords: checkin-needed

Dão Gottwald [:dao]

Comment 9

•

14 years ago

this patch has bitrotted

Keywords: checkin-needed

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 10

•

14 years ago

Should apply with some fuzz

Keywords: checkin-needed

Dão Gottwald [:dao]

Comment 11

•

14 years ago

Well, I actually tried importing it...

Mounir Lamouri (:mounir)

Assignee

Comment 12

•

13 years ago

Attached patch Patch v0.2.2 — Details — Splinter Review

I've just generated a new patch. It should be easier to apply.

Attachment #428734 - Attachment is obsolete: true

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 13

•

13 years ago

http://hg.mozilla.org/mozilla-central/rev/850c6580ce66

Status: ASSIGNED → RESOLVED

Closed: 13 years ago

Resolution: --- → FIXED

Dão Gottwald [:dao]

Updated

•

13 years ago

Keywords: checkin-needed

Target Milestone: --- → mozilla1.9.3a3

Eric Shepherd [:sheppy]

Comment 14

•

13 years ago

Updated the doc here:

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

Keywords: dev-doc-needed → dev-doc-complete

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

Bugzilla

Quick Search

Negative maxlength should be treated the same as unspecified maxlength

Categories

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

Tracking

()

People

(Reporter: ayg, Assigned: mounir)

References

(
URL
)

Details

(Keywords: dev-doc-complete, html5)

Crash Data

Security

(public)

User Story

Attachments

(1 file, 3 obsolete files)

Description

Comment 1

Updated

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Updated

Comment 7

Updated

Updated

Updated

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Updated

Comment 14