New issue
Advanced search Search tips

Issue 817712 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocked on:
issue 789907



Sign in to add a comment

Text selection popup menu doesn't dismiss in the case of IME clear function

Reported by zuojingl...@xiaomi.com, Mar 1 2018

Issue description

Steps to reproduce the problem:
1. Intall 'baidu input method' and select the skin as the demo.mp4 shows
2. Open url:https://www.google.com
3. Enter a few characters to the search box in google.com 
4. Click the the search box
5. Slide down the 'clear key' (aka 'o/p' key) in the baidu input.

What is the expected behavior?
The text selection popup menu should dismiss.

What went wrong?
Text selection popup menu doesn't dismiss.

Did this work before? N/A 

Chrome version: trunk  Channel: canary
OS Version: 
Flash Version:
 
demo.mp4
9.0 MB View Download
Labels: Needs-triage-Mobile
Cc: pnangunoori@chromium.org
Components: UI>Input>Text
Labels: Triaged-Mobile Needs-Feedback
zuojinglong@ - Could you please help by providing the URL to install the 'baidu input method' keyboard. If you are using Canary version of Chrome, please verify by updating your Canary to latest #67.0.3363.3 and Stable #65.0.3325.109.

If the issue is still reproduced, please share the device details where the issue is reproduced.

Thanks in advance!

Comment 4 Deleted

Project Member

Comment 6 by sheriffbot@chromium.org, Mar 7 2018

Labels: -Needs-Feedback
Thank you for providing more feedback. Adding the requester to the cc list.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: shouqun@chromium.org
Tested on  Redmi Note 3 Build/MMB29M using latest Chrome #64.0.3282.137 and Canary #67.0.3364.3 and able to reproduce the issue as the screen cast provided in C#1 using the Baidu Input method in C#5. 

Requesting Dev UI>Input>Text team to look into the CL provided in Comment #5 and let us know if the bisect is still required. If required, we can help in providing the bisect.

Thanks!
Status: Untriaged (was: Unconfirmed)
Untriaging the issue as the issue is reproducible.

Thanks!

Comment 10 by boliu@chromium.org, Mar 19 2018

Cc: changwan@chromium.org
random ime behaving randomly..
Cc: amaralp@chromium.org
Does anyone know how this clear key works? Is it selecting everything in the input and then deleting?
Yes, it is selecting everything in the input and then deleting.
Blockedon: 789907
So I think I know what is happening. When the IME selects the entire input it triggers the floating text selection menu and makes a selection with touch handles. Then it clears the selection. The problem is that the creating and clearing of the selection happen so quickly that they are coalesced into one compositor frame and so we never get a SELECTION_HANDLES_SHOWN or (more importantly) SELECTION_HANDLES_CLEARED events. This means that the selection menu is never cleared since it is normally cleared on the SELECTION_HANDLES_CLEARED event.
Cc: ctzsm@chromium.org
Owner: amaralp@chromium.org
Status: Assigned (was: Untriaged)
> the creating and clearing of the selection happen so quickly that they are
> coalesced into one compositor frame

This just seems plain wrong. Where do we do this? Could you take a look?
> This just seems plain wrong. Where do we do this? Could you take a look?

I think it is common for IME to change selections and then only the final selection is submitted to the compositor frame. One example is |InputMethodController::DeleteSurroundingText()|. Here is the sequence of events that I think are happening in this bug:

1) The clear key is pressed on the keyboard which triggers |DeleteSurroundingText()|.
2) |DeleteSurroundingText()| selects the text that it will delete. Since there was a touch selection handle present before the text selection the soon to be deleted range selection also has handles. A range selection with handles triggers the floating selection menu by sending a context menu event to the browser.
3) |DeleteSurroundingText()| deletes current selection.
4) The context menu event gets propagated to the browser through |RenderFrameImpl::ShowDeferredContextMenu()|. (At this point |selection_text_.empty()| is true so zuojinglong@'s patch prevents the selection menu from being shown).
5) The context menu event reaches the browser and the selection menu is shown.
6) The compositor frame is submitted with no selection since the selection was deleted. This puts us in an incorrect state where the selection menu is shown but there are no handles.

In my opinion there are two issues here:

1) I don't think the IME should be changing the FrameSelection in the intermediate selection. Ideally, the IME should delete the text without having to make the range selection.
2) The selection menu shouldn't be shown until the corresponding selection is received by the browser. This would fix this issue since the intermediate range selection never makes it to the browser and so the selection menu would never be shown.

I'm currently working on #2.
Yes, 1) is a known issue.

For 2), would it be also possible to prevent from coalescing SELECTION_HANDLES_SHOWN and SELECTION_HANDLES_CLEARED into one message? Where exactly does this happen?
> For 2), would it be also possible to prevent from coalescing SELECTION_HANDLES_SHOWN and SELECTION_HANDLES_CLEARED into one message? Where exactly does this happen?

Coalescing was a bad word choice on my part. There is no coalescing of selection messages. Actually no SELECTION_HANDLES_* messages are triggered in the scenario I outlined. A frame with the handles shown is never sent to the browser. This means browser doesn't know the intermediate IME selection ever happened so there is no SELECTION_HANDLES_SHOWN or SELECTION_HANDLES_CLEARED. What I was saying was that if the intermediate range selection lasted long enough to be submitted with a frame then the browser would get a frame with the range selection and handles. This would trigger SELECTION_HANDLES_SHOWN. Then the browser gets a second frame after the selection is deleted. This frame has no selection so the SELECTION_HANDLES_CLEARED is sent. The SELECTION_HANDLES_CLEARED would destroy the selection menu and there would be no issues. The issue here is that the range selection is created and then deleted immediately so a frame is submitted only after the selection is deleted. The underlying problem is that we assumed (incorrectly) that any frame selection change in Blink would be propagated to the browser through frame submissions. This isn't the case when two selection changes happen in quick succession like in IME.
Project Member

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

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

commit e9c5bc10a79fbdb0a6e215cb77491ffb7c93a855
Author: Jinglong Zuo <zuojinglong@xiaomi.com>
Date: Tue May 01 17:47:19 2018

Don't show text selection popup menu if current selected text is cleared.

If text selection clear action is just after selecting all text action in editable input box,
then the state of text selection in renderer is not correct, which causes the selection popup
menu will be incorrectly shown.
Fix this issue by checking selection state before popup text selection menu.

Bug: 817712

Change-Id: I4dcda47d5f0fe5f20febc2e1def30574c6060724
Reviewed-on: https://chromium-review.googlesource.com/940290
Commit-Queue: Antoine Labour <piman@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Reviewed-by: Pedro Amaral <amaralp@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555099}
[modify] https://crrev.com/e9c5bc10a79fbdb0a6e215cb77491ffb7c93a855/AUTHORS
[modify] https://crrev.com/e9c5bc10a79fbdb0a6e215cb77491ffb7c93a855/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/e9c5bc10a79fbdb0a6e215cb77491ffb7c93a855/content/renderer/render_view_browsertest.cc

r555099 caused  bug 841489 . PTAL. Also, is this bug truly Android-only? If so, why does the fix need to touch cross-platform code?
I think we might want to revert the patch. It's a hack and I was able to reproduce the original bug (although only once is many tries) so there still
is a race condition.

Does anyone know how this "clear button" works? Do we have a key event that
clears the input? Or can the app use javascript to do a SelectAll and then
delete?

> Also, is this bug truly Android-only? If so, why does the fix need to touch cross-platform code?

This bug is more of a symptom of an underlying problem. So far it has mostly
manifested itself in Android IME.
Project Member

Comment 21 by bugdroid1@chromium.org, May 10 2018

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

commit cc7bafbb63f838eb80ce3ef498dcc82f994cb2c0
Author: Changwan Ryu <changwan@chromium.org>
Date: Thu May 10 19:13:13 2018

Revert "Don't show text selection popup menu if current selected text is cleared."

This reverts commit e9c5bc10a79fbdb0a6e215cb77491ffb7c93a855.

Reason for revert: caused  crbug.com/841489 

Original change's description:
> Don't show text selection popup menu if current selected text is cleared.
> 
> If text selection clear action is just after selecting all text action in editable input box,
> then the state of text selection in renderer is not correct, which causes the selection popup
> menu will be incorrectly shown.
> Fix this issue by checking selection state before popup text selection menu.
> 
> Bug: 817712
> 
> Change-Id: I4dcda47d5f0fe5f20febc2e1def30574c6060724
> Reviewed-on: https://chromium-review.googlesource.com/940290
> Commit-Queue: Antoine Labour <piman@chromium.org>
> Reviewed-by: Changwan Ryu <changwan@chromium.org>
> Reviewed-by: Pedro Amaral <amaralp@chromium.org>
> Reviewed-by: Antoine Labour <piman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#555099}

TBR=boliu@chromium.org,haraken@chromium.org,changwan@chromium.org,piman@chromium.org,amaralp@chromium.org,zuojinglong@xiaomi.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 817712
Change-Id: I97f01cd58f23939066a2179a819d20534a60123f
Reviewed-on: https://chromium-review.googlesource.com/1054112
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Changwan Ryu <changwan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557617}
[modify] https://crrev.com/cc7bafbb63f838eb80ce3ef498dcc82f994cb2c0/AUTHORS
[modify] https://crrev.com/cc7bafbb63f838eb80ce3ef498dcc82f994cb2c0/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/cc7bafbb63f838eb80ce3ef498dcc82f994cb2c0/content/renderer/render_view_browsertest.cc

Comment 22 Deleted

amaralp@, the "clear button", first send 'InputMsg_SelectAll' message, and then
send a WebKeyboardEvent whose windows_key_code is ui::VKEY_BACK and native_key_code is AKEYCODE_DEL. And then TypingCommand::DeleteKeyPressed will be called to delete the selection.

thestig@, sorry to reply later, the original issue is caused by the specify skin in the 'baidu input method' on android platform, so I think the original issue occurs on Android-only. 
Maybe the patch as the following is beter.
#if defined(OS_ANDROID)
  if (selection_text_.empty() && !params.selection_text.empty())
    return;
#endif

Sign in to add a comment