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).
little mistake - of course, placeholder is working Only maxlength problem
Could you please clarify what this bug is about? Is it about newlines not being counted as characters for maxlength, or something else?
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.
Thank you for the clarification - renaming the bug accordingly.
*** Bug 56171 has been marked as a duplicate of this bug. ***
Sorry, I forgot to commit my patch last night due to Japan earthquake. I'll commit this weekend...
Created attachment 85609 [details] Patch
Alexey, Thank you for your suggestion how to test!! I wrote the test and uploaded the patch. Please review it. Thanks,
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,
I don't plan to review this patch.
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.
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.
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.
(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.
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.
(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.
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.
(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.
Created attachment 85770 [details] Patch
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.
Niwa-san, Could you review my patch?
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.
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.
(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".
Created attachment 85904 [details] Patch
Please review again. (In reply to comment #25) > Created an attachment (id=85904) [details] > Patch
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))
Also, if you're not a committer yet, you should probably ask for cq+ as well.
Created attachment 85907 [details] Patch
Comment on attachment 85907 [details] Patch Could you review again?
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.
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.
Created attachment 85914 [details] Patch
Comment on attachment 85914 [details] Patch Please review again.
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?
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?
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?
(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.
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.
(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.
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.
Created attachment 86022 [details] Patch
Comment on attachment 86022 [details] Patch Please review again.
The commit-queue encountered the following flaky tests while processing attachment 86022 [details]: http/tests/appcache/top-frame-1.html bug 56542 (author: ap@webkit.org) The commit-queue is continuing to process your patch.
Comment on attachment 86022 [details] Patch Clearing flags on attachment: 86022 Committed r81328: <http://trac.webkit.org/changeset/81328>
All reviewed patches have been landed. Closing bug.