Bug 98173

Created attachment 166706 [details] Patch

Comment on attachment 166706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166706&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:4077 > + FrameSelection* fs = focusedFrame->selection(); Please don't use abbreviations like fs. > Source/WebKit/chromium/src/WebViewImpl.cpp:4081 > + if (node->renderer() && !fs->contains(point) && (node->isContentEditable() || node->isTextNode()) && !result.isLiveLink() > + && node->dispatchEvent(Event::create(eventNames().selectstartEvent, true, true))) { > + VisiblePosition pos(node->renderer()->positionForPoint(result.localPoint())); > + WebFrameImpl::selectWordAroundPosition(focusedFrame, pos); This code looks fishy to me. Why isn't this in EventHandler.cpp? In particular, we should be using canMouseDownStartSelect and share most of code with selectClosestWordFromMouseEvent.

Created attachment 166892 [details] Patch

Thanks for the review. The second patch changes this to be implemented fully within EventHandler. The code to perform the selection is now shared as much as possible with selectClosestWordFromMouseEvent. I did not call canMouseDownStartSelect for this as that seems to be concerned with preventing mouse drags from starting selection when there is a draggable element.

Comment on attachment 166892 [details] Patch Attachment 166892 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14131721 New failing tests: fast/events/touch/gesture/context-menu-on-two-finger-tap.html

Created attachment 166915 [details] Patch

Comment on attachment 166915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166915&action=review Excellent! This patch looks much, much better. > Source/WebCore/page/EventHandler.cpp:455 > + selectClosestWordFromHitTestResult(result.hitTestResult(), (result.event().clickCount() == 2 && m_frame->editor()->isSelectTrailingWhitespaceEnabled()) ? ShouldAppendTrailingWhitespace : DontAppendTrailingWhitespace); This is a really long time. I would split into two lines (make sure to add curly brackets around the if statement in that case).

Comment on attachment 166915 [details] Patch Attachment 166915 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14118975 New failing tests: fast/events/touch/gesture/context-menu-on-two-finger-tap.html

Comment on attachment 166915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166915&action=review > Source/WebCore/page/EventHandler.cpp:2523 > + return handleGestureLongPress(gestureEvent); why do this here instead of in WebViewImpl? Am curious. > Source/WebKit/chromium/src/WebViewImpl.cpp:-732 > - eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent); As per the failing layout test, this changes breaks touch summoning of context menus on CrOS/Windows Chrome. Please consider putting this back and introducing a new different gesture event for text editing initiation. I suggest GestureStartTextSelection.

Comment on attachment 166915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166915&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:732 > + eventSwallowed = mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(platformEvent); Oh oops, this is going to fire a gesture event :(

Comment on attachment 166915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166915&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:-732 >> - eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent); > > As per the failing layout test, this changes breaks touch summoning of context menus on CrOS/Windows Chrome. Please consider putting this back and introducing a new different gesture event for text editing initiation. I suggest GestureStartTextSelection. "Start Text Selection" isn't a gesture though, is it? It's the result of a gesture for us, but a gesture should be something the user does, like tap or longpress, or two-finger tap. The failing test is to do with two-finger-tap. I'll check to see if this change has had any unintended effects on that. This change should still allow context menus to work, as sendContextMenuEventForGesture is now called by handleGestureLongPress, which is called by handleGestureEvent. The code to perform selection is OS(ANDROID) only. >> Source/WebKit/chromium/src/WebViewImpl.cpp:732 >> + eventSwallowed = mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(platformEvent); > > Oh oops, this is going to fire a gesture event :( This will have the effect that the longpress gesture goes to the node in the same way that a tap gesture already does. If it's not swallowed, then the result will be the same as before, in that sendContextMenuEventForGesture will be called. Is that not ok?

Comment on attachment 166915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166915&action=review >>> Source/WebKit/chromium/src/WebViewImpl.cpp:-732 >>> - eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent); >> >> As per the failing layout test, this changes breaks touch summoning of context menus on CrOS/Windows Chrome. Please consider putting this back and introducing a new different gesture event for text editing initiation. I suggest GestureStartTextSelection. > > "Start Text Selection" isn't a gesture though, is it? It's the result of a gesture for us, but a gesture should be something the user does, like tap or longpress, or two-finger tap. > > The failing test is to do with two-finger-tap. I'll check to see if this change has had any unintended effects on that. > > This change should still allow context menus to work, as sendContextMenuEventForGesture is now called by handleGestureLongPress, which is called by handleGestureEvent. The code to perform selection is OS(ANDROID) only. I think the #if should be in WebViewImpl and not EventHandler. For instance, why (for android) is m_contextMenuAllowed set to true on long press when no context menu is shown.

Comment on attachment 166915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166915&action=review >>>> Source/WebKit/chromium/src/WebViewImpl.cpp:-732 >>>> - eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent); >>> >>> As per the failing layout test, this changes breaks touch summoning of context menus on CrOS/Windows Chrome. Please consider putting this back and introducing a new different gesture event for text editing initiation. I suggest GestureStartTextSelection. >> >> "Start Text Selection" isn't a gesture though, is it? It's the result of a gesture for us, but a gesture should be something the user does, like tap or longpress, or two-finger tap. >> >> The failing test is to do with two-finger-tap. I'll check to see if this change has had any unintended effects on that. >> >> This change should still allow context menus to work, as sendContextMenuEventForGesture is now called by handleGestureLongPress, which is called by handleGestureEvent. The code to perform selection is OS(ANDROID) only. > > I think the #if should be in WebViewImpl and not EventHandler. For instance, why (for android) is m_contextMenuAllowed set to true on long press when no context menu is shown. Longpress on android can show a context menu, for example on a link or an image. Selection of text only occurs if the longpress was on non-link text. The reason for the failing test is that I didn't notice that WebInputEvent::GestureTwoFingerTap uses the same code path as GestureLongPress in WebViewImpl::handleGestureEvent, whereas EventHandler::handleGestureEvent doesn't handle two finger tap so this change disables that gesture. I will restore two finger tap in a new patch.

Created attachment 167097 [details] Patch

The last patch restores GestureTwoFingerTap handling to how it was (except without the detectContentOnTouch call which is Android-only and shouldn't be called for a two-finger tap). The layout test context-menu-on-two-finger-tap.html should now pass.

(In reply to comment #13) > (From update of attachment 166915 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166915&action=review > > >>>> Source/WebKit/chromium/src/WebViewImpl.cpp:-732 > >>>> - eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent); > >>> > >>> As per the failing layout test, this changes breaks touch summoning of context menus on CrOS/Windows Chrome. Please consider putting this back and introducing a new different gesture event for text editing initiation. I suggest GestureStartTextSelection. > >> > >> "Start Text Selection" isn't a gesture though, is it? It's the result of a gesture for us, but a gesture should be something the user does, like tap or longpress, or two-finger tap. > >> > >> The failing test is to do with two-finger-tap. I'll check to see if this change has had any unintended effects on that. > >> > >> This change should still allow context menus to work, as sendContextMenuEventForGesture is now called by handleGestureLongPress, which is called by handleGestureEvent. The code to perform selection is OS(ANDROID) only. > > > > I think the #if should be in WebViewImpl and not EventHandler. For instance, why (for android) is m_contextMenuAllowed set to true on long press when no context menu is shown. > > Longpress on android can show a context menu, for example on a link or an image. Selection of text only occurs if the longpress was on non-link text. > > The reason for the failing test is that I didn't notice that WebInputEvent::GestureTwoFingerTap uses the same code path as GestureLongPress in WebViewImpl::handleGestureEvent, whereas EventHandler::handleGestureEvent doesn't handle two finger tap so this change disables that gesture. I will restore two finger tap in a new patch. I was not talking about the failing test at all. My concern is that for android, we are setting the state of context menu allowed to true but not showing the context menu in some cases. This seems wrong. It appears to me that a script listening to selection change will be able to invoke context menu since WebViewImpl will allow it. FWIW, I am not a reviewer :) so you can totally ignore my comments if the reviewers agree to this.

Comment on attachment 167097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167097&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:726 > + m_contextMenuAllowed = true; > + PlatformGestureEventBuilder platformEvent(mainFrameImpl()->frameView(), event); > + eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent); > + m_contextMenuAllowed = false; I agree with Varun that it's strange to set m_contextMenuAllowed true and then not show the context menu. On the other hand, I don't understand the value of having this boolean variable in the first place.

Comment on attachment 167097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167097&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:726 >> + m_contextMenuAllowed = false; > > I agree with Varun that it's strange to set m_contextMenuAllowed true and then not show the context menu. > On the other hand, I don't understand the value of having this boolean variable in the first place. m_contextMenuAllowed is only read in ContextMenuClientImpl::getCustomMenuFromDefaultItems, where the aim is to ensure that the context menu is only shown in response to user input. I'd argue that, as longpress is a gesture that can show a context menu, it's correct for m_contextMenuAllowed to be true when a longpress is handled by the EventHandler, regardless of whether or not the default handling will show the menu.

Comment on attachment 167097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167097&action=review > Source/WebCore/page/EventHandler.cpp:433 > +void EventHandler::selectClosestWordFromHitTestResult(const HitTestResult& result, AppendTrailingWhitespace appendTrailingWhitespace) this code looks a lot like WebFrameImpl::selectWordAroundPosition Is there an opportunity to remove some duplicate code especially since selectClosestWord* doesn't seem to be used anywhere else yet? > Source/WebCore/page/EventHandler.cpp:2575 > +#if OS(ANDROID) I'd prefer it if you added a flag or a different kind of event instead of the #ifdef.

Comment on attachment 167097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167097&action=review >> Source/WebCore/page/EventHandler.cpp:2575 >> +#if OS(ANDROID) > > I'd prefer it if you added a flag or a different kind of event instead of the #ifdef. In this case, ifdef is better.

Comment on attachment 167097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167097&action=review >> Source/WebCore/page/EventHandler.cpp:433 >> +void EventHandler::selectClosestWordFromHitTestResult(const HitTestResult& result, AppendTrailingWhitespace appendTrailingWhitespace) > > this code looks a lot like WebFrameImpl::selectWordAroundPosition Is there an opportunity to remove some duplicate code especially since selectClosestWord* doesn't seem to be used anywhere else yet? selectClosestWordFromMouseEvent and selectClosestWordOrLinkFromMouseEvent are preexisting methods, and are used when double-clicks are handled and for selecting a word on Mac when a context menu is generated. This method selectClosestWordFromHitTestResult was added just to make the code no longer dependent on a mouse event so we could use it from a touch gesture - as you can see it mostly uses the previous code from selectClosestWordFromMouseEvent. My first attempt at this change (patch 1) was in WebViewImpl and used selectWordAroundPosition, but on rniwa's suggestion I moved it into EventHandler. I think this it makes sense for it to be in EventHandler, seeing as it's to do with handling gesture events.

Ryosuke (and other reviewers), what do you think of the latest version of this patch? Does it need further changes?

Comment on attachment 167097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167097&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:743 > - eventSwallowed = mainFrameImpl()->frame()->eventHandler()->sendContextMenuEventForGesture(platformEvent); > + eventSwallowed = mainFrameImpl()->frame()->eventHandler()->handleGestureEvent(platformEvent); handleGestureEvent is going to fire a gesture event. Why is that okay? If that's intended, we should have a test for that, right? Or am I misunderstanding something?

> handleGestureEvent is going to fire a gesture event. Why is that okay? > If that's intended, we should have a test for that, right? Or am I misunderstanding something? I don't think we expose gesture events to the web. We might do that in the future, depending on how the touch events / pointer events / gesture events discussion plays out in the standards world.

Adam clarified his comment. If we look at GestureEvent::create, we see that we only instantiate the event for a subset of gestures.

Comment on attachment 167097 [details] Patch Clearing flags on attachment: 167097 Committed r130547: <http://trac.webkit.org/changeset/130547>

All reviewed patches have been landed. Closing bug.