Bug 54443 – Textarea maxlength doesn't account for newlines

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[bugzilla33]

Created attachment 82430 [details]
testcase

http://dev.w3.org/html5/markup/textarea.html

Textarea maxlength & placeholder attributes do not work.

maxlength tip: new line character (enter) should be regarded as a one character (length = 1).

[bugzilla33]

little mistake - of course, placeholder is working
Only maxlength problem

[Alexey Proskuryakov]

Could you please clarify what this bug is about? Is it about newlines not being counted as characters for maxlength, or something else?

[bugzilla33]

Yes,
I think - new lines character should be counted as 1 character.

Firefox counts new line (\n) in textarea as 1 character.

All databases counts new line (\n) as 1 character when storing data in text fields.

[Alexey Proskuryakov]

Thank you for the clarification - renaming the bug accordingly.

[Alexey Proskuryakov]

*** Bug 56171 has been marked as a duplicate of this bug. ***

[Naoki Takano]

Sorry, I forgot to commit my patch last night due to Japan earthquake.

I'll commit this weekend...

[Naoki Takano]

Created attachment 85609 [details]
Patch

[Naoki Takano]

Alexey,

Thank you for your suggestion how to test!!

I wrote the test and uploaded the patch.

Please review it.

Thanks,

[Naoki Takano]

Alexey,

Could you review?

(In reply to comment #8)
> Alexey,
> 
> Thank you for your suggestion how to test!!
> 
> I wrote the test and uploaded the patch.
> 
> Please review it.
> 
> Thanks,

[Alexey Proskuryakov]

I don't plan to review this patch.

[Naoki Takano]

Ok,

Could you tell me who is the right person to contact if you know?

(In reply to comment #10)
> I don't plan to review this patch.

[Ryosuke Niwa]

Comment on attachment 85609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85609&action=review

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:96
> +document.execCommand('insertLineBreak'); // This linebreak should be ignored because limitation up to 3 characters.

You didn't realize about the bug I spotted below because your previous 3 calls have instantiated an open TypingCommand.

> Source/WebCore/editing/TypingCommand.cpp:227
> +            // Check if we can insert new line break with \n(line feed) string.
> +            ExceptionCode ec = 0;
> +            RefPtr<BeforeTextInsertedEvent> evt = BeforeTextInsertedEvent::create(String("\n"));
> +            root->dispatchEvent(evt, ec);
> +
> +            if (evt->text().length()) {
> +                lastTypingCommand->setShouldRetainAutocorrectionIndicator(options & RetainAutocorrectionIndicator);
> +                lastTypingCommand->insertLineBreak();
> +            }

I don't think this is the right place to put this code.  I don't see any reason this fix should be restricted to the line breaks inserted after the user had typed something else (i.e. there is an open typing command).  r- because of this.

[Naoki Takano]

Niwa-san,

Thank you for your review.

But I cannot understand what is "open typing command".

Could you tell me what kind of situation it happens?

(In reply to comment #12)
> (From update of attachment 85609 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85609&action=review
> 
> > LayoutTests/fast/forms/script-tests/textarea-maxlength.js:96
> > +document.execCommand('insertLineBreak'); // This linebreak should be ignored because limitation up to 3 characters.
> 
> You didn't realize about the bug I spotted below because your previous 3 calls have instantiated an open TypingCommand.
> 
> > Source/WebCore/editing/TypingCommand.cpp:227
> > +            // Check if we can insert new line break with \n(line feed) string.
> > +            ExceptionCode ec = 0;
> > +            RefPtr<BeforeTextInsertedEvent> evt = BeforeTextInsertedEvent::create(String("\n"));
> > +            root->dispatchEvent(evt, ec);
> > +
> > +            if (evt->text().length()) {
> > +                lastTypingCommand->setShouldRetainAutocorrectionIndicator(options & RetainAutocorrectionIndicator);
> > +                lastTypingCommand->insertLineBreak();
> > +            }
> 
> I don't think this is the right place to put this code.  I don't see any reason this fix should be restricted to the line breaks inserted after the user had typed something else (i.e. there is an open typing command).  r- because of this.

[Ryosuke Niwa]

(In reply to comment #13)
> Thank you for your review.
> 
> But I cannot understand what is "open typing command".

TypingCommand can be open or closed.  Open means that TypingCommand is still accepting more inputs.  This is necessary to group together multiple input events so that we can undo them at once.

> Could you tell me what kind of situation it happens?

In your patch, you're fixing only when you're adding more inputs to the existing TyingCommand (the last one on the undo stack).  But you don't fix the case where there is no typing command or the TypingCommand which is on the top of the undo stack has already been closed (e.g. by selection change).  So try inserting a line break into <input type="text" value="\n\n\n" maxlength="3">.  I bet your fix won't work in this case.

[Naoki Takano]

I see.

I didn't consider such a situation. Actually this is not only new line problem, but also normal characters.

Let's say,
 <input type="text" value="aaaaaaa" maxlength="3">
It shows
aaaaaaa
in text input.

I suppose it might be different problem form the bug entry pointed out though, because the reporter only told "new line" though.
Anyway, I'll look into it more.

Thanks,

(In reply to comment #14)
> (In reply to comment #13)
> > Thank you for your review.
> > 
> > But I cannot understand what is "open typing command".
> 
> TypingCommand can be open or closed.  Open means that TypingCommand is still accepting more inputs.  This is necessary to group together multiple input events so that we can undo them at once.
> 
> > Could you tell me what kind of situation it happens?
> 
> In your patch, you're fixing only when you're adding more inputs to the existing TyingCommand (the last one on the undo stack).  But you don't fix the case where there is no typing command or the TypingCommand which is on the top of the undo stack has already been closed (e.g. by selection change).  So try inserting a line break into <input type="text" value="\n\n\n" maxlength="3">.  I bet your fix won't work in this case.

[Ryosuke Niwa]

(In reply to comment #15)
> I didn't consider such a situation. Actually this is not only new line problem, but also normal characters.
> 
> Let's say,
>  <input type="text" value="aaaaaaa" maxlength="3">
> It shows
> aaaaaaa
> in text input.

I think you misunderstood me.  I'm talking about the case where you try inserting the 4th new line break into <input type="text" value="\n\n\n" maxlength="3">.  I'm not talking about the case where the number of line breaks is larger than the maximum length.

[Naoki Takano]

Sorry, I want to make sure again.

<input type="text" value="\n\n\n" maxlength="3">
Now I noticed that this doesn't show correct newlines. Does it?

Because input type="text" is just one line.

Or do you mean as following?
<textarea value="\n\n\n" maxlength="3">

And then after that, do I have to try to insert 4th new line break?
Or still do I misunderstand you?

If you can, could you tell me the bug step by step?

Thanks,

(In reply to comment #16)
> (In reply to comment #15)
> > I didn't consider such a situation. Actually this is not only new line problem, but also normal characters.
> > 
> > Let's say,
> >  <input type="text" value="aaaaaaa" maxlength="3">
> > It shows
> > aaaaaaa
> > in text input.
> 
> I think you misunderstood me.  I'm talking about the case where you try inserting the 4th new line break into <input type="text" value="\n\n\n" maxlength="3">.  I'm not talking about the case where the number of line breaks is larger than the maximum length.

[Ryosuke Niwa]

(In reply to comment #17)
> <input type="text" value="\n\n\n" maxlength="3">
> Now I noticed that this doesn't show correct newlines. Does it?

Oops, you're absolutely right.

> Or do you mean as following?
> <textarea value="\n\n\n" maxlength="3">

<textarea maxlength="3">


</textarea>

is what I meant (3 lines in textarea).

> And then after that, do I have to try to insert 4th new line break?

Right.

[Naoki Takano]

Created attachment 85770 [details]
Patch

[Naoki Takano]

Niwa-san,

I uploaded new patch.

Basically I just move the checking part to more inner function.

Could you review?

Thanks,

(In reply to comment #18)
> (In reply to comment #17)
> > <input type="text" value="\n\n\n" maxlength="3">
> > Now I noticed that this doesn't show correct newlines. Does it?
> 
> Oops, you're absolutely right.
> 
> > Or do you mean as following?
> > <textarea value="\n\n\n" maxlength="3">
> 
> <textarea maxlength="3">
> 
> 
> </textarea>
> 
> is what I meant (3 lines in textarea).
> 
> > And then after that, do I have to try to insert 4th new line break?
> 
> Right.

[Naoki Takano]

Niwa-san,

Could you review my patch?

[Ryosuke Niwa]

Comment on attachment 85770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85770&action=review

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:96
> +// Here confirms correct count for consecutive linebreaks inputs.
> +textArea = createFocusedTextAreaWithMaxLength3();
> +textArea.innerHTML = 'a\n\n';
> +document.execCommand('insertLineBreak'); // This linebreak should be ignored because limitation up to 3 characters.
> +shouldBe('textArea.value', '"a\\n\\n"');

You should add one more test case where you call InsertLineBreak multiple times.

[Naoki Takano]

Ok,

How about the cpp part?

No problem?

(In reply to comment #22)
> (From update of attachment 85770 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85770&action=review
> 
> > LayoutTests/fast/forms/script-tests/textarea-maxlength.js:96
> > +// Here confirms correct count for consecutive linebreaks inputs.
> > +textArea = createFocusedTextAreaWithMaxLength3();
> > +textArea.innerHTML = 'a\n\n';
> > +document.execCommand('insertLineBreak'); // This linebreak should be ignored because limitation up to 3 characters.
> > +shouldBe('textArea.value', '"a\\n\\n"');
> 
> You should add one more test case where you call InsertLineBreak multiple times.

[Ryosuke Niwa]

(In reply to comment #23)
> How about the cpp part?
> No problem?

View in context: https://bugs.webkit.org/attachment.cgi?id=85770&action=review
> Source/WebCore/editing/TypingCommand.cpp:408
> +    RefPtr<BeforeTextInsertedEvent> evt = BeforeTextInsertedEvent::create(String("\n"));

Please do not use abbreviations like "evt".

[Naoki Takano]

Created attachment 85904 [details]
Patch

[Naoki Takano]

Please review again.

(In reply to comment #25)
> Created an attachment (id=85904) [details]
> Patch

[Ryosuke Niwa]

Comment on attachment 85904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85904&action=review

> LayoutTests/ChangeLog:5
> +        Textarea maxlength doesn't account for newlines

"doesn't" should be "doesn't"

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:52
> +    return textArea;

This line isn't necessary as textAre is declared in the global scope.

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:93
> +textArea = createFocusedTextAreaWithMaxLength3();

No need to update textArea manually here.

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:95
> +document.execCommand('insertLineBreak'); // This linebreak should be ignored because limitation up to 3 characters.

Given we already call createFocusedTextAreaWithMaxLength3, the comment is redundant.  Please get rid of it.

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:103
> +document.execCommand('insertLineBreak'); // This linebreak should be ignored because limitation up to 3 characters.

Ditto about the comment.

> Source/WebCore/ChangeLog:5
> +        Textarea maxlength doesn't account for newlines

Same comment about "'"

> Source/WebCore/ChangeLog:8
> +        When a user presses a return key, TypingCommand::insertLineBreak() is called. So before append a new line, check if we can add the new line.

This seems to be excessively long.  Please split the line at the end of the first sentence so that each line is ~150 columns.

> Source/WebCore/editing/TypingCommand.cpp:412
> +    // Check if we can insert new line break with \n(line feed) string.
> +    ExceptionCode ec = 0;
> +    RefPtr<BeforeTextInsertedEvent> event = BeforeTextInsertedEvent::create(String("\n"));
> +    endingSelection().rootEditableElement()->dispatchEvent(event, ec);
> +    if (!event->text().length())
> +        return;
> +

Now that I think about it, it seems odd that we don't do the same thing for insertParagraphSeparator (i.e. execCommand('InsertParagraph', false, null))

[Ryosuke Niwa]

Also, if you're not a committer yet, you should probably ask for cq+ as well.

[Naoki Takano]

Created attachment 85907 [details]
Patch

[Naoki Takano]

Comment on attachment 85907 [details]
Patch

Could you review again?

[Ryosuke Niwa]

Comment on attachment 85907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85907&action=review

> LayoutTests/ChangeLog:9
> +        (createFocusedTextAreaWithMaxLength3): Added to make sure consecutive insertbreaks work correctly for textarea maxlength.

Please split this into two lines.  It's way too long to fit in one line.

> Source/WebCore/editing/TypingCommand.cpp:50
> +// Helper function to check if we can insert new line feed with \n(line feed) string.

This comment seems to repeat what code does.  In general, we'd like to make code self-explanatory and add as little comment as possible.

> Source/WebCore/editing/TypingCommand.cpp:51
> +static bool canAppendNewLineFeed(TypingCommand& typingCommand)

It's uncommon for a function to take a reference to an EditCommand like this because EditCommand is ref-counted.

> Source/WebCore/editing/TypingCommand.cpp:416
> +    if (!canAppendNewLineFeed(*this))
> +        return;

I'd pass typingCommand.endingSelection() or the root editable element instead.

[Ryosuke Niwa]

Comment on attachment 85907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85907&action=review

>> Source/WebCore/editing/TypingCommand.cpp:50
>> +// Helper function to check if we can insert new line feed with \n(line feed) string.
> 
> This comment seems to repeat what code does.  In general, we'd like to make code self-explanatory and add as little comment as possible.

I forgot to say that you should remove this comment.

[Naoki Takano]

Created attachment 85914 [details]
Patch

[Naoki Takano]

Comment on attachment 85914 [details]
Patch

Please review again.

[Ryosuke Niwa]

Comment on attachment 85914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85914&action=review

> LayoutTests/fast/forms/script-tests/textarea-maxlength.js:95
> +shouldBe('textArea.value', '"a\\n\\n"');

I'm a bit confused.  Why am I not seeing changes in textarea-maxlength-expected.txt ?  Aren't these changed suppose to affect the test ouput?

[Naoki Takano]

Hmm...

My git repo looks strange;-(

Even if "git diff", it cannot detect expected.txt diff.

I'll take all the repo again, and make the patch later...

Thanks,

(In reply to comment #35)
> (From update of attachment 85914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85914&action=review
> 
> > LayoutTests/fast/forms/script-tests/textarea-maxlength.js:95
> > +shouldBe('textArea.value', '"a\\n\\n"');
> 
> I'm a bit confused.  Why am I not seeing changes in textarea-maxlength-expected.txt ?  Aren't these changed suppose to affect the test ouput?

[Naoki Takano]

Niwa-san,

While I'm looking around the test code, I noticed textarea-maxlength-expected.txt doesn't affect the test output.

As you know, if we really need expectation check, we have to add the test into test_expectations.txt, right?
http://www.chromium.org/developers/testing/webkit-layout-tests#TOC-Test-Expectations

But textarea-maxlength.html doesn't exist in test_expectations.txt. I guess run_webkit_tests checks the test output strings and confirms "PASS" or "FAIL".

I confirmed the following cases,
1, Even if I change textarea-maxlength-expected.txt, the test passes. So the file doesn't affect anything. Also I delete it, still the test passes.
2, If I change textarea-maxlength.html in order to fail the test intentionally, it fails.
3, Even if I try to generate new-baseline with --new-baseline option, I cannot change textarea-maxlength-expected.txt.

What do you think?

(In reply to comment #36)
> Hmm...
> 
> My git repo looks strange;-(
> 
> Even if "git diff", it cannot detect expected.txt diff.
> 
> I'll take all the repo again, and make the patch later...
> 
> Thanks,
> 
> (In reply to comment #35)
> > (From update of attachment 85914 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=85914&action=review
> > 
> > > LayoutTests/fast/forms/script-tests/textarea-maxlength.js:95
> > > +shouldBe('textArea.value', '"a\\n\\n"');
> > 
> > I'm a bit confused.  Why am I not seeing changes in textarea-maxlength-expected.txt ?  Aren't these changed suppose to affect the test ouput?

[Ryosuke Niwa]

(In reply to comment #37)
> While I'm looking around the test code, I noticed textarea-maxlength-expected.txt doesn't affect the test output.

You mean "doesn't get affected"?

> As you know, if we really need expectation check, we have to add the test into test_expectations.txt, right?

No.  Every test that doesn't have an entry in test_expectations.txt will be ran and the results are compared against the expected result.  So not having an entry means that we expect the test to run normally and its output to match the expected result.

> But textarea-maxlength.html doesn't exist in test_expectations.txt. I guess run_webkit_tests checks the test output strings and confirms "PASS" or "FAIL".

I don't think so.  Your patch should have changed in http://trac.webkit.org/browser/trunk/LayoutTests/fast/forms/textarea-maxlength-expected.txt as in http://trac.webkit.org/changeset/48903

> I confirmed the following cases,
> 1, Even if I change textarea-maxlength-expected.txt, the test passes. So the file doesn't affect anything. Also I delete it, still the test passes.

That doesn't make any sense.  You're probably not running it correctly.  Did you rebuild WebKit before running LayoutTests?

> 2, If I change textarea-maxlength.html in order to fail the test intentionally, it fails.
> 3, Even if I try to generate new-baseline with --new-baseline option, I cannot change textarea-maxlength-expected.txt.

Which port are you using to do this?  You should be running Tools/Scripts/run-webkit-tests (optionally --debug if you're using debug build) and I don't think it has --new-baseline option.

[Naoki Takano]

According to http://www.chromium.org/developers/testing/webkit-layout-tests#TOC-Running-the-Tests
 ./webkit/tools/layout_tests/run_webkit_tests.sh
looks correct.

But, do you mean
Tools/Scripts/run-webkit-tests
is correct?

I didn't know there are two version of run-webkit-tests or run_webkit_tests.

Which is correct one?

(In reply to comment #38)
> (In reply to comment #37)
> > While I'm looking around the test code, I noticed textarea-maxlength-expected.txt doesn't affect the test output.
> 
> You mean "doesn't get affected"?
> 
> > As you know, if we really need expectation check, we have to add the test into test_expectations.txt, right?
> 
> No.  Every test that doesn't have an entry in test_expectations.txt will be ran and the results are compared against the expected result.  So not having an entry means that we expect the test to run normally and its output to match the expected result.
> 
> > But textarea-maxlength.html doesn't exist in test_expectations.txt. I guess run_webkit_tests checks the test output strings and confirms "PASS" or "FAIL".
> 
> I don't think so.  Your patch should have changed in http://trac.webkit.org/browser/trunk/LayoutTests/fast/forms/textarea-maxlength-expected.txt as in http://trac.webkit.org/changeset/48903
> 
> > I confirmed the following cases,
> > 1, Even if I change textarea-maxlength-expected.txt, the test passes. So the file doesn't affect anything. Also I delete it, still the test passes.
> 
> That doesn't make any sense.  You're probably not running it correctly.  Did you rebuild WebKit before running LayoutTests?
> 
> > 2, If I change textarea-maxlength.html in order to fail the test intentionally, it fails.
> > 3, Even if I try to generate new-baseline with --new-baseline option, I cannot change textarea-maxlength-expected.txt.
> 
> Which port are you using to do this?  You should be running Tools/Scripts/run-webkit-tests (optionally --debug if you're using debug build) and I don't think it has --new-baseline option.

[Ryosuke Niwa]

(In reply to comment #39)
> According to http://www.chromium.org/developers/testing/webkit-layout-tests#TOC-Running-the-Tests
>  ./webkit/tools/layout_tests/run_webkit_tests.sh
> looks correct.

Ah, you're using WebKit inside Chromium checkout.  I don't know what's the proper way to run layout tests inside Chromium checkout because I always use Apple's Mac port.

> But, do you mean
> Tools/Scripts/run-webkit-tests
> is correct?

For Chromium port, you need to use Tools/Scripts/new-run-webkit-tests.  See http://trac.webkit.org/wiki/Chromium.

[Naoki Takano]

I see.

./third_party/WebKit/LayoutTests/platform/chromium-linux/fast/forms/textarea-maxlength-expected.txt
is generated. That is different location...

I recreate patch.

(In reply to comment #40)
> (In reply to comment #39)
> > According to http://www.chromium.org/developers/testing/webkit-layout-tests#TOC-Running-the-Tests
> >  ./webkit/tools/layout_tests/run_webkit_tests.sh
> > looks correct.
> 
> Ah, you're using WebKit inside Chromium checkout.  I don't know what's the proper way to run layout tests inside Chromium checkout because I always use Apple's Mac port.
> 
> > But, do you mean
> > Tools/Scripts/run-webkit-tests
> > is correct?
> 
> For Chromium port, you need to use Tools/Scripts/new-run-webkit-tests.  See http://trac.webkit.org/wiki/Chromium.

[Naoki Takano]

Created attachment 86022 [details]
Patch

[Naoki Takano]

Comment on attachment 86022 [details]
Patch

Please review again.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 86022 [details]:

http/tests/appcache/top-frame-1.html bug 56542 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 86022 [details]
Patch

Clearing flags on attachment: 86022

Committed r81328: <http://trac.webkit.org/changeset/81328>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.