Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
113663003:
Keep br while replacing plain texts with content ending with newline (Closed)
|
Created:
7 years ago by ziran.sun Modified:
6 years, 11 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionKeep br while replacing plain texts with content ending with newline.
The <br> after the insert position was removed while inserting a text ending with a newline. This caused the loss of the newline at the end of the inserted text. This fix adds extra condition checking for this scenario and keeps the <br> while replacing plain texts with content ending with newline.
BUG=121627
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165565
Patch Set 1 #Patch Set 2 : Add test case for paste newline in a all selected textarea #
Total comments: 11
Patch Set 3 : update as per review comments #Patch Set 4 : remove wrongly name test file #
Total comments: 4
Patch Set 5 : Update code as per second review comments #
Total comments: 2
Patch Set 6 : Keep br after the insert positioin when replacing plain texts with #Patch Set 7 : Add test description and adjust format #
Total comments: 5
Patch Set 8 : Use consistent command names #Patch Set 9 : Use "" for html code #
Messages
Total messages: 24 (0 generated)
Please populate "Reviewers" field for asking reviewing. It automatically send email to reviewers. You don't need to send by yourself. https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... File LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html (right): https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:10: nit: Please remove an extra blank line. https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:12: testRunner.dumpAsText(); You don't need to call testRunner.dumpAsText(). js-test.js does for you. See http://src.chromium.org/viewvc/blink/trunk/LayoutTests/resources/js-test.js?r... https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:14: var newlineText = '\n'; nit: We don't need to use |newlineText|. https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:18: //copy a new line nit: Please put a space after "//" https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:25: nit: Please remove an extra blank line. https://codereview.chromium.org/113663003/diff/20001/Source/core/editing/Repl... File Source/core/editing/ReplaceSelectionCommand.cpp (right): https://codereview.chromium.org/113663003/diff/20001/Source/core/editing/Repl... Source/core/editing/ReplaceSelectionCommand.cpp:1130: if (!(fragment.hasInterchangeNewlineAtEnd() && selectionIsPlainText)) { nit: Please merge L1130 condition into L1129.
On 2013/12/20 01:23:05, Yoshi wrote: > Please populate "Reviewers" field for asking reviewing. It automatically send > email to reviewers. You don't need to send by yourself. > > https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... > File LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html > (right): > > https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... > LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:10: > nit: Please remove an extra blank line. > > https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... > LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:12: > testRunner.dumpAsText(); > You don't need to call testRunner.dumpAsText(). js-test.js does for you. > See > http://src.chromium.org/viewvc/blink/trunk/LayoutTests/resources/js-test.js?r... > > https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... > LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:14: > var newlineText = '\n'; > nit: We don't need to use |newlineText|. > > https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... > LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:18: > //copy a new line > nit: Please put a space after "//" > > https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... > LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:25: > nit: Please remove an extra blank line. > > https://codereview.chromium.org/113663003/diff/20001/Source/core/editing/Repl... > File Source/core/editing/ReplaceSelectionCommand.cpp (right): > > https://codereview.chromium.org/113663003/diff/20001/Source/core/editing/Repl... > Source/core/editing/ReplaceSelectionCommand.cpp:1130: if > (!(fragment.hasInterchangeNewlineAtEnd() && selectionIsPlainText)) { > nit: Please merge L1130 condition into L1129. Hi Yoshi, Code has been updated as per review comments. Is it okay to have a check if the updates okay? Many thanks, Ziran
Code update as per review comments. is it okay to check if they are okay? Many thanks, https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... File LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html (right): https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:10: On 2013/12/20 01:23:06, Yoshi wrote: > nit: Please remove an extra blank line. Done. https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:12: testRunner.dumpAsText(); On 2013/12/20 01:23:06, Yoshi wrote: > You don't need to call testRunner.dumpAsText(). js-test.js does for you. > See > http://src.chromium.org/viewvc/blink/trunk/LayoutTests/resources/js-test.js?r... Done. https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:14: var newlineText = '\n'; On 2013/12/20 01:23:06, Yoshi wrote: > nit: We don't need to use |newlineText|. Done. https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:18: //copy a new line On 2013/12/20 01:23:06, Yoshi wrote: > nit: Please put a space after "//" Done. https://codereview.chromium.org/113663003/diff/20001/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:25: On 2013/12/20 01:23:06, Yoshi wrote: > nit: Please remove an extra blank line. Done.
+leviw for OWNERS review https://codereview.chromium.org/113663003/diff/40002/LayoutTests/editing/past... File LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html (right): https://codereview.chromium.org/113663003/diff/40002/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:19: shouldBeEqualToString('textarea.value', '\n'); nit: Please use one kind of quote in JavaScript. We prefer single-quote in JavaScript. See http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Strings https://codereview.chromium.org/113663003/diff/40002/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:23: nit: Please remove this extra blank line.
https://codereview.chromium.org/113663003/diff/40002/LayoutTests/editing/past... File LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html (right): https://codereview.chromium.org/113663003/diff/40002/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:19: shouldBeEqualToString('textarea.value', '\n'); On 2014/01/06 06:05:07, Yoshi wrote: > nit: Please use one kind of quote in JavaScript. We prefer single-quote in > JavaScript. > See > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Strings Done. https://codereview.chromium.org/113663003/diff/40002/LayoutTests/editing/past... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:23: On 2014/01/06 06:05:07, Yoshi wrote: > nit: Please remove this extra blank line. Done.
The test and code seem reasonable. Can you please write a description of the change in your CL description? https://codereview.chromium.org/113663003/diff/130001/LayoutTests/editing/pas... File LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html (right): https://codereview.chromium.org/113663003/diff/130001/LayoutTests/editing/pas... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:19: shouldBeEqualToString('textarea.value', '\n'); It would be nice if you added a description of what this test is testing somewhere in the test.
https://codereview.chromium.org/113663003/diff/130001/LayoutTests/editing/pas... File LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html (right): https://codereview.chromium.org/113663003/diff/130001/LayoutTests/editing/pas... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:19: shouldBeEqualToString('textarea.value', '\n'); On 2014/01/07 19:48:10, Levi wrote: > It would be nice if you added a description of what this test is testing > somewhere in the test. Done.
https://codereview.chromium.org/113663003/diff/230001/LayoutTests/editing/pas... File LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html (right): https://codereview.chromium.org/113663003/diff/230001/LayoutTests/editing/pas... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:4: <textarea id='test' cols=5 rows=4> nit: Please use double quote for HTML markup. Note: we use single quote in JS, though. https://codereview.chromium.org/113663003/diff/230001/LayoutTests/editing/pas... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:18: document.execCommand('SelectAll'); nit: We may want to have consistency of command names, {'Copy', 'SelectAll', 'Paste'} or {'copy', 'selectall', 'paste'}. Sorry for so picky.
https://codereview.chromium.org/113663003/diff/230001/LayoutTests/editing/pas... File LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html (right): https://codereview.chromium.org/113663003/diff/230001/LayoutTests/editing/pas... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:4: <textarea id='test' cols=5 rows=4> On 2014/01/09 01:18:26, Yoshi wrote: > nit: Please use double quote for HTML markup. Note: we use single quote in JS, > though. Done. https://codereview.chromium.org/113663003/diff/230001/LayoutTests/editing/pas... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:18: document.execCommand('SelectAll'); On 2014/01/09 01:18:26, Yoshi wrote: > nit: We may want to have consistency of command names, {'Copy', 'SelectAll', > 'Paste'} or {'copy', 'selectall', 'paste'}. Sorry for so picky. Done. https://codereview.chromium.org/113663003/diff/230001/LayoutTests/editing/pas... LayoutTests/editing/pasteboard/paste-newline-in-all-selected-textarea.html:18: document.execCommand('SelectAll'); On 2014/01/09 01:18:26, Yoshi wrote: > nit: We may want to have consistency of command names, {'Copy', 'SelectAll', > 'Paste'} or {'copy', 'selectall', 'paste'}. Sorry for so picky. Thanks for this. I need to pay more attentions on these details :).
You still need a description of the CL (click 'Edit Issue')
On 2014/01/10 19:59:05, Levi wrote: > You still need a description of the CL (click 'Edit Issue') Done.
On 2014/01/14 10:07:37, ziran.sun wrote: > On 2014/01/10 19:59:05, Levi wrote: > > You still need a description of the CL (click 'Edit Issue') > > Done. Yoshi, leiv Could you check if the code updates meet your comments? If so, is there any chance to have to code merged in? or let me know if I should do more work on this. Thanks, Ziran
The first line of your CL description should be a short summary, then an empty line, then a description. You can just copy the "Subject" from the CL.
On 2014/01/15 18:04:39, Levi wrote: > The first line of your CL description should be a short summary, then an empty > line, then a description. > > You can just copy the "Subject" from the CL. Thanks. Just updated the CL description. Let me know if it's okay.
Update CL descriptions. Thanks.
Okay. LGTM.
On 2014/01/15 22:30:19, Levi wrote: > Okay. LGTM. Thanks! Yoshi/Levi, just checking if it's okay to merge in? If so, is it okay for you to help with this?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/113663003/320001
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/113663003/320001
Message was sent while issue was closed.
Change committed as 165565 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
