Text selection popup menu doesn't dismiss in the case of IME clear function
Reported by
zuojingl...@xiaomi.com,
Mar 1 2018
|
||||||||||
Issue descriptionSteps 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:
,
Mar 5 2018
,
Mar 7 2018
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!
,
Mar 7 2018
This issue can be reproduced in both lasted stable and canary version. I have made a CL to fix this issue. https://chromium-review.googlesource.com/c/chromium/src/+/940290) Please take a look. The URL to download the 'baidu input method' is: http://downapp.baidu.com/baiduinput/AndroidPhone/8.0.5.6/1/1000d/20180207122013/baiduinput_AndroidPhone_8-0-5-6_1000d.apk?responseContentDisposition=attachment%3Bfilename%3D%22baiduinput_AndroidPhone_1000d.apk%22&responseContentType=application%2Fvnd.android.package-archive&request_id=1520406706_9672551305&type=static Thanks.
,
Mar 7 2018
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
,
Mar 7 2018
,
Mar 9 2018
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!
,
Mar 9 2018
Untriaging the issue as the issue is reproducible. Thanks!
,
Mar 19 2018
random ime behaving randomly..
,
Mar 28 2018
Does anyone know how this clear key works? Is it selecting everything in the input and then deleting?
,
Mar 29 2018
Yes, it is selecting everything in the input and then deleting.
,
Mar 29 2018
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.
,
Mar 29 2018
> 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?
,
Mar 29 2018
> 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.
,
Apr 3 2018
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?
,
Apr 3 2018
> 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.
,
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
,
May 9 2018
r555099 caused bug 841489 . PTAL. Also, is this bug truly Android-only? If so, why does the fix need to touch cross-platform code?
,
May 9 2018
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.
,
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
,
May 11 2018
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 |
||||||||||
Comment 1 by zuojingl...@xiaomi.com
, Mar 1 20189.0 MB
9.0 MB View Download