535043 - Support maxlength on textarea

Status: RESOLVED FIXED

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

Description

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

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: 

HTML4 does not permit the maxlength attribute on textarea.  However, HTML5 does:

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-textarea-element

WebKit already supports this, but Gecko doesn't.  It would be a moderately useful feature and trivial to add.  So trivial that I thought I'd take a dive into the codebase and give it a shot, in fact.  :)  Patch coming shortly, with a test case.  Possibly not a great test case, but it's already caught one unrelated bug, so it can't be too terrible.

Bug 502462 might be related or the same, I'm not sure.

Reproducible: Always

Comment 1

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

Attachment: Support maxlength on textareas

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>

I kept in the test I added for this case, but marked it todo.  Please tell me if this is wrong.

Requesting review from jst by suggestion of roc.  Is there anything more I should test?  I threw in whatever came to mind, which wasn't much, since it's a pretty simple feature.  Also, I wrote the patch itself by dint of "grep -iR maxlength ." followed by copy-pasting anything that looked vaguely relevant, so it quite possibly contains errors, but miraculously, it does seem to work.  :)

Comment 2

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

Also, I did confirm that this causes no additional test failures for make mochitest-plain on my Linux machine.  (There were ten failures, but they also occurred in the revision I originally checked out.)  I also got no output from make check 2>&1 | grep UNEXPECTED.  I'm not sure exactly what commands I'm supposed to run and what I'm supposed to look for in the output to figure out whether I've caused any regressions -- the wiki pages I found weren't clear on that.

Comment 3

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

jst, do you know if you'll have time to review this patch anytime soon?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I believe jst is on vacation right now, should be back in a week.

Comment 5

[Robert Longson [:longsonr]]

If you change an interface (nsIDOMHTMLTextAreaElement.idl) you must regenerate the uuid.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

And nsIDOMHTMLTextAreaElement is actually frozen. 
Add the new thing to nsIDOMNSHTMLTextAreaElement.

Comment 7

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

Attachment: Support maxlength on textareas, v2

Is this better?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 418850 [details] [diff] [review]
Support maxlength on textareas, v2

I'll try find time to review this tomorrow.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Did you check that other browsers which support maxlength give similar results in the test? (The test uses some gecko only features, so it doesn't run elsewhere as is.)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 418850 [details] [diff] [review]
Support maxlength on textareas, v2


>+  } else if (htmlMaxLength < 0) {
>+    // Per the HTML5 spec, out-of-range values are supposed to translate to -1,
>+    // not 0, but they don't?
>+    todo_is(domMaxLength, -1,
>+      'maxlength is out of range but maxLength DOM attribute is not -1');
Where is 'out-of-range values are supposed to translate to -1' actually defined in HTML5?
(Since the draft is so messy, it is hard to find anything useful there.)
All I found is "The maxLength IDL attribute must reflect the maxlength content attribute, limited to only non-negative numbers." And I interpret that so that
.maxlength shouldn't ever have negative value.

Could you please add testcases for dynamic maxlength changes.
I.e. test that maxlength handling works also after setting maxlength attribute.
Test both .setAttribute() and .maxlength = <somenewvalue> cases.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

> Where is 'out-of-range values are supposed to translate to -1' actually defined
> in HTML5?
Ok, Anne pointed me the right place.

But still, could you please add those tests for dynamic cases.

Comment 12

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

Attachment: Support maxlength on textareas, v3

New version of patch, checking setAttribute() and maxLength setting too.

Comment 13

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

Attachment: Cross-browser test case

This automatically tests whether the DOM and IDL attributes behave properly, but can't do the synthesizeKey() stuff.  You have to try typing manually.  WebKit (Chrome 4.0.266.0) passes all tests.  Opera 10.10 and IE6 in ies4linux don't implement maxlength on textareas, so they of course fail the manual "can't actually type past maxlength" part, in addition to some of the scripted stuff.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 419069 [details] [diff] [review]
Support maxlength on textareas, v3

>+SimpleTest.waitForFocus();

waitForFocus takes usually a callback method, which is called when
window is loaded and focused. Without a callback it isn't very useful.
So please make the callback method to run the tests.

And please file followup bugs for all the "todo" cases.

Those fixed, r=me

Comment 15

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

Attachment: Support maxlength on textareas, v4

Should use waitForFocus properly.  Also, I filed follow-ups bug 536891 and bug 536895.  I should now request super-review from someone, right?  Who would you suggest?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 419257 [details] [diff] [review]
Support maxlength on textareas, v4

>+SimpleTest.waitForFocus(function() {
>+	var textAreas = document.getElementsByTagName('textarea');
>+	for (var i = 0; i < textAreas.length; i++) {
>+	  checkTextArea(textAreas[i]);
>+	}
>+
>+	textArea = textAreas[0];
>+	testNums = [-42, -1, 0, 2, 257];
>+	for (var i = 0; i < testNums.length; i++) {
>+	  textArea.removeAttribute('maxlength');
>+
>+	  var caught = false;
>+	  try {
>+		textArea.maxLength = testNums[i];
>+	  } catch (e) {
>+		caught = true;
>+	  }
>+	  if (testNums[i] < 0) {
>+		todo(caught, 'Setting negative maxLength should throw exception');
>+	  } else {
>+		ok(!caught, 'Setting nonnegative maxLength should not throw exception');
>+	  }
>+	  checkTextArea(textArea);
>+
>+	  textArea.setAttribute('maxlength', testNums[i]);
>+	  checkTextArea(textArea);
>+	}
>+});
You use tabs here. Replace with spaces (and indentation should be 2 spaces).
r=me

jst could sr the API change.

Comment 17

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

Attachment: Support maxlength on textareas, v4.1

Oops, the last patch introduced whitespace errors.  This one should be better.  I have to figure out how to get vim to use different tab standards for different projects.

Comment 18

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

Comment on attachment 419293 [details] [diff] [review]
Support maxlength on textareas, v4.1

Oops, seems I posted without seeing your new post.  Requesting sr from jst.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 419293 [details] [diff] [review]
Support maxlength on textareas, v4.1

sr=jst, sorry for the delay!

Comment 20

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

Thanks!  Added checkin-needed keyword, since I take it that's the next step.

Comment 21

[Lars Gunther]

Is there a way to feature detect this specific functionality? If not, do we need a new bug (and some discussion perhaps in HTML5 WG...)?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Does
if ("maxLength" in someTextArea) {
  ...
}
work?

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/511a92d06acf

Comment 24

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

(In reply to comment #22)
> Does
> if ("maxLength" in someTextArea) {
>   ...
> }
> work?

Yes, "maxLength" in document.createElement("textarea") works for feature-sniffing.

(In reply to comment #23)
> http://hg.mozilla.org/mozilla-central/rev/511a92d06acf

Thanks!