If TOUCH_ADJUSTMENT is enabled, use bestClickableNodeForTouchPoint to possibly adjust the location of a context menu event. This change uses the same set of candidates for touch adjustment as is used for a GestureTap event (which admittedly is a simplifying assumption).
Created attachment 155902 [details] Patch
(In reply to comment #1) > Created an attachment (id=155902) [details] > Patch I have not yet added tests, so this patch is only for initial discussion.
Created attachment 155920 [details] Patch
(In reply to comment #0) > If TOUCH_ADJUSTMENT is enabled, use bestClickableNodeForTouchPoint to possibly adjust the location of a context menu event. This change uses the same set of candidates for touch adjustment as is used for a GestureTap event (which admittedly is a simplifying assumption). That is a good start, if we want better targets later we can use the same algorithm but with a more specific filter. Specifically though, I guess the current implementation will miss elements with oncontextmenu event handlers.
Comment on attachment 155920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155920&action=review > Source/WebCore/ChangeLog:11 > + If TOUCH_ADJUSTMENT is enabled, use bestClickableNodeForTouchPoint to possibly > + adjust the location of a context menu event. This change uses the same set of > + candidates for touch adjustment as is used for a GestureTap event (which > + admittedly is a simplifying assumption). I am going to tell you: if you do care about touch and hold entering text selection (i.e. make a #text a target) that won't work as is.
Comment on attachment 155920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155920&action=review > Source/WebCore/page/EventHandler.h:173 > + bool adjustGesturePosition(const PlatformGestureEvent&, IntPoint& adjustedPoint); You might want to add a Gesture Type parameter to adjustGesturePosition, so it later will be able to choose the correct touch-adjustment algorithm or filter to use. The parameter might not be used yet, but you could explain that in a FIXME.
(In reply to comment #6) > (From update of attachment 155920 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155920&action=review > > > Source/WebCore/page/EventHandler.h:173 > > + bool adjustGesturePosition(const PlatformGestureEvent&, IntPoint& adjustedPoint); > > You might want to add a Gesture Type parameter to adjustGesturePosition, so it later will be able to choose the correct touch-adjustment algorithm or filter to use. > > The parameter might not be used yet, but you could explain that in a FIXME. Good suggestion. I will add a FIXME to explain this but will hold off on adding the parameter until it is actually going to be used.
Created attachment 156155 [details] Patch
Comment on attachment 156155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156155&action=review > Source/WebCore/page/EventHandler.cpp:2542 > + return (targetNode); no () needed > LayoutTests/touchadjustment/touch-links-longpress.html:42 > + function testLongPress(touchpoint) > + { > + if (eventSender.gestureLongPress) > + eventSender.gestureLongPress(touchpoint.left, touchpoint.top); > + else > + debug("gestureLongPress not implemented by this platform."); > + } does it mean it will pass on chromium only?
(In reply to comment #9) > (From update of attachment 156155 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156155&action=review > > > Source/WebCore/page/EventHandler.cpp:2542 > > + return (targetNode); > > no () needed I will update this in my next patch. > > > LayoutTests/touchadjustment/touch-links-longpress.html:42 > > + function testLongPress(touchpoint) > > + { > > + if (eventSender.gestureLongPress) > > + eventSender.gestureLongPress(touchpoint.left, touchpoint.top); > > + else > > + debug("gestureLongPress not implemented by this platform."); > > + } > > does it mean it will pass on chromium only? Since it is in touchadjustment/ it will only be run by chromium and qt. But since I believe qt does not implement gestureLongPress, I expect it to fail. So I will go ahead and skip this in the qt TestExpectations file.
Created attachment 156164 [details] Patch
Comment on attachment 156164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156164&action=review > LayoutTests/touchadjustment/touch-links-longpress.html:38 > + if (eventSender.gestureLongPress) my previous question was more: is gestureLongPress support by chromium only? Qt might fail and it is skipped,but what about mac,mac-win,gtk,efl,blackerry,etc?
(In reply to comment #12) > (From update of attachment 156164 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156164&action=review > > > LayoutTests/touchadjustment/touch-links-longpress.html:38 > > + if (eventSender.gestureLongPress) > > my previous question was more: > > is gestureLongPress support by chromium only? Qt might fail and it is skipped,but what about mac,mac-win,gtk,efl,blackerry,etc? I am not sure whether or not the other platforms support gestureLongPress. But since this new test is inside of touchadjustment/ and only chromium and qt have this directory unskipped, there should be no failures on the other platforms.
Comment on attachment 156164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156164&action=review Looks good. I have two questions left: > Source/WebCore/page/EventHandler.cpp:2473 > + // For now we use the adjusted position to ensure the later redundant hit-tests hits the right node. this comment reads a bit strange to me. > Source/WebCore/page/EventHandler.cpp:2661 > + PlatformMouseEvent mouseEvent(adjustedPoint, event.globalPosition(), RightButton, eventType, 1, false, false, false, false, WTF::currentTime()); do we care about adjusting the globalPosition as well? >>> LayoutTests/touchadjustment/touch-links-longpress.html:38 >>> + if (eventSender.gestureLongPress) >> >> my previous question was more: >> >> is gestureLongPress support by chromium only? Qt might fail and it is skipped,but what about mac,mac-win,gtk,efl,blackerry,etc? > > I am not sure whether or not the other platforms support gestureLongPress. But since this new test is inside of touchadjustment/ and only chromium and qt have this directory unskipped, there should be no failures on the other platforms. Ok.
(In reply to comment #14) > (From update of attachment 156164 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156164&action=review > > Looks good. I have two questions left: > > > Source/WebCore/page/EventHandler.cpp:2473 > > + // For now we use the adjusted position to ensure the later redundant hit-tests hits the right node. > > this comment reads a bit strange to me. This refers to the FIXME in EventHandler::handleGestureTap. I have moved the comment and changed the wording to hopefully make it a bit clearer. > > > Source/WebCore/page/EventHandler.cpp:2661 > > + PlatformMouseEvent mouseEvent(adjustedPoint, event.globalPosition(), RightButton, eventType, 1, false, false, false, false, WTF::currentTime()); > > do we care about adjusting the globalPosition as well? Based on an IRC chat with Allan Jensen (carewolf), so far as we know the global position is not used, so it is not set in this patch or when creating the synthetic mouse events in EventHandler::handleGestureTap. > > >>> LayoutTests/touchadjustment/touch-links-longpress.html:38 > >>> + if (eventSender.gestureLongPress) > >> > >> my previous question was more: > >> > >> is gestureLongPress support by chromium only? Qt might fail and it is skipped,but what about mac,mac-win,gtk,efl,blackerry,etc? > > > > I am not sure whether or not the other platforms support gestureLongPress. But since this new test is inside of touchadjustment/ and only chromium and qt have this directory unskipped, there should be no failures on the other platforms. > > Ok.
Created attachment 156391 [details] Patch
Comment on attachment 156391 [details] Patch The previous patch received an r+ from tonikitoo. Since there is a chromium change I will also need approval from abarth.
Comment on attachment 156391 [details] Patch I will need to make some small changes now that https://bugs.webkit.org/show_bug.cgi?id=91012 is in the commit queue.
Comment on attachment 156391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156391&action=review > Source/WebKit/chromium/src/WebInputEventConversion.cpp:168 > + m_area = IntSize(e.boundingBox.width, e.boundingBox.height); LGTM
Comment on attachment 156391 [details] Patch Clearing flags on attachment: 156391 Committed r124642: <http://trac.webkit.org/changeset/124642>
All reviewed patches have been landed. Closing bug.