489202 - selection moves to top when html containing meta, link, or style elements is pasted or inserted

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Steve Kobes]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.8) Gecko/2009032712 Ubuntu/8.04 (hardy) Firefox/3.0.8
Build Identifier: 

Certain HTML elements, notably <meta>, <link>, and <style>, cause the cursor position to be lost when pasted into an editable document.  This is a common use case since content copied out of Microsoft Word will usually include all three of these elements.

This also occurs when the HTML is inserted with document.execCommand('inserthtml', false, htmlContent).

Example:

1. Go to midas demo: http://www.mozilla.org/editor/midasdemo/
2. Copy some formatted text from an MS Word document and paste it into midas demo.

Actual result:
Cursor jumps to the top of the document.

Expected result:
Cursor is placed immediately following the pasted content (as it is when pasting plain text).

Reproducible: Always

Comment 1

[Seth Spitzer]

this might be related to bug #493189

Comment 2

[Jonathan Kamens]

I see this problem with nightly Thunderbird build (currently Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110716 Thunderbird/8.0a1). Simple Linux test case:

* Launch Google Chrome
* Browse to a Web site
* Open a message compose window in Thunderbird
* Type a line of text in the body of the message and hit enter so you're no longer at the top of the message
* Copy the URL from the URL field in Chrome
* Paste into the compose window with ctrl-v
* Note how the cursor jumps to the top of the message

Comment 3

[Jonathan Kamens]

Attachment: patch which I believe fixes the issue

OK, so I've done some research about this, and I think I've fixed the Thunderbird issue I reported in Comment 2, but I'm not longer convinced that the Thunderbird issue is necessarily the same as the issue reported in the bug Description. Or perhaps the bug Description is wrong? Here's what I've discovered...

When nsHTMLEditor::DoInsertHTMLWithContext in mozilla/editor/libeditor/html/nsHTMLDataTransfer.cpp is pasting HTML from another document (e.g., you copy from chrome and paste into Thunderbird, rather than copying from one part of the message you're composing and paste into another part), it constructs a document fragment to paste, and this document fragment (obviously) isn't part of the body of the document being pasted into.

In this situation, the block of code that actually does the insert is the block of code starting at line 647. This block of code sets lastInsertNode to the document fragment node being inserted ("lastInsertNode = parent").

Later, starting at line 667, the code attempts to set the insertion point to after lastInsertNode. This fails, because lastInsertNode isn't actually in the body of the message being edited -- it's a document fragment node, as noted above.

As a result, nsHTMLEditRules::ConfirmSelectionInBody in nsHTMLEditRules.cpp ends up moving the cursor to the beginning of the body, because when selection->Collapse is called at line 741 of nsHTMLDataTransfer.cpp, it's passing in the document fragment node, which isn't in the body, so the failsafe "selection not in body" code in nsHTMLEditRules.cpp kicks in.

I confirmed this by putting some debug printf's into ConfirmSelectionInBody and watching it get called with a selection whose tag name is #document-fragment.

Attached is my proposed fix to this problem. I can't claim to grok all of this code well enough to know for 100% certain that it is correct, but it seems to work and I don't think it is likely to cause any harm.

I worry that there is no one who understands the editor code well enough to be able to confirm my diagnosis and review my fix. :-(

Comment 4

[(no longer active)]

Jonathan, thanks a lot for the detailed analysis and also for the patch.  I think your analysis makes a lot of sense, and I think that your patch is correct, but I need two things to be able to r+ this first.

1. Can you tell me how one can reproduce this using Firefox (on www.mozilla.org/editor/midasdemo/ for example)?  I wanted to test this but I was not successful in reproducing the problem in Firefox at all.
2. Can you please add a unit test for this?  See https://developer.mozilla.org/en/Mochitest for a detailed guide on how unit tests work.  You can find a lot of unit test samples in http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/.  Note that coming up with a way to reproduce this in Firefox is also going to help with creating a unit test for the bug.

Thanks a lot!  :-)

Comment 5

[Jonathan Kamens]

Hi Ehsan,
I don't know why I let you guys talk me into this stuff. :-)
1 I've figured out. 2 I'm a bit stumped on...
It's not happening in Firefox because it only happens when appType is set to APP_TYPE_EDITOR, which Thunderbird does when creating a compose window but Firefox doesn't.
I'm about 95% of the way to having a successful test case. The problem is that I can't for the life of me figure out how to set the appType property on the correct object in my test so that the code in nsHTMLDataTransfer.cpp will notice the setting and behave like Thunderbird. Here's the relevant code in nsHTMLDataTransfer.cpp: http://pastebin.com/Zwt3hSyB
I will attach what I've got so far for a test. Search for XXX and you will see my problem... I'm setting an appType property on an object, but it's apparently not the right one since the code in nsHTMLDAtaTransfer.cpp doesn't see the setting. And I can't figure out how to replicate the logic in nsHTMLDataTransfer.cpp in JavaScript to get to the right property to set it.
I spent like six hours last night working on this, ending when I finally had to give up at 4:30am, so any help you can provide would be very much appreciated!

Comment 6

[Jonathan Kamens]

Attachment: incomplete test case for this bug

Comment 7

[Jonathan Kamens]

Attachment: fix, unit test file, unit test Makefile.in patch

Thanks to some lovely help from [e-mail] on the #developers IRC channel, I've solved the unit test problem. Attached is a new patch which includes the fix, a unit test, and a patch to the unit test Makefile.in.

For the record, Fabien Cazenave also ovvered to help, but Neil beat him to it. Thanks for all the help, folks!

Comment 8

[(no longer active)]

Comment on attachment 548496 [details] [diff] [review]
fix, unit test file, unit test Makefile.in patch

This looks really really good, Jonathan!  Sorry that I didn't catch you on irc (I don't read the channels usually unless someone pings me) so in the future feel free to do so!

Comment 9

[(no longer active)]

Pushed to mozilla-inbound.

Comment 10

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/624d57aab356