Last Comment Bug 535043 - Support maxlength on textarea
: Support maxlength on textarea
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- enhancement with 1 vote (vote)
: mozilla1.9.3a1
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 536891 536895 589026 590554 626256
Blocks: html5forms 566348
  Show dependency treegraph
 
Reported: 2009-12-15 17:33 PST by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2011-01-16 18:59 PST (History)
14 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Support maxlength on textareas (5.56 KB, patch)
2009-12-15 17:45 PST, Aryeh Gregor (:ayg) (next working March 28-April 26)
no flags Details | Diff | Splinter Review
Support maxlength on textareas, v2 (5.42 KB, patch)
2009-12-22 07:34 PST, Aryeh Gregor (:ayg) (next working March 28-April 26)
no flags Details | Diff | Splinter Review
Support maxlength on textareas, v3 (6.04 KB, patch)
2009-12-23 17:36 PST, Aryeh Gregor (:ayg) (next working March 28-April 26)
bugs: review+
Details | Diff | Splinter Review
Cross-browser test case (1.87 KB, text/html)
2009-12-23 17:41 PST, Aryeh Gregor (:ayg) (next working March 28-April 26)
no flags Details
Support maxlength on textareas, v4 (6.06 KB, patch)
2009-12-27 13:00 PST, Aryeh Gregor (:ayg) (next working March 28-April 26)
bugs: review+
Details | Diff | Splinter Review
Support maxlength on textareas, v4.1 (6.10 KB, patch)
2009-12-28 04:45 PST, Aryeh Gregor (:ayg) (next working March 28-April 26)
bugs: review+
jst: superreview+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2009-12-15 17:33:34 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: 

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) (next working March 28-April 26) 2009-12-15 17:45:30 PST
Created attachment 417843 [details] [diff] [review]
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) (next working March 28-April 26) 2009-12-16 10:27:36 PST
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) (next working March 28-April 26) 2009-12-21 18:02:46 PST
jst, do you know if you'll have time to review this patch anytime soon?
Comment 4 Olli Pettay [:smaug] 2009-12-22 05:31:43 PST
I believe jst is on vacation right now, should be back in a week.
Comment 5 Robert Longson 2009-12-22 05:46:08 PST
If you change an interface (nsIDOMHTMLTextAreaElement.idl) you must regenerate the uuid.
Comment 6 Olli Pettay [:smaug] 2009-12-22 06:42:52 PST
And nsIDOMHTMLTextAreaElement is actually frozen. 
Add the new thing to nsIDOMNSHTMLTextAreaElement.
Comment 7 Aryeh Gregor (:ayg) (next working March 28-April 26) 2009-12-22 07:34:56 PST
Created attachment 418850 [details] [diff] [review]
Support maxlength on textareas, v2

Is this better?
Comment 8 Olli Pettay [:smaug] 2009-12-22 07:44:29 PST
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] 2009-12-23 06:10:18 PST
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] 2009-12-23 07:59:22 PST
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] 2009-12-23 12:14:34 PST
> 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) (next working March 28-April 26) 2009-12-23 17:36:30 PST
Created attachment 419069 [details] [diff] [review]
Support maxlength on textareas, v3

New version of patch, checking setAttribute() and maxLength setting too.
Comment 13 Aryeh Gregor (:ayg) (next working March 28-April 26) 2009-12-23 17:41:11 PST
Created attachment 419073 [details]
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] 2009-12-27 10:29:28 PST
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) (next working March 28-April 26) 2009-12-27 13:00:26 PST
Created attachment 419257 [details] [diff] [review]
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] 2009-12-28 04:43:26 PST
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) (next working March 28-April 26) 2009-12-28 04:45:21 PST
Created attachment 419293 [details] [diff] [review]
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) (next working March 28-April 26) 2009-12-28 06:25:59 PST
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, jst@mozilla.com) 2010-01-17 16:11:37 PST
Comment on attachment 419293 [details] [diff] [review]
Support maxlength on textareas, v4.1

sr=jst, sorry for the delay!
Comment 20 Aryeh Gregor (:ayg) (next working March 28-April 26) 2010-01-17 18:05:24 PST
Thanks!  Added checkin-needed keyword, since I take it that's the next step.
Comment 21 Lars Gunther 2010-01-17 23:54:32 PST
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] 2010-01-18 01:55:58 PST
Does
if ("maxLength" in someTextArea) {
  ...
}
work?
Comment 23 Dão Gottwald [:dao] 2010-01-18 07:55:33 PST
http://hg.mozilla.org/mozilla-central/rev/511a92d06acf
Comment 24 Aryeh Gregor (:ayg) (next working March 28-April 26) 2010-01-18 08:59:55 PST
(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!

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