Bug 85282 - Extra line-breaks added when copying from source.
Summary: Extra line-breaks added when copying from source.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Windows XP
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-01 07:42 PDT by Alexander Pavlov (apavlov)
Modified: 2012-05-08 02:48 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.83 KB, patch)
2012-05-01 07:57 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (13.68 KB, patch)
2012-05-02 09:50 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (6.57 KB, patch)
2012-05-03 03:48 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (6.59 KB, patch)
2012-05-03 04:23 PDT, Alexander Pavlov (apavlov)
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2012-05-01 07:42:39 PDT
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".
Comment 1 Ryosuke Niwa 2012-05-01 07:48:05 PDT
Where are we calling replaceNewlinesWithWindowsStyleNewlines()?
Comment 2 Alexander Pavlov (apavlov) 2012-05-01 07:51:31 PDT
(In reply to comment #1)
> Where are we calling replaceNewlinesWithWindowsStyleNewlines()?

In Pasteboard::writeSelection() and Pasteboard::writePlainText().
Comment 3 Alexander Pavlov (apavlov) 2012-05-01 07:57:53 PDT
Created attachment 139628 [details]
Patch
Comment 4 Alexander Pavlov (apavlov) 2012-05-01 07:59:55 PDT
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).
Comment 5 Ryosuke Niwa 2012-05-01 08:06:16 PDT
(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?
Comment 6 Alexander Pavlov (apavlov) 2012-05-01 08:21:42 PDT
(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.
Comment 7 Ryosuke Niwa 2012-05-01 08:49:33 PDT
Did you try execCommand('copy') and then execCommand('paste')? (only works in DRT)
Comment 8 Alexander Pavlov (apavlov) 2012-05-01 09:30:04 PDT
(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).
Comment 9 Ryosuke Niwa 2012-05-01 09:38:22 PDT
(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?
Comment 10 Alexander Pavlov (apavlov) 2012-05-02 02:48:04 PDT
(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.
Comment 11 Alexander Pavlov (apavlov) 2012-05-02 09:50:27 PDT
Created attachment 139832 [details]
Patch
Comment 12 WebKit Review Bot 2012-05-02 09:52:27 PDT
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.
Comment 13 Ryosuke Niwa 2012-05-02 23:06:12 PDT
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.
Comment 14 Alexander Pavlov (apavlov) 2012-05-03 03:48:00 PDT
Created attachment 139980 [details]
Patch
Comment 15 Alexander Pavlov (apavlov) 2012-05-03 04:23:29 PDT
Created attachment 139982 [details]
Patch
Comment 16 Alexander Pavlov (apavlov) 2012-05-03 04:26:04 PDT
(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.
Comment 17 Alexander Pavlov (apavlov) 2012-05-07 05:21:10 PDT
@reviewers: ping
Comment 18 Alexander Pavlov (apavlov) 2012-05-08 02:48:17 PDT
Committed r116408: <http://trac.webkit.org/changeset/116408>