1123505 - Unbalanced tags in copied multi-range selection breaks pasting into rich text editors

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Jefferson]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150108202552

Steps to reproduce:

Scenario 1: In Google search results, select from the end of the first result to the end of the third result, copy to the clipboard, then paste into a rich text editor (e.g., Gmail compose, Yahoo! Mail compose, Outlook.com compose).

Scenario 2: In Google search results, select from the end of the third result to the beginning of the second result, copy to the clipboard, then paste into a rich text editor (e.g., Gmail compose, Yahoo! Mail compose, Outlook.com compose).

This was tested with Google search results. Page structure is:

 <ol>
   <div>
     <li>
       <div>
         <h3>
         <div>



Actual results:

Scenario 1: Copied results appear in the email message. (With messy numbering, but that's to be expected with list elements.)

Clipboard Inspector shows the beginning of the fragment as: <li class="g"><div class="rc" data-hveid="31">

Scenario 2: Nothing appears in the email message except perhaps a line break.

Clipboard Inspector shows the beginning of the fragment as: <h3 class="r"> 

The parent div tag and its parent li tag are not included in Scenario 2.



Expected results:

Scenario 1: Works as expected.

Scenario 2: Copied results should appear.

Notes:

(A) Various other selections of Google results are not paste-able in Firefox 35. 

(B) Related SuMo thread: https://support.mozilla.org/questions/1041949

(C) Could this be related to bug 1061810?

Comment 1

[:Gijs (he/him)]

(In reply to Jefferson from comment #0)

I can reproduce scenario 2 being broken (not tried scenario 1).

> Notes:
> 
> (A) Various other selections of Google results are not paste-able in Firefox
> 35. 
> 
> (B) Related SuMo thread: https://support.mozilla.org/questions/1041949
> 
> (C) Could this be related to bug 1061810?

Possibly. Masayuki-san and/or Olli, do you know?

FWIW, the pasting also doesn't work in a plain <div contenteditable="true">, but if I copy the outer HTML of the results with DOMI and paste them in another page, and try to select from there, it works fine, so I expect Google's styles are involved somehow. I've not tried to narrow it down further just yet. :-(

Comment 2

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

Is this a regression? 
Possibly related to Bug 739396?

Comment 3

[Jefferson]

(In reply to :Gijs Kruitbosch from comment #1)
> I expect Google's styles are involved somehow.

Using View > Page Style > No Style on Google results and then re-selecting seems to work around Scenario 2. But I can't see any unusual style rules for the parent div and its parent li tags. Perhaps a mouseup script is doing something.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hack in ContentEventHandler shouldn't cause this bug.

As far as I tested with mozregression, the regression range is (the latest good is 2014-09-10):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=152ef25e89ae&tochange=98ea98c8191a

Comment 5

[:Gijs (he/him)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> Hack in ContentEventHandler shouldn't cause this bug.
> 
> As far as I tested with mozregression, the regression range is (the latest
> good is 2014-09-10):
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=152ef25e89ae&tochange=98ea98c8191a

Olli's suggestion of bug 739396 is in that range, at least...

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase #2

It's not an error in the code that bug 739396 landed per se.  It's just that
it triggers an existing bug in serialization / copying code with multi-range
selections.  This testcase doesn't use 'user-select:none' but sets up the
same ranges using script that you would get with Select All in Testcase #1.
The same problem occurs when copy-pasting this into a non-plaintext target.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 1, fix

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 2, tests

Comment 11

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c33a42fd008

Comment 12

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

Comment on attachment 8557073 [details] [diff] [review]
part 1, fix


>   nsINode* endParent = aRange->GetEndParent();
>   NS_ENSURE_TRUE(endParent, NS_ERROR_FAILURE);
>   int32_t endOffset = aRange->EndOffset();
> 
>+  mStartDepth = mEndDepth = 0;
So this looks ok

>+      if (i == 0) {
>+        firstRangeStartDepth = mStartDepth;
>+      }
>     }
>+    mStartDepth = firstRangeStartDepth;
But I don't understand this bit.
Why do we need this?


Would be good to test tables, since this code has special cases for <tr>

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8557073 [details] [diff] [review]
part 1, fix

>+      if (i == 0) {
>+        firstRangeStartDepth = mStartDepth;
>+      }

This saves 'mStartDepth' for the first range in 'firstRangeStartDepth'.

>+    mStartDepth = firstRangeStartDepth;

This restores 'mStartDepth' after the loop so that it has the value of
the first range since in a multi-range selection it's clobbered as we
loop for each range.  The end result is that 'mStartDepth' is that of
the first range and 'mEndDepth' is that of the last range.

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 2, tests

> Would be good to test tables, since this code has special cases for <tr>

Sure, I've added couple of tests for that.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be42d298b0ea

Comment 15

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

(In reply to Mats Palmgren (:mats) from comment #13)

> >+    mStartDepth = firstRangeStartDepth;
> 
> This restores 'mStartDepth' after the loop
But why?

Comment 16

[Mats Palmgren (inactive)]

Ah, sorry I misunderstood your question.

nsHTMLEditor::CreateDOMFragmentFromPaste expects mStartDepth to be the depth for
the first node in the fragment, and mEndDepth for the last:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp?rev=6274430b28ba#2023

Comment 17

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

It does? I guess I missed that somehow earlier when I was looking at that method. I'll re-review tomorrow.

Comment 18

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

Comment on attachment 8557073 [details] [diff] [review]
part 1, fix

This code is so bizarre. 
(I'd say we're misusing DataTransfer by using text/_moz_htmlinfo hack in it.)

Comment 19

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a52cd3989c
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb30c6b2e138

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5a52cd3989c
https://hg.mozilla.org/mozilla-central/rev/fb30c6b2e138

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 8557073 [details] [diff] [review]
part 1, fix

Approval Request Comment
[Feature/regressing bug #]: bug 739396
[User impact if declined]: copy-paste from a Google search results page in Firefox into a Gecko-hosted HTML editor (e.g. online mail composition, or Thunderbird HTML composer) fails.  I imagine it's a somewhat common operation so uplifting to Aurora seems motivated to get the fix out sooner.
[Describe test coverage new/current, TreeHerder]: on m-c since 2015-02-03
[Risks and why]: low risk, fairly simple code changes
[String/UUID change made/needed]: none

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8557073 [details] [diff] [review]
part 1, fix

Seems like we should fix this case. Glad to have a pretty low risk patch to do so. Aurora+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89a690cd0c5f
https://hg.mozilla.org/releases/mozilla-aurora/rev/04d93fda6f3f

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out from Aurora for test_copypaste.xhtml failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/c1e63dabd5f5

https://treeherder.mozilla.org/logviewer.html#?job_id=578431&repo=mozilla-aurora

Comment 25

[Mats Palmgren (inactive)]

Sorry about that, I guess this bug depends on 1127835 for that test.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5223d6e4ab50
https://hg.mozilla.org/releases/mozilla-aurora/rev/7e3f344f8356

Comment 27

[Mats Palmgren (inactive)]

Thanks Ryan!