Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1102933003:
[Contextual Search] Add support for tap on the selection. (Closed)
|
Created:
5 years, 8 months ago by Donn Denman Modified:
5 years, 7 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, penghuang+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Contextual Search] Add support for tap on the selection.
Adds handling to show the selection handles for a tap or long-press
on an existing selection that does not yet show the selection handles.
This approach works fine when the selection does not wrap,
and it's currently very rare that we have a multi-line selection.
However, some day we'll have the ability to expand the selection,
so we should revisit this implementation then.
More info on this in issue 393025.
Based on CL 1086393003 by jdduke@.
BUG=chromium:393025, chromium:420281
Committed: https://crrev.com/3de51de0426a9063026d21e60072b1b6bed4a2df
Cr-Commit-Position: refs/heads/master@{#329542}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add helper method WillHandleTapOrLongPress(). #Patch Set 3 : 1st cut at unit tests #
Total comments: 2
Patch Set 4 : Added unit tests. #
Total comments: 6
Patch Set 5 : Updated unit tests as suggested by Jared. #
Total comments: 2
Patch Set 6 : Fixed unit tests and merged with force-update change. #
Total comments: 5
Patch Set 7 : Moved implementation methods to match declaration order. Used INACTIVE for active_status_ state ch… #
Messages
Total messages: 25 (3 generated)
donnd@chromium.org changed reviewers: + jdduke@chromium.org
Jared, I think we decided to just try to land this as-is. If that sounds right then I can work on unit tests tonight. Can you think of anything else that might be missing or wrong in this CL? TIA!
jdduke@chromium.org changed reviewers: + mohsen@chromium.org
This needs a test or two in touch_selection_controller_unittest.cc. +mohsen to verify that this is compatible with Aura behavior. https://chromiumcodereview.appspot.com/1102933003/diff/1/content/browser/rend... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://chromiumcodereview.appspot.com/1102933003/diff/1/content/browser/rend... content/browser/renderer_host/render_widget_host_view_android.cc:1646: const blink::WebGestureEvent& gesture_event = Hmm, this cast isn't safe for non-gesture events. You can do: if (selection_controller_ && WebInputEvent::isGestureEventTye(input_event.type)) { const blink::WebGestureEvent& gesture_event = ... } https://chromiumcodereview.appspot.com/1102933003/diff/1/content/browser/rend... content/browser/renderer_host/render_widget_host_view_android.cc:1650: if (selection_controller_->WillHandleLongPressEvent(gesture_location)) { Nit: No need for braces for this if conditional. https://chromiumcodereview.appspot.com/1102933003/diff/1/ui/touch_selection/t... File ui/touch_selection/touch_selection_controller.cc (right): https://chromiumcodereview.appspot.com/1102933003/diff/1/ui/touch_selection/t... ui/touch_selection/touch_selection_controller.cc:194: if (!is_selection_active_ && !is_insertion_active_) { Can we split this logic out into a shared function? if (WillHandleTapOrLongPress(location)) return true;
Jared, PTAL. Thanks! https://chromiumcodereview.appspot.com/1102933003/diff/1/content/browser/rend... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://chromiumcodereview.appspot.com/1102933003/diff/1/content/browser/rend... content/browser/renderer_host/render_widget_host_view_android.cc:1646: const blink::WebGestureEvent& gesture_event = On 2015/05/01 20:52:06, jdduke wrote: > Hmm, this cast isn't safe for non-gesture events. You can do: > > if (selection_controller_ && > WebInputEvent::isGestureEventTye(input_event.type)) { > const blink::WebGestureEvent& gesture_event = > ... > } Done. https://chromiumcodereview.appspot.com/1102933003/diff/1/content/browser/rend... content/browser/renderer_host/render_widget_host_view_android.cc:1650: if (selection_controller_->WillHandleLongPressEvent(gesture_location)) { On 2015/05/01 20:52:06, jdduke wrote: > Nit: No need for braces for this if conditional. Done. https://chromiumcodereview.appspot.com/1102933003/diff/1/ui/touch_selection/t... File ui/touch_selection/touch_selection_controller.cc (right): https://chromiumcodereview.appspot.com/1102933003/diff/1/ui/touch_selection/t... ui/touch_selection/touch_selection_controller.cc:194: if (!is_selection_active_ && !is_insertion_active_) { On 2015/05/01 20:52:06, jdduke wrote: > Can we split this logic out into a shared function? > > if (WillHandleTapOrLongPress(location)) > return true; Done.
On 2015/05/01 22:46:03, Donn Denman wrote: > Jared, PTAL. Thanks! > Still waiting on those tests.
On 2015/05/04 14:57:54, jdduke wrote: > On 2015/05/01 22:46:03, Donn Denman wrote: > > Jared, PTAL. Thanks! > > > > Still waiting on those tests. Jared, I started on the tests, but could use some quick help with two things: 1) Updated the existing tests (since they were heavy users of OnTouchEvent which changed its API), do you think this approach is fine? 2) I'm unsure how to establish a selection without handles. Am I doing the right kind of thing in my WIP test? Thanks!
On 2015/05/05 16:57:35, Donn Denman wrote: > Jared, I started on the tests, but could use some quick help with two things: > 1) Updated the existing tests (since they were heavy users of OnTouchEvent which > changed its API), do you think this approach is fine? Yup, looks good. > 2) I'm unsure how to establish a selection without handles. Am I doing the > right kind of thing in my WIP test? > See my comment. https://codereview.chromium.org/1102933003/diff/40001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/1102933003/diff/40001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller_unittest.cc:982: OnLongPressEvent(); Remove this call to |OnLongPressEvent()|, this will prevent the selection from activating.
Jared, I added tests, but not sure if it would be possible to make them all one or two tests instead of 4. Thanks for your help! PTAL. https://codereview.chromium.org/1102933003/diff/40001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/1102933003/diff/40001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller_unittest.cc:982: OnLongPressEvent(); On 2015/05/05 17:37:16, jdduke wrote: > Remove this call to |OnLongPressEvent()|, this will prevent the selection from > activating. Done.
https://codereview.chromium.org/1102933003/diff/60001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/1102933003/diff/60001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:171: activate_selection_automatically_ = false; This will need to be rebased. https://codereview.chromium.org/1102933003/diff/60001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/1102933003/diff/60001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller_unittest.cc:992: } I would go ahead and merge the 2 tap tests into one, and the 2 longpress tests into one. You basically would just have what you have here but: EXPECT_FALSE(...outer_point); EXPECT_THAT(GetAndResetEvents(), IsEmpty()); EXPECT_TRUE(...inner_point); EXPECT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_SHOWN); No need for the |changeselectiontofivefiftyten| helper.
https://codereview.chromium.org/1102933003/diff/60001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/1102933003/diff/60001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller_unittest.cc:992: } On 2015/05/08 22:05:52, jdduke wrote: > I would go ahead and merge the 2 tap tests into one, and the 2 longpress tests > into one. You basically would just have what you have here but: > > EXPECT_FALSE(...outer_point); > EXPECT_THAT(GetAndResetEvents(), IsEmpty()); > EXPECT_TRUE(...inner_point); > EXPECT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_SHOWN); > > No need for the |changeselectiontofivefiftyten| helper. Oh, and just call the tests HandlesShowOnTapInsideRect HandlesShowOnLongpressInsideRect
Jared, I know you're travelling, so don't feel any pressure to review this unless you want to. I think it's ready now, though I did have to add a call to HideAndDisallowShowingAutomatically() for the test of long-press but not to tap -- I guess the selection behaves differently. https://chromiumcodereview.appspot.com/1102933003/diff/60001/ui/touch_selecti... File ui/touch_selection/touch_selection_controller.cc (right): https://chromiumcodereview.appspot.com/1102933003/diff/60001/ui/touch_selecti... ui/touch_selection/touch_selection_controller.cc:171: activate_selection_automatically_ = false; On 2015/05/08 22:05:52, jdduke wrote: > This will need to be rebased. Done. https://chromiumcodereview.appspot.com/1102933003/diff/60001/ui/touch_selecti... File ui/touch_selection/touch_selection_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/1102933003/diff/60001/ui/touch_selecti... ui/touch_selection/touch_selection_controller_unittest.cc:992: } On 2015/05/08 22:07:04, jdduke wrote: > On 2015/05/08 22:05:52, jdduke wrote: > > I would go ahead and merge the 2 tap tests into one, and the 2 longpress tests > > into one. You basically would just have what you have here but: > > > > EXPECT_FALSE(...outer_point); > > EXPECT_THAT(GetAndResetEvents(), IsEmpty()); > > EXPECT_TRUE(...inner_point); > > EXPECT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_SHOWN); > > > > No need for the |changeselectiontofivefiftyten| helper. > > Oh, and just call the tests > > HandlesShowOnTapInsideRect > HandlesShowOnLongpressInsideRect Done. https://chromiumcodereview.appspot.com/1102933003/diff/60001/ui/touch_selecti... ui/touch_selection/touch_selection_controller_unittest.cc:992: } On 2015/05/08 22:05:52, jdduke wrote: > I would go ahead and merge the 2 tap tests into one, and the 2 longpress tests > into one. You basically would just have what you have here but: > > EXPECT_FALSE(...outer_point); > EXPECT_THAT(GetAndResetEvents(), IsEmpty()); > EXPECT_TRUE(...inner_point); > EXPECT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_SHOWN); > > No need for the |changeselectiontofivefiftyten| helper. Done.
lgtm with test nit. https://chromiumcodereview.appspot.com/1102933003/diff/80001/ui/touch_selecti... File ui/touch_selection/touch_selection_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/1102933003/diff/80001/ui/touch_selecti... ui/touch_selection/touch_selection_controller_unittest.cc:998: ChangeSelection(start_rect, visible, end_rect, visible); I think you can remove this |ChangeSelection| line, same in the longpress test. https://chromiumcodereview.appspot.com/1102933003/diff/80001/ui/touch_selecti... ui/touch_selection/touch_selection_controller_unittest.cc:1020: // Reset the selection. Hmm, why reset? It should be a simple matter of testing outside the rect, then inside the rect, no need to reset the selection and establish a new one. I would remove the HideAndDisallowShowingAutomatically() call here and also the ChangeSelection call below.
On 2015/05/11 21:32:18, jdduke wrote: https://chromiumcodereview.appspot.com/1102933003/diff/80001/ui/touch_selecti... > ui/touch_selection/touch_selection_controller_unittest.cc:1020: // Reset the > selection. > Hmm, why reset? It should be a simple matter of testing outside the rect, then > inside the rect, no need to reset the selection and establish a new one. I would > remove the HideAndDisallowShowingAutomatically() call here and also the > ChangeSelection call below. Oh dang I didn't read the first line of your reply... Hmm, sounds like we might have a bug then?
On 2015/05/11 21:33:02, jdduke wrote: > On 2015/05/11 21:32:18, jdduke wrote: > https://chromiumcodereview.appspot.com/1102933003/diff/80001/ui/touch_selecti... > > ui/touch_selection/touch_selection_controller_unittest.cc:1020: // Reset the > > selection. > > Hmm, why reset? It should be a simple matter of testing outside the rect, then > > inside the rect, no need to reset the selection and establish a new one. I > would > > remove the HideAndDisallowShowingAutomatically() call here and also the > > ChangeSelection call below. > > Oh dang I didn't read the first line of your reply... Hmm, sounds like we might > have a bug then? I don't think you should have to call HideAndDisallowShowingAutomatically (just as a reminder to me, not lgtm until we fix that).
On 2015/05/11 21:34:11, jdduke wrote: > On 2015/05/11 21:33:02, jdduke wrote: > > On 2015/05/11 21:32:18, jdduke wrote: > > > https://chromiumcodereview.appspot.com/1102933003/diff/80001/ui/touch_selecti... > > > ui/touch_selection/touch_selection_controller_unittest.cc:1020: // Reset the > > > selection. > > > Hmm, why reset? It should be a simple matter of testing outside the rect, > then > > > inside the rect, no need to reset the selection and establish a new one. I > > would > > > remove the HideAndDisallowShowingAutomatically() call here and also the > > > ChangeSelection call below. > > > > Oh dang I didn't read the first line of your reply... Hmm, sounds like we > might > > have a bug then? > > I don't think you should have to call HideAndDisallowShowingAutomatically (just > as a reminder to me, not lgtm until we fix that). OK, looks like you exposed a real bug here :) Thanks! Fix is here: https://codereview.chromium.org/1127383007/.
On 2015/05/11 21:53:30, jdduke wrote: > On 2015/05/11 21:34:11, jdduke wrote: > > On 2015/05/11 21:33:02, jdduke wrote: > > > On 2015/05/11 21:32:18, jdduke wrote: > > > > > > https://chromiumcodereview.appspot.com/1102933003/diff/80001/ui/touch_selecti... > > > > ui/touch_selection/touch_selection_controller_unittest.cc:1020: // Reset > the > > > > selection. > > > > Hmm, why reset? It should be a simple matter of testing outside the rect, > > then > > > > inside the rect, no need to reset the selection and establish a new one. I > > > would > > > > remove the HideAndDisallowShowingAutomatically() call here and also the > > > > ChangeSelection call below. > > > > > > Oh dang I didn't read the first line of your reply... Hmm, sounds like we > > might > > > have a bug then? > > > > I don't think you should have to call HideAndDisallowShowingAutomatically > (just > > as a reminder to me, not lgtm until we fix that). > > OK, looks like you exposed a real bug here :) Thanks! Fix is here: > https://codereview.chromium.org/1127383007/. Jared, ready for another review, and please note my comment in touch_selection_controller.cc. Thanks!
Here's that comment. https://chromiumcodereview.appspot.com/1102933003/diff/100001/ui/touch_select... File ui/touch_selection/touch_selection_controller.cc (right): https://chromiumcodereview.appspot.com/1102933003/diff/100001/ui/touch_select... ui/touch_selection/touch_selection_controller.cc:189: if (active_status_ != SELECTION_ACTIVE && Jared, please double-check this conditional -- I needed to rework it to adapt for recent changes.
https://chromiumcodereview.appspot.com/1102933003/diff/100001/ui/touch_select... File ui/touch_selection/touch_selection_controller.cc (right): https://chromiumcodereview.appspot.com/1102933003/diff/100001/ui/touch_select... ui/touch_selection/touch_selection_controller.cc:182: bool TouchSelectionController::WillHandleTapOrLongPress( Nit: The implementation order should match the declaration order. Please move this method implementation to where it should go relative to how it's declared in the header. https://chromiumcodereview.appspot.com/1102933003/diff/100001/ui/touch_select... ui/touch_selection/touch_selection_controller.cc:189: if (active_status_ != SELECTION_ACTIVE && On 2015/05/12 21:42:49, Donn Denman wrote: > Jared, please double-check this conditional -- I needed to rework it to adapt > for recent changes. This can just be if (active_status_ == INACTIVE && ...
Jared, PTAL. Thanks! https://chromiumcodereview.appspot.com/1102933003/diff/100001/ui/touch_select... File ui/touch_selection/touch_selection_controller.cc (right): https://chromiumcodereview.appspot.com/1102933003/diff/100001/ui/touch_select... ui/touch_selection/touch_selection_controller.cc:182: bool TouchSelectionController::WillHandleTapOrLongPress( On 2015/05/12 22:33:01, jdduke wrote: > Nit: The implementation order should match the declaration order. Please move > this method implementation to where it should go relative to how it's declared > in the header. Oh, didn't know that! Done. https://chromiumcodereview.appspot.com/1102933003/diff/100001/ui/touch_select... ui/touch_selection/touch_selection_controller.cc:189: if (active_status_ != SELECTION_ACTIVE && On 2015/05/12 22:33:00, jdduke wrote: > On 2015/05/12 21:42:49, Donn Denman wrote: > > Jared, please double-check this conditional -- I needed to rework it to adapt > > for recent changes. > > This can just be > > if (active_status_ == INACTIVE && > ... Done. Noticed we don't need separate nested if's, so consolidated those.
lgtm!
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1102933003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3de51de0426a9063026d21e60072b1b6bed4a2df Cr-Commit-Position: refs/heads/master@{#329542} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
