Per crbug.com/55019, the node is not used as the drag image if you start a drag by clicking on the anchor link text and trying to drag it. The root cause is here: http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp&l=1496 When you try to drag a link without highlighting anything, WebKit treats it as if you were dragging just the inner unnamed text node. I can modify the loop to check for DRAG_ELEMENT on text nodes, but that results in only the text being set as the drag image. Maybe in http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/EventHandler.cpp&q=draggableNode&l=602 we should iterate up the node hierarchy while the current node is draggable until we find a node who's parent is not draggable; that way, we coalesce all children of a draggable node together as a web author might expect. Thoughts?
What does Firefox do?
(In reply to comment #1) > What does Firefox do? It renders the entire anchor as the drag image as one might expect. Also, upon further inspection, RenderObject::draggableNode is actually called from EventHandler::handleDrag. It appears eventMayStartDrag is actually a Mac specific method...
Created attachment 91053 [details] Patch
I'm not a reviewer. +dglazkov, who is (and who can cc someone else if he feels not up to this)
Created attachment 91060 [details] Patch Add missing check that curr->parent() is not NULL
This looks confusing to me. What if there are actually several nested draggable elements, and the user is dragging an innermost one?
Comment on attachment 91060 [details] Patch (In reply to comment #6) > This looks confusing to me. What if there are actually several nested draggable elements, and the user is dragging an innermost one? (Note I'm assuming that draggable is implemented in these examples, while it currently isn't) Case 1: <a href="foo.html">Link</a> Anchors default to draggable="true". However, right now, a drag started by a click on "Link" is treated as if only the anonymous text node were dragged. You have to start the drag on the padding area (I think) to get a drag image to be rendered for the anchor element. Case 2: <div draggable>Draggable text with a <a href="foo.html">link</a><div>Div inside a div</div></div> Dragging the anchor will result in just the anchor getting dragged (on Firefox and Chrome). But starting a drag on any other element inside the div will result in the entire div getting dragged. I think case 1 is something that we should fix. Case 2 is weird but will probably require a more authoritative statement from the spec. The attached patch doesn't really address case 1 correctly though, so I'm marking it as R- while I debate a better fix...
Created attachment 91641 [details] Test case I've been investigating this more, and this logic seems to be a lot more complicated than necessary: Today, the code does something like this... EventHandler::handleMouseMoveEvent(...) Does a hit test, I think? This gives us a MouseEventWIthHitTestResults. Dispatch the mouse event. If the event isn't canceled (I think?), treat it as a potential drag event. EventHandler::handleMouseDraggedEvent(...) chains into... EventHandler::handleDrag(...) Hit test the document. If we hit a node, call node->renderer()->draggableNode() RenderObject::draggableNode(...) Using the node we hit, iterate upwards through the tree repeating the following steps: Element node and drag mode = element? Great, we're done. Return this node and set m_dragSrcIsDHTML to true. Text node or element node with drag mode = auto? Call EventHandler::shouldDragAutoNode(...) EventHandler::shouldDragAutoNode(...) chains into... DragController::mayStartDragAtEventLocation(...) which does *another* hit test using EventHandler::hitTestResultAtPoint (which does all sorts of stuff I don't really undestand). If this one hits an image, link, or selection, then it returns true, other wise false. This extra hit test / short-circuiting is the cause (I believe) of some of the complications. I believe the logic should be simplified into something more like this: In selection? Draggable, return the node that was hit. Otherwise, iterate up the tree. If the node has drag mode == element or is a node that is draggable by default (anchor or image) and the drag mode is not none, return that node. Also, what exactly is HitTestResult::innerNonSharedNode()? I see it used several times throughout the dragging code but I'm not really sure what it means...
Created attachment 92755 [details] Patch
I'd like to see what people think of this change before I add the new file to all the build systems. If people are generally positive, I'll go ahead and fix the build for all the other platforms.
Comment on attachment 92755 [details] Patch Attachment 92755 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8655413
Comment on attachment 92755 [details] Patch Attachment 92755 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8640399
Comment on attachment 92755 [details] Patch Attachment 92755 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8648419
Comment on attachment 92755 [details] Patch Attachment 92755 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8655435
Comment on attachment 92755 [details] Patch You need to add DragState.h to XCode & Visual Studio project files
Created attachment 92763 [details] Patch
Comment on attachment 92763 [details] Patch Attachment 92763 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8652486
Comment on attachment 92763 [details] Patch Attachment 92763 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8660475
Comment on attachment 92763 [details] Patch Attachment 92763 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8660494
Comment on attachment 92763 [details] Patch Attachment 92763 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8653540
Comment on attachment 92763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92763&action=review Can we split this into 2 patches? The first patch would just be a refactoring patch and not change any behavior. It would introduce the DragState class and probably move draggableNode into DragController. The second patch would be the behavioral change. > LayoutTests/ChangeLog:11 > + * fast/css/user-drag-none.html: Can you add a sentence or two explaining why these changed? > LayoutTests/editing/pasteboard/drag-selected-image-to-contenteditable.html:47 > + eventSender.leapForward(500); Hmm, how was this passing on Mac before? > Source/WebCore/page/DragController.cpp:580 > +// Determine which node the user wants to drag. In order of priority: > +// 1. The user is dragging a selection. > +// 2. The user is dragging an element where draggable=true. > +// 3. The user is dragging an element where draggable is the default auto value (e.g. unset) and > +// the element is either an anchor element with an href attribute or an image element. WebKit normally avoids comments that describe what the code is doing. I think the preferred style is to make each of these a function, e.g., isDraggingSelection, isDraggableElement, and isElementDraggableByDefault. > Source/WebCore/page/DragController.cpp:609 > + && (m_dragSourceAction& DragSourceActionImage)) { Nit: missing space before & > Source/WebCore/page/DragController.cpp:746 > + m_dragOffset = IntPoint((int)(dragOrigin.x() - dragLoc.x()), (int)(dragOrigin.y() - dragLoc.y())); Nit: static_cast instead of (int) > Source/WebCore/page/DragState.h:31 > +#include <wtf/FastAllocBase.h> > +#include <wtf/Noncopyable.h> > +#include <wtf/RefPtr.h> Can we use wtf/Forward.h for some of these? > Source/WebCore/page/EventHandler.cpp:2640 > + default: > + break; Can we enumerate the other cases here rather than using default:? That will give us a compile break if someone adds a new value to the enum rather than silently falling through.
Created attachment 93078 [details] Patch
Created attachment 93080 [details] Patch
(In reply to comment #21) > (From update of attachment 92763 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92763&action=review > > Can we split this into 2 patches? The first patch would just be a refactoring patch and not change any behavior. It would introduce the DragState class and probably move draggableNode into DragController. The second patch would be the behavioral change. Done. > > > LayoutTests/ChangeLog:11 > > + * fast/css/user-drag-none.html: > > Can you add a sentence or two explaining why these changed? > Done. > > LayoutTests/editing/pasteboard/drag-selected-image-to-contenteditable.html:47 > > + eventSender.leapForward(500); > > Hmm, how was this passing on Mac before? > This was required because of a behavior change which I documented in the ChangeLog. > > Source/WebCore/page/DragController.cpp:580 > > +// Determine which node the user wants to drag. In order of priority: > > +// 1. The user is dragging a selection. > > +// 2. The user is dragging an element where draggable=true. > > +// 3. The user is dragging an element where draggable is the default auto value (e.g. unset) and > > +// the element is either an anchor element with an href attribute or an image element. > > WebKit normally avoids comments that describe what the code is doing. I think the preferred style is to make each of these a function, e.g., isDraggingSelection, isDraggableElement, and isElementDraggableByDefault. > I think the code is self-explanatory enough now to avoid pulling those out into separate blocks. Let me know what you think. > > Source/WebCore/page/DragController.cpp:609 > > + && (m_dragSourceAction& DragSourceActionImage)) { > > Nit: missing space before & > Fixed. > > Source/WebCore/page/DragController.cpp:746 > > + m_dragOffset = IntPoint((int)(dragOrigin.x() - dragLoc.x()), (int)(dragOrigin.y() - dragLoc.y())); > > Nit: static_cast instead of (int) > Done. > > Source/WebCore/page/DragState.h:31 > > +#include <wtf/FastAllocBase.h> > > +#include <wtf/Noncopyable.h> > > +#include <wtf/RefPtr.h> > > Can we use wtf/Forward.h for some of these? > We would have to split the definition for the constructor into a .cpp file. I kept the original style where the constructor was inline. > > Source/WebCore/page/EventHandler.cpp:2640 > > + default: > > + break; > > Can we enumerate the other cases here rather than using default:? That will give us a compile break if someone adds a new value to the enum rather than silently falling through. Done.
Comment on attachment 93078 [details] Patch Attachment 93078 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8688224
Comment on attachment 93080 [details] Patch Attachment 93080 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8690178
Comment on attachment 93080 [details] Patch Attachment 93080 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8685302
Created attachment 93155 [details] Patch Fix Mac build (hopefully)
View in context: https://bugs.webkit.org/attachment.cgi?id=93080&action=review This looks like a move in the right direction. I just have a mix of style nits and some questions so I can understand the change better. > LayoutTests/ChangeLog:13 > + mouse down and the mouse move. The change in dragging priority matches IE and Firefox > + behavior. AFAICT, this matches OSX's behavior as well. For example, if I put an image in a Stickies note, select text + the image, and try to drag by clicking on the image, I get the whole selection (text + image). However, if I don't click and hold on the image before the drag, I only get the image in my drag. Is it possible to make sure both behaviors are retained on Mac (i.e., include tests for the no delay case as well)? On Win/Linux, I wouldn't expect the delay to be necessary (EventHandler::TextDragDelay is 0). > Source/WebCore/page/DragController.cpp:698 > + if (state.m_dragType == DragSourceActionSelection || !imageURL.isEmpty() || !linkURL.isEmpty()) Nit: Can this be an 'else if'? > Source/WebCore/page/DragController.cpp:702 > + m_sourceDragOperation = static_cast<DragOperation>(m_sourceDragOperation | DragOperationGeneric | DragOperationCopy); Why did the previous code replace m_sourceDragOperation and we now add? > Source/WebCore/page/DragController.cpp:785 > + // image or link had no URL. Oh well. Nit: I would replace "Oh well." with "so there's nothing to drag." > Source/WebCore/page/EventHandler.cpp:891 > +void EventHandler::updateDragSourceActionsAllowed(DragState::EventDispatchPolicy& policy) const Nit: Maybe this method can just return a DragState::EventDispatchPolicy value? > Source/WebCore/page/EventHandler.cpp:-913 > - flagUA = ((mask & DragSourceActionImage) || (mask & DragSourceActionLink) || (mask & DragSourceActionSelection)); Where did this check go? Is it no longer needed? It's not clear to me what flagUA was for. > Source/WebCore/page/EventHandler.cpp:908 > + if (mask & DragSourceActionDHTML) > + policy = DragState::DISPATCH_EVENTS; > + else > + policy = DragState::DO_NOT_DISPATCH_EVENTS; Nit: Maybe use a ternary operator? > Source/WebCore/page/EventHandler.cpp:2640 > + default: > + ASSERT_NOT_REACHED(); I think if you enumerate all the cases, you don't need the default: case at all which makes new DragSourceActions give a compile error rather than a runtime error. I.e., I would change default: to DragSourceActionNone: DragSourceActionAny:.
(In reply to comment #29) > View in context: https://bugs.webkit.org/attachment.cgi?id=93080&action=review > > This looks like a move in the right direction. I just have a mix of style nits and some questions so I can understand the change better. > > > LayoutTests/ChangeLog:13 > > + mouse down and the mouse move. The change in dragging priority matches IE and Firefox > > + behavior. > > AFAICT, this matches OSX's behavior as well. For example, if I put an image in a Stickies note, select text + the image, and try to drag by clicking on the image, I get the whole selection (text + image). > > However, if I don't click and hold on the image before the drag, I only get the image in my drag. Is it possible to make sure both behaviors are retained on Mac (i.e., include tests for the no delay case as well)? On Win/Linux, I wouldn't expect the delay to be necessary (EventHandler::TextDragDelay is 0). > I'm currently working on seeing if it's possible to implement the behavior you've described. I'll have to figure out how to write a OS-specific layout test too though. Any tips? > > Source/WebCore/page/DragController.cpp:698 > > + if (state.m_dragType == DragSourceActionSelection || !imageURL.isEmpty() || !linkURL.isEmpty()) > > Nit: Can this be an 'else if'? > It is intentionally an if to make sure that <anchors> and <images> that were explicitly marked draggable still get the additional behavior for links/images. I'm not sure if that's a desirable thing or not though. > > Source/WebCore/page/DragController.cpp:702 > > + m_sourceDragOperation = static_cast<DragOperation>(m_sourceDragOperation | DragOperationGeneric | DragOperationCopy); > > Why did the previous code replace m_sourceDragOperation and we now add? > If the dragged node only set move as an operation, we'd unset it. > > Source/WebCore/page/DragController.cpp:785 > > + // image or link had no URL. Oh well. > > Nit: I would replace "Oh well." with "so there's nothing to drag." > Done. > > Source/WebCore/page/EventHandler.cpp:891 > > +void EventHandler::updateDragSourceActionsAllowed(DragState::EventDispatchPolicy& policy) const > > Nit: Maybe this method can just return a DragState::EventDispatchPolicy value? > I didn't do that since it doesn't seem clear to me what the return value from a function like this would mean. Maybe the function needs a better name? (I can't think of one at the moment though...) > > Source/WebCore/page/EventHandler.cpp:-913 > > - flagUA = ((mask & DragSourceActionImage) || (mask & DragSourceActionLink) || (mask & DragSourceActionSelection)); > > Where did this check go? Is it no longer needed? It's not clear to me what flagUA was for. > > > Source/WebCore/page/EventHandler.cpp:908 > > + if (mask & DragSourceActionDHTML) > > + policy = DragState::DISPATCH_EVENTS; > > + else > > + policy = DragState::DO_NOT_DISPATCH_EVENTS; > > Nit: Maybe use a ternary operator? > Done. > > Source/WebCore/page/EventHandler.cpp:2640 > > + default: > > + ASSERT_NOT_REACHED(); > > I think if you enumerate all the cases, you don't need the default: case at all which makes new DragSourceActions give a compile error rather than a runtime error. I.e., I would change default: to DragSourceActionNone: DragSourceActionAny:. Done.
Created attachment 93181 [details] Patch
> > Source/WebCore/page/EventHandler.cpp:-913 > > - flagUA = ((mask & DragSourceActionImage) || (mask & DragSourceActionLink) || (mask & DragSourceActionSelection)); > > Where did this check go? Is it no longer needed? It's not clear to me what flagUA was for. > It was used for two things; - Potential short-circuiting of logic in eventMayStartDrag(), a Mac-specific method. By removing it, eventMayStartDrag() can no longer return early if DragController::delegateDragSourceAction() returns DragSourceActionNone. I think this is fairly minor, since we'll still get the same return value in the end (draggableNode() won't return anything and we'll return false). If people feel strongly about it, I can certainly add a check to early return if DragController::dragSourceAction() == DragSourceActionNone. - Double-checked permissions in draggableNode. This is redundant, since we have to check the drag source actions allowed anyway in draggableNode when determining the node to drag.
Comment on attachment 93181 [details] Patch Thinking more about how to resolve selection vs image drag issue on Mac.
Created attachment 93389 [details] Patch
Created attachment 93391 [details] Patch
Comment on attachment 93391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93391&action=review Just for curious, do we need the pixel test? typical editing tests are covered by text test with Markup.dump(). (I'm not an expert in this area and this might be a stupid question....) > Source/WebCore/page/DragController.cpp:577 > + state.m_dragType = DragSourceActionSelection; If/else or ternary op? usually assigning-only-once is good idea. > Source/WebCore/page/DragState.h:46 > + }; We usually use CamelCase for defining enum. (there are some old code that don't follow this though.)
Comment on attachment 93391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93391&action=review This is looking really good. One more round and I think we'll be done. > LayoutTests/platform/mac/editing/pasteboard/drag-selections-to-contenteditable.html:1 > +<html> Morita is right, this could be a dump as markup test. You can include LayoutTest/resources/dump-as-markup.js to use Markup.dump. It would make the pixel result unnecessary (seems like dumping the DOM tree of the contentEditable region is sufficient). > Source/WebCore/page/DragController.cpp:686 > + if (!state.m_dragSrc->contains(hitTestResult.innerNode())) > + // The original node being dragged isn't under the mouse for some reason... bail out. > + return false; Is this check new? Can the comment describe why this might happen rather than just describing what the code is doing? > Source/WebCore/page/EventHandler.cpp:2732 > + // 4. We enter this block again, and since it's now marked as a selection drag, we > + // cancel the drag. > + m_mouseDownTimestamp = event.event().timestamp() - 2 * TextDragDelay; This seems hacky-- is there other code that depends on m_mouseDownTimestamp? Maybe we can put an extra bool in dragState() to keep track of this case? > Source/WebCore/page/EventHandler.cpp:2745 > + if (!ExactlyOneBitSet(dragState().m_dragType)) { > + ASSERT((dragState().m_dragType & DragSourceActionSelection)); > + ASSERT((dragState().m_dragType & ~DragSourceActionSelection) == DragSourceActionDHTML > + || (dragState().m_dragType & ~DragSourceActionSelection) == DragSourceActionImage > + || (dragState().m_dragType & ~DragSourceActionSelection) == DragSourceActionLink); > + dragState().m_dragType = DragSourceActionSelection; > + } Why is this needed?
Created attachment 93521 [details] Patch
(In reply to comment #36) > (From update of attachment 93391 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93391&action=review > > Just for curious, do we need the pixel test? typical editing tests are covered by text test with Markup.dump(). > (I'm not an expert in this area and this might be a stupid question....) > I've changed the layout test. > > Source/WebCore/page/DragController.cpp:577 > > + state.m_dragType = DragSourceActionSelection; > > If/else or ternary op? usually assigning-only-once is good idea. > Done. > > Source/WebCore/page/DragState.h:46 > > + }; > > We usually use CamelCase for defining enum. (there are some old code that don't follow this though.) Done. (In reply to comment #37) > (From update of attachment 93391 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93391&action=review > > This is looking really good. One more round and I think we'll be done. > > > LayoutTests/platform/mac/editing/pasteboard/drag-selections-to-contenteditable.html:1 > > +<html> > > Morita is right, this could be a dump as markup test. You can include LayoutTest/resources/dump-as-markup.js to use Markup.dump. It would make the pixel result unnecessary (seems like dumping the DOM tree of the contentEditable region is sufficient). > Done. > > Source/WebCore/page/DragController.cpp:686 > > + if (!state.m_dragSrc->contains(hitTestResult.innerNode())) > > + // The original node being dragged isn't under the mouse for some reason... bail out. > > + return false; > > Is this check new? Can the comment describe why this might happen rather than just describing what the code is doing? > Done. > > Source/WebCore/page/EventHandler.cpp:2732 > > + // 4. We enter this block again, and since it's now marked as a selection drag, we > > + // cancel the drag. > > + m_mouseDownTimestamp = event.event().timestamp() - 2 * TextDragDelay; > > This seems hacky-- is there other code that depends on m_mouseDownTimestamp? Maybe we can put an extra bool in dragState() to keep track of this case? > Done. > > Source/WebCore/page/EventHandler.cpp:2745 > > + if (!ExactlyOneBitSet(dragState().m_dragType)) { > > + ASSERT((dragState().m_dragType & DragSourceActionSelection)); > > + ASSERT((dragState().m_dragType & ~DragSourceActionSelection) == DragSourceActionDHTML > > + || (dragState().m_dragType & ~DragSourceActionSelection) == DragSourceActionImage > > + || (dragState().m_dragType & ~DragSourceActionSelection) == DragSourceActionLink); > > + dragState().m_dragType = DragSourceActionSelection; > > + } > > Why is this needed? It's just an assertion to make sure that the code is updated if we ever add new edge cases. Right now, I want to make sure only one other bit other than selection is ever set and that bit has to be for either an image/link/HTML drag.
Created attachment 93525 [details] Patch
Comment on attachment 93525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93525&action=review > LayoutTests/platform/mac/editing/pasteboard/drag-selections-to-contenteditable.html:33 > + log(document.getElementById('target').children[0].tagName); > + log(document.getElementById('target').children[1].tagName); > + log(document.getElementById('target').children[2].tagName); > + log(document.getElementById('target').children[3].tagName); Nit: It might be good to verify that there are exactly 4 children, although I guess we'd see the extra output in the text dump.
Committed r86472: <http://trac.webkit.org/changeset/86472>
It looks like this broke the Windows build: http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/16362/steps/compile-webkit/logs/stdio
Comment on attachment 93521 [details] Patch Attachment 93521 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8693345
(In reply to comment #43) > It looks like this broke the Windows build: > > http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/16362/steps/compile-webkit/logs/stdio Working on fixing this at the moment. It should be a simple fix, I'm just testing that it works before committing.
Comment on attachment 93525 [details] Patch Attachment 93525 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8698221
Unfortunately I can't figure out how to fix this the right way, nor can I figure out why EWS started hating the last two patches. I tried pulling the ctor/dtor out into its own .cpp file but that didn't make the build any happier. For now I'm adding extra #includes to DragState.h.