524975 - Copy-Paste of simple HTML adds extra space not present in original content

Status: RESOLVED WORKSFORME

(Core :: DOM: Serializers, defect, P2)

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
1. Open URL (Testcase #5 in bug 39098)
2. press CTRL+C
3. focus the TEXTAREA
4. press CTRL+V

ACTUAL RESULT
This is a draggable  bit of text.

EXPECTED RESULT
This is a draggable bit of text.

PLATFORMS AND BUILDS TESTED
Bug occurs in Firefox trunk on Linux (and most likely all platforms).
Bug occurs in Firefox 3.6b2pre 20091028
Bug does not occur in Firefox 3.0.x or 3.5.3

Regression window: 2009-04-25-03 -- 2009-04-26-03
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dfff608ebad0&tochange=b552b1ef8aa0
I'm guessing it's a regression from bug 422403.

Comment 1

[Mats Palmgren (inactive)]

I discovered this by making the drag-n-drop serialization of the selection
use the same code as the kbd Copy command (in bug 39098).
This made the "content/events/test/test_dragstart.html" mochitest fail:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256695352.1256705398.7411.gz
It appears the bug occurs both for the text/plain and text/html serialization.

Comment 2

[Laurent Jouanneau]

for text/html serialization, it is not a regression. It is just that the new html serializer does a better job for pretty printing. so you have to update that test.

For text/plain, I don't know yet. I didn't modify the plain text serializer with the patch of bug 422403.

I'm investigating on this.

Comment 3

[Laurent Jouanneau]

I understood the issue.

nsHTMLCopyEncoder is used for the serialization, and is called into SelectionCopyHelper (nsCopySupport.cpp).

There is a first serialization here http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCopySupport.cpp#134 of the selection. It returns

<div id="draggable" ondragstart="doDragStartSelection(event)">This is a <em>draggable</em>\n bit of text.</div>

Note the \n added by the serializer, because of the default flag on wrapping in the HTML serializer. As I said before, this \n is added because of the better pretty printing made by the serializer.

Then this string is converted by nsHTMLFormatConverter http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCopySupport.cpp#153 . This converter remove all html tag, and it seems it replaces all \n by a space, this why we have two spaces between "draggable" and "bit".

A solution is to disable pretty printing in the nsHTMLCopyEncoder. We have to verify if it doesn't break something somewhere.


I notice an other issue in the SelectionCopyHelper function (not related to the bug) : if the selection to copy is not in a plain text context (bIsHTMLCopy = true), then the selection is serialized twice ! The comment for the first serialization says "We always require a plaintext version", and then the kUnicodeMime is passed to the init method of the encoder. However, it is useless, because nsHTMLCopyEncoder always uses text/html, it doesn't use the given mime type  : http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocumentEncoder.cpp#1116

In the "if (bIsHTMLCopy)" statement, the selection is serialized a second time. The only difference is the called method: EncodeToString vs EncodeToStringWithContext.

I think we could optimize this piece of code : only do the first serialization when bIsHTMLCopy == false. 

Do you agree ? Olly ? Mats ?

If ok I can provide a patch to fix these two issues.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #3)
> A solution is to disable pretty printing in the nsHTMLCopyEncoder. We have to
> verify if it doesn't break something somewhere.
Sounds reasonable.


> I notice an other issue in the SelectionCopyHelper function (not related to the
> bug)
I this a regression too? If it is, is it a serious one?
And in any case,  please file a new bug for this issue.

Comment 5

[Laurent Jouanneau]

Attachment: patch v1

here is the patch, which only changes flags for the encoder for the copy. I added also a test file.

For the second issue, it is not really a regression, just an optimization we can do. I will create a new bug for that and will attach a patch to it.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 409067 [details] [diff] [review]
patch v1

How does this work with long lines?
Based on the documentation OutputRaw may wrap.
But I'm not sure if the documentation is right, since here you say it doesn't wrap 
https://bugzilla.mozilla.org/show_bug.cgi?id=422403#c7

Comment 7

[Laurent Jouanneau]

I don't know which documentation you talk about, but as indicated in the comment for the OutputRaw flag in the nsIDocumentEncoder.idl, using these flag disable the wrapping and the formating. And this is the real behavior not only with the patch of bug 422403, but it was also the previous behavior (gecko 1.9.1 and before).

Of course, with OutputRaw, there isn't any more new \n, but existing \n are kept. so if in the orignal source code there is a line break like this

<div>This is a <em>draggable</em>
 bit of text.</div>

However, with such cases, I think we will still have the issue, (here, a double space between "draggable" and "bit"), because of the behavior of nsHTMLFormatConverter used in nsCopySupport. As I said, it seems it simply replaces \n by a space, after removing html tags. I don't know if this issue in nsHTMLFormatConverter can be fixed (and how, I don't know this part of gecko). 

I don't know if using the plain text serializer instead of nsHTMLFormatConverter could be an other solution...

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Ah, oops, I misunderstood the comment in nsIDocumentEncoder.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Does the patch bring back the exactly same behavior what we have on 1.9.1?
I wonder especially the removal of flags = 0;

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 409067 [details] [diff] [review]
patch v1

Hmm, did this pass regression tests?
This patch breaks copy from a plain text context (see Testcase #1).
I believe the first value for 'flags' should be:
  nsIDocumentEncoder::OutputPreformatted | nsIDocumentEncoder::OutputRaw
the second should be:
  nsIDocumentEncoder::OutputRaw

(fwiw, that passed regression tests on TryServer together with the patch
in bug 39098)

Comment 12

[Laurent Jouanneau]

Attachment: patch v2

I added the OutputPreformatted flag and re introduce "flags = 0". You're right, it was useless to remove both.

In fact, I didn't see the code of nsHTMLCopyEncoder::setSelection, which select a different serializer in some case. It uses the plaintext serializer when the selection to copy is inside an input and textarea or in a preformatted html element. And it uses the html serializer in the other cases. I thought it uses always the html serializer. And so, the second issue I explained is no more valid for some cases.

I updated the unit test file to include your test case.

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 409324 [details] [diff] [review]
patch v2

Looks good, r=mats.  It still breaks the "test_dragstart.html"
test together with the patch in bug 39098, but that bug isn't
for 1.9.2, so I'll add a \n to the expected result on trunk.

As far as I know this is a change in behavior, and should be considered
when approving this patch for 1.9.2.  I don't see a problem with having
the extra newline in HTML-to-HTML copy on 1.9.2, OTOH, I don't see the
benefit from it either.

I'm requesting an explicit approval1.9.2 to consider the above.

Comment 14

[Laurent Jouanneau]

checked in: http://hg.mozilla.org/mozilla-central/rev/4ae32b827a7f

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 409324 [details] [diff] [review]
patch v2

(this is a blocker, doesn't need explicit approval, please land ASAP)

Comment 16

[Johnny Stenback (:jst)]

Jonas, any thoughts on whether we'll be better off with or without this patch for 1.9.2?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wow, this bug is confusing for someone not familiar with our serializers :)

A few questions:
1. So the problem is that when copying, we would first prettyprint the selection, and then concatenate all the textnodes in the result of the prettyprint? And the fix is to not prettyprint but rather *just* concatenate the existing textnodes?

2. Does this patch make us behave as we did in 1.9.1 in all cases? Comment 13 seems to indicate "no". If the answer is "no", then when and how do we differ?

3. If this patch is correct, then why does it combined with the patch in bug 39098 break a existing testcase? Is that a problem with this patch, the patch in bug 39098, or a problem with the testcase?

Comment 18

[Laurent Jouanneau]

1) selection inside input/textarea is serialized with the plaintext serializer (so concatenation of textnode + pretty printing), otherwise, it is serialized with the html serializer. And in this case, tags are then removed and \n are replaced by spaces (by using the html converter) to have a text/plain version in the clipboard.

The new html serializer does a better job on wrapping for the pretty printing, so in the example, we have a new \n. The problem is that the \n are replaced by a space, by the html converter. So a solution is to not do pretty printing during the serialization.

2) Yes, for most of case. 

3) No, Mats just changed the way the drag-n-drop serialization is made, by using the same code as the kbd Copy command, so it uses the html serializer, and because of the new wrapping behavior, it changes a test, and  then Mats discovers the bug 524975.

I think we should land my patch to 1.9.2, since kbd copy command doesn't behave anymore as in 1.9.1.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #18)
> 1) selection inside input/textarea is serialized with the plaintext serializer
> (so concatenation of textnode + pretty printing), otherwise, it is serialized
> with the html serializer. And in this case, tags are then removed and \n are
> replaced by spaces (by using the html converter) to have a text/plain version
> in the clipboard.
> 
> The new html serializer does a better job on wrapping for the pretty printing,
> so in the example, we have a new \n. The problem is that the \n are replaced by
> a space, by the html converter. So a solution is to not do pretty printing
> during the serialization.

Ok, so it sounds like the answer is simply "yes" to my question? Or more precisely "yes. Except when copying from input/textarea, in which case none of the code relevant to this bug is used".

Is that correct?

> 2) Yes, for most of case. 

Well, that's a "no" then :). So in which cases are we now different from 1.9.1?

> 3) No, Mats just changed the way the drag-n-drop serialization is made, by
> using the same code as the kbd Copy command, so it uses the html serializer,
> and because of the new wrapping behavior, it changes a test, and  then Mats
> discovers the bug 524975.

That still doesn't answer the question: If mats patch were to land, is the problem in his patch, in your patch or in the test. It sounds like you're saying that it's in the test?


Another important question here is does this patch change any behavior that webpages see? I.e. is it possible that this patch will break (or fix) websites? Or is there a risk that it will?

Comment 20

[Laurent Jouanneau]

let's summary. There are two issues:

1) this current bug: "copy-paste of simple HTML adds extra space not present in original content". This is clearly a regression after my work on the serializer (bug 422403). I already explains why in previous comments. So  should be landed in 1.9.2 (IMHO) since 1.9.2 has the new serializer. 

However, with the patch I provided on this bug 524975, Copy-paste will behave in a different manner for HTML content. 1.9.1: it was serialized with pretty printing flags. With my patch: without pretty printing. I don't think this difference is important, at least for the text/html flavor for the clipboard.

2) the test on drag-n-drop. The problem is in the test. Since mats change the code of the serialization for drag-n-drop (and then the result of the serialization, as I explained), he has to adapt the test.


> Another important question here is does this patch change any behavior that
webpages see?

Clearly, it doesn't.

However, I just found a regression with my patch: in fact, copying HTML produces now non pretty printing, not only for the text/html flavor, but also for text/unicode flavor. So copying this DOM:

bla
<ul>
  <li>foo</li>
  <li>bar</li>
</ul>

When we paste it, we have

bla
foo
bar

instead of 

bla
  * foo
  * bar

like in gecko 1.9.1.

I reopen the bug. (or should I create a new bug ?)
I'm working on a new patch.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #20)
> let's summary. There are two issues:
> 
> 1) this current bug: "copy-paste of simple HTML adds extra space not present in
> original content". This is clearly a regression after my work on the serializer
> (bug 422403). I already explains why in previous comments. So  should be landed
> in 1.9.2 (IMHO) since 1.9.2 has the new serializer. 
> 
> However, with the patch I provided on this bug 524975, Copy-paste will behave
> in a different manner for HTML content. 1.9.1: it was serialized with pretty
> printing flags. With my patch: without pretty printing. I don't think this
> difference is important, at least for the text/html flavor for the clipboard.

My gut reaction is that the behavior with your patch is better, yes. However it's a scary change this late in the game.

> 2) the test on drag-n-drop. The problem is in the test. Since mats change the
> code of the serialization for drag-n-drop (and then the result of the
> serialization, as I explained), he has to adapt the test.

Ok, if the problem is in the test then that's all good. Let's never talk about it in this bug any more :) I only care about bugs in the code, not in the tests.

> However, I just found a regression with my patch: in fact, copying HTML
> produces now non pretty printing, not only for the text/html flavor, but also
> for text/unicode flavor. So copying this DOM:
> 
> bla
> <ul>
>   <li>foo</li>
>   <li>bar</li>
> </ul>
> 
> When we paste it, we have
> 
> bla
> foo
> bar
> 
> instead of 
> 
> bla
>   * foo
>   * bar
> 
> like in gecko 1.9.1.
> 
> I reopen the bug. (or should I create a new bug ?)
> I'm working on a new patch.

This scares me, a lot. This would have been a bad regression to ship with.

I think given how late in the game this is, I don't think we should take anything on the branch. These changes are better done early in the cycle.

For example while not prettyprinting copied HTML seems like a good idea, it's a bigger change than i'm not comfortable with right before release.

So my recommendation is to not take this for the 1.9.2 release. *Possibly* it's something to consider for 1.9.2.x.


This leaves us with the extra space regression. Either we can leave things as is now and consider fixing it in 1.9.2.x, or we can consider backing out bug 422403 on the branch. Do you have an opinion?

Comment 22

[Laurent Jouanneau]

Ok. I'm agree to leave the 1.9.2 branch as its current state. I think it's a good idea to not land the patch in it, since it introduces a more critical regression (described in my previous comment) than the extra space bug IMHO. And I will have more time to provide a better patch for bug 524975 :-)

Backing out bug 422403 will be harder, because other changes have been made on the serializer after the check in of the html5 parser. I prefer to spend time on the patch for bug 524975 instead of on the backout of bug 422403 (which is a huge patch).

Comment 23

[Johnny Stenback (:jst)]

blocking1.9.2- per the above comments, we'll work on getting this fixed on trunk for the next release! Thank you Laurent for your contributions here!

Comment 24

[Johnny Stenback (:jst)]

Laurent, do you think you would have time to work on an updated patch for this bug?

Comment 25

[Laurent Jouanneau]

sorry for the delay. I'm working on it this week.

Comment 26

[Laurent Jouanneau]

Attachment: Additional tests for the trunk

My apologizes, it seems in fact there is no regression. Perhaps my old builds were bad. I can't reproduce the regression I found. I added some tests to verify that copy paste of a piece HTML produce a well formated text in text/plain. See this patch.

Comment 27

[Laurent Jouanneau]

Attachment: Patch for the 1.9.2 branch

The full patch for the 1.9.2 branch.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

So why would we take the patch for 1.9.2?

Comment 29

[Boris Zbarsky [:bzbarsky]]

This looks like the underlying issue is exactly what bug 549295 is about, which is that the behavior of not passing any wrapping flags at all was totally changed from "wrap if the text is long" to "wrap all the time"....

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Laurent, does the patch fix bug 549295 on FF 3.6? Or vice-versa?

Comment 31

[Laurent Jouanneau]

no the patch doesn't fix bug 549295. But the patch of bug 549295 probably fixes (or at least, helps to fix) bug 524975. I have to verify it.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 424229 [details] [diff] [review]
Patch for the 1.9.2 branch

Please renominate when the reviews are complete.

Comment 33

[Laurent Jouanneau]

checked in additionnal tests http://hg.mozilla.org/mozilla-central/rev/e9e8eaec0cbb

Comment 34

[Laurent Jouanneau]

fixed a failed test: http://hg.mozilla.org/mozilla-central/rev/d898c3dbdc27

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #31)
> no the patch doesn't fix bug 549295. But the patch of bug 549295 probably fixes
> (or at least, helps to fix) bug 524975. I have to verify it.

Any news here?

Comment 36

[Laurent Jouanneau]

As Boris said, the regression comes from the issue described in bug 549295, and the patch of bug 549295 fix the current bug, even without the attachement 409324 already landed into the trunk.

However I think the real issue is in the html converter used by SelectionCopyHelper, when it replaces all LF by a space. If a LF is preceded by a space, it should not replace it by a space, but only remove it. However I don't know how the html convert works, so perhaps I say silly things :-)

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Should we back out attachment 409324 [details] [diff] [review] from trunk, if the real regression fix is
bug 549295?

Comment 38

[Laurent Jouanneau]

well, attachment 409324 [details] [diff] [review] just add a flag for the encoder. I'm not sure that this flag does some regression, so we can keep it... In fact, now with the patch for bug 549295, the real question is : do we need to wrap on long lines (old behavior before the new serializer) or not, for serialized content for the clipboard. If yes, we should revert the change.

However, if we revert this change, we should keep the test I added, and I can remove the added flag directly with the patch for bug 549295.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

We should have the old behavior, unless there is some very good reason
not to.

Comment 40

[Laurent Jouanneau]

Ok I will remove the flag in my patch for bug 549295

Comment 41

[Anthony DiSante]

I'm not sure if this is the most relevant bug to my issue; searching bugzilla for "copy paste space" (without the quotes) returns several bugs.  I'm seeing this behavior in Thunderbird 3.0.4/Linux, whereas in Tbird 2.x, the bug was not present.

What happens is, when you select the last word in a paragraph, you need to be careful not to select "too far" past the end of the word (into the empty area to the right of the word).  If you go too far, your selection will not visibly contain any whitespace; but when you copy & paste it, a trailing space appears in the pasted text.  This also happens when you double-click to select the word, as long as it's the last word in the paragraph (and there's no punctuation after it).  This happens even for plain-text messages (content-type: text/plain).

Firefox does a similar thing if the word you're double-clicking is followed by a newline in the source code, as opposed to being immediately followed by an HTML tag.  You can plainly see that there's no space in the visible selection, yet upon pasting, it's there.

From the comments here about serializers and test cases and regressions, and the several other linked bugs, this seems like a complex issue.  I hope my report helps, or is at least relevant; please let me know if there's a better bug for me to look at instead.

Comment 42

[Laurent Jouanneau]

Finally, I forgot to remove the flag in the patch for bug 549295.

And because of this  nsIDocumentEncoder::OutputRaw flag, it causes some bugs like bug 572543.

I really should remove this flag :-) Patch soon.

Comment 43

[Aaron Digulla]

I have the same issue with Firefox 4.0.1: Firefox adds an invisible space if there is a newline between the last word and a tag (as in "word&#10;<div>") or if there is a space after the word (as in "word <br/>".

This is really annoying if you copy passwords since you can't see the additional space character when you paste.

Comment 44

[Q.uentin]

Any news ? I have this bug

Comment 45

[Dror]

Still had this issue with Firefox 28.0.
Ironically, the workaround was to add   after the word and then it worked fine.

Comment 46

[aaron]

Persists to FF 29

Comment 47

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Should be fixed by bug 333064?

Comment 48

[Mats Palmgren (inactive)]

I think it must have been fixed earlier than that.
I can't reproduce this bug using FF46 on Linux.

QA: please test this bug on all platforms; resolve as worksforme if OK.  Thanks.

Comment 49

[aaron]

Persists to FF 46.0.1 on OSX 10.10
To reproduce:

1. Open this issue, https://bugzilla.mozilla.org/show_bug.cgi?id=524975 , in a new private window (so that you're not authenticated)
2. Scroll down to this comment
3. Double-click my name ("aaron"), to highlight the entire thing
4. copy it (either ctrl-C or right-click, copy)
5. paste it somewhere and observe the leading space character, not present in the HTML (which is like <span>aaron</span>)

See video documentation: https://www.youtube.com/watch?v=HI1LqAUaMrI

Comment 50

[Mats Palmgren (inactive)]

So the DOM range we get when double-clicking the name per STR in comment 49 :
let range = window.getSelection().getRangeAt(0)
range.commonAncestorContainer is this:

<span class="bz_comment_user"><a href="user_profile?user_id=497669">
  <img src="https://secure.gravatar.com/avatar/d89dd360c498c1bf4ca9d9a01d08596a?d=mm&amp;size=64" width="32" height="32" align="middle" border="0"></a>
          <span class="vcard vcard_497669"><span class="ln">aaron</span>
</span>
       </span>

range.startContainer is the text node before the vcard_497669 span.
range.startOffset is 1.
range.endContainer is the text node after the vcard_497669 span (i.e. the newline).
range.endOffset is 0.

So, given that range, copy-paste should actually result in " aaron".

The error seems to be that the double-click included the space in
the text node before the vcard_497669 span.  It shouldn't do that,
at least not on platforms that excludes the surrounding space when
double-clicking words in plain text.

I'm pretty sure this is a different bug than what I originally reported
though, but let's keep the bug open for now until we know if the latter
bug is on file.

Comment 51

[Federico Bruni]

I can confirm the problem described in comment #49. I'm using FF 47 on Linux (Fedora 23).

Comment 52

[simon]

Happens to me as well. Pasting into a terminal adds 0~ and 1~ around the copied text. Really **** behavior. Never using Firefox again.

Comment 53

[aaron]

This appears to be fixed
FF 51.0.1
OSX 10.10.5

Comment 54

[aaron]

(In reply to aaron from comment #53)
> This appears to be fixed
> FF 51.0.1
> OSX 10.10.5

Nope, sorry, spoke too soon.
It is not fixed in latest 51.0.1, OSX 10.10.5

Comment 55

[Peter Magyari (Desktop QA)]

I know this is a very old issue (sorry for the poke) but I cannot reproduce it on the latest Firefox versions on my end.
It would be nice to close it if it does not occur anymore.

Have any of you by any chance seen this issue occur recently?

Thank you!

Comment 56

[Federico Bruni]

I cannot reproduce it on Firefox 94.0, Fedora 34.

Comment 57

[Peter Magyari (Desktop QA)]

Since you cannot reproduce it anymore either, I'm going to close it for now.
If the issue still occurs to someone, please feel free to re-open the report.
Thank you for the update!