Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2626903004:
Don't show the virtual keyboard if there's no text input client (Closed)
|
Created:
3 years, 11 months ago by afakhry Modified:
3 years, 11 months ago Reviewers:
sadrul CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't show the virtual keyboard if there's no text input client
If a mousedown event was set to preventDefault() and the focus was
in a text input field, it won't blur the focus from the text input
field, and the following mouseup event will update the TextInputState of
the widget with the same TextInputType and ShowIme::IF_NEEDED.
If this is a minimize button of an app for example, the widget will hide,
but the later received IPC msg to UpdateTextInputState() will cause the
virtual keyboard to show up.
We need to make sure that the virtual keyboard is not shown as a result of
UpdateTextInputState() unless there's a valid text input client.
BUG=430026
TEST=Enable the virtual keyboard from settings. Open feedback app
(Alt+Shift+i) and minimize it. The virtual keyboard should not show.
Review-Url: https://codereview.chromium.org/2626903004
Cr-Commit-Position: refs/heads/master@{#443141}
Committed: https://chromium.googlesource.com/chromium/src/+/e850e3f2f671adc21ef92570dea76f9146b19e7a
Patch Set 1 #
Total comments: 4
Patch Set 2 : Solution 2 #Messages
Total messages: 18 (10 generated)
afakhry@chromium.org changed reviewers: + sadrul@chromium.org
Sadrul, could you please review this CL? Thanks!
https://codereview.chromium.org/2626903004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2626903004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:2306: GetInputMethod()->GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) { Two alternate options that I think would be better: 1. Move this check into InputMethod itself. 2. Instead of checking the input-type, check that |this| is the current input-client. This seems to be what ARC does as well [*]. The first option is probably going to be a bit tedious, because we have a few InputMethod implementations, and you probably have to update InputMethodObserver::OnShowImeIfNeeded() implementations too. The second option would be fairly easy, and be consistent with what we do for ARC. We could unify both farther with an API change like: 3. Change InputMethod::ShowImeIfNeeded() to InputMethod::ShowImeIfNeeded(TextInputClient*), so that the input-client lets the IME know what client is requesting the keyboard to be made visible. The IME can then make sure it's a valid input-client, and also that the input-client is the currently active one. [*] : https://cs.chromium.org/chromium/src/components/arc/ime/arc_ime_service.cc?q=...
https://codereview.chromium.org/2626903004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2626903004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:2306: GetInputMethod()->GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) { On 2017/01/11 02:46:06, sadrul wrote: > Two alternate options that I think would be better: > > 1. Move this check into InputMethod itself. > 2. Instead of checking the input-type, check that |this| is the current > input-client. This seems to be what ARC does as well [*]. > > The first option is probably going to be a bit tedious, because we have a few > InputMethod implementations, and you probably have to update > InputMethodObserver::OnShowImeIfNeeded() implementations too. The second option > would be fairly easy, and be consistent with what we do for ARC. > > We could unify both farther with an API change like: > 3. Change InputMethod::ShowImeIfNeeded() to > InputMethod::ShowImeIfNeeded(TextInputClient*), so that the input-client lets > the IME know what client is requesting the keyboard to be made visible. The IME > can then make sure it's a valid input-client, and also that the input-client is > the currently active one. > > [*] : > https://cs.chromium.org/chromium/src/components/arc/ime/arc_ime_service.cc?q=... Thank you very much for the detailed suggestions! For suggestion (2), |this| (which is the RenderWidgetHostViewAura) is never the current input-client. So this solution breaks showing the virtual keyboard inside text fields and text areas in the webcontents. For suggestion (1), when you said "you probably have to update InputMethodObserver::OnShowImeIfNeeded() implementations too", did you mean to forward the current input-client to the observers and let them decide? I was thinking more like: auto* client = GetTextInputClient(); if (client && client->GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) { for (InputMethodObserver& observer : observer_list_) observer.OnShowImeIfNeeded(); }
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2626903004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2626903004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:2306: GetInputMethod()->GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) { On 2017/01/11 17:50:49, afakhry wrote: > On 2017/01/11 02:46:06, sadrul wrote: > > Two alternate options that I think would be better: > > > > 1. Move this check into InputMethod itself. > > 2. Instead of checking the input-type, check that |this| is the current > > input-client. This seems to be what ARC does as well [*]. > > > > The first option is probably going to be a bit tedious, because we have a few > > InputMethod implementations, and you probably have to update > > InputMethodObserver::OnShowImeIfNeeded() implementations too. The second > option > > would be fairly easy, and be consistent with what we do for ARC. > > > > We could unify both farther with an API change like: > > 3. Change InputMethod::ShowImeIfNeeded() to > > InputMethod::ShowImeIfNeeded(TextInputClient*), so that the input-client lets > > the IME know what client is requesting the keyboard to be made visible. The > IME > > can then make sure it's a valid input-client, and also that the input-client > is > > the currently active one. > > > > [*] : > > > https://cs.chromium.org/chromium/src/components/arc/ime/arc_ime_service.cc?q=... > > > Thank you very much for the detailed suggestions! > > For suggestion (2), |this| (which is the RenderWidgetHostViewAura) is never the > current input-client. This is unexpected to me. RenderWidgetHostViewAura is-a TextInputClient. When it has focus, what is the 'current' input client? > So this solution breaks showing the virtual keyboard > inside text fields and text areas in the webcontents. > > For suggestion (1), when you said "you probably have to update > InputMethodObserver::OnShowImeIfNeeded() implementations too", did you mean to > forward the current input-client to the observers and let them decide? Yes. > I was > thinking more like: > > auto* client = GetTextInputClient(); > if (client && client->GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) { > for (InputMethodObserver& observer : observer_list_) > observer.OnShowImeIfNeeded(); > } > I guess this would work too.
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
https://codereview.chromium.org/2626903004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2626903004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:2306: GetInputMethod()->GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) { On 2017/01/11 17:53:27, sadrul wrote: > On 2017/01/11 17:50:49, afakhry wrote: > > On 2017/01/11 02:46:06, sadrul wrote: > > > Two alternate options that I think would be better: > > > > > > 1. Move this check into InputMethod itself. > > > 2. Instead of checking the input-type, check that |this| is the current > > > input-client. This seems to be what ARC does as well [*]. > > > > > > The first option is probably going to be a bit tedious, because we have a > few > > > InputMethod implementations, and you probably have to update > > > InputMethodObserver::OnShowImeIfNeeded() implementations too. The second > > option > > > would be fairly easy, and be consistent with what we do for ARC. > > > > > > We could unify both farther with an API change like: > > > 3. Change InputMethod::ShowImeIfNeeded() to > > > InputMethod::ShowImeIfNeeded(TextInputClient*), so that the input-client > lets > > > the IME know what client is requesting the keyboard to be made visible. The > > IME > > > can then make sure it's a valid input-client, and also that the input-client > > is > > > the currently active one. > > > > > > [*] : > > > > > > https://cs.chromium.org/chromium/src/components/arc/ime/arc_ime_service.cc?q=... > > > > > > Thank you very much for the detailed suggestions! > > > > For suggestion (2), |this| (which is the RenderWidgetHostViewAura) is never > the > > current input-client. > > This is unexpected to me. RenderWidgetHostViewAura is-a TextInputClient. When it > has focus, what is the 'current' input client? > Oops, sorry, I was looking at the *raw* pointer values without casting. Indeed |this| == GetTextInputClient(). In this case, I opted for solution (2). > > So this solution breaks showing the virtual keyboard > > inside text fields and text areas in the webcontents. > > > > For suggestion (1), when you said "you probably have to update > > InputMethodObserver::OnShowImeIfNeeded() implementations too", did you mean to > > forward the current input-client to the observers and let them decide? > > Yes. > > > I was > > thinking more like: > > > > auto* client = GetTextInputClient(); > > if (client && client->GetTextInputType() != ui::TEXT_INPUT_TYPE_NONE) { > > for (InputMethodObserver& observer : observer_list_) > > observer.OnShowImeIfNeeded(); > > } > > > > I guess this would work too.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by afakhry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484190254117120,
"parent_rev": "73fbd9b3b88c3f8b73230861924e6e680231fcbf", "commit_rev":
"e850e3f2f671adc21ef92570dea76f9146b19e7a"}
Message was sent while issue was closed.
Description was changed from ========== Don't show the virtual keyboard if there's no text input client If a mousedown event was set to preventDefault() and the focus was in a text input field, it won't blur the focus from the text input field, and the following mouseup event will update the TextInputState of the widget with the same TextInputType and ShowIme::IF_NEEDED. If this is a minimize button of an app for example, the widget will hide, but the later received IPC msg to UpdateTextInputState() will cause the virtual keyboard to show up. We need to make sure that the virtual keyboard is not shown as a result of UpdateTextInputState() unless there's a valid text input client. BUG=430026 TEST=Enable the virtual keyboard from settings. Open feedback app (Alt+Shift+i) and minimize it. The virtual keyboard should not show. ========== to ========== Don't show the virtual keyboard if there's no text input client If a mousedown event was set to preventDefault() and the focus was in a text input field, it won't blur the focus from the text input field, and the following mouseup event will update the TextInputState of the widget with the same TextInputType and ShowIme::IF_NEEDED. If this is a minimize button of an app for example, the widget will hide, but the later received IPC msg to UpdateTextInputState() will cause the virtual keyboard to show up. We need to make sure that the virtual keyboard is not shown as a result of UpdateTextInputState() unless there's a valid text input client. BUG=430026 TEST=Enable the virtual keyboard from settings. Open feedback app (Alt+Shift+i) and minimize it. The virtual keyboard should not show. Review-Url: https://codereview.chromium.org/2626903004 Cr-Commit-Position: refs/heads/master@{#443141} Committed: https://chromium.googlesource.com/chromium/src/+/e850e3f2f671adc21ef92570dea7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e850e3f2f671adc21ef92570dea7... |
