Closed Bug 535043 Opened 15 years ago Closed 14 years ago

Support maxlength on textarea

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: ayg, Assigned: ayg)

References

Details

(Keywords: dev-doc-complete, html5)

Attachments

(2 files, 4 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: 

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
Attached patch Support maxlength on textareas (obsolete) — Splinter Review
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.  :)
Attachment #417843 - Flags: review?(jst)
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.
jst, do you know if you'll have time to review this patch anytime soon?
I believe jst is on vacation right now, should be back in a week.
If you change an interface (nsIDOMHTMLTextAreaElement.idl) you must regenerate the uuid.
And nsIDOMHTMLTextAreaElement is actually frozen. 
Add the new thing to nsIDOMNSHTMLTextAreaElement.
Is this better?
Attachment #417843 - Attachment is obsolete: true
Attachment #418850 - Flags: review?(jst)
Attachment #417843 - Flags: review?(jst)
Comment on attachment 418850 [details] [diff] [review]
Support maxlength on textareas, v2

I'll try find time to review this tomorrow.
Attachment #418850 - Flags: review?(jst) → review?(Olli.Pettay)
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 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.
> 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.
Assignee: nobody → Simetrical+ff
Status: UNCONFIRMED → NEW
Ever confirmed: true
New version of patch, checking setAttribute() and maxLength setting too.
Attachment #418850 - Attachment is obsolete: true
Attachment #419069 - Flags: review?
Attachment #418850 - Flags: review?(Olli.Pettay)
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.
Attachment #419069 - Flags: review? → review?(Olli.Pettay)
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
Attachment #419069 - Flags: review?(Olli.Pettay) → review+
Depends on: 536891
Depends on: 536895
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?
Attachment #419069 - Attachment is obsolete: true
Attachment #419257 - Flags: review?
Attachment #419257 - Flags: review? → review?(Olli.Pettay)
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.
Attachment #419257 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #419257 - Attachment is obsolete: true
Attachment #419293 - Flags: review?(Olli.Pettay)
Attachment #419293 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #419293 - Flags: superreview?(jst)
Comment on attachment 419293 [details] [diff] [review]
Support maxlength on textareas, v4.1

sr=jst, sorry for the delay!
Attachment #419293 - Flags: superreview?(jst) → superreview+
Thanks!  Added checkin-needed keyword, since I take it that's the next step.
Keywords: checkin-needed
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...)?
Does
if ("maxLength" in someTextArea) {
  ...
}
work?
http://hg.mozilla.org/mozilla-central/rev/511a92d06acf
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(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!
Blocks: 344614
Keywords: html5
Blocks: 566348
Depends on: 589026
Depends on: 590554
Depends on: 626256
You need to log in before you can comment on or make changes to this bug.