New issue
Advanced search Search tips

Issue 840007 link

Starred by 1 user
Status: Fixed
Owner:
ctzsm@chromium.org
Closed: May 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

[Smart Selection] Context menu didn't show in some cases if there was a selection expansion.

Project Member Reported by ctzsm@chromium.org, May 4 2018 
Copied from b/78631924

==================================================================

adjustSelectionByCharacterOffset() from Java side reached renderer FrameInputHandlerImpl::AdjustSelectionBytCharacterOffset(), Blink WebLocalFrameImpl::SelectRange() successfully updated the selection range based on Smart Selection's suggestion, and then we are trying to trigger the selection menu.

In EventHandler::ShowNonLocatedContextMenu() we got the left selection bound's middle point as the new point for MouseEvent, so far so good, but when we call ContextMenuController#ShowContextMenu(), we did another hit test to get node according to the MouseEvent location. We got a wrong node because this Email content used a weird "zoom: 0.584615;" CSS property, which probably introduced a rounding error, so the "Hotel Address" <td> tag overlaps "2720 W. Valley Blvd" <td> tag a little bit, so we got the "Hotel Address" <td> element. Since this node is not the correct one, so it has no selection, if it's a non-editable mode and there is no selection, we don't need to show selection menu. So in the end, the Java side showSelectionMenu() isn't called.

For this specific issue, I think it should be won't fix from WebView/Chrome side, since zoom CSS property is a non-standard [1], not sure if Gmail or the original sender added it, but the usage of zoom should be remove/replaced.

However this bug raised a question about our current implementation of Smart Selection. We probably want to avoid the second hit test and get the current selection directly, after that we trigger showSelectionMenu(). Imagine that we have two divs, one overlaps the other, but the content of the div below the other will trigger selection expansion according to Smart Selection, so we will have the same issue in the end. See my attached HTML file and a video to illustrate this problem.

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/zoom
no-selection-menu.mp4
911 KB View Download
test2.html
263 bytes View Download
Project Member

Comment 1 by bugdroid1@chromium.org, May 8 2018 

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

commit b6fb8ae4f4e3c3af4e9d9b68038b8f67626c9f62
Author: Shimi Zhang <ctzsm@chromium.org>
Date: Tue May 08 17:37:59 2018

[Smart Selection] Set |selected_text| when |kMenuSourceAdjustSelection|

Java adjustSelectionByCharacterOffset() will be triggered if
|TextClassifier| API thinks we should adjust previous selection, the
renderer FrameInputHandlerImpl::AdjustSelectionByCharacterOffset() is
called with the need to show context menu later.

Eventually it will call EventHanlder::ShowNonLocatedContextMenu(),
which will give us the left selection bound's middle point as the new
point for MouseEvent.

In ContextMenuController#ShowContextMenu() there is another hit test
based on this point. In some cases, this point is under other HTML
element, so we won't get current selection text based on the hit test
result.

Later in SelectionPopupController::ShowSelectionMenu() we checked that
if the selection text is empty and this is non-editable element, we
won't call Java side SelectionPopupControllerImpl#showSelectionMenu().

Users are expecting context menu, but there won't be one.

In this CL, we set |data.selected_text| when |source_type| is
|kMenuSourceAdjustSelection|. Hit test isn't needed in this case.

Bug:  840007 , b/78631924
Change-Id: I9631773655300b2fc1dc5f84faa161093fae900f
Reviewed-on: https://chromium-review.googlesource.com/1045510
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Shimi Zhang <ctzsm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556856}
[modify] https://crrev.com/b6fb8ae4f4e3c3af4e9d9b68038b8f67626c9f62/third_party/blink/renderer/core/page/context_menu_controller.cc

Comment 2 by ctzsm@chromium.org, May 8 2018

Status: Fixed (was: Assigned)
►

Sign in to add a comment