106874 – LongPress should start drag&drop draggable elements in iframes

RESOLVED FIXED106874

LongPress should start drag&drop draggable elements in iframes

https://bugs.webkit.org/show_bug.cgi?id=106874

Summary LongPress should start drag&drop draggable elements in iframes

yongsheng

Reported 2013-01-14 22:35:55 PST

When a draggable element is in a subframe, like an iframe, EventHandler should dispatch the event to the sub frame.

Attachments
Patch (6.14 KB, patch) 2013-01-14 23:27 PST, yongsheng	no flags	Details Formatted Diff Diff
Patch (6.15 KB, patch) 2013-01-15 18:37 PST, yongsheng	no flags	Details Formatted Diff Diff
Patch (9.42 KB, patch) 2013-01-17 22:45 PST, yongsheng	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

yongsheng

Comment 1 2013-01-14 23:27:12 PST

Created attachment 182707 [details] Patch

Benjamin Poulain

Comment 2 2013-01-15 00:01:51 PST

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.

yongsheng

Comment 3 2013-01-15 00:04:42 PST

(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?

Benjamin Poulain

Comment 4 2013-01-15 00:21:57 PST

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.

yongsheng

Comment 5 2013-01-15 00:27:41 PST

> 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.

Simon Hausmann

Comment 6 2013-01-15 00:59:37 PST

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?

yongsheng

Comment 7 2013-01-15 01:07:55 PST

(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.

Benjamin Poulain

Comment 8 2013-01-15 01:28:10 PST

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.

Benjamin Poulain

Comment 9 2013-01-15 01:29:15 PST

Comment on attachment 182707 [details] Patch Moving back to review. My concerns were incorrect.

yongsheng

Comment 10 2013-01-15 01:34:08 PST

(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.

Antonio Gomes

Comment 11 2013-01-15 11:06:39 PST

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

yongsheng

Comment 12 2013-01-15 18:28:08 PST

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.

yongsheng

Comment 13 2013-01-15 18:37:53 PST

Created attachment 182898 [details] Patch

Antonio Gomes

Comment 14 2013-01-16 04:29:54 PST

(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?

yongsheng

Comment 15 2013-01-16 16:48:59 PST

(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.

Antonio Gomes

Comment 16 2013-01-17 07:48:19 PST

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.

yongsheng

Comment 17 2013-01-17 22:45:21 PST

Created attachment 183381 [details] Patch

yongsheng

Comment 18 2013-01-17 22:47:39 PST

(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!

yongsheng

Comment 19 2013-01-20 17:20:22 PST

Antonio, thanks a lot for reviewing it. Could you please help upload it or change it as "commit-queue:+"?

WebKit Review Bot

Comment 20 2013-01-20 21:14:05 PST

Comment on attachment 183381 [details] Patch Clearing flags on attachment: 183381 Committed r140292: <http://trac.webkit.org/changeset/140292>

WebKit Review Bot

Comment 21 2013-01-20 21:14:10 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component UI Events

Assignee

yongsheng

Reported

2013-01-14 22:35 PST

Modified

2013-01-20 21:14 PST History

CC List

7 users Show

URL

Keywords

Depends on

Blocks