Last Comment Bug 489202 - selection moves to top when html containing meta, link, or style elements is pasted or inserted
: selection moves to top when html containing meta, link, or style elements is ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla8
Assigned To: Jonathan Kamens
:
Mentors:
Depends on: 787434
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-20 11:53 PDT by Steve Kobes
Modified: 2012-08-31 18:02 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch which I believe fixes the issue (519 bytes, patch)
2011-07-23 23:49 PDT, Jonathan Kamens
no flags Details | Diff | Review
incomplete test case for this bug (3.21 KB, application/octet-stream)
2011-07-26 06:01 PDT, Jonathan Kamens
no flags Details
fix, unit test file, unit test Makefile.in patch (4.17 KB, patch)
2011-07-26 09:38 PDT, Jonathan Kamens
ehsan: review+
Details | Diff | Review

Description Steve Kobes 2009-04-20 11:53:04 PDT
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 2010-02-01 22:50:08 PST
this might be related to bug #493189
Comment 2 Jonathan Kamens 2011-07-23 20:56:06 PDT
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 2011-07-23 23:49:10 PDT
Created attachment 547986 [details] [diff] [review]
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 :Ehsan Akhgari (out sick) 2011-07-25 14:39:21 PDT
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 2011-07-26 06:00:45 PDT
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 2011-07-26 06:01:22 PDT
Created attachment 548430 [details]
incomplete test case for this bug
Comment 7 Jonathan Kamens 2011-07-26 09:38:25 PDT
Created attachment 548496 [details] [diff] [review]
fix, unit test file, unit test Makefile.in patch

Thanks to some lovely help from neil@parkwaycc.co.uk 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 :Ehsan Akhgari (out sick) 2011-07-26 10:55:05 PDT
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 :Ehsan Akhgari (out sick) 2011-07-26 11:00:53 PDT
Pushed to mozilla-inbound.
Comment 10 Marco Bonardo [::mak] 2011-07-27 03:27:56 PDT
http://hg.mozilla.org/mozilla-central/rev/624d57aab356

Note You need to log in before you can comment on or make changes to this bug.