Bug 78855

Created attachment 127460 [details] Patch

Comment on attachment 127460 [details] Patch Do you want to do this at the WebKit layer? Or should this be the defaultEventHandler for block elements?

rniwa should be able to offer some thoughts as to where to impelement this behavior. We implement double-click, triple-click in WebCore iirc, (word-based, and paragraph-based selection on Mac).

Comment on attachment 127460 [details] Patch I'm pretty sure this is not where you want this code to go. I recommend asking rniwa in #webkit. He'll have suggestions. Darin Adler, and Enrica are also good people for Selection-related questions.

See the shouldSelectOnContextualMenuClick() in editing/EditingBehavior.h and the code in EventHandler::sendContextMenuEvent that checks that value.

What Tony said. The patch posted here doesn't look right.

Thanks for the comments. One reason that this was implemented this way is that when the selection occurs we don't want to generate a context menu. So it seemed like it wouldn't be right to call through to EventHandler's sendContextMenuEvent() when we don't want to send a context menu event, so I thought changes to mouseDown were necessary in order to not call mouseContextMenu. I guess instead we could allow it to go through to sendContextMenuEvent, leverage the existing shouldSelectOnContextualMenuClick, and ignore or suppress the context menu event. How does that sound?

For better or worse, the web allows pages to disable text selection. It's unclear to me if this implementation would still allow this. In any case, it is my impression that this feature is well-trodden territory in WebCore. I'm sorry I don't have the context to give you the exact lines of code to change, but you should be able to implement this very similarly to how double-click/tripple-click work. I'm happy to do some code spelunking tomorrow to help you find the right hooks if they're not obvious from rniwa/tony's comment's above.

Yes, I agree this should respect the page's request to disable selection. The original patch does attempt to do that, though it seems we need a new approach generally. Implement similar to double/triple click: OK, so it sounds to me like that means the place to implement would be in EventHandler::handleMousePressEvent or EventHandler::handleMousePressEventSingleClick, using (a new) EditingBehavior to guard the new behaviour. Similar to how double-clicks work, we would use EventHandler::selectClosestWordFromMouseEvent to make the selection. Better?

(In reply to comment #7) > > One reason that this was implemented this way is that when the selection occurs we don't want to generate a context menu. So it seemed like it wouldn't be right to call through to EventHandler's sendContextMenuEvent() when we don't want to send a context menu event, so I thought changes to mouseDown were necessary in order to not call mouseContextMenu. Even though we're not going to show a context menu, don't we still need to fire the DOM event? That is, the web page should still see the event fired on a long click and be able to cancel it (which would prevent the selection from happening). I tried this on Chrome on Android to see the behavior: a selection toolbar shows up instead of a context menu. It might be better to put the logic for what to show in the showContextMenu callback in WebViewClient (i.e., sometimes show the context menu, sometimes show the selection toolbar). Hopefully there's enough information in WebContextMenuData, or maybe you can add addition information there as needed. I agree that it's a bit weird that showContextMenu doesn't show a context menu, but I think we do want to trigger this behavior off of the context menu event succeeding. Maybe we could rename the callback.

> That is, the web page should still see the event fired on a long click and be able to cancel it (which would prevent the selection from happening). It doesn't seem right to me that a page that wants to disallow context menus would also disallow selection if we did it this way. 'Longpress to select' on Android is the equivalent of dragging to select on other platforms - it's not the equivalent of pulling up a context menu. If a page wants to disallow selection, I'd have thought that using something like -webkit-user-select would make more sense. My new suggested approach as in my previous comment is that we implement this in EventHandler::handleMousePressEventSingleClick, so that this becomes a new behaviour associated with a right-click. WebViewImpl::mouseDown can still fire a context menu event, and on the browser side we will ignore it if it is from a selection. How does that sound?

(In reply to comment #10) > (In reply to comment #7) > > > > One reason that this was implemented this way is that when the selection occurs we don't want to generate a context menu. So it seemed like it wouldn't be right to call through to EventHandler's sendContextMenuEvent() when we don't want to send a context menu event, so I thought changes to mouseDown were necessary in order to not call mouseContextMenu. > > Even though we're not going to show a context menu, don't we still need to fire the DOM event? That is, the web page should still see the event fired on a long click and be able to cancel it (which would prevent the selection from happening). You mean contextmenu event? I don't think we should be firing that event unless we're intending to show a context menu. Any web sites that want to cancel selection would be able to do so by canceling selectstart so this shouldn't be an issue for them. Furthermore, this may result in websites showing its own contextmenu after canceling the event, which would be inconsistent with the platform convention. http://www.w3.org/TR/html5/interactive-elements.html#context-menus *If the user requested a context menu using a pointing device* The user agent must dispatch an event with the name contextmenu... Clearly, the user would never request a contextmenu on Android. I'll also note that we should check Safari's behavior on iOS. (In reply to comment #11) > > That is, the web page should still see the event fired on a long click and be able to cancel it (which would prevent the selection from happening). > > It doesn't seem right to me that a page that wants to disallow context menus would also disallow selection if we did it this way. 'Longpress to select' on Android is the equivalent of dragging to select on other platforms - it's not the equivalent of pulling up a context menu. If a page wants to disallow selection, I'd have thought that using something like -webkit-user-select would make more sense. > > My new suggested approach as in my previous comment is that we implement this in EventHandler::handleMousePressEventSingleClick, so that this becomes a new behaviour associated with a right-click. WebViewImpl::mouseDown can still fire a context menu event, and on the browser side we will ignore it if it is from a selection. I don't think we should fire a contextmenu event if we're not intending to show contextmenu.

My line of thinking was that a page might want to disable all longpress events, but on Android, that action is sometimes a contextmenu event and sometimes a text selection event. Disabling the single event requires knowing to handle both cases. Maybe this is already possible by just grabbing the mouse down event and checking the button?

> Maybe this is already possible by just grabbing the mouse down event and checking the button? That's what my original patch does, isn't it? What we've just discussed is basically the motivation for that approach - checking in WebViewImpl::mouseDown for a right mouse click and performing a selection instead of sending a context menu event.

(In reply to comment #14) > > Maybe this is already possible by just grabbing the mouse down event and checking the button? > > That's what my original patch does, isn't it? What we've just discussed is basically the motivation for that approach - checking in WebViewImpl::mouseDown for a right mouse click and performing a selection instead of sending a context menu event. Sorry, I was asking if this code runs before or after the javascript event for the mouse down is run on the page. I.e., is it possible for a page to cancel text selection by canceling the mouse down event. Also, what happens if the user cancels the select start event? Does it show a context menu?

This patch appears to be stalled. What do we need to do to make progress here. Oli, do you have thoughts on Tony's questions in Comment #15 ?

I've been taking another look at this, and I now think the approach originally proposed of having right-click perform selection is not ideal. Instead, I think this would make more sense as part of gesture handling, meaning most of the change would move to a GestureLongPress handler within handleInputEvent. That should avoid any troublesome interactions with context menus or mousedown events. Does that sound reasonable?

> Does that sound reasonable? Other folks might have opinions, but that seems like a reasonable approach to me. Thanks.

Sounds fine to me too. Should we obsolete this bug?

Yes, this can be closed now. Thanks for the reviews.