86583 - can't drag XML links, HTML images or HTML image map links on first try

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

The links at http://www.mozilla.org/newlayout/xml/debugdemos/books/books.xml
should be draggable.  In 061804 2001, if I try to drag from the middle of a
link, I end up selecting from the middle of the link.

Comment 1

[Tom Mraz]

Isn't draggable on Linux too.

Comment 2

[Blake Ross]

Rather than looking for xlink:href in the dnd code, is there an easy way of 
checking for a |href| attribute regardless of the namespace?

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Heh, I was just about to file this bug on myself ;)

Technically all that is required for a simple XLink is xlink:type="simple"
attribute (where xlink is the namespace prefix and can be any string, it just
has to be tied to the correct namespace of course). But if xlink:href is empty,
the link does not go anywhere (well, it points to itself).

After that digression, I think we can ignore the xlink:type attribute for now,
but we still need to check that we have non-empty xlink:href attribute. Use the
getAttributeNS method to get the value (like in context menus).

One thing you also must pay attention to is XML Base. That is in DOM 3 Node,
baseURI property. So when you get the xlink:href value for the element you will
also need to get the baseURI property from the element (node) and make an
absolute URI from those two. We do that in the context menu, so take a look at that.

It would be nice if the Core DOM contained a single propetry that would have the
completely resolved URI for all URI attributes, but unfortunately there is no
such thing. Actually, if you think about this a bit more it is almost impossible
to put such properties in the _Core_...

A final notice: also make sure that you do not break XHTML drag&drop. You'd
better test both default namespace and namespace with prefix.

Comment 4

[Blake Ross]

Okay, great, thanks for the breakdown.

Going to push this out a bit for now.

Comment 5

[Blake Ross]

usability/polish, 0.9.4.

Comment 6

[Blake Ross]

--> heikki

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix clicks on XML links (partial)

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The first attachment enables Shift/Ctrl+click functionality for simple XLinks
(and in theory should enable them for HTML <link> elements). For some reason the
link is still loaded in the original window as well, preventDefault() on the
event does not seem to work.

However, that stuff I just found while trying to figure out how to fix this D&D
bug. I have not found out what code I need to modify - the obvious D&D JS code
in xpfe does not seem to be called for XML documents: contentAreaDD.js,
nsDragAndDrop.js. Any pointers appreciated.

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Blake, can you review the first patch please?

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

timeless, can you review the patch to get Ctrl+Click working for XML documents?

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: C++ part of Ctrl+click fix, also keyboard activation of link

Oops, sorry, I forgot to attach the C++ part. Now could someone please review
the two patches please?

Comment 12

[harishd]

Comment on attachment 60356 [details] [diff] [review]
C++ part of Ctrl+click fix, also keyboard activation of link

>+          nsCOMPtr<nsIContent> mouseContent;
Do you need this?

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Nope, seems like an unused variable. Since I copied this from
nsGenericHTMLElement I'll remove that from there as well. Thanks.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 48956 [details] [diff] [review]
Fix clicks on XML links (partial)

>Index: communicator/resources/content/contentAreaClick.js
...
>+          href = linkNode.getAttributeNS("http://www.w3.org/1999/xlink","href");

Space after comma (',') like in the rest of this file.

...

>+        href = makeURLAbsolute(target.baseURI,href);

Space after comma...

sr=jst

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 60356 [details] [diff] [review]
C++ part of Ctrl+click fix, also keyboard activation of link

>@@ -465,6 +465,11 @@
>     case NS_MOUSE_LEFT_CLICK:
>       {
>         if (nsEventStatus_eConsumeNoDefault != *aEventStatus) {
>+          nsInputEvent* inputEvent = NS_STATIC_CAST(nsInputEvent*, aEvent);
>+          if (inputEvent->isControl || inputEvent->isMeta ||
>+              inputEvent->isAlt ||inputEvent->isShift) {

Add a space after '||'

sr=jst

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Pink, what code should I be looking at to find out why drag & drop does not work
for XML links?

Comment 17

[Mike Pinkerton (not reading bugmail)]

that would be the JS in navigatorDD.js (at least, that's where it used to be).
blake should be able to point out if it has moved.

Comment 18

[Blake Ross]

It's been moved to contentAreaDD.js.

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The first two patches checked in with whitespace corrections jst wanted.

I am now concentrating on contentAreaDD.js. After some testing it seems drag &
drop does not work with AREA (imagemap) elements as it should. I have written
the code that should in theory do the work with that as well as XLinks, but the
problem seems to be that onDragStart is not getting called when a) you try to
D&D an image map link the first time and b) you try to D&D an XLink when there
is no selection. 

Also I believe the title text the code is extracting for IMG and AREA is
incorrect; the current code tries to get the contents of alt attribute, but I
think title would be more appropriate.

Also there does not seem to be code to do D&D for LINK elements.

Comment 20

[Blake Ross]

Yeah, I've noticed in the past that problem with image maps. It's a lower level 
problem.

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: WIP for D&D fix

This patch should fix things at least a little bit, but still onDragStart
wontbe called for imagemaps when you first try them (or click anywhere on the
page before drag so you clear selection) and also get called for XLinks when
there is no selection.

Anybody have any idea why onDragStart won't be called without selection
(focus?)

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: More WIP

Phew, finally found the file where I believe this happens:
content/base/src/nsContentAreaDragDrop.cpp (how could I have missed it, I
say!). That whole file needs a namespace sanity check.

Some of the stuff here might not make sense, I wrote it before looking at the
last 3 or 3 functions... Time to sleep.

Comment 23

[scottputterman]

nsbeta1- per ADT triage.

Comment 24

[Heikki Toivonen (remove -bugzilla when emailing directly)]

After some more investigation I have found so many problems that I think this
should be fixed for 1.0. Off the top of my head (I am probably forgetting some):

* Can't drag (X)HTML images
* Can't drag (X)HTML image map links on first try (link needs focus first)
* Can't drag XLinks
* Namespaces are not taken into acocunt at all in the drag handling code
* The implementation is needlessly slow (unnecessary calls and loops)
These happens on top100 sites, like http://home.netscape.com (try dragging the
Autos image map link for example).

I have a fix that seems to be working. I can probably make it faster if needed.
This needs more testing, and of course reviews.

I have tested that:

* Dragging image works (absolute URL)
* Dragging images surrounded by link works (gets absolute URL of link)
* Dragging image map link works
* Dragging XLink works
* Some testing on selecting text with link and then dragging; seems to work

Attaching patch.

Comment 25

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Possible fix

Comment 26

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Let's try the real patch this time

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Forgot absolute img URL; this should be the one

Comment 28

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Tested with page load test and there was no change in numbers.

Also tried some fast drag & drop operations and I always seemed to get what I
expected. For example, several months ago we used to have an issue where you
couldn't move the mouse very fast over a link and get a drag started on that. No
matter how fast I moved the mouse I was still able to grab the link.

Reviews?

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 75751 [details] [diff] [review]
Forgot absolute img URL; this should be the one

- In nsContentAreaDragDrop.cpp:

+#include "nsIDOMHTMLAnchorElement.h"
+#include "nsIDOMHTMLBodyElement.h"
+#include "nsIDOMHTMLHtmlElement.h"
 #include "nsITransferable.h"
 #include "nsIDragService.h"
 #include "nsIDragSession.h"
[...]
+#include "nsIDOMHTMLElement.h"

You don't need to #inclide nsIDOMHTMLElement.h when you're already #including
other nsIDOMHTML*Element headers.

+    if ( nodeType == nsIDOMNode::ELEMENT_NODE ) {
+      // a?
+      nsCOMPtr<nsIDOMHTMLAnchorElement> a(do_QueryInterface(curr));
+      if (a) {
+	 *outAnchor = curr;
+	 NS_ADDREF(*outAnchor);
+	 return;
+      } else {

Sigh! Else-after-return! Loose the else.

+	 // area?
+	 nsCOMPtr<nsIDOMHTMLAreaElement> area(do_QueryInterface(curr));
+	 if (area) {
+	   *outAnchor = curr;
+	   NS_ADDREF(*outAnchor);
+	   return;
+	 } else {

Same thing, loose the else.

-nsContentAreaDragDrop::FindParentNode(nsIDOMNode* inNode, const PRUnichar*
inLocalName, nsIDOMNode** outParent)
+nsContentAreaDragDrop::FindParentLinkNode(nsIDOMNode* inNode, 
+				       nsIDOMNode** outParent)

Fix the indentation of the second line arguments here to line up with the onese
on the first line.

r/sr=jst if you fix those nits.

Comment 30

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 75751 [details] [diff] [review]
Forgot absolute img URL; this should be the one

r=pink

Comment 31

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fixed jst's concerns

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 76492 [details] [diff] [review]
Fixed jst's concerns

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 33

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in.

Comment 34

[Terri Preston]

Verified fixed Win XP build 2002041512, Mac OS X build 2002041605 and linux
build 2002041615