Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(361)

Issue 2785853002: Selection Action mode triggered like a context menu (Closed)

Created:
3 years, 8 months ago by amaralp
Modified:
3 years, 6 months ago
CC:
agrieve+watch_chromium.org, android-webview-reviews_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, dtapuska+blinkwatch_chromium.org, dtapuska+chromiumwatch_chromium.org, jam, jochen+watch_chromium.org, kinuko+watch, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, nona+watch_chromium.org, Navid Zolghadr, Peter Beverloo, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Selection Action mode triggered like a context menu Currently the selection action mode is triggered by selection events (SELECTION_HANDLES_SHOWN). The goal of this CL is to make the mode be triggered by context menu events. This is beneficial for two reasons: 1) The paste popup menu is already being triggered by context menu events and we'd like to eventually unify the selection and paste menus. 2) Currently the selection action mode uses state from the IME which can lead to race conditions causing the menu to show stale information. Using context menu events fixes this problem because we can pass all relevant state through |ContextMenuParams|. In Blink we make range touch selections trigger contextmenu events. This means that a long-press or double tap that results in a range selection sends a context menu event. Because of Android O's smart select we also need to know if the context menu was triggered by moving touch handles or select all so we plumb that information through to ContextMenuParams. BUG=627234 Review-Url: https://codereview.chromium.org/2785853002 Cr-Commit-Position: refs/heads/master@{#476355} Committed: https://chromium.googlesource.com/chromium/src/+/58d4d0b2d9511fe57bde3cbe82e486658183456c

Patch Set 1 #

Patch Set 2 : Fixed touch emulator unittest #

Patch Set 3 : updating layout #

Patch Set 4 : rebase #

Patch Set 5 : fixing aura #

Patch Set 6 : forgot the not #

Patch Set 7 : Fixing select all and ime #

Patch Set 8 : fixed some bugs #

Patch Set 9 : formatting #

Patch Set 10 : rebase #

Patch Set 11 : fixes failing tests #

Patch Set 12 : moved contextmenuallowedscope #

Patch Set 13 : actually added ContextMenuAllowedScope #

Patch Set 14 : fixing include and core_export #

Patch Set 15 : Random fixes #

Patch Set 16 : Rebased #

Patch Set 17 : Another rebase #

Patch Set 18 : Changed Smart Selection #

Patch Set 19 : Rebase #

Patch Set 20 : Fixing rebase bug #

Total comments: 13

Patch Set 21 : fixed rebase issues #

Patch Set 22 : made dependent patch #

Patch Set 23 : random fixes #

Patch Set 24 : random fixes #

Patch Set 25 : fix tests #

Total comments: 8

Patch Set 26 : removed select all plumbing #

Patch Set 27 : fix rebase problems #

Total comments: 4

Patch Set 28 : nits and fix tests #

Total comments: 4

Patch Set 29 : Addressing bokan's comments #

Patch Set 30 : fixing renaming in test #

Patch Set 31 : fixing tests #

Total comments: 16

Patch Set 32 : Yosin's comments #

Patch Set 33 : fixing stylus selection #

Patch Set 34 : fixing rebase #

Patch Set 35 : fix rebase #

Patch Set 36 : fix rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+166 lines, -92 lines) Patch
M android_webview/browser/aw_web_contents_view_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/android/content_view_core_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +1 line, -1 line 0 comments Download
M content/browser/android/content_view_core_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 3 chunks +19 lines, -6 lines 0 comments Download
M content/browser/renderer_host/input/stylus_text_selector.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/input/stylus_text_selector.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/input/stylus_text_selector_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +3 lines, -1 line 0 comments Download
M content/browser/renderer_host/input/touch_selection_controller_client_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +6 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 3 chunks +3 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 2 chunks +8 lines, -3 lines 0 comments Download
M content/browser/web_contents/web_contents_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +2 lines, -1 line 0 comments Download
M content/child/assert_matching_enums.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +1 line, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +5 lines, -4 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 10 chunks +32 lines, -33 lines 0 comments Download
M content/public/browser/android/content_view_core.h View 1 chunk +1 line, -1 line 0 comments Download
M content/shell/browser/shell_web_contents_view_delegate_android.cc View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/events/hit-test-counts.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/events/hit-test-counts-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 3 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +10 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/core/editing/FrameSelection.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 2 chunks +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/editing/SelectionController.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/SelectionController.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 7 chunks +31 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/core/input/EventHandler.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +14 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/input/GestureManager.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/web/ContextMenuClientImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/web/WebViewImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 1 chunk +7 lines, -3 lines 0 comments Download
M third_party/WebKit/public/web/WebMenuSourceType.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +2 lines, -1 line 0 comments Download
M ui/base/ui_base_types.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +2 lines, -1 line 0 comments Download

Depends on Patchset:

Messages

Total messages: 169 (139 generated)
amaralp
PTAL
3 years, 8 months ago (2017-04-21 05:18:42 UTC) #80
boliu
On 2017/04/21 05:18:42, amaralp wrote: > PTAL For CL with many reviewers, you should specify ...
3 years, 8 months ago (2017-04-21 18:50:16 UTC) #83
boliu
https://codereview.chromium.org/2785853002/diff/380001/content/browser/renderer_host/input/touch_emulator_client.h File content/browser/renderer_host/input/touch_emulator_client.h (right): https://codereview.chromium.org/2785853002/diff/380001/content/browser/renderer_host/input/touch_emulator_client.h#newcode25 content/browser/renderer_host/input/touch_emulator_client.h:25: bool from_touch = false) = 0; default arg on ...
3 years, 8 months ago (2017-04-21 21:20:19 UTC) #84
aelias_OOO_until_Jul13
Nice cleanup overall. https://codereview.chromium.org/2785853002/diff/380001/content/browser/android/content_view_core_impl.cc File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2785853002/diff/380001/content/browser/android/content_view_core_impl.cc#newcode627 content/browser/android/content_view_core_impl.cc:627: params.from_touch; Is it possible to use ...
3 years, 8 months ago (2017-04-21 21:42:19 UTC) #85
Jinsuk Kim
Nice refactoring. FYI I'm working on https://crrev.com/2826303003 not to use content_view_core for selection events. Would ...
3 years, 8 months ago (2017-04-24 06:33:16 UTC) #86
Shimi Zhang
amaralp@, when could this CL land? If it still takes a while, I think I'll ...
3 years, 7 months ago (2017-05-22 18:23:33 UTC) #91
amaralp1
PTAL boliu@ - android_webview, content/browser/android, CVC.java/SPC.java, cvc.h, cvci.cc/h tedchoc@ - chrome_web_contents_view_delegate.cc, shell_web_contents_view_delegate_android.cc aelias@ - touch_selection_controller_client_aura.cc, ...
3 years, 7 months ago (2017-05-23 07:56:29 UTC) #101
boliu
I'll wait for blink to be approved first. Probably should update CL description to say ...
3 years, 7 months ago (2017-05-23 17:34:13 UTC) #102
aelias_OOO_until_Jul13
My OWNERS files lgtm, although I have a few comments on Source/core: https://codereview.chromium.org/2785853002/diff/480001/third_party/WebKit/Source/core/input/EventHandler.cpp File third_party/WebKit/Source/core/input/EventHandler.cpp ...
3 years, 7 months ago (2017-05-23 20:52:05 UTC) #106
boliu
lgtm % previous comments
3 years, 7 months ago (2017-05-23 21:18:21 UTC) #107
yosin_UTC9
https://codereview.chromium.org/2785853002/diff/520001/third_party/WebKit/Source/core/editing/FrameSelection.cpp File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/2785853002/diff/520001/third_party/WebKit/Source/core/editing/FrameSelection.cpp#newcode726 third_party/WebKit/Source/core/editing/FrameSelection.cpp:726: if (IsHandleVisible()) { It is OK to show context ...
3 years, 7 months ago (2017-05-24 04:51:23 UTC) #114
amaralp1
https://codereview.chromium.org/2785853002/diff/480001/content/browser/renderer_host/input/touch_selection_controller_client_aura.cc File content/browser/renderer_host/input/touch_selection_controller_client_aura.cc (right): https://codereview.chromium.org/2785853002/diff/480001/content/browser/renderer_host/input/touch_selection_controller_client_aura.cc#newcode169 content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:169: const bool from_touch = params.source_type == ui::MENU_SOURCE_LONG_PRESS || On ...
3 years, 7 months ago (2017-05-24 08:30:35 UTC) #119
amaralp1
3 years, 7 months ago (2017-05-24 08:30:39 UTC) #120
Ted C
On 2017/05/24 08:30:39, amaralp1 wrote: chrome_web_contents_view_delegate - lgtm
3 years, 7 months ago (2017-05-24 16:20:23 UTC) #123
Shimi Zhang
3 years, 7 months ago (2017-05-24 17:32:27 UTC) #124
amaralp1
bokan@ could you please take a look at core/input? Thanks!
3 years, 7 months ago (2017-05-24 17:52:42 UTC) #126
bokan
https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Source/core/input/EventHandler.cpp File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Source/core/input/EventHandler.cpp#newcode1813 third_party/WebKit/Source/core/input/EventHandler.cpp:1813: WebInputEventResult EventHandler::SendContextMenuEventForKey( This method should be renamed as it's ...
3 years, 7 months ago (2017-05-24 19:15:53 UTC) #127
amaralp
On 2017/05/24 at 19:15:53, bokan wrote: > https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Source/core/input/EventHandler.cpp > File third_party/WebKit/Source/core/input/EventHandler.cpp (right): > > https://codereview.chromium.org/2785853002/diff/540001/third_party/WebKit/Source/core/input/EventHandler.cpp#newcode1813 ...
3 years, 7 months ago (2017-05-25 02:49:52 UTC) #134
yosin_UTC9
https://codereview.chromium.org/2785853002/diff/600001/content/browser/android/content_view_core_impl.h File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/2785853002/diff/600001/content/browser/android/content_view_core_impl.h#newcode59 content/browser/android/content_view_core_impl.h:59: bool ShowSelectionMenu(const ContextMenuParams& params) override; It is better to ...
3 years, 7 months ago (2017-05-25 04:47:21 UTC) #137
amaralp1
https://codereview.chromium.org/2785853002/diff/600001/content/browser/android/content_view_core_impl.h File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/2785853002/diff/600001/content/browser/android/content_view_core_impl.h#newcode59 content/browser/android/content_view_core_impl.h:59: bool ShowSelectionMenu(const ContextMenuParams& params) override; On 2017/05/25 at 04:47:20, ...
3 years, 7 months ago (2017-05-25 07:11:56 UTC) #138
amaralp1
3 years, 7 months ago (2017-05-25 07:11:58 UTC) #139
bokan
Source/core and Source/web lgtm but please wait on yosin@ to approve overall editing logic.
3 years, 7 months ago (2017-05-25 14:26:54 UTC) #140
amaralp
aelias@, I found a problem with stylus selection and made some changes to stylus code ...
3 years, 6 months ago (2017-05-31 02:33:59 UTC) #153
aelias_OOO_until_Jul13
content/browser/renderer_host/input lgtm
3 years, 6 months ago (2017-05-31 03:02:58 UTC) #154
amaralp1
avi@ for content/child/assert_matching_enums.cc sadrul@ for ui/base/ui_base_types.h yosin@ could you take a look at the editing ...
3 years, 6 months ago (2017-05-31 04:40:45 UTC) #156
Avi (use Gerrit)
On 2017/05/31 04:40:45, amaralp1 wrote: > avi@ for content/child/assert_matching_enums.cc > sadrul@ for ui/base/ui_base_types.h > yosin@ ...
3 years, 6 months ago (2017-05-31 04:43:40 UTC) #157
yosin_UTC9
lgtm Sorry for late response. I missed... m(_ _)m
3 years, 6 months ago (2017-05-31 07:47:28 UTC) #158
sadrul
lgtm
3 years, 6 months ago (2017-06-01 15:52:39 UTC) #163
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2785853002/700001
3 years, 6 months ago (2017-06-01 17:20:53 UTC) #166
commit-bot: I haz the power
3 years, 6 months ago (2017-06-01 18:20:39 UTC) #169
Message was sent while issue was closed.
Committed patchset #36 (id:700001) as
https://chromium.googlesource.com/chromium/src/+/58d4d0b2d9511fe57bde3cbe82e4...

Powered by Google App Engine
This is Rietveld 408576698