[chromium] Context menu generated from touch gestures on textareas has context of the cursor position instead of the position where the event occurs.
Created attachment 169047 [details] Patch
Created attachment 169048 [details] Patch
Looks reasonable, but I'm not an expert on this code code.
Describing the bug a bit more as requested by reviewers: in a textarea, type a mis-spelled word, then move the cursor to some other word in the same area, then summon context menu by doing a touch gesture on the mis-spelled word. The context menu does not contain spelling suggestions because its the menu for the word where the cursor is, not where the gesture was made. This is chromium specific. the chromium bug is here: https://code.google.com/p/chromium/issues/detail?id=152783
Created attachment 169223 [details] Patch
Comment on attachment 169048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169048&action=review > Source/WebCore/page/EventHandler.cpp:2527 > + case PlatformEvent::GestureTwoFingerTap: > + return handleGestureTwoFingerTap(gestureEvent); This is clearly not a Chromium-specific bug given that you're modifying platform-agnostic code in WebCore. > Source/WebCore/page/EventHandler.cpp:2799 > + handleMousePressEvent(mouseEvent); Why are we adding this? > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Please use HTML5 style DOCTYPE: <!DOCTYPE html> > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:4 > +<script src="../../../js/resources/js-test-pre.js"></script> Why are we including js-test-pre if we're not using any of its features? > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:6 > +<body onload="test()"> Do we really need to wait until load event fires? > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:24 > + if (text.selectionStart) > + return text.selectionStart; All ports ship with selectionStart IDL property. There's no need for this function to exist. > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:32 > + if (text.setSelectionRange) { All ports ship with setSelectionRange. There's no need to check this condition. > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:37 > + } else { > + debug("setSelectionRange not supported by this platform"); > + } No curly brackets around a single statement. > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:51 > + if (longPressTested && getCursorPosition() == 63) { > + document.getElementById("result").innerHTML = "PASS"; > + } Ditto. > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:68 > + if (eventSender.gestureLongPress) { > + eventSender.gestureLongPress(x, y); > + } else { Ditto. > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:74 > + if (eventSender.gestureTwoFingerTap) { > + eventSender.gestureTwoFingerTap(x + 20, y); > + } else { Ditto.
Comment on attachment 169223 [details] Patch Please see my previous comment for why I'm r-ing this patch.
Created attachment 169241 [details] Patch
(In reply to comment #6) > (From update of attachment 169048 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169048&action=review > > > Source/WebCore/page/EventHandler.cpp:2527 > > + case PlatformEvent::GestureTwoFingerTap: > > + return handleGestureTwoFingerTap(gestureEvent); > > This is clearly not a Chromium-specific bug given that you're modifying platform-agnostic code in WebCore. Done. > > > Source/WebCore/page/EventHandler.cpp:2799 > > + handleMousePressEvent(mouseEvent); > > Why are we adding this? This makes gesture-driven context menu behave exactly like right-click context menu. For right click, we send a mouse press event before the context menu. This does any tasks necessary before getting the context menu, including placing the cursor at the right place so we can get the correct context. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:1 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > Please use HTML5 style DOCTYPE: <!DOCTYPE html> > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:4 > > +<script src="../../../js/resources/js-test-pre.js"></script> > > Why are we including js-test-pre if we're not using any of its features? > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:6 > > +<body onload="test()"> > > Do we really need to wait until load event fires? > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:24 > > + if (text.selectionStart) > > + return text.selectionStart; > > All ports ship with selectionStart IDL property. There's no need for this function to exist. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:32 > > + if (text.setSelectionRange) { > > All ports ship with setSelectionRange. There's no need to check this condition. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:37 > > + } else { > > + debug("setSelectionRange not supported by this platform"); > > + } > > No curly brackets around a single statement. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:51 > > + if (longPressTested && getCursorPosition() == 63) { > > + document.getElementById("result").innerHTML = "PASS"; > > + } > > Ditto. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:68 > > + if (eventSender.gestureLongPress) { > > + eventSender.gestureLongPress(x, y); > > + } else { > > Ditto. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:74 > > + if (eventSender.gestureTwoFingerTap) { > > + eventSender.gestureTwoFingerTap(x + 20, y); > > + } else { > > Ditto.
clicked "commit" without replying to your other comments :( (In reply to comment #6) > (From update of attachment 169048 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169048&action=review > > > Source/WebCore/page/EventHandler.cpp:2527 > > + case PlatformEvent::GestureTwoFingerTap: > > + return handleGestureTwoFingerTap(gestureEvent); > > This is clearly not a Chromium-specific bug given that you're modifying platform-agnostic code in WebCore. > > > Source/WebCore/page/EventHandler.cpp:2799 > > + handleMousePressEvent(mouseEvent); > > Why are we adding this? > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:1 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > Please use HTML5 style DOCTYPE: <!DOCTYPE html> > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:4 > > +<script src="../../../js/resources/js-test-pre.js"></script> > > Why are we including js-test-pre if we're not using any of its features? > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:6 > > +<body onload="test()"> > > Do we really need to wait until load event fires? Done. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:24 > > + if (text.selectionStart) > > + return text.selectionStart; > > All ports ship with selectionStart IDL property. There's no need for this function to exist. Done. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:32 > > + if (text.setSelectionRange) { > > All ports ship with setSelectionRange. There's no need to check this condition. Done. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:37 > > + } else { > > + debug("setSelectionRange not supported by this platform"); > > + } > > No curly brackets around a single statement. Done. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:51 > > + if (longPressTested && getCursorPosition() == 63) { > > + document.getElementById("result").innerHTML = "PASS"; > > + } > > Ditto. Done. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:68 > > + if (eventSender.gestureLongPress) { > > + eventSender.gestureLongPress(x, y); > > + } else { > > Ditto. Done. > > > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:74 > > + if (eventSender.gestureTwoFingerTap) { > > + eventSender.gestureTwoFingerTap(x + 20, y); > > + } else { > > Ditto. Done.
(In reply to comment #9) > (In reply to comment #6) > > > > > Source/WebCore/page/EventHandler.cpp:2799 > > > + handleMousePressEvent(mouseEvent); > > > > Why are we adding this? > > This makes gesture-driven context menu behave exactly like right-click context menu. For right click, we send a mouse press event before the context menu. This does any tasks necessary before getting the context menu, including placing the cursor at the right place so we can get the correct context. I'm sorry but I don't think I can review this patch. I don't know enough about this code to tell the consequences of this change.
Comment on attachment 169241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169241&action=review Please fix my comments before landing > Source/WebKit/chromium/ChangeLog:13 > + * src/WebViewImpl.cpp: > + (WebKit::WebViewImpl::handleGestureEvent): I would write that you are now using the same code path as WebInputEvent::GestureLongPress > Source/WebCore/page/EventHandler.cpp:2799 > + handleMousePressEvent(mouseEvent); I would add a comment why mouse release it not needed > LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:7 > +This test checks that on long press gesture in a text area, the cursor is or two finger?
Created attachment 169275 [details] Patch
Comment on attachment 169241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169241&action=review >> Source/WebKit/chromium/ChangeLog:13 >> + (WebKit::WebViewImpl::handleGestureEvent): > > I would write that you are now using the same code path as WebInputEvent::GestureLongPress Done. >> Source/WebCore/page/EventHandler.cpp:2799 >> + handleMousePressEvent(mouseEvent); > > I would add a comment why mouse release it not needed Done. >> LayoutTests/fast/events/touch/gesture/right-click-gestures-set-cursor-at-correct-position.html:7 >> +This test checks that on long press gesture in a text area, the cursor is > > or two finger? Done.
Comment on attachment 169275 [details] Patch Code looks reasonable. I would replace 'fake' by 'synthetic' throughout though.
Created attachment 169417 [details] Patch
(In reply to comment #15) > (From update of attachment 169275 [details]) > Code looks reasonable. I would replace 'fake' by 'synthetic' throughout though. Done.
Comment on attachment 169417 [details] Patch Clearing flags on attachment: 169417 Committed r132119: <http://trac.webkit.org/changeset/132119>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 100019
Created attachment 170258 [details] Patch
Comment on attachment 170258 [details] Patch Clearing flags on attachment: 170258 Committed r132283: <http://trac.webkit.org/changeset/132283>