Bug 106874

Summary:

LongPress should start drag&drop draggable elements in iframes

Product:

WebKit

Reporter:

yongsheng <yongsheng.zhu>

Component:

UI Events

Assignee:

yongsheng <yongsheng.zhu>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

allan.jensen, benjamin, hausmann, laszlo.gombos, tonikitoo, varunjain, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

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.