858918 - Pasting html in a contenteditable span duplicates the blank span instead of pasting contents

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[kapouer]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.43 Safari/537.31

Steps to reproduce:

http://jsfiddle.net/7kpND/8/
(no script involved, only htm)

- Copy a small text with markup from a web page
- Paste it in a contenteditable SPAN





Actual results:

SPAN is duplicated, nothing in it


Expected results:

Chromium pastes all markup inside the SPAN.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

WFM using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130406 Firefox/23.0 ID:20130406030922 CSet: 768af8d8fad4 and

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 ID:20130326150557 CSet: c90d44bfa96c

Is this Linux only?
Can you give an Example Text the Issue happens with for you?

Comment 2

[kapouer]

I reproduce this on:
* debian wheezy/sid firefox 20
* windows xp firefox 12
* windows 7 firefox 9, 12, 20

By selecting "Frameworks and extensions" on the upper left of the jsfiddle,
and pasting it (using ctrl-v or right-click) into the span.

Comment 3

[Loic]

Old issue, it's already present in Firefox 4 nightlies.

Comment 4

[Ankit Thakkar]

When this issue will resolve ?

Comment 5

[Ritesh Narain]

Hi,

Is there any plan on resolving this issue? We are actively engaging our users to use Firefox but this issue is stopping them from using it properly and hence we would be forced to move back to IE11/Chrome options - something we are looking to avoid.

Comment 6

[Rushabh Doshi]

Hello,

It is really pain and difficult to work especially when pasting doesn't work properly from word document. We would be highly appreciated if you can resolve the issue in near future.

Comment 7

[sid]

Do we a team working on this issue? Would appreciate if we get an ETA of when this issue would be resolved. This has been a bit of a pain when we are getting more of our users using Mozilla as the preferred browser.

Cheers,
Sid

Comment 8

[Loic]

CC'ed moz dev.

Comment 9

[(no longer active)]

I don't think anybody is working on this at this moment, sorry.

Sid, can you please clarify which product is affected by this?  Thanks!

Comment 10

[Maulin]

We are also facing same issue. In our web application user can copy Rich Text from any source (like word,excel) and paste in Rich Text box. Its working fine in other browsers(IE, Chrome) but broken in FF due to this bug. Is there any ETA for this?

Comment 11

[Ritesh Narain]

www.nicedit.com can be used to replicate the problem. 

Ritesh

Comment 12

[Ashish]

We also are facing the same issue. This works fine in other browsers like IE \ Chrome. We are forced to ask our clients not to use  Firefox because of this limitation. Can you please have this fixed ASAP ...... Ashish

Comment 13

[(no longer active)]

Aryeh, can you please look into this?  Thanks!

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I can't look into fixing it right now.  I could try looking into it the week of October 19 instead of doing more cleanup work, but as usual, I probably won't be available to fix any regressions that may crop up, so be warned.

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I looked a bit, and found an issue, and fixed it, only to find that it failed for a different reason.  Then I fixed a second thing, and a third thing, and got a (legitimate-looking) test failure, which I'm looking at now.  (Yes, so I guess it turns out I can start looking into fixing it now.)

Basically, our code does not expect to have to deal with inline editing hosts.  I could try to fix this case, but it has a high risk of causing regressions.  Should I go ahead anyway?

Comment 16

[(no longer active)]

Yes, please.  We can evaluate the risk based on the patches...  Thanks!

Comment 17

[Ritesh Narain]

Thanks, the fact that there is a visibility of the fix means that we can get our clients to keep using FF until this issue is resolved. Much appreciated.

Ritesh

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 - Don't try to split non-editable nodes

I have patches that fix the problem, but with regressions in test_runtest.html that I didn't have time to track down.  I won't be able to work on this bug further until next break, in October, so I'll post the patches I have in case someone else wants to try finishing them up.

This patch stops us from splitting editing hosts or non-editable nodes.  It causes no regressions that I saw (I didn't run the full suite), and has low regression potential, because it should only change behavior for inline editing hosts.

After this patch, the editing host in the test case is no longer split, but the text is inserted outside the editing host.

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Provisional patch part 2

This patch stops the test case from aborting over here:

http://hg.mozilla.org/mozilla-central/file/85135c5c6ba8/editor/libeditor/nsHTMLDataTransfer.cpp#l636

I didn't actually test if this is necessary for the test case to be fixed.  A real version of this patch would probably change GetFirstEditableLeaf too.  This function has two callers, and the one I gave above already handles the case where it returns null, but the one at nsHTMLEditRules.cpp line 2226 doesn't seem to, so that should probably be audited to make sure it won't blow up.

I don't know of any test regressions this causes.

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Provisional patch part 3

This one actually fixes the problem -- the text is inserted as desired.  However, it also causes a test_runtest.html regression, where an extra <br> is now incorrectly inserted for some reason.  I didn't look into why this is.

Tip if anyone wants to take this: you can run test_runtest.html in a reasonable amount of time if you edit dom/imptests/editing/conformancetest/data.js to remove all the tests you don't want to run.  The test that regressed is one of the inserthtml ones.  (Probably this is obvious, but I only just thought of it.)

Comment 21

[Ritesh Narain]

Hi,

Any idea when this issue is expected to be resolved?

Ritesh

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

As noted in comment 14, I will be able to try looking at it again in the week of October 19 (i.e., in four weeks).  It doesn't *look* too hard, so *hopefully* I will then be able to land a fix within a day or two, and if so, it will make it into Firefox 37, which will be immediately available to anyone who wants to use a testing version of Firefox, and is slated to be released in stable form on March 31.

Needless to say, it is always possible that unforeseen difficulties will arise and it will wind up happening much later or never, so I can't make any promises, but I'd say there's a greater than 50% chance that this will be fixed in Firefox 37.

Comment 23

[Ritesh Narain]

Thanks Aryeh. Much appreciated

Ritesh

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8480008 [details] [diff] [review]
Patch part 1 - Don't try to split non-editable nodes

I don't *think* this should affect anything other than inline editing hosts, and hopefully should only do good things to them.

Comment 25

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2 -- Don't insert block content outside of inline editing hosts

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6a837f0c3e84

This fixes the problem.  It causes the regressions that you will note under dom/imptests/, in the case where you try to paste a comment inside a block that contains no visible content other than a <br>.  In these cases, we now insert an extra <br>.  I spent a while trying to track down and fix these regressions, but eventually gave up, and think we may want to take them if those are the only regressions.  What I found is:

nsHTMLEditor::InsertNodeAtPoint calls CanContain to check if the node can be put at the desired location.  CanContain always returns false for comments, which is just a bug, and InsertNodeAtPoint returns an error in this case.  InsertNodeAtPoint is being called here from nsHTMLEditor::DoInsertHTMLWithContext, line 593.  After the failure, it tries to call InsertNodeAtPoint again on the parent of the comment for some reason that I don't understand, on line 608.  The parent is a DocumentFragment, which inserts successfully, even though its sole child did not.  Apparently CanContain doesn't treat DocumentFragments correctly either (it should recurse to their children).

For whatever reason, if InsertNodeAtPoint behaves a bit differently and succeeds in inserting the comment, as it should, later code decides that there's a missing <br> and inserts one.  This is wrong, because there's already a <br> in the block.  Maybe it gets confused by comments.  I traced this back to nsHTMLEditRules::AdjustSelection, and from there to nsWSRunObject, and gave up.  I don't know why AdjustSelection doesn't add a <br> as things stand.  It could be there's something simple I'm missing.

Anyway, this bug looks very specific, so probably it won't break any sites in itself, and I'm not bothered by the regression.  I am entirely unsure that this won't cause other regressions, however, given how buggy all this code is.

Comment 26

[(no longer active)]

Comment on attachment 8480008 [details] [diff] [review]
Patch part 1 - Don't try to split non-editable nodes

Review of attachment 8480008 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditorStyle.cpp
@@ +650,5 @@
> +    if (// node is the correct inline prop
> +        (aProperty && node->Tag() == aProperty) ||
> +        // node is href - test if really <a href=...
> +        (aProperty == nsGkAtoms::href && nsHTMLEditUtils::IsLink(node)) ||
> +         // or node is any prop, and we asked to split them all

Nit: indentation.

Comment 27

[(no longer active)]

Comment on attachment 8507847 [details] [diff] [review]
Patch part 2 -- Don't insert block content outside of inline editing hosts

Review of attachment 8507847 [details] [diff] [review]:
-----------------------------------------------------------------

I agree, it's probably worth taking this regression...

Comment 28

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e719c2d7a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/df99359c999f

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8e719c2d7a7
https://hg.mozilla.org/mozilla-central/rev/df99359c999f

Comment 30

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Ritesh -- the fix has been accepted into Firefox 36, and you should be able to get it in the next nightly build:

https://nightly.mozilla.org/

Please confirm that your site now works, and if not, file a new bug.  (It might only be in the next nightly, not the current one, so if it doesn't work with the current download, try re-downloading in a day.)  From here, it will move to Aurora in the week of November 24, Beta in the week of January 12, and final release should be sometime after February 24.