When a draggable element is in a subframe, like an iframe, EventHandler should dispatch the event to the sub frame.
Created attachment 182707 [details] Patch
That seems completely arbitrary. It should probably be abstracted by platform. CC'ing Simon. He probably has an opinion on what long press should do for Qt.
(In reply to comment #2) > That seems completely arbitrary. It should probably be abstracted by platform. > > CC'ing Simon. He probably has an opinion on what long press should do for Qt. thanks, you're right. If different platforms have different behaviors, i'd like to define a function sth like PassLongPressEventToSubFrame, so that qt, chromium, gtk and others can have their own implementations. what do you think?
Comment on attachment 182707 [details] Patch > thanks, you're right. If different platforms have different behaviors, i'd like to define a function sth like PassLongPressEventToSubFrame, so that qt, chromium, gtk and others can have their own implementations. > what do you think? It depends a bit if a behavior depends on the platform and/or the port. For example if Linux/Windows have a particular behavior that should be true for all ports, a PlatformStragegy could be a better solution. If the behavior is per port, simply using #ifdef is ok. First, ask the other ports (possibly on webkit-dev). Once you know the kind of abstraction you need, you can make design decisions for EventHandler. I r- this for now. Let's get some feedback first on what people want.
> First, ask the other ports (possibly on webkit-dev). Once you know the kind of abstraction you need, you can make design decisions for EventHandler. > > I r- this for now. Let's get some feedback first on what people want. ok, I'll post this issue to mailing list firstly.
I agree, this seems arbitrary. I think Chromium is using the same code path. My guts feeling is that for Qt it shouldn't start a drag, but it may depend on the platform. The bug report is not really clear as to _why_ this behavior is wanted. Can you elaborate?
(In reply to comment #6) > I agree, this seems arbitrary. I think Chromium is using the same code path. > > My guts feeling is that for Qt it shouldn't start a drag, but it may depend on the platform. > > The bug report is not really clear as to _why_ this behavior is wanted. Can you elaborate? Let's see the below example: example.html: <html> <body> <iframe src='aframe.html'></iframe> </body> </html> aframe.html: <html> <head> <script> function handleDragStart(event) { console.log("handleDragStart called"); } </head> <body> <div id="div1" draggable="true" ondragstart="handleDragStart(event)"></div> </body> </html> When opening 'example.html', if a user want to drag the element "div1" on a touch-based device, the LongPress gesture won't be dispatched to "div1" because current implementation doesn't consider subframes. So the function 'handleDragStart' never be called. I don't think it's an expected behavior.
Wow, I did not notice there was already a handleDrag there. Google actually already forced that behavior based on Android: https://bugs.webkit.org/show_bug.cgi?id=101545 Forget my comments, I believed you were introducing that behavior.
Comment on attachment 182707 [details] Patch Moving back to review. My concerns were incorrect.
(In reply to comment #8) > Wow, I did not notice there was already a handleDrag there. > > Google actually already forced that behavior based on Android: https://bugs.webkit.org/show_bug.cgi?id=101545 > Forget my comments, I believed you were introducing that behavior. Yes, this patch is to fix a bug for #101545. The bug is that webkit can't handle draggable elements in subframes. So I fix it with a layout test case. Note that it's only for touch-based drag&drop. No problem for mouse-based drag&drop. (In reply to comment #9) > (From update of attachment 182707 [details]) > Moving back to review. My concerns were incorrect. thanks a lot. sorry for my inaccurate description.
Comment on attachment 182707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182707&action=review It looks good. Question: does the test pass on Firefox? > Source/WebCore/page/EventHandler.h:146 > + bool didStartDrag() { return m_didStartDrag; } const
thanks for your review. (In reply to comment #11) > (From update of attachment 182707 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182707&action=review > > It looks good. Question: does the test pass on Firefox? You mean touch-based firefox version? I tried Firefox for Android and it doesn't support touch-based drag&drop. > > > Source/WebCore/page/EventHandler.h:146 > > + bool didStartDrag() { return m_didStartDrag; } > > const ok, I'll add it.
Created attachment 182898 [details] Patch
(In reply to comment #12) > thanks for your review. > (In reply to comment #11) > > (From update of attachment 182707 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=182707&action=review > > > > It looks good. Question: does the test pass on Firefox? > You mean touch-based firefox version? I tried Firefox for Android and it doesn't support touch-based drag&drop. So you are basically bringing the touch-based behavior in pair with mouse-based behavior?
(In reply to comment #14) > (In reply to comment #12) > > thanks for your review. > > (In reply to comment #11) > > > (From update of attachment 182707 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=182707&action=review > > > > > > It looks good. Question: does the test pass on Firefox? > > You mean touch-based firefox version? I tried Firefox for Android and it doesn't support touch-based drag&drop. > > So you are basically bringing the touch-based behavior in pair with mouse-based behavior? I believe the code in webkit already brings it similar to mouse-based behavior. This patch is just to let the gestures pass into sub frame thus elements in sub frame can trigger a drag/drop operation. If elements are in main frame(no subframes), they work well in current webkit.
Comment on attachment 182898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182898&action=review r- to get one further test running. > Source/WebCore/page/EventHandler.cpp:2552 > + RefPtr<Frame> subframe = subframeForHitTestResult(mev); > + if (subframe && !m_mouseDownMayStartDrag) { > + subframe->eventHandler()->handleDrag(mev, DontCheckDragHysteresis); > + if (subframe->eventHandler()->didStartDrag()) > + return true; > + } can I assume that it works for nested inner frames? if we have 3 levels (mainframe, middle frame and inner frame), and the deeper one handles Drag. Does it work? If so, please add a test.
Created attachment 183381 [details] Patch
(In reply to comment #16) > (From update of attachment 182898 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182898&action=review > > r- to get one further test running. > > > Source/WebCore/page/EventHandler.cpp:2552 > > + RefPtr<Frame> subframe = subframeForHitTestResult(mev); > > + if (subframe && !m_mouseDownMayStartDrag) { > > + subframe->eventHandler()->handleDrag(mev, DontCheckDragHysteresis); > > + if (subframe->eventHandler()->didStartDrag()) > > + return true; > > + } > > can I assume that it works for nested inner frames? > > if we have 3 levels (mainframe, middle frame and inner frame), and the deeper one handles Drag. Does it work? If so, please add a test. you're right. Previous impl doesn't work for 3 and more levels. Re-change the patch and add two cases to test 2 and 3 levels. Thanks a lot!
Antonio, thanks a lot for reviewing it. Could you please help upload it or change it as "commit-queue:+"?
Comment on attachment 183381 [details] Patch Clearing flags on attachment: 183381 Committed r140292: <http://trac.webkit.org/changeset/140292>
All reviewed patches have been landed. Closing bug.