211438 - nsContentAreaDragDrop misidentifies plain-text as a URL if the selection includes an anchor

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Rich Walsh]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.5a) Gecko/20030702
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.5a) Gecko/20030702

Despite recent changes in nsContentAreaDragDrop, two bugs remain which make
plain-text drag & drop unusable in many situations.  The problem is apparent
when dragging to a plain-text window in Mozilla or to an external application
which accepts Moz's d&d text (see Bugzilla #174842).  The attached patch fixes this.

Bugs:
1. If a block of selected text happens to contain an anchor, the entire block is
identified as a URL and the selection is used as its title.  This occurs because
the current code looks to see if a link lies within the selection.  This is the
reverse of what it should do:  look to see if the selection lies within a link.

2. Even when the above is fixed, the block will still be identified as a URL if
the mouse happens to be pointing at an anchor within the selection when a drag
begins.  This occurs because the current code sends selected content down the
same logic path as unselected content.  The result is that selected content is
reevaluated as though the selection didn't exist.

Patch:
The patch fixes #1 by ensuring that the selection's starting & ending nodes
(after adjustment) both refer to the same link.  If they don't, the selection is
handled as plain-text.  #2 is fixed by a minor change which keeps selected
content from being reexamined by the no-selection logic.

All affected methods are members of the new nsTransferableFactory:
 1. GetDraggableSelectionData() - modified 
 1. FindFirstAnchor() - removed
 1. GetSelectedLink() - added
 2. Produce() - modified


Reproducible: Always

Steps to Reproduce:
Open a plain-text mail composition window.  In the browser, select a paragraph
which includes embedded anchors.  Point at the plain-text, then drag to the
composition window:  the first embedded anchor's URL will be inserted.  If the
selection contains another anchor, point at it then drag:  its URL will be
inserted when dropped.



Expected Results:  
When selected content contains a mix of text and anchor nodes, the selection
should be rendered as plain-text, regardless of the pointer's location when the
drag begins.  If the selection lies wholly within an anchor, it should be
rendered as a URL whose title is the selected text.

Comment 1

[Rich Walsh]

Attachment: diff showing proposed changes to nsContentAreaDragDrop.cpp

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126919 [details] [diff] [review]
diff showing proposed changes to nsContentAreaDragDrop.cpp

Please ping me when this patch has review and super-review (see
http://mozilla.org/hacking/reviewers.html) and I'll check it in...

Comment 3

[Mike Kaply [:mkaply]]

Comment on attachment 126919 [details] [diff] [review]
diff showing proposed changes to nsContentAreaDragDrop.cpp

bryner, could you review this one?

Comment 4

[Brian Ryner (not reading)]

Comment on attachment 126919 [details] [diff] [review]
diff showing proposed changes to nsContentAreaDragDrop.cpp

The logic here looks good, I just have a few minor comments.

>         else
>+        // an image is selected
>+        if (selectedImageOrLinkNode)
>+          image = do_QueryInterface(selectedImageOrLinkNode);
>+        else
>         {

We usually put "else if" on a single line in mozilla.

>@@ -1406,7 +1344,7 @@
> 
>       nsCOMPtr<nsIDOMNode> selectionEnd;
>       inSelection->GetFocusNode(getter_AddRefs(selectionEnd));
>-    
>+

It's usually best to avoid extra white space changes, so that cvsblame will
show who committed the whole block of code.

>@@ -1460,6 +1391,121 @@
>   return NS_OK;
> }
> 
>+// static
>+nsresult nsTransferableFactory::GetSelectedLink(nsISelection* inSelection,
>+                                                nsIDOMNode **outLinkNode)
>+{
>+  NS_ENSURE_ARG(inSelection);
>+  NS_ENSURE_ARG_POINTER(outLinkNode);
>+  

Since this method is private, you can optimize some.  outLinkNode has already
been null-checked.

>+    if (!range)
>+      return NS_ERROR_NO_INTERFACE;
>+

That error code is normally used only by QueryInterface.  But you're not
checking the result of this function anyway... if there's no useful reason to
return a value, don't return a value.

Also, some things I'd like to make sure are tested:
- HTML drag and drop of anchors/images (I don't anticipate it breaking, but
can't hurt to check)
- All of the combinations of selecting text in and around anchors:
  selection start: outside anchor, inside anchor
  selection end: outside anchor, inside anchor
  direction: left to right, right to left

r=bryner with those points addressed.

Comment 5

[Rich Walsh]

Attachment: revised diff that addresses reviewer's comments

Comment 6

[Rich Walsh]

I've submitted a revised patch that addresses the reviewer's comments.

>We usually put "else if" on a single line in mozilla.
>[snip]
>It's usually best to avoid extra white space changes, so that cvsblame will
>show who committed the whole block of code.

Revised as suggested.

>>+nsresult nsTransferableFactory::GetSelectedLink(nsISelection* inSelection,
>>+                                                nsIDOMNode **outLinkNode)
>>+{
>>+  NS_ENSURE_ARG(inSelection);
>>+  NS_ENSURE_ARG_POINTER(outLinkNode);
>
>Since this method is private, you can optimize some.  outLinkNode has already
>been null-checked.

Actually, both arguments had already been checked, so I removed both tests.

>>+    if (!range)
>>+      return NS_ERROR_NO_INTERFACE;
>
>That error code is normally used only by QueryInterface.  But you're not
>checking the result of this function anyway... if there's no useful reason to
>return a value, don't return a value.

I revised this method and eliminated the return values.  It is now:

void nsTransferableFactory::GetSelectedLink(nsISelection* inSelection,
                                            nsIDOMNode **outLinkNode)

>Also, some things I'd like to make sure are tested:
>- HTML drag and drop of anchors/images (I don't anticipate it breaking, but
>can't hurt to check)
>- All of the combinations of selecting text in and around anchors:
>  selection start: outside anchor, inside anchor
>  selection end: outside anchor, inside anchor
>  direction: left to right, right to left

All of these items (and more) were tested extensively prior to submitting
the original patch.  I retested using the revised patch and everything
continues to work as intended:
- if the selection extends beyond the anchor's text (on either or both ends),
  the selection is handled as text, regardless of whether the pointer is
  over the anchor or over plain-text when the drag begins;
- if the selection lies wholly within the anchor's text, dragging from
  any part of the anchor produces a link whose title is the selected text;
- for images embedded in anchors:  if the image is not selected, the href
  is dragged;  if it is selected, a link to the image's source is dragged;
  if the image is included in a larger selection, it is ignored.

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 138832 [details] [diff] [review]
revised diff that addresses reviewer's comments

carrying over r from bryner

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm not likely to get to this review anytime in the near future (we're talking
probably weeks here).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138832 [details] [diff] [review]
revised diff that addresses reviewer's comments

>Index: nsContentAreaDragDrop.cpp

>+void nsTransferableFactory::GetSelectedLink(nsISelection* inSelection,
>+                                                nsIDOMNode **outLinkNode)

>+    nsCOMPtr<nsIDOMNode> link;
>+    FindParentLinkNode(selectionStart, getter_AddRefs(link));
>+    if (link)
>+      CallQueryInterface(link, outLinkNode);

No need to QI here -- it's the same type.  Just do:

NS_IF_ADDREF(*outLinkNode = link);

>+  // trim trailing node if the selection ends before its text begins
>+
>+  if (!endOffset)

I think that test would be clearer as (endOffset == 0)

>+    if (link == link2)
>+      CallQueryInterface(link, outLinkNode);

Again, no need to QI.

With those nits, sr=bzbarsky

Comment 10

[Boris Zbarsky [:bzbarsky]]

mkaply, I assume you can land this?  I may not get a chance to any time in the
next few days..

Comment 11

[Rich Walsh]

Attachment: revised diff that addresses superreviewer's comments

Comment 12

[Rich Walsh]

This fix was superreviewed 2 1/2 months ago.  Why has it never been landed?

Comment 13

[Mike Kaply [:mkaply]]

It got lost and I Found it yesterday. I'm landing it today on the trunk.

Comment 14

[Mike Kaply [:mkaply]]

Fix checked in.

Comment 15

[Henrik Gemal]

file:
mozilla/parser/htmlparser/src/win32.order

got checked in the following branch:
:pserver:[e-mail]:/cvsroot

according to bonsai.mozilla.org