New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 763201 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocked on:
issue 764588



Sign in to add a comment

Selection menu sometimes doesn't trigger

Project Member Reported by ctzsm@chromium.org, Sep 8 2017

Issue description

What steps will reproduce the problem?
(1) Smart select an entity.
(2) Repeat several times.

Sometimes selection menu doesn't show.

Logic of SmartSelectionCallback in SelectionPopupController.java is not very correct.
 

Comment 1 by toki@google.com, Sep 8 2017

Cc: toki@google.com

Comment 2 by ctzsm@chromium.org, Sep 13 2017

Blockedon: 764588

Comment 3 by ctzsm@chromium.org, Sep 13 2017

Cc: aelias@chromium.org
amaralp@, aelias@, I found that
1) when selection menu didn't trigger, there is only a SELECTION_HANDLES_SHOWN event triggered.
2) when selection menu shows, there are SELECTION_HANDLES_SHOWN and SELECTION_HANDLES_MOVED events.
Because we are depends on SELECTION_HANDLES_MOVED event to show the selection menu, so the bug happened.

I think there is a special case of Smart Selection we didn't handle correctly, when Smart Selection need to adjust selection range, sometimes this SELECTION_HANDLES_MOVED event somehow merged to previous SELECTION_HANDLES_SHOWN event IMO.

Is that possible somehow two events merged into one? If so, Could you please give guidance on this? Thanks!

Comment 4 by sgu...@chromium.org, Sep 27 2017

Cc: -sgu...@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37deeb2f390514b35a43225ed7d39ca2d51e9d3a

commit 37deeb2f390514b35a43225ed7d39ca2d51e9d3a
Author: Shimi Zhang <ctzsm@chromium.org>
Date: Thu Sep 28 00:59:01 2017

[Smart Selection] Fix selection menu flakiness

Current Smart Selection workflow:

1) When receiving showSelectionMenu() call in SelectionPopupController,
it asks SmartSelectionClient to do classification and suggestion.
2) A callback onClassified() in SelectionPopupController is called when
received classification/suggestion result.
3) onClassified() will call adjustSelectionByCharacterOffset() if
necessary.
4) When selection range expands, SelectionPopupController will receive
a onSelectionEvent() call, then it triggers selection menu if
SELECTION_HANDLES_MOVED event comes.

This workflow has one flaw since SELECTION_HANDLES_* events are not
very reliable for this use case. Because under certain condition the
original long press caused selection and the later selection expansion
event can be draw at once. In this case, there is only one
SELECTION_HANDLES_SHOWN event without a following
SELECTION_HANDLES_MOVED event will happen.

This CL changed 4), let adjustSelectionByCharacterOffset() triggers
another showSelectionMenu() call and handle the last selection menu
event from that point. It removes the dependency on
SELECTION_HANDLES_* events, therefore resolved the bug.

Bug:  763201 
Change-Id: I3688fbb0a31e3794486cc9eab162f4392ab6ff80
Reviewed-on: https://chromium-review.googlesource.com/669881
Commit-Queue: Shimi Zhang <ctzsm@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Donn Denman <donnd@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504844}
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/components/printing/test/print_render_frame_helper_browsertest.cc
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/browser/android/selection_popup_controller.cc
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/browser/frame_host/input/legacy_ipc_frame_input_handler.cc
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/browser/frame_host/input/legacy_ipc_frame_input_handler.h
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/browser/web_contents/web_contents_android.cc
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/browser/web_contents/web_contents_android.h
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/child/assert_matching_enums.cc
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/common/input/input_handler.mojom
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/common/input_messages.h
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/public/android/java/src/org/chromium/content/browser/input/LGEmailActionModeWorkaround.java
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/public/android/junit/src/org/chromium/content/browser/SelectionPopupControllerTest.java
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/public/browser/web_contents.h
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/renderer/input/frame_input_handler_impl.cc
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/renderer/input/frame_input_handler_impl.h
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/content/renderer/render_frame_impl.h
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/third_party/WebKit/Source/core/exported/WebInputMethodControllerImpl.cpp
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.cpp
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.h
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/third_party/WebKit/public/BUILD.gn
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/third_party/WebKit/public/platform/WebMenuSourceType.h
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/third_party/WebKit/public/web/WebLocalFrame.h
[add] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/third_party/WebKit/public/web/selection_menu_behavior.mojom
[modify] https://crrev.com/37deeb2f390514b35a43225ed7d39ca2d51e9d3a/ui/base/ui_base_types.h

Comment 6 by ctzsm@chromium.org, Sep 29 2017

Cc: changwan@chromium.org
Status: Fixed (was: Assigned)
This should be fixed.

Sign in to add a comment