Closed Bug 86583 Opened 23 years ago Closed 23 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: jruderman, Assigned: hjtoi-bugzilla)

References

()

Details

(Keywords: top100)

Attachments

(2 files, 6 obsolete files)

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.
Isn't draggable on Linux too.
OS: Windows NT → All
Hardware: PC → All
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.3
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?
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.
Okay, great, thanks for the breakdown.

Going to push this out a bit for now.
Target Milestone: mozilla0.9.3 → mozilla1.0
usability/polish, 0.9.4.
Target Milestone: mozilla1.0 → mozilla0.9.4
--> heikki
Assignee: blakeross → heikki
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.4 → ---
Priority: -- → P3
Target Milestone: --- → mozilla0.9.5
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.
Status: NEW → ASSIGNED
Keywords: helpwanted
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Blake, can you review the first patch please?
timeless, can you review the patch to get Ctrl+Click working for XML documents?
Oops, sorry, I forgot to attach the C++ part. Now could someone please review
the two patches please?
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?
Attachment #60356 - Flags: review+
Attachment #48956 - Flags: review+
Nope, seems like an unused variable. Since I copied this from
nsGenericHTMLElement I'll remove that from there as well. Thanks.
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
Attachment #48956 - Flags: superreview+
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
Attachment #60356 - Flags: superreview+
Pink, what code should I be looking at to find out why drag & drop does not work
for XML links?
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.
It's been moved to contentAreaDD.js.
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.
Yeah, I've noticed in the past that problem with image maps. It's a lower level 
problem.
Attached patch WIP for D&D fix (obsolete) — Splinter Review
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?)
Attachment #48956 - Attachment is obsolete: true
Attachment #60356 - Attachment is obsolete: true
Target Milestone: mozilla0.9.7 → mozilla0.9.9
Target Milestone: mozilla0.9.9 → mozilla1.0
Attached patch More WIP (obsolete) — Splinter Review
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.
nsbeta1- per ADT triage.
Keywords: nsbeta1+nsbeta1-
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.
Summary: can't drag XML links → can't drag XML links, HTML images or HTML image map links on first try
Whiteboard: [fixinhand]
Attached patch Possible fix (obsolete) — Splinter Review
Attachment #60735 - Attachment is obsolete: true
Attachment #74509 - Attachment is obsolete: true
Attachment #75749 - Attachment is obsolete: true
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 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.
Attachment #75751 - Flags: superreview+
Comment on attachment 75751 [details] [diff] [review]
Forgot absolute img URL; this should be the one

r=pink
Attachment #75751 - Flags: review+
Comment on attachment 76492 [details] [diff] [review]
Fixed jst's concerns

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #76492 - Flags: approval+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: [fixinhand]
Verified fixed Win XP build 2002041512, Mac OS X build 2002041605 and linux
build 2002041615
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: