183582 - HTML Drop does not provide contextual info

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[kinmoz]

If I select some flat text in the browser, and then click and drag it to a
composer window and drop it ... nothing ever appears in composer.

It looks like things fail because the subtree iterator used to create the list
of nodes to paste is being made empty in |iter->Init(range)| because |cStartP|
is a text node.

Note that this is only a bug on Win32 platforms, so I'm not sure if this is
related to the CF_HTML changes or not.

Here's the stack to the |MakeEmpty()| call:

nsContentSubtreeIterator::Init(nsContentSubtreeIterator * const 0x03ed3720,
nsIDOMRange * 0x04199840) line 1367
nsDOMSubtreeIterator::Init(nsIDOMRange * 0x04199840) line 192
nsHTMLEditor::CreateListOfNodesToPaste(nsIDOMNode * 0x03ed3de4,
nsCOMArray<nsIDOMNode> & {...}, int 0, int 0) line 2266 + 17 bytes
nsHTMLEditor::InsertHTMLWithCharsetAndContext(const nsAString & {...}, const
nsAString & {...}, const nsAString & {...}, const nsAString & {...}) line 321 +
32 bytes
nsHTMLEditor::InsertHTMLWithContext(const nsAString & {...}, const nsAString &
{...}, const nsAString & {...}) line 269 + 30 bytes
nsHTMLEditor::InsertFromTransferable(nsHTMLEditor * const 0x04fddeb8,
nsITransferable * 0x04fbf628, const nsAString & {...}, const nsAString & {...})
line 930 + 29 bytes
nsHTMLEditor::InsertFromDrop(nsHTMLEditor * const 0x04fddeb8, nsIDOMEvent *
0x03fb13f0) line 1243 + 38 bytes
nsTextEditorDragListener::DragDrop(nsTextEditorDragListener * const 0x0501bbc0,
nsIDOMEvent * 0x03fb13f0) line 832 + 25 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x04fdcfb0,
nsIPresContext * 0x04f6db80, nsEvent * 0x0012dd18, nsIDOMEvent * * 0x0012d9c0,
nsIDOMEventTarget * 0x04f81e14, unsigned int 2, nsEventStatus * 0x0012db98) line
2003 + 41 bytes
nsDocument::HandleDOMEvent(nsDocument * const 0x04f81de0, nsIPresContext *
0x04f6db80, nsEvent * 0x0012dd18, nsIDOMEvent * * 0x0012d9c0, unsigned int 2,
nsEventStatus * 0x0012db98) line 3483
nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x04fc4cc0,
nsIPresContext * 0x04f6db80, nsEvent * 0x0012dd18, nsIDOMEvent * * 0x0012d9c0,
unsigned int 7, nsEventStatus * 0x0012db98) line 2060 + 44 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012dd18, nsIView * 0x04ff0260,
unsigned int 1, nsEventStatus * 0x0012db98) line 6133 + 41 bytes
PresShell::HandleEvent(PresShell * const 0x04fda7ec, nsIView * 0x04ff0260,
nsGUIEvent * 0x0012dd18, nsEventStatus * 0x0012db98, int 0, int & 1) line 6050 +
25 bytes
nsViewManager::HandleEvent(nsView * 0x04feff90, nsGUIEvent * 0x0012dd18, int 0)
line 2209
nsView::HandleEvent(nsViewManager * 0x04f6ee60, nsGUIEvent * 0x0012dd18, int 0)
line 304
nsViewManager::DispatchEvent(nsViewManager * const 0x04f6ee60, nsGUIEvent *
0x0012dd18, nsEventStatus * 0x0012dc9c) line 1943 + 23 bytes
HandleEvent(nsGUIEvent * 0x0012dd18) line 83
nsWindow::DispatchEvent(nsWindow * const 0x04ff005c, nsGUIEvent * 0x0012dd18,
nsEventStatus & nsEventStatus_eIgnore) line 1069 + 10 bytes
nsNativeDragTarget::DispatchDragDropEvent(unsigned int 1403, _POINTL {...}) line 209
nsNativeDragTarget::ProcessDrag(IDataObject * 0x03fb03d0, unsigned int 1403,
unsigned long 0, _POINTL {...}, unsigned long * 0x0012df3c) line 234
nsNativeDragTarget::Drop(nsNativeDragTarget * const 0x04ff0190, IDataObject *
0x03fb03d0, unsigned long 0, _POINTL {...}, unsigned long * 0x0012df3c) line 336
OLE32! 77b20018()
OLE32! 77b201ef()
OLE32! 77af3b07()
OLE32! 77af38c9()
nsDragService::StartInvokingDragSession(nsDragService * const 0x0162ec78,
IDataObject * 0x03fb03d0, unsigned int 7) line 162 + 25 bytes
nsDragService::InvokeDragSession(nsDragService * const 0x0162ec78, nsIDOMNode *
0x04250a14, nsISupportsArray * 0x03fb0358, nsIScriptableRegion * 0x00000000,
unsigned int 7) line 131
nsContentAreaDragDrop::DragGesture(nsContentAreaDragDrop * const 0x03f976e8,
nsIDOMEvent * 0x03fb0780) line 1009
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x03ef6420,
nsIPresContext * 0x015a9090, nsEvent * 0x0012efb0, nsIDOMEvent * * 0x0012ef64,
nsIDOMEventTarget * 0x03282ab0, unsigned int 2, nsEventStatus * 0x0012eff8) line
2006 + 41 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x03e9c608, nsIPresContext *
0x015a9090, nsEvent * 0x0012efb0, nsIDOMEvent * * 0x0012ef64, unsigned int 2,
nsEventStatus * 0x0012eff8) line 3377
nsXULElement::HandleChromeEvent(nsXULElement * const 0x03e9c614, nsIPresContext
* 0x015a9090, nsEvent * 0x0012efb0, nsIDOMEvent * * 0x0012ef64, unsigned int 2,
nsEventStatus * 0x0012eff8) line 4595 + 39 bytes
GlobalWindowImpl::HandleDOMEvent(GlobalWindowImpl * const 0x03f7f418,
nsIPresContext * 0x015a9090, nsEvent * 0x0012efb0, nsIDOMEvent * * 0x0012ef64,
unsigned int 2, nsEventStatus * 0x0012eff8) line 797
nsDocument::HandleDOMEvent(nsDocument * const 0x041e7100, nsIPresContext *
0x015a9090, nsEvent * 0x0012efb0, nsIDOMEvent * * 0x0012ef64, unsigned int 2,
nsEventStatus * 0x0012eff8) line 3491
nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x042258f8,
nsIPresContext * 0x015a9090, nsEvent * 0x0012efb0, nsIDOMEvent * * 0x0012ef64,
unsigned int 2, nsEventStatus * 0x0012eff8) line 2060 + 44 bytes
nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x04235130,
nsIPresContext * 0x015a9090, nsEvent * 0x0012efb0, nsIDOMEvent * * 0x0012ef64,
unsigned int 2, nsEventStatus * 0x0012eff8) line 2053 + 57 bytes
nsGenericDOMDataNode::HandleDOMEvent(nsGenericDOMDataNode * const 0x042509f8,
nsIPresContext * 0x015a9090, nsEvent * 0x0012efb0, nsIDOMEvent * * 0x0012ef64,
unsigned int 7, nsEventStatus * 0x0012eff8) line 834 + 37 bytes
nsEventStateManager::GenerateDragGesture(nsIPresContext * 0x015a9090, nsGUIEvent
* 0x0012f7bc) line 1451
nsEventStateManager::PreHandleEvent(nsEventStateManager * const 0x04234fe8,
nsIPresContext * 0x015a9090, nsEvent * 0x0012f7bc, nsIFrame * 0x0424cdb0,
nsEventStatus * 0x0012f5b4, nsIView * 0x0424bf80) line 403
PresShell::HandleEventInternal(nsEvent * 0x0012f7bc, nsIView * 0x0424bf80,
unsigned int 1, nsEventStatus * 0x0012f5b4) line 6121 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x04231bec, nsIView * 0x0424bf80,
nsGUIEvent * 0x0012f7bc, nsEventStatus * 0x0012f5b4, int 1, int & 1) line 6050 +
25 bytes
nsViewManager::HandleEvent(nsView * 0x0424bf80, nsGUIEvent * 0x0012f7bc, int 1)
line 2209
nsView::HandleEvent(nsViewManager * 0x03e4ac68, nsGUIEvent * 0x0012f7bc, int 1)
line 304
nsViewManager::DispatchEvent(nsViewManager * const 0x03e4ac68, nsGUIEvent *
0x0012f7bc, nsEventStatus * 0x0012f6b8) line 1943 + 23 bytes
HandleEvent(nsGUIEvent * 0x0012f7bc) line 83
nsWindow::DispatchEvent(nsWindow * const 0x0424bd7c, nsGUIEvent * 0x0012f7bc,
nsEventStatus & nsEventStatus_eIgnore) line 1069 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f7bc) line 1090
nsWindow::DispatchMouseEvent(unsigned int 300, unsigned int 1, nsPoint *
0x00000000) line 5281 + 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 300, unsigned int 1, nsPoint *
0x00000000) line 5538
nsWindow::ProcessMessage(unsigned int 512, unsigned int 1, long 6357167, long *
0x0012fc48) line 3999 + 28 bytes
nsWindow::WindowProc(HWND__ * 0x00100484, unsigned int 512, unsigned int 1, long
6357167) line 1338 + 27 bytes
USER32! 77e11d0a()
USER32! 77e11bc8()
USER32! 77e11cef()
nsAppShellService::Run(nsAppShellService * const 0x010668c0) line 472
main1(int 1, char * * 0x00284fc0, nsISupports * 0x00276f08) line 1541 + 32 bytes
main(int 1, char * * 0x00284fc0) line 1902 + 37 bytes
mainCRTStartup() line 338 + 17 bytes

Comment 1

[Joe Francis]

I'm setting up my windows box tomorrow.  In the meantime, interested parties can
check out the function nsHTMLEditor::InsertFromDrop(), in the file
nsHTMLDataTransfer.cpp.  The question is what value are we getting for
|bHavePrivateHTMLFlavor| in the line:
  rv = dragSession->IsDataFlavorSupported(kHTMLContext, &bHavePrivateHTMLFlavor);

We should get PR_FALSE.  If we are getting PR_TRUE that could explain the
problem, and would mean that I don't understand exactly what
|dragSession->IsDataFlavorSupported()| really means.

Comment 2

[kinmoz]

So I put a breakpoint in nsHTMLEditor::InsertFromTransferable() and noticed that
|GetAnyTransferData()| is returning a flavor of |kNativeHTMLMime| so we are
actually going down the CF_HTML processing path.

Comment 3

[kinmoz]

So to get this right, I think we're going to have to re-write the code in
|nsContentAreaDragDrop::BuildDragData()|, in nsContentAreaDragDrop.cpp, to use
the encoder that copy uses instead of |selection->ToStringWithFormat()| so we
can put HTMLContext info on the clipboard during a drag from the browser.

Comment 4

[kinmoz]

... or perhaps we just need to add a ToStringWithFormatAndContext() method to
nsISelectionPrivate so we can  get back the context data we need.

Comment 5

[Asa Dotzler [:asa]]

Do we know what caused this regression? Can that change be backed out or corrected?

Comment 6

[Joe Francis]

Attachment: patch to content/base

This patch changes drag code to use document encoder to do the serialization,
and to also pick up the special internal data flavors we use for context info
in html paste.

Kin tells me that drag has become broken in a different way in the meantime, so
I don't know if this patch can be verified at the moment.

Comment 7

[kinmoz]

The other unrelated bug that breaks drag that joe mentions above is bug 185033.

Comment 8

[Joe Francis]

Changing summary, redoing nominations:  Turns out drop was busted for two
different reason, both of which have already been fixed.  

So the work here is merely to make drop work "properly"; ie, like copy/paste. 
You can now drop partial tables, partial lists, parts of styled text, etc, and
get same (smart) behavior you would from copy/paste.

Comment 9

[Kathleen :Brade]

Comment on attachment 109178 [details] [diff] [review]
patch to content/base

In the block of code near line 795, you deleted this line:  -	  
privSelection->ToStringWithFormat("text/plain", 0, 0,
getter_Copies(titleString));

I don't see where you set titleString at all; is that intentional?

Around line 950 you have a comment with a typo (dont):
+  // add a special flavor, even if we dont have html context data

Move the following line down (after the return NS_ERROR_FAILURE):
+  nsAutoString contextData ( inContextString );

This comment doesn't make sense:
+  // add a special flavor if we're have html info data

move this line down below the return NS_ERROR_FAILURE line:
+    nsAutoString infoData ( inInfoString );

Comment 10

[Michael Buckland]

Discussed in edt.  Plussing.

Comment 11

[kinmoz]

Comment on attachment 109178 [details] [diff] [review]
patch to content/base

I think titleString is intended to be the plaintext version of the selection,
so some plaintext encoder code needs to be added to handle that too.

Comment 12

[Simon Fraser [no longer active]]

Bah, this will conflict with some D&D fixes I've made for chimera.

Comment 13

[Kevin McCluskey (gone)]

nsbeta1+

Comment 14

[Kai Engert (:KaiE:)]

-> me

Comment 15

[Kevin McCluskey (gone)]

moving milestone to match bug 97625

Comment 16

[Kai Engert (:KaiE:)]

Attachment: Updated patch that resolves the conflicts

Comment 17

[Kai Engert (:KaiE:)]

Attachment: Patch v5

This patch (my version v5) addresses Kathy's change requests.

Comment 18

[Kai Engert (:KaiE:)]

Note: This version of the patch no longer removes the assignment to titleString.

Comment 19

[Kathleen :Brade]

Comment on attachment 124512 [details] [diff] [review]
Patch v5

r=brade

Comment 20

[Simon Fraser [no longer active]]

Comment on attachment 124512 [details] [diff] [review]
Patch v5

I have reservations about this patch.

The argument lists to BuildDragData() and CreateTransferable() are getting
longer and longer. We need to better factor this code.

You 'unfactored' the serialization code; rather than calling
SerializeNodeOrSelection(), you added more nsIDocumentEncoder code.

Comment 21

[Kai Engert (:KaiE:)]

Attachment: Patch v8

> The argument lists to BuildDragData() and CreateTransferable() are getting
> longer and longer. We need to better factor this code.

That was a lot of work unfortunately, I spent about 4 hours on this.
I began to isolate scopes, to minimize the validity periods of variables. But
the code uses so many variables, that whatever I did, I would still have had to
use functions with many variables.

I ended up introducing a new class.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: Updated full nsContentAreaDragDrop.cpp to ease reviewing

Comment 23

[Kai Engert (:KaiE:)]

Attachment: Updated full nsContentAreaDragDrop.h to ease reviewing

Comment 24

[Joe Francis]

Comment on attachment 124619 [details] [diff] [review]
Patch v8

r=jfrancis

Comment 25

[Simon Fraser [no longer active]]

Comment on attachment 124619 [details] [diff] [review]
Patch v8

Please move all the nsTransferableFactory methods to the top or bottom of the
file, rather than scattererd among nsCADD methods.

Do you really want to use nsAutoStrings as member variables of
nsTransferableFactory?

Is there any reason for mInstanceAlreadyUsed? It's never cleared.

Please put the initializers for nsTransferableFactory one per line.

Fix those and sr=sfraser

Comment 26

[Kai Engert (:KaiE:)]

> Please move all the nsTransferableFactory methods to the top or bottom of the
> file, rather than scattererd among nsCADD methods.

done


> Do you really want to use nsAutoStrings as member variables of
> nsTransferableFactory?

change to nsString


> Is there any reason for mInstanceAlreadyUsed? It's never cleared.

Yes, I want to make sure that each constructed instance gets used only once,
because it consumes data was passed in on construction, and because I have not
added the extra code that would be required to reset the object's private
variables back to a clean state each time.


> Please put the initializers for nsTransferableFactory one per line.

Please note this will break the style of the file, that already uses 
  : mListenerInstalled(PR_FALSE), mNavigator(nsnull)
But I'm changing it.

Comment 27

[Kai Engert (:KaiE:)]

> Please move all the nsTransferableFactory methods to the top or bottom of the
> file, rather than scattererd among nsCADD methods.

done

Actually, this causes me some headache.

The patch that leaves the methods in place, applies cleanly to both 1_4 branch
and to the trunk.

If I move the methods to either the beginning or the end of the file, the patch
fails completely on the other tree and requires manual merging.


I suggest something else which I guess it ok with you.
I will attach a new patch that does everything but the reordering.
I will attach a separate patch that does the reordering only.

Comment 28

[Kai Engert (:KaiE:)]

Attachment: Patch v8 b

This patch has the requested code changes, but not the reordering change.

Note, you were only asking about mInstanceAlreadyUsed, but did not request a
change, therefore I left it as is.

Comment 29

[Kai Engert (:KaiE:)]

Comment on attachment 125853 [details] [diff] [review]
Patch v8 b

carrying forward reviews

Comment 30

[Kai Engert (:KaiE:)]

Attachment: Patch - no code change - shuffling methods only

Comment 31

[Kai Engert (:KaiE:)]

Comment on attachment 125854 [details] [diff] [review]
Patch - no code change - shuffling methods only

This reordering was requested by Simon and therefore has sr=sfraser

Comment 32

[Kai Engert (:KaiE:)]

both patches checked in to trunk, marking fixed.