Closed Bug 858918 Opened 10 years ago Closed 8 years ago

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


(Core :: DOM: Editor, defect)

19 Branch
Not set





(Reporter: kapouer, Assigned: ayg)


(Depends on 2 open bugs)



(2 files, 2 obsolete files)

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:
(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.
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?
Flags: needinfo?(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.
Flags: needinfo?(kapouer)
Old issue, it's already present in Firefox 4 nightlies.
Component: Untriaged → Editor
Product: Firefox → Core
When this issue will resolve ?

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.
Summary: paste html in contenteditable span → Pasting html in a contenteditable span duplicates the blank span instead of pasting contents

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.
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.

CC'ed moz dev.
Flags: needinfo?(ehsan)
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!
Flags: needinfo?(ehsan) → needinfo?(anzacsid)
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? can be used to replicate the problem. 

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
Aryeh, can you please look into this?  Thanks!
Flags: needinfo?(anzacsid) → needinfo?(ayg)
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.
Flags: needinfo?(ayg)
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?
Ever confirmed: true
Flags: needinfo?(ehsan)
Yes, please.  We can evaluate the risk based on the patches...  Thanks!
Flags: needinfo?(ehsan)
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.

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.
Attached patch Provisional patch part 2 (obsolete) — Splinter Review
This patch stops the test case from aborting over here:

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.
Attached patch Provisional patch part 3 (obsolete) — Splinter Review
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.)

Any idea when this issue is expected to be resolved?

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.
See Also: → 1072600
Thanks Aryeh. Much appreciated

Assignee: nobody → ayg
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.
Attachment #8480008 - Flags: review?(ehsan.akhgari)

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.
Attachment #8480014 - Attachment is obsolete: true
Attachment #8480017 - Attachment is obsolete: true
Attachment #8507847 - Flags: review?(ehsan.akhgari)
Attachment #8480008 - Attachment description: Provisional patch part 1 → Patch part 1 - Don't try to split non-editable nodes
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.
Attachment #8480008 - Flags: review?(ehsan.akhgari) → review+
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...
Attachment #8507847 - Flags: review?(ehsan.akhgari) → review+
Closed: 8 years ago
Flags: in-testsuite- → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Ritesh -- the fix has been accepted into Firefox 36, and you should be able to get it in the next nightly build:

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.
See Also: 1072600
Depends on: 1250705
Depends on: 1498271
You need to log in before you can comment on or make changes to this bug.