Bug 99520

Summary:

Context menu generated from touch gestures on textareas has context of the cursor position instead of the position where the event occurs.

Product:

WebKit

Reporter:

Varun Jain <varunjain>

Component:

New Bugs

Assignee:

Varun Jain <varunjain>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, olilan, rjkroege, rniwa, tonikitoo, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

100019

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Varun Jain

Reported 2012-10-16 16:03:52 PDT

[chromium] Context menu generated from touch gestures on textareas has context of the cursor position instead of the position where the event occurs.

Attachments
Patch (9.97 KB, patch) 2012-10-16 16:06 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (9.79 KB, patch) 2012-10-16 16:12 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (9.76 KB, patch) 2012-10-17 11:25 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (9.22 KB, patch) 2012-10-17 13:11 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (9.75 KB, patch) 2012-10-17 15:32 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (9.77 KB, patch) 2012-10-18 08:26 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Patch (9.81 KB, patch) 2012-10-23 16:01 PDT, Varun Jain	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Varun Jain

Comment 1 2012-10-16 16:06:29 PDT

Created attachment 169047 [details] Patch

Varun Jain

Comment 2 2012-10-16 16:12:27 PDT

Created attachment 169048 [details] Patch

Adam Barth

Comment 3 2012-10-16 18:00:07 PDT

Looks reasonable, but I'm not an expert on this code code.

Varun Jain

Comment 4 2012-10-17 11:21:56 PDT

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

Varun Jain

Comment 5 2012-10-17 11:25:49 PDT

Created attachment 169223 [details] Patch

Ryosuke Niwa

Comment 6 2012-10-17 11:26:29 PDT

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.

Ryosuke Niwa

Comment 7 2012-10-17 11:36:22 PDT

Comment on attachment 169223 [details] Patch Please see my previous comment for why I'm r-ing this patch.

Varun Jain

Comment 8 2012-10-17 13:11:47 PDT

Created attachment 169241 [details] Patch

Varun Jain

Comment 9 2012-10-17 13:14:54 PDT

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

Varun Jain

Comment 10 2012-10-17 13:16:07 PDT

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.

Ryosuke Niwa

Comment 11 2012-10-17 13:20:11 PDT

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

Kenneth Rohde Christiansen

Comment 12 2012-10-17 14:58:01 PDT

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?

Varun Jain

Comment 13 2012-10-17 15:32:19 PDT

Created attachment 169275 [details] Patch

Varun Jain

Comment 14 2012-10-17 15:32:59 PDT

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.

Antonio Gomes

Comment 15 2012-10-18 08:22:03 PDT

Comment on attachment 169275 [details] Patch Code looks reasonable. I would replace 'fake' by 'synthetic' throughout though.

Varun Jain

Comment 16 2012-10-18 08:26:51 PDT

Created attachment 169417 [details] Patch

Varun Jain

Comment 17 2012-10-18 08:27:05 PDT

(In reply to comment #15) > (From update of attachment 169275 [details]) > Code looks reasonable. I would replace 'fake' by 'synthetic' throughout though. Done.

WebKit Review Bot

Comment 18 2012-10-22 12:21:13 PDT

Comment on attachment 169417 [details] Patch Clearing flags on attachment: 169417 Committed r132119: <http://trac.webkit.org/changeset/132119>

WebKit Review Bot

Comment 19 2012-10-22 12:21:17 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 20 2012-10-22 13:16:31 PDT

Re-opened since this is blocked by bug 100019

Varun Jain

Comment 21 2012-10-23 16:01:24 PDT

Created attachment 170258 [details] Patch

WebKit Review Bot

Comment 22 2012-10-23 17:26:29 PDT

Comment on attachment 170258 [details] Patch Clearing flags on attachment: 170258 Committed r132283: <http://trac.webkit.org/changeset/132283>

WebKit Review Bot

Comment 23 2012-10-23 17:26:34 PDT

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.