Bug 85282 – Extra line-breaks added when copying from source.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

What steps will reproduce the problem?
1. View a page's source.
2. Select and copy source code.
3. Paste source code into a text editor.

What is the expected result?
The text to be pasted as normal.

What happens instead?
An additional one or two line-breaks are inserted between each line.

Upstreaming http://code.google.com/p/chromium/issues/detail?id=91377

The issue is caused by the replaceNewlinesWithWindowsStyleNewlines() implementation, which replaces ALL \n's by \r\n, so that "\r\n" turns into "\r\r\n".

[Ryosuke Niwa]

Where are we calling replaceNewlinesWithWindowsStyleNewlines()?

[Alexander Pavlov (apavlov)]

(In reply to comment #1)
> Where are we calling replaceNewlinesWithWindowsStyleNewlines()?

In Pasteboard::writeSelection() and Pasteboard::writePlainText().

[Alexander Pavlov (apavlov)]

Created attachment 139628 [details]
Patch

[Alexander Pavlov (apavlov)]

Is there any workable way to test this, by the way? A test involving event.clipboardData.get/setData() did not seem to show any difference, getData() returning the data passed into setData() intact (all combinations of \r's and \n's).

[Ryosuke Niwa]

(In reply to comment #4)
> Is there any workable way to test this, by the way? A test involving event.clipboardData.get/setData() did not seem to show any difference, getData() returning the data passed into setData() intact (all combinations of \r's and \n's).

That's odd. Could you investigate which function is generating text with \r\n?

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> (In reply to comment #4)
> > Is there any workable way to test this, by the way? A test involving event.clipboardData.get/setData() did not seem to show any difference, getData() returning the data passed into setData() intact (all combinations of \r's and \n's).
> 
> That's odd. Could you investigate which function is generating text with \r\n?

I'm not quite clear on what you mean. I was talking about my test that tried to handle a text with \r, \n, \r\n, \r\r\n etc. The original copied text in the browser may or may not contain \r\n.

[Ryosuke Niwa]

Did you try execCommand('copy') and then execCommand('paste')? (only works in DRT)

[Alexander Pavlov (apavlov)]

(In reply to comment #7)
> Did you try execCommand('copy') and then execCommand('paste')? (only works in DRT)

I'm not sure how I can execCommand('copy') different CR/LF sequences other than the way I've come up with: preElement.textContent = "Test1\r\nTest2\rTest3\n...".replace(/[\\]r/g, "\r").replace(/[\\]n/g, "\n");
then copy and paste it.

Here is the result (after execCommand("paste") for the copied char sequences):

Both versions (original/fixed) of code do this:
\r -> \n
\n -> \n
\r\n -> \n
\r\r\n -> \n\n
\r\n\r\n -> \n\n

Either textContent setter fixes the thing in place, or the "paste" call somehow fixes the result, because other applications get this copied content wrong (with duplicated newlines).

[Ryosuke Niwa]

(In reply to comment #8)
> Either textContent setter fixes the thing in place, or the "paste" call somehow fixes the result, because other applications get this copied content wrong (with duplicated newlines).

It's textContent appears to preserve \r so it's probably the paste call. Can't you get the original text out of dataTransfer on paste event?

[Alexander Pavlov (apavlov)]

(In reply to comment #9)
> (In reply to comment #8)
> > Either textContent setter fixes the thing in place, or the "paste" call somehow fixes the result, because other applications get this copied content wrong (with duplicated newlines).
> 
> It's textContent appears to preserve \r so it's probably the paste call. Can't you get the original text out of dataTransfer on paste event?

OK, got it sorted out. Also, marking this as a Windows-only bug.

[Alexander Pavlov (apavlov)]

Created attachment 139832 [details]
Patch

[WebKit Review Bot]

Attachment 139832 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1
LayoutTests/platform/gtk/test_expectations.txt:496:  More specific entry on line 328 overrides line 496 fast/workers/storage/use-same-database-in-page-and-workers.html  [test/expectations] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 139832 [details]
Patch

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

r- due to nits.

> Source/WebCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description.

> Source/WebCore/ChangeLog:11
> +        Test: editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html

Please add this test to LayoutTests/platform/win/editing/pasteboard instead so that you don't have to add them to Skipped lists.

> Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:48
>  void replaceNewlinesWithWindowsStyleNewlines(String& str)

Is there anyway we can share this function with Windows port?

> Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:54
> +    int index = 0;
> +    int strLength = static_cast<int>(str.length());

We should probably use size_t for these two variables.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:306
> +    int index = 0;
> +    int strLength = static_cast<int>(str.length());

Ditto.

> LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:1
> +<html>

Missing DOCTYPE.

> LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:9
> +  var data = event.clipboardData.getData('text/plain');
> +  document.getElementById("result").textContent += data.replace(/\r/g, "\\r").replace(/\n/g, "\\n");
> +  event.preventDefault();

Please use 4-space indentation.

> LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:22
> +  var selection = window.getSelection();

I don't think this variable is buying us anything.

> LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:25
> +  selection.addRange(range);

s/selection/getSelection()/

> LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:27
> +  selection.removeAllRanges();

Ditto.

[Alexander Pavlov (apavlov)]

Created attachment 139980 [details]
Patch

[Alexander Pavlov (apavlov)]

Created attachment 139982 [details]
Patch

[Alexander Pavlov (apavlov)]

(In reply to comment #13)
> (From update of attachment 139832 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139832&action=review
> 
> r- due to nits.
> 
> > Source/WebCore/ChangeLog:9
> > +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the long description.

Fixed.

> > Source/WebCore/ChangeLog:11
> > +        Test: editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html
> 
> Please add this test to LayoutTests/platform/win/editing/pasteboard instead so that you don't have to add them to Skipped lists.

Done.

> > Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:48
> >  void replaceNewlinesWithWindowsStyleNewlines(String& str)
> 
> Is there anyway we can share this function with Windows port?

As discussed in chat, this requires a separate change.

> > Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:54
> > +    int index = 0;
> > +    int strLength = static_cast<int>(str.length());
> 
> We should probably use size_t for these two variables.

This snippet was copied from the replace() function defined in RegularExpression.cpp, but I decided to stick with unsigned, since it is the exact type used by class String for indexing and length().

> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:306
> > +    int index = 0;
> > +    int strLength = static_cast<int>(str.length());
> 
> Ditto.

Same as above.

> > LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:1
> > +<html>
> 
> Missing DOCTYPE.

Added.

> > LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:9
> > +  var data = event.clipboardData.getData('text/plain');
> > +  document.getElementById("result").textContent += data.replace(/\r/g, "\\r").replace(/\n/g, "\\n");
> > +  event.preventDefault();
> 
> Please use 4-space indentation.

Fixed.

> > LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:22
> > +  var selection = window.getSelection();
> 
> I don't think this variable is buying us anything.

Fixed.

> > LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:25
> > +  selection.addRange(range);
> 
> s/selection/getSelection()/

Fixed.

> > LayoutTests/editing/pasteboard/pasting-crlf-isnt-translated-to-crcrlf-win.html:27
> > +  selection.removeAllRanges();
> 
> Ditto.

Fixed.

[Alexander Pavlov (apavlov)]

@reviewers: ping

[Alexander Pavlov (apavlov)]

Committed r116408: <http://trac.webkit.org/changeset/116408>