1650720 - Copying \r\n inside <pre> converts to \n\n

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: bug1528442-2.html

1. Open the attachment
2. Copy the content
3. Paste it somewhere

Expected: Single newline per line
Actual: Double newlines per line

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

This is confusing.

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

This is confusing, EncodeForTextUnicode() tries encoding the text with the mime type text/html instead of text/plain. The comment says:

  // note that we assign text/unicode as mime type, but in fact
  // nsHTMLCopyEncoder ignore it and use text/html or text/plain depending where
  // the selection is. if it is a selection into input/textarea element or in a
  // html content with pre-wrap style : text/plain. Otherwise text/html. see
  // nsHTMLCopyEncoder::SetSelection

I'm not sure why the copy would ever be text/html even when it's not pre-wrap style. This comment is wrong anyway, as it only becomes text/plain for <input> and <textarea> but not for pre-wrap style. That means, copying <pre> actually uses text/html.

The second confusing part is that EncodeForTextUnicode() tries encoding in raw-mode first (which does not affect newlines) and then retries in non-raw-mode, which causes the bug by incorrectly replacing \n with \r\n, making \r\n to \r\r\n. Why try twice?

Emilio, do you happen to have some idea about this, as one of the relevant functions has your FIXME comment?

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Not really, that FIXME is about assuming that all the selection is in the first selection range, which is not true for table selection for example.

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

Oops, okay. Keeping pinging semi-randomly based on commit history...

Mirko, it seems you refactored some of the code, do you have some idea about the mime type thing?

Comment 5

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

(In reply to Kagami :saschanaz from comment #2)

This is confusing, EncodeForTextUnicode() tries encoding the text with the mime type text/html instead of text/plain. The comment says:

  // note that we assign text/unicode as mime type, but in fact
  // nsHTMLCopyEncoder ignore it and use text/html or text/plain depending where
  // the selection is. if it is a selection into input/textarea element or in a
  // html content with pre-wrap style : text/plain. Otherwise text/html. see
  // nsHTMLCopyEncoder::SetSelection

I'm not sure why the copy would ever be text/html even when it's not pre-wrap style. This comment is wrong anyway, as it only becomes text/plain for <input> and <textarea> but not for pre-wrap style.

I'm not sure how relevant it is, but in another case text/plain is set too.

That means, copying <pre> actually uses text/html.

The second confusing part is that EncodeForTextUnicode() tries encoding in raw-mode first (which does not affect newlines) and then retries in non-raw-mode, which causes the bug by incorrectly replacing \n with \r\n, making \r\n to \r\r\n. Why try twice?

I'm unfamiliar with this detail, it needs investigation.

Emilio, do you happen to have some idea about this, as one of the relevant functions has your FIXME comment?

Comment 6

[kumar]

May be this behavior is also related to this bug.

Steps to reproduce

• Go to IEEE Explore page and open any research paper. For example: https://ieeexplore.ieee.org/document/7919538
• Click on Cite this button
• Select BibTex option
• Copy BibTex content using mouse and keyboard that's select text using mouse and the press Ctrl+C
• Paste it in any text editor

Actual ouput

Every sentence ends with an extra new line.

Expected output

There shouldn't be any extra new line after each sentence.

Additional

Like 152844#c4 example, I cannot reproduce this bug in HTML based editors like Librewriter; seems like problem is with text serialization.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

No immediate plan to work on this, please feel free to take it.

Comment 9

[Alice0775 White]

This page ( https://detail.chiebukuro.yahoo.co.jp/qa/question_detail/q12303673327 ) is also affected .

Chrome works as expected.

Comment 10

[Alice0775 White]

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=92fe907ddac8&tochange=bbe5086163c9

Suspect: Bug 731896

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 731896

:hsivonen, since you are the author of the regressor, bug 731896, could you take a look?

For more information, please visit BugBot documentation.

Comment 12

[Henri Sivonen (:hsivonen)]

You are supposed to use only LF in the DOM. On Windows, in-DOM LF gets converted to CRLF to match Windows convention. Putting a CR in the DOM is bad, and the outcome follows logically from the previous sentence, since CR stays as-is and LF gets replaced with CRLF.

From a logical point of view, the behavior described in comment 0 is as expected. However, if Web pages run into this in a way that users perceive as a problem that does not occur in Chrome (per comment 9), it makes sense to find out what Chrome does and start doing that.

Comment 13

[Alice0775 White]

Attachment: screenshot about comment#9

Comment 14

[Edgar Chen [:edgar]]

On macOS, the \r\n is replaced to \n\n on Firefox, whereas it is replaced to \n on Chromium. So it worth to test each platform, they could have different behavior.

Comment 15

[Edgar Chen [:edgar]]

There are two main actions involved,

1. Serialize selection into plaintext and put into system clipboard
2. Text editor read the clipboard plaintext data and insert the text.

---

For the second action, all browsers behave the same on each platform to turn the platform line break into DOM line break, e.g.

• \r\n ---> \n
• \r ---> \n

---

But on the first steps, Firefox behave differently than other browsers,

Mac:

	Firefox	Chrome	Safari
\r	\n	\r	\r
\n	\n	\n	\n
\r\r	\n\n	\r\r	\r\r
\r\n	\n\n	\r\n	\r\n
\n\r	\n\n	\n\r	\n\r
\n\n	\n\n	\n\n	\n\n
\r\r\r	\n\n\n	\r\r\r	\r\r\r
\r\r\n	\n\n\n	\r\r\n	\r\r\n
\r\n\r	\n\n\n	\r\n\r	\r\n\r
\r\n\n	\n\n\n	\r\n\n	\r\n\n
\n\r\r	\n\n\n	\n\r\r	\n\r\r
\n\r\n	\n\n\n	\n\r\n	\n\r\n
\n\n\r	\n\n\n	\n\n\r	\n\n\r
\n\n\n	\n\n\n	\n\n\n	\n\n\n

Linux:

	Firefox	Chrome
\r	\n	\r
\n	\n	\n
\r\r	\n\n	\r\r
\r\n	\n\n	\r\n
\n\r	\n\n	\n\r
\n\n	\n\n	\n\n
\r\r\r	\n\n\n	\r\r\r
\r\r\n	\n\n\n	\r\r\n
\r\n\r	\n\n\n	\r\n\r
\r\n\n	\n\n\n	\r\n\n
\n\r\r	\n\n\n	\n\r\r
\n\r\n	\n\n\n	\n\r\n
\n\n\r	\n\n\n	\n\n\r
\n\n\n	\n\n\n	\n\n\n

Windows:

	Firefox	Chrome
\r	\n	\r
\n	\n	\r\n
\r\r	\n\n	\r\r
\r\n	\n\n	\r\n
\n\r	\n\n	\r\n\r
\n\n	\n\n	\r\n\r\n
\r\r\r	\n\n\n	\r\r\r
\r\r\n	\n\n\n	\r\r\n
\r\n\r	\n\n\n	\r\n\r
\r\n\n	\n\n\n	\r\n\r\n
\n\r\r	\n\n\n	\r\n\r\r
\n\r\n	\n\n\n	\r\n\r\n
\n\n\r	\n\n\n	\r\n\r\n\r
\n\n\n	\n\n\n	\r\n\r\n\r\n

Comment 16

[Edgar Chen [:edgar]]

So on Linux and Mac, Safari and Chrome doesn't change the DOM text while serializing. And on Windows, Chrome convert LF to CRLF for Windows convention.

Firefox always covert CR to LF. It is handled in nsPlainTextSerializer.

Comment 17

[Edgar Chen [:edgar]]

selection.toString() has similar issue, but Chrome doesn't convert LF to CRLF even on Windows.

Comment 18

[Edgar Chen [:edgar]]

Attachment: Bug 1650720 - Rename DoAddText() to DoAddLineBreak();

Comment 19

[Edgar Chen [:edgar]]

Attachment: Bug 1650720 - Make nsIContentSerializer::AppendText() and AppendCDATASection() take Text;

Comment 20

[Phabricator Automation]

Comment on attachment 9513584 [details]
Bug 1650720 - Make nsIContentSerializer::AppendText() and AppendCDATASection() take Text;

Revision D265012 was moved to bug 1989141. Setting attachment 9513584 [details] to obsolete.

Comment 21

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=157656

Comment 22

[Edgar Chen [:edgar]]

Attachment: Bug 1650720 - Part 1: Move some serializer mochitest tests to dom/serializers/tests/; r?masayuki

Comment 23

[Edgar Chen [:edgar]]

Attachment: Bug 1650720 - Part 1: Add plaintext serializer tests for linebreak; r?masayuki

Comment 24

[Edgar Chen [:edgar]]

Attachment: Bug 1650720 - Part 2: Simplify nsPlainTextSerializer::ConvertToLinesAndOutput; r?masayuki

Comment 25

[Edgar Chen [:edgar]]

Attachment: Bug 1650720 - Part 3: Align link break coversion for clipboard copy with other browsers; r?masayuki

Comment 26

[Phabricator Automation]

Comment on attachment 9519423 [details]
Bug 1650720 - Part 1: Move some serializer mochitest tests to dom/serializers/tests/; r?masayuki

Revision D268242 was moved to bug 1997661. Setting attachment 9519423 [details] to obsolete.

Comment 27

[Edgar Chen [:edgar]]

Attachment: Bug 1650720 - Part 4: Add line break tests for copying selection; r?masayuki

Comment 28

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=161683

Comment 29

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=cd4cde85046adf31c9e982b9b672ccaa9852311c

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/21953f6384e8
https://hg.mozilla.org/integration/autoland/rev/2e952869cddc
Part 1: Add plaintext serializer tests for linebreak; r=masayuki
https://github.com/mozilla-firefox/firefox/commit/5e63b05f5423
https://hg.mozilla.org/integration/autoland/rev/25946606b292
Part 2: Simplify nsPlainTextSerializer::ConvertToLinesAndOutput; r=masayuki
https://github.com/mozilla-firefox/firefox/commit/3ced2a72e8f7
https://hg.mozilla.org/integration/autoland/rev/8301dc1d5d6d
Part 3: Align link break coversion for clipboard copy with other browsers; r=masayuki
https://github.com/mozilla-firefox/firefox/commit/62ce647fff1b
https://hg.mozilla.org/integration/autoland/rev/1678c77161c6
Part 4: Add line break tests for copying selection; r=masayuki

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/920ab7c17333
https://hg.mozilla.org/integration/autoland/rev/9fa4d47ced08
Revert "Bug 1650720 - Part 4: Add line break tests for copying selection; r=masayuki" for causing mochitest failures @ test_aboutmemory3.xhtml

Comment 32

[amarc]

Backed out for causing mochitest failures @ test_aboutmemory3.xhtml

• Backout link
• Push with failures
• Failure Log

Comment 33

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/55882 for changes under testing/web-platform/tests

Comment 34

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 35

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/55885 for changes under testing/web-platform/tests

Comment 36

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 37

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=162481

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1eaf3a5aecce
https://hg.mozilla.org/integration/autoland/rev/f498fb9dee3e
Part 1: Add plaintext serializer tests for linebreak; r=masayuki
https://github.com/mozilla-firefox/firefox/commit/0c709bd9cb0d
https://hg.mozilla.org/integration/autoland/rev/cda95ac89f88
Part 2: Simplify nsPlainTextSerializer::ConvertToLinesAndOutput; r=masayuki
https://github.com/mozilla-firefox/firefox/commit/b587521d0093
https://hg.mozilla.org/integration/autoland/rev/165dd0e12faf
Part 3: Align link break coversion for clipboard copy with other browsers; r=masayuki
https://github.com/mozilla-firefox/firefox/commit/8631543a5f89
https://hg.mozilla.org/integration/autoland/rev/68933e8da80f
Part 4: Add line break tests for copying selection; r=masayuki

Comment 39

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/55963 for changes under testing/web-platform/tests

Comment 40

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/f498fb9dee3e
https://hg.mozilla.org/mozilla-central/rev/cda95ac89f88
https://hg.mozilla.org/mozilla-central/rev/165dd0e12faf
https://hg.mozilla.org/mozilla-central/rev/68933e8da80f

Comment 41

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot