78855 2012-02-16 15:57:36 -0800 [chromium] On Android, allow right-click to select word. 2012-05-09 10:55:38 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 66687 1 olilan olilan abarth darin enrica eric rniwa tony oldest_to_newest 558623 0 olilan 2012-02-16 15:57:36 -0800 [chromium] On Android, allow right-click to select word. 558627 1 127460 olilan 2012-02-16 16:01:09 -0800 Created attachment 127460 Patch 558633 2 127460 eric 2012-02-16 16:06:07 -0800 Comment on attachment 127460 Patch Do you want to do this at the WebKit layer? Or should this be the defaultEventHandler for block elements? 558637 3 eric 2012-02-16 16:07:24 -0800 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). 558638 4 127460 eric 2012-02-16 16:08:11 -0800 Comment on attachment 127460 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. 558648 5 tony 2012-02-16 16:14:02 -0800 See the shouldSelectOnContextualMenuClick() in editing/EditingBehavior.h and the code in EventHandler::sendContextMenuEvent that checks that value. 558651 6 rniwa 2012-02-16 16:21:45 -0800 What Tony said. The patch posted here doesn't look right. 558732 7 olilan 2012-02-16 17:21:09 -0800 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? 558737 8 eric 2012-02-16 17:26:00 -0800 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. 558772 9 olilan 2012-02-16 18:06:02 -0800 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? 560990 10 tony 2012-02-21 10:21:32 -0800 (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. 561012 11 olilan 2012-02-21 10:41:25 -0800 > 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? 561049 12 rniwa 2012-02-21 11:08:47 -0800 (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. 561123 13 tony 2012-02-21 12:14:47 -0800 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? 563030 14 olilan 2012-02-23 04:28:23 -0800 > 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. 563197 15 tony 2012-02-23 10:14:47 -0800 (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? 619178 16 abarth 2012-05-08 22:42:16 -0700 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 ? 619309 17 olilan 2012-05-09 02:33:10 -0700 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? 619519 18 abarth 2012-05-09 08:40:25 -0700 > Does that sound reasonable? Other folks might have opinions, but that seems like a reasonable approach to me. Thanks. 619617 19 tony 2012-05-09 10:20:16 -0700 Sounds fine to me too. Should we obsolete this bug? 619646 20 olilan 2012-05-09 10:55:38 -0700 Yes, this can be closed now. Thanks for the reviews. 127460 2012-02-16 16:01:09 -0800 2012-02-16 16:08:11 -0800 Patch bug-78855-20120216160108.patch text/plain 2893 olilan U3VidmVyc2lvbiBSZXZpc2lvbjogMTA3ODMwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IDUzYWRiN2M1NDllODE2NzM1ZWU5NGY2NjNjOTkyZjQ4MDQ4ZDhkNzkuLjEwOTU1ZGJlZDJiYTI5 OTVkZjc4MGNjOWY2NDE1MWY2ODY0NmVlMDAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTggQEAKKzIwMTItMDItMTYgIE9saSBMYW4gIDxvbGlsYW5AY2hyb21pdW0ub3Jn PgorCisgICAgICAgIFtjaHJvbWl1bV0gT24gQW5kcm9pZCwgYWxsb3cgcmlnaHQtY2xpY2sgdG8g c2VsZWN0IHdvcmQuCisgICAgICAgIAorICAgICAgICBPbiBBbmRyb2lkLCBwZXJmb3JtcyBhIGhp dCB0ZXN0IGFmdGVyIGEgcmlnaHQtY2xpY2sgYW5kIGlmIHRoZSBjbGljayB3YXMgb24gdW5zZWxl Y3RlZCB0ZXh0LAorICAgICAgICBzZWxlY3RzIHRoZSB3b3JkIHRoYXQgd2FzIGNsaWNrZWQgaW5z dGVhZCBvZiB0aGUgdXN1YWwgY2xpY2sgaGFuZGxpbmcuIEFzIGxvbmdwcmVzc2VzIG9uIEFuZHJv aWQKKyAgICAgICAgZ2VuZXJhdGUgcmlnaHQtY2xpY2tzLCB0aGlzIGltcGxlbWVudHMgdGhlIGxv bmdwcmVzcy10by1zZWxlY3QgZnVuY3Rpb25hbGl0eS4KKworICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Nzg4NTUKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHNyYy9XZWJWaWV3SW1wbC5jcHA6CisgICAgICAg IChXZWJLaXQ6OldlYlZpZXdJbXBsOjptb3VzZURvd24pOgorCiAyMDEyLTAyLTE1ICBBbmRlcnMg Q2FybHNzb24gIDxhbmRlcnNjYUBhcHBsZS5jb20+CiAKICAgICAgICAgUmVtb3ZlIFNjcm9sbGFi bGVBcmVhOjpoYW5kbGVHZXN0dXJlRXZlbnQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vc3JjL1dlYlZpZXdJbXBsLmNwcCBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL1dl YlZpZXdJbXBsLmNwcAppbmRleCAzYWNmOTI0NDFkMmE5N2EwZjkxNjllZGQ5YjYxNGJiMTVmYjdl ZTFlLi4yNDIwZDk5MjY4NWY5OTRjMTlkOTJlZjQ4MWNlNzEyZDRmMzE1MThhIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJWaWV3SW1wbC5jcHAKKysrIGIvU291cmNl L1dlYktpdC9jaHJvbWl1bS9zcmMvV2ViVmlld0ltcGwuY3BwCkBAIC01MDgsNiArNTA4LDM2IEBA IHZvaWQgV2ViVmlld0ltcGw6Om1vdXNlRG93bihjb25zdCBXZWJNb3VzZUV2ZW50JiBldmVudCkK ICAgICAgICAgaGlkZVNlbGVjdFBvcHVwKCk7CiAgICAgfQogCisgICAgLy8gT24gQW5kcm9pZCBh IHJpZ2h0LWNsaWNrICh3aGljaCBtYXkgYmUgZ2VuZXJhdGVkIGJ5IGEgbG9uZ3ByZXNzKSBvbgor ICAgIC8vIHVuc2VsZWN0ZWQgdGV4dCBzaG91bGQgc2VsZWN0IHRoZSB3b3JkIHRoYXQgd2FzIGNs aWNrZWQuCisjaWYgT1MoQU5EUk9JRCkKKyAgICBpZiAoZXZlbnQuYnV0dG9uID09IFdlYk1vdXNl RXZlbnQ6OkJ1dHRvblJpZ2h0KSB7CisgICAgICAgIEZyYW1lKiBmb2N1c2VkRnJhbWUgPSBwYWdl KCktPmZvY3VzQ29udHJvbGxlcigpLT5mb2N1c2VkT3JNYWluRnJhbWUoKTsKKyAgICAgICAgSW50 UG9pbnQgcG9pbnQoZXZlbnQueCwgZXZlbnQueSk7CisgICAgICAgIHBvaW50ID0gZm9jdXNlZEZy YW1lLT52aWV3KCktPndpbmRvd1RvQ29udGVudHMocG9pbnQpOworCisgICAgICAgIEhpdFRlc3RS ZXF1ZXN0IHJlcXVlc3QoSGl0VGVzdFJlcXVlc3Q6OkFjdGl2ZSk7CisgICAgICAgIEhpdFRlc3RS ZXN1bHQgcmVzdWx0KHBvaW50KTsKKyAgICAgICAgZm9jdXNlZEZyYW1lLT5kb2N1bWVudCgpLT5y ZW5kZXJWaWV3KCktPmxheWVyKCktPmhpdFRlc3QocmVxdWVzdCwgcmVzdWx0KTsKKworICAgICAg ICBOb2RlKiBub2RlID0gcmVzdWx0LmlubmVyTm9kZSgpOworICAgICAgICBpZiAoIW5vZGUpCisg ICAgICAgICAgICByZXR1cm47CisgICAgICAgIEVsZW1lbnQqIGVsZW1lbnQgPSBub2RlLT5wYXJl bnRFbGVtZW50KCk7CisgICAgICAgIGlmICghbm9kZS0+aW5Eb2N1bWVudCgpICYmIGVsZW1lbnQg JiYgZWxlbWVudC0+aW5Eb2N1bWVudCgpKQorICAgICAgICAgICAgbm9kZSA9IGVsZW1lbnQ7CisK KyAgICAgICAgRnJhbWVTZWxlY3Rpb24qIHNjID0gZm9jdXNlZEZyYW1lLT5zZWxlY3Rpb24oKTsK KyAgICAgICAgaWYgKCFzYy0+Y29udGFpbnMocG9pbnQpICYmIChub2RlLT5pc0NvbnRlbnRFZGl0 YWJsZSgpIHx8IG5vZGUtPmlzVGV4dE5vZGUoKSkgJiYgIXJlc3VsdC5pc0xpdmVMaW5rKCkKKyAg ICAgICAgICAgICAgICAmJiBub2RlLT5kaXNwYXRjaEV2ZW50KEV2ZW50OjpjcmVhdGUoZXZlbnRO YW1lcygpLnNlbGVjdHN0YXJ0RXZlbnQsIHRydWUsIHRydWUpKSkgeworICAgICAgICAgICAgVmlz aWJsZVBvc2l0aW9uIHBvcyhub2RlLT5yZW5kZXJlcigpLT5wb3NpdGlvbkZvclBvaW50KHJlc3Vs dC5sb2NhbFBvaW50KCkpKTsKKyAgICAgICAgICAgIFdlYkZyYW1lSW1wbDo6c2VsZWN0V29yZEFy b3VuZFBvc2l0aW9uKGZvY3VzZWRGcmFtZSwgcG9zKTsKKyAgICAgICAgICAgIGlmIChzYy0+aXNS YW5nZSgpKQorICAgICAgICAgICAgICAgIHJldHVybjsKKyAgICAgICAgfQorICAgIH0KKyNlbmRp ZgorCiAgICAgLy8gRGlzcGF0Y2ggdGhlIGNvbnRleHRtZW51IGV2ZW50IHJlZ2FyZGxlc3Mgb2Yg aWYgdGhlIGNsaWNrIHdhcyBzd2FsbG93ZWQuCiAgICAgLy8gT24gV2luZG93cywgd2UgaGFuZGxl IGl0IG9uIG1vdXNlIHVwLCBub3QgZG93bi4KICNpZiBPUyhEQVJXSU4pCg==