Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1388283002:
Fix OSK flickering issue (Closed)
|
Created:
5 years, 2 months ago by Changwan Ryu Modified:
5 years, 1 month ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix OSK flickering issue
On Android, on-screen keyboard will flicker when you move focus from
one input form to another. We have a delayed OSK dismissal hack, but
we found that this is not a good solution for three reasons:
1) Flickering still happens, even though it's less severe.
2) Hiding of OSK will be delayed when touching a non-editable web node.
3) It makes keyboard showing & hiding very difficult to test.
The main cause of the flickering is that we call UpdateTextInputState()
in the middle of focus change:
- FocusController::setFocusedElement()
- FocusController::clearSelectionIfNecessary()
- FrameSelection::clear()
- Editor::notifyComponentsOnChangedSelection()
- ...
- RenderWidget::UpdateTextInputState()
- WebViewImpl::textInputInfo()
This can be avoided if we guard non-keyboard events in OnHandleInputType.
Also because focuschange (which should result in state update) can happen
through / not through OnHandleInputType, I made
StartHandlingImeEvent and FinishHandlingImeEvent nestable.
BUG=484139, 542619
Committed: https://crrev.com/f2a707b1d5492b35833bee9945c197595ba11d89
Cr-Commit-Position: refs/heads/master@{#357056}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 13
Patch Set 6 : addressed comments for imeadapter and removed platform guard #Patch Set 7 : fix more tests except for contextualsearchmanagertests #
Total comments: 6
Patch Set 8 : switched to ime guard approach #
Total comments: 4
Patch Set 9 : fixed tests and changed some names #Patch Set 10 : added OS_ANDROID guard #Patch Set 11 : rebased #
Total comments: 3
Patch Set 12 : refactor variables into imeeventguard #Patch Set 13 : fix a small logic error #Patch Set 14 : rebased #
Total comments: 4
Patch Set 15 : fixed a nit and updated comment #Messages
Total messages: 34 (9 generated)
aelias@chromium.org changed reviewers: + aelias@chromium.org
https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:202: // TODO(changwan): check if we can remove this line. Please attempt to remove it now and come to a conclusion on whether it's needed or not, there's no reason to leave this question open as a TODO. https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:214: // We will try to show keyboard even when text input type hasn't changed in case Can we just early-return and do nothing in the type-unchanged case, by guaranteeing that "the user has dismissed the keyboard" always results in mTextInputType getting set to TextInputType.NONE? What is the case this is working around? https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:237: mInputMethodManagerWrapper.restartInput(mViewEmbedder.getAttachedView()); This line and the following two (mTextInput* clearing) don't seem to belong here anymore, can they be either deleted or moved elsewhere? Let's make this method simply attach the native object. https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:415: if (mTextInputType != TextInputType.NONE && mInputConnection != null && isEditable) { Is this change needed? I don't see how it's useful. (If this is purely an "optimization", let's not make this change. I don't like increasing code complexity without having a measurable performance effect.) https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:450: mTextInputType = TextInputType.NONE; I don't see why detaching the native object should have an effect on the text input type, can this line be removed? (Possibly a few more "if (mNativeImeAdapterAndroid == 0) return;" lines need to be added elsewhere to compensate.) https://codereview.chromium.org/1388283002/diff/80001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1388283002/diff/80001/content/renderer/render... content/renderer/render_widget.cc:1894: #if defined(OS_ANDROID) I don't think we should make this kind of change behind a platform branch, it makes the flow too hard to reason about. Let's try to ship the task posting approach on all platforms.
https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:202: // TODO(changwan): check if we can remove this line. On 2015/10/15 19:58:40, aelias wrote: > Please attempt to remove it now and come to a conclusion on whether it's needed > or not, there's no reason to leave this question open as a TODO. Removed. https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:214: // We will try to show keyboard even when text input type hasn't changed in case On 2015/10/15 19:58:40, aelias wrote: > Can we just early-return and do nothing in the type-unchanged case, by > guaranteeing that "the user has dismissed the keyboard" always results in > mTextInputType getting set to TextInputType.NONE? What is the case this is > working around? There is no Android API to get notified of user's OSK dismissal, so we cannot set mTextInputType to NONE when it happens. I've updated the comment. https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:237: mInputMethodManagerWrapper.restartInput(mViewEmbedder.getAttachedView()); On 2015/10/15 19:58:40, aelias wrote: > This line and the following two (mTextInput* clearing) don't seem to belong here > anymore, can they be either deleted or moved elsewhere? Let's make this method > simply attach the native object. Removed. Also removed restartInput() which was accidentally added. https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:415: if (mTextInputType != TextInputType.NONE && mInputConnection != null && isEditable) { On 2015/10/15 19:58:40, aelias wrote: > Is this change needed? I don't see how it's useful. (If this is purely an > "optimization", let's not make this change. I don't like increasing code > complexity without having a measurable performance effect.) This will have a performance effect, but more importantly we should try to avoid creating a new input connection when type is NONE. I've moved the logic to restartInputIfNotHidden(). https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:450: mTextInputType = TextInputType.NONE; On 2015/10/15 19:58:40, aelias wrote: > I don't see why detaching the native object should have an effect on the text > input type, can this line be removed? (Possibly a few more "if > (mNativeImeAdapterAndroid == 0) return;" lines need to be added elsewhere to > compensate.) Removed here and added more null checks when calling native functions. https://codereview.chromium.org/1388283002/diff/80001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1388283002/diff/80001/content/renderer/render... content/renderer/render_widget.cc:1894: #if defined(OS_ANDROID) On 2015/10/15 19:58:40, aelias wrote: > I don't think we should make this kind of change behind a platform branch, it > makes the flow too hard to reason about. Let's try to ship the task posting > approach on all platforms. Done.
changwan@chromium.org changed reviewers: + aurimas@chromium.org
Looks like it caused test failures in RenderViewImplTest. Please address them and ping again when they're fixed.
On 2015/10/16 03:06:31, aelias wrote: > Looks like it caused test failures in RenderViewImplTest. Please address them > and ping again when they're fixed. I've fixed or disabled most tests except for ContextualSearchManagerTests. Do you think the changes here make sense? Especially, I've disabled web_view_interactive_tests because in some cases - when focus moves from widget B (embedder) to widget A (has an input, is a guest webview) - widget A sends TextInputStateChanged(type=TEXT) - widget B sends TextInputStateChanged(type=NONE) These are received by one RenderWidgetHostViewAura. Now RWHVA (which is its TextInputClient by itself) thinks the current text input type is NONE. I think this hidden race condition was there before this CL. WDYT?
On 2015/10/21 at 16:53:06, changwan wrote: > Especially, I've disabled web_view_interactive_tests because in some cases > - when focus moves from widget B (embedder) to widget A (has an input, is a guest webview) > - widget A sends TextInputStateChanged(type=TEXT) > - widget B sends TextInputStateChanged(type=NONE) > > These are received by one RenderWidgetHostViewAura. > > Now RWHVA (which is its TextInputClient by itself) thinks the current text input type is NONE. > > I think this hidden race condition was there before this CL. WDYT? cc'ing lazyboy@ who added this test. This seems like a real bug that should be fixed. Even if preexisting, if the test started failing more consistently, that means the bug is probably more visible/frequent with this patch, so we should be careful. I don't see it on the trybots for previous patchsets so I'm not sure if it's a tree closer though? lazyboy@, I'm thinking a fix could be to wait for a full round-trip on the unfocus event before we send the click event to the other process? What do you think?
I realized there is a different way to address this problem without posting a task. There is an RAII class ImeEventGuard that sets this handling_ime_event_ to true while active, and calls UpdateTextInputState when it's finished. This guard can be present during gesture events as well, not just touch events. Without the task post, hopefully you don't need to change the Android unit tests to wait for a particular state after all, and perhaps the WebView-tag focus change test won't be affected either. https://codereview.chromium.org/1388283002/diff/120001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1388283002/diff/120001/content/renderer/rende... content/renderer/render_widget.cc:1062: #if defined(OS_ANDROID) Let's delete this OS_ANDROID. https://codereview.chromium.org/1388283002/diff/120001/content/renderer/rende... content/renderer/render_widget.cc:1069: scoped_ptr<ImeEventGuard> ime_event_guard_maybe; We can unconditionally create the IME guard here, not just for keyboard events. (And we can remove the shortlist of "special" keys, it looks like those are just fixing the problem of show_ime state getting lost, which I suggest another fix for below.) https://codereview.chromium.org/1388283002/diff/120001/content/renderer/rende... content/renderer/render_widget.cc:1889: return; Prior to this early return, I think we should do 'need_show_ime_ |= (show_ime == SHOW_IME_IF_NEEDED); text_field_is_dirty_ |= (change_source == FROM_NON_IME);'. Otherwise a show request could be lost.
Description was changed from ========== Fix OSK flickering issue On Android, on-screen keyboard will flicker when you move focus from one input form to another. We have a delayed OSK dismissal hack, but we found that this is not a good solution for three reasons: 1) Flickering still happens, even though it's less severe. 2) Hiding of OSK will be delayed when touching a non-editable web node. 3) It makes keyboard showing & hiding very difficult to test. The main cause of the flickering is that we call UpdateTextInputState() in the middle of focus change: - FocusController::setFocusedElement() - FocusController::clearSelectionIfNecessary() - FrameSelection::clear() - Editor::notifyComponentsOnChangedSelection() - ... - RenderWidget::UpdateTextInputState() - WebViewImpl::textInputInfo() This can be avoided if we post UpdateTextInputState() on the renderer's main thread. Now that delayed dismissal hack is removed, we can straighten up ImeAdapter's keyboard show and hide logic, and add tests around it with ease. Following this fix, we can no longer test intermediate state changes. It's because the following may happen: 1) IME event#1 arrives RenderWidget (e.g., ImeConfirmComposition) 2) IME event#2 arrives RenderWidget (e.g., HandleInputEvent) 3) IME event#1 posts first UpdateTextInputStateInternal(). 4) IME event#2 changes text input info. 5) UpdateTextInputStateInternal() is called, and it may have reflected event #1 only, or may have reflected event#2 as well, depending on the timing of event#2's arrival. So instead we now wait for state updates to converge to the expected results. ========== to ========== Fix OSK flickering issue On Android, on-screen keyboard will flicker when you move focus from one input form to another. We have a delayed OSK dismissal hack, but we found that this is not a good solution for three reasons: 1) Flickering still happens, even though it's less severe. 2) Hiding of OSK will be delayed when touching a non-editable web node. 3) It makes keyboard showing & hiding very difficult to test. The main cause of the flickering is that we call UpdateTextInputState() in the middle of focus change: - FocusController::setFocusedElement() - FocusController::clearSelectionIfNecessary() - FrameSelection::clear() - Editor::notifyComponentsOnChangedSelection() - ... - RenderWidget::UpdateTextInputState() - WebViewImpl::textInputInfo() This can be avoided if we guard non-keyboard events in OnHandleInputType. Also because focuschange (which should result in state update) can happen through / not through OnHandleInputType, I made StartHandlingImeEvent and FinishHandlingImeEvent nestable. ==========
https://codereview.chromium.org/1388283002/diff/120001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1388283002/diff/120001/content/renderer/rende... content/renderer/render_widget.cc:1062: #if defined(OS_ANDROID) On 2015/10/22 00:07:56, aelias wrote: > Let's delete this OS_ANDROID. Done. (try is still running). https://codereview.chromium.org/1388283002/diff/120001/content/renderer/rende... content/renderer/render_widget.cc:1069: scoped_ptr<ImeEventGuard> ime_event_guard_maybe; On 2015/10/22 00:07:56, aelias wrote: > We can unconditionally create the IME guard here, not just for keyboard events. > (And we can remove the shortlist of "special" keys, it looks like those are just > fixing the problem of show_ime state getting lost, which I suggest another fix > for below.) This is a great idea! In order to implement it I had to make StartHandlingImeEvent / FinishHandlingImeEvent nestable. https://codereview.chromium.org/1388283002/diff/120001/content/renderer/rende... content/renderer/render_widget.cc:1889: return; On 2015/10/22 00:07:56, aelias wrote: > Prior to this early return, I think we should do 'need_show_ime_ |= (show_ime == > SHOW_IME_IF_NEEDED); text_field_is_dirty_ |= (change_source == FROM_NON_IME);'. > Otherwise a show request could be lost. Hmm... Since we do not store show_ime_, I came up with pending_show_ime_if_needed_ so that show_ime in an ignored state update can be stored.
https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1388283002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:202: // TODO(changwan): check if we can remove this line. On 2015/10/16 02:10:16, Changwan Ryu wrote: > On 2015/10/15 19:58:40, aelias wrote: > > Please attempt to remove it now and come to a conclusion on whether it's > needed > > or not, there's no reason to leave this question open as a TODO. > > Removed. This has been moved out to ContentViewCore.
https://codereview.chromium.org/1388283002/diff/140001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/1388283002/diff/140001/content/renderer/rende... content/renderer/render_widget.h:665: int handling_ime_events_count_; Please rename to "ime_guard_count_". Strictly speaking, we are not necessarily counting only IME events. https://codereview.chromium.org/1388283002/diff/140001/content/renderer/rende... content/renderer/render_widget.h:770: bool pending_update_from_non_ime_; We don't need to add this extra state. We can just set text_field_is_dirty_, next to where you set pending_show_ime_if_needed_ in UpdateTextInputState. The effect is the same.
https://codereview.chromium.org/1388283002/diff/140001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/1388283002/diff/140001/content/renderer/rende... content/renderer/render_widget.h:665: int handling_ime_events_count_; On 2015/10/22 03:25:07, aelias wrote: > Please rename to "ime_guard_count_". Strictly speaking, we are not necessarily > counting only IME events. Done. Also changed {Start,Finish}HandlingImeEvent names. https://codereview.chromium.org/1388283002/diff/140001/content/renderer/rende... content/renderer/render_widget.h:770: bool pending_update_from_non_ime_; On 2015/10/22 03:25:07, aelias wrote: > We don't need to add this extra state. We can just set text_field_is_dirty_, > next to where you set pending_show_ime_if_needed_ in UpdateTextInputState. The > effect is the same. Hmm... They are slightly different because when text_field_is_dirty_ is set, it will always send an update while this variable is just used in order to flip one flag.
On 2015/10/22 06:18:18, Changwan Ryu wrote: > https://codereview.chromium.org/1388283002/diff/140001/content/renderer/rende... > File content/renderer/render_widget.h (right): > > https://codereview.chromium.org/1388283002/diff/140001/content/renderer/rende... > content/renderer/render_widget.h:665: int handling_ime_events_count_; > On 2015/10/22 03:25:07, aelias wrote: > > Please rename to "ime_guard_count_". Strictly speaking, we are not > necessarily > > counting only IME events. > > Done. Also changed {Start,Finish}HandlingImeEvent names. > > https://codereview.chromium.org/1388283002/diff/140001/content/renderer/rende... > content/renderer/render_widget.h:770: bool pending_update_from_non_ime_; > On 2015/10/22 03:25:07, aelias wrote: > > We don't need to add this extra state. We can just set text_field_is_dirty_, > > next to where you set pending_show_ime_if_needed_ in UpdateTextInputState. > The > > effect is the same. > > Hmm... They are slightly different because when text_field_is_dirty_ is set, it > will always send an update while this variable is just used in order to flip one > flag. BTW, we still see WebViewInteractiveTest.Focus_FocusRestored flakiness with a fairly different approach.
Description was changed from ========== Fix OSK flickering issue On Android, on-screen keyboard will flicker when you move focus from one input form to another. We have a delayed OSK dismissal hack, but we found that this is not a good solution for three reasons: 1) Flickering still happens, even though it's less severe. 2) Hiding of OSK will be delayed when touching a non-editable web node. 3) It makes keyboard showing & hiding very difficult to test. The main cause of the flickering is that we call UpdateTextInputState() in the middle of focus change: - FocusController::setFocusedElement() - FocusController::clearSelectionIfNecessary() - FrameSelection::clear() - Editor::notifyComponentsOnChangedSelection() - ... - RenderWidget::UpdateTextInputState() - WebViewImpl::textInputInfo() This can be avoided if we guard non-keyboard events in OnHandleInputType. Also because focuschange (which should result in state update) can happen through / not through OnHandleInputType, I made StartHandlingImeEvent and FinishHandlingImeEvent nestable. ========== to ========== Fix OSK flickering issue On Android, on-screen keyboard will flicker when you move focus from one input form to another. We have a delayed OSK dismissal hack, but we found that this is not a good solution for three reasons: 1) Flickering still happens, even though it's less severe. 2) Hiding of OSK will be delayed when touching a non-editable web node. 3) It makes keyboard showing & hiding very difficult to test. The main cause of the flickering is that we call UpdateTextInputState() in the middle of focus change: - FocusController::setFocusedElement() - FocusController::clearSelectionIfNecessary() - FrameSelection::clear() - Editor::notifyComponentsOnChangedSelection() - ... - RenderWidget::UpdateTextInputState() - WebViewImpl::textInputInfo() This can be avoided if we guard non-keyboard events in OnHandleInputType. Also because focuschange (which should result in state update) can happen through / not through OnHandleInputType, I made StartHandlingImeEvent and FinishHandlingImeEvent nestable. BUG=484139, 542619 ==========
aelias@chromium.org changed reviewers: + sievers@chromium.org
Ok, lgtm modulo comment below. PTAL aurimas@ for browser-side OWNERS, sievers@ for content/renderer changes. > BTW, we still see WebViewInteractiveTest.Focus_FocusRestored flakiness with a fairly different approach. Ok, in that case just bring back the OS(ANDROID) around the ImeGuard creation and file a bug about the preexisting FocusRestored race.
On 2015/10/21 22:15:28, aelias wrote: > On 2015/10/21 at 16:53:06, changwan wrote: > > Especially, I've disabled web_view_interactive_tests because in some cases > > - when focus moves from widget B (embedder) to widget A (has an input, is a > guest webview) > > - widget A sends TextInputStateChanged(type=TEXT) > > - widget B sends TextInputStateChanged(type=NONE) > > > > These are received by one RenderWidgetHostViewAura. > > > > Now RWHVA (which is its TextInputClient by itself) thinks the current text > input type is NONE. > > > > I think this hidden race condition was there before this CL. WDYT? > > cc'ing lazyboy@ who added this test. This seems like a real bug that should be > fixed. Even if preexisting, if the test started failing more consistently, that > means the bug is probably more visible/frequent with this patch, so we should be > careful. I don't see it on the trybots for previous patchsets so I'm not sure > if it's a tree closer though? > > lazyboy@, I'm thinking a fix could be to wait for a full round-trip on the > unfocus event before we send the click event to the other process? What do you > think? Unfortunately, the approach taken to implement this for webview seems inherently racy :( I'm hoping we're going to be in better place once http://crbug.com/341918 has finished, but that's going to be a while to be used in webview. The other (hacky) option could be to remember whichever widget set the text input type to non-NONE last and let only that widget reset it to NONE. If other widget tries to set it to NONE, we ignore it.
On 2015/10/22 17:03:09, lazyboy wrote: > On 2015/10/21 22:15:28, aelias wrote: > > On 2015/10/21 at 16:53:06, changwan wrote: > > > Especially, I've disabled web_view_interactive_tests because in some cases > > > - when focus moves from widget B (embedder) to widget A (has an input, is a > > guest webview) > > > - widget A sends TextInputStateChanged(type=TEXT) > > > - widget B sends TextInputStateChanged(type=NONE) > > > > > > These are received by one RenderWidgetHostViewAura. > > > > > > Now RWHVA (which is its TextInputClient by itself) thinks the current text > > input type is NONE. > > > > > > I think this hidden race condition was there before this CL. WDYT? > > > > cc'ing lazyboy@ who added this test. This seems like a real bug that should > be > > fixed. Even if preexisting, if the test started failing more consistently, > that > > means the bug is probably more visible/frequent with this patch, so we should > be > > careful. I don't see it on the trybots for previous patchsets so I'm not sure > > if it's a tree closer though? > > > > lazyboy@, I'm thinking a fix could be to wait for a full round-trip on the > > unfocus event before we send the click event to the other process? What do > you > > think? > > Unfortunately, the approach taken to implement this for webview seems inherently > racy :( > I'm hoping we're going to be in better place once http://crbug.com/341918 has > finished, but that's going to be a while to be used in webview. > The other (hacky) option could be to remember whichever widget set the text > input type > to non-NONE last and let only that widget reset it to NONE. If other widget > tries to > set it to NONE, we ignore it. Filed crbug.com/546645. Added back OS_ANDROID guard for now.
On 2015/10/22 17:52:05, Changwan Ryu wrote: > On 2015/10/22 17:03:09, lazyboy wrote: > > On 2015/10/21 22:15:28, aelias wrote: > > > On 2015/10/21 at 16:53:06, changwan wrote: > > > > Especially, I've disabled web_view_interactive_tests because in some cases > > > > - when focus moves from widget B (embedder) to widget A (has an input, is > a > > > guest webview) > > > > - widget A sends TextInputStateChanged(type=TEXT) > > > > - widget B sends TextInputStateChanged(type=NONE) > > > > > > > > These are received by one RenderWidgetHostViewAura. > > > > > > > > Now RWHVA (which is its TextInputClient by itself) thinks the current text > > > input type is NONE. > > > > > > > > I think this hidden race condition was there before this CL. WDYT? > > > > > > cc'ing lazyboy@ who added this test. This seems like a real bug that should > > be > > > fixed. Even if preexisting, if the test started failing more consistently, > > that > > > means the bug is probably more visible/frequent with this patch, so we > should > > be > > > careful. I don't see it on the trybots for previous patchsets so I'm not > sure > > > if it's a tree closer though? > > > > > > lazyboy@, I'm thinking a fix could be to wait for a full round-trip on the > > > unfocus event before we send the click event to the other process? What do > > you > > > think? > > > > Unfortunately, the approach taken to implement this for webview seems > inherently > > racy :( > > I'm hoping we're going to be in better place once http://crbug.com/341918 has > > finished, but that's going to be a while to be used in webview. > > The other (hacky) option could be to remember whichever widget set the text > > input type > > to non-NONE last and let only that widget reset it to NONE. If other widget > > tries to > > set it to NONE, we ignore it. > > Filed crbug.com/546645. Added back OS_ANDROID guard for now. ping. sievers@, and aurimas@, could you take a look? There are some other issues that depend on this to be fixed.
https://codereview.chromium.org/1388283002/diff/200001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1388283002/diff/200001/content/renderer/rende... content/renderer/render_widget.cc:1876: pending_update_from_non_ime_ |= from_non_ime; Since we already have the guard class, it'd be nice to encapsulate these variables there (also this patch ends up duplicating the 'from_non_ime' tracking). One way to do it would be to track the guard instance at the bottom of the frame instead of the counter in this class: void RenderWidget::OnImeEventGuardStart(ImeGuard* guard) { if (!ime_guard_) ime_guard_ = guard; } void RenderWidget::OnImeEventGuardFinish(ImeGuard* guard) { if (ime_guard_ != guard) return; ime_guard_ = nullptr; UpdateSelectionBounds(); UpdateTextInputState(guard->show_ime, guard->change_source); } void RenderWidget::UpdateTextInputState() { if (image_guard_) { if (show_ime == SHOW_IME_IF_NEEDED) ime_guard_->show_ime_ = show_ime; return; } ui::TextInputType new_type = GetTextInputType(); ... } wdyt?
https://codereview.chromium.org/1388283002/diff/200001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1388283002/diff/200001/content/renderer/rende... content/renderer/render_widget.cc:1876: pending_update_from_non_ime_ |= from_non_ime; On 2015/10/26 20:13:12, sievers wrote: > Since we already have the guard class, it'd be nice to encapsulate these > variables there (also this patch ends up duplicating the 'from_non_ime' > tracking). > > One way to do it would be to track the guard instance at the bottom of the frame > instead of the counter in this class: > > void RenderWidget::OnImeEventGuardStart(ImeGuard* guard) { > if (!ime_guard_) > ime_guard_ = guard; > } > > void RenderWidget::OnImeEventGuardFinish(ImeGuard* guard) { > if (ime_guard_ != guard) > return; > ime_guard_ = nullptr; > UpdateSelectionBounds(); > UpdateTextInputState(guard->show_ime, guard->change_source); > } > > void RenderWidget::UpdateTextInputState() { > if (image_guard_) { > if (show_ime == SHOW_IME_IF_NEEDED) > ime_guard_->show_ime_ = show_ime; > return; > } > > ui::TextInputType new_type = GetTextInputType(); > ... > } > > wdyt? Hmm... The lifetime of one ImeEventGuard may be nesting or overlapping that of another. In case of nesting, the first call to OnImeEventGuardFinish() will trigger UpdateTextInputState() which isn't intended. Without keeping the count, we cannot tell whether the current ImeEventGuard is the last one or not. So if we are to move some of the variables inside ImeEventGuard, we will still need to keep ime_guard_count_ and pending_update_from_non_ime_ outside of ImeEventGuard, and only pending_show_ime_if_needed_ can be moved inside it. I'm not sure if that can increase the readability here. Please advise.
https://codereview.chromium.org/1388283002/diff/200001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1388283002/diff/200001/content/renderer/rende... content/renderer/render_widget.cc:1876: pending_update_from_non_ime_ |= from_non_ime; On 2015/10/28 05:17:07, Changwan Ryu wrote: > On 2015/10/26 20:13:12, sievers wrote: > > Since we already have the guard class, it'd be nice to encapsulate these > > variables there (also this patch ends up duplicating the 'from_non_ime' > > tracking). > > > > One way to do it would be to track the guard instance at the bottom of the > frame > > instead of the counter in this class: > > > > void RenderWidget::OnImeEventGuardStart(ImeGuard* guard) { > > if (!ime_guard_) > > ime_guard_ = guard; > > } > > > > void RenderWidget::OnImeEventGuardFinish(ImeGuard* guard) { > > if (ime_guard_ != guard) > > return; > > ime_guard_ = nullptr; > > UpdateSelectionBounds(); > > UpdateTextInputState(guard->show_ime, guard->change_source); > > } > > > > void RenderWidget::UpdateTextInputState() { > > if (image_guard_) { > > if (show_ime == SHOW_IME_IF_NEEDED) > > ime_guard_->show_ime_ = show_ime; > > return; > > } > > > > ui::TextInputType new_type = GetTextInputType(); > > ... > > } > > > > wdyt? > > Hmm... The lifetime of one ImeEventGuard may be nesting or overlapping that of > another. In case of nesting, the first call to OnImeEventGuardFinish() will > trigger UpdateTextInputState() which isn't intended. Without keeping the count, > we cannot tell whether the current ImeEventGuard is the last one or not. > > So if we are to move some of the variables inside ImeEventGuard, we will still > need to keep ime_guard_count_ and pending_update_from_non_ime_ outside of > ImeEventGuard, and only pending_show_ime_if_needed_ can be moved inside it. I'm > not sure if that can increase the readability here. Please advise. My bad. Overlapping is not a possible scenario here. I've updated the code as you suggested. Looks much cleaner now! PTAL.
lgtm https://codereview.chromium.org/1388283002/diff/260001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1388283002/diff/260001/content/renderer/rende... content/renderer/render_widget.cc:1893: ime_event_guard_->from_ime() & guard->from_ime()); '&' -> '&&' https://codereview.chromium.org/1388283002/diff/260001/content/renderer/rende... content/renderer/render_widget.cc:1893: ime_event_guard_->from_ime() & guard->from_ime()); nit: i found the previous reverse logic a bit easier to follow, also the naming along the lines of 'was_updated_from_non_ime'. but i guess the comment explains it somewhat.
https://codereview.chromium.org/1388283002/diff/260001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1388283002/diff/260001/content/renderer/rende... content/renderer/render_widget.cc:1893: ime_event_guard_->from_ime() & guard->from_ime()); On 2015/10/30 00:05:52, sievers wrote: > '&' -> '&&' Done. https://codereview.chromium.org/1388283002/diff/260001/content/renderer/rende... content/renderer/render_widget.cc:1893: ime_event_guard_->from_ime() & guard->from_ime()); On 2015/10/30 00:05:52, sievers wrote: > nit: i found the previous reverse logic a bit easier to follow, also the naming > along the lines of 'was_updated_from_non_ime'. but i guess the comment explains > it somewhat. Modified the comment to make it more readable. Thanks for pointing it out.
The CQ bit was checked by changwan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1388283002/#ps280001 (title: "fixed a nit and updated comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1388283002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1388283002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1388283002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1388283002/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/f2a707b1d5492b35833bee9945c197595ba11d89 Cr-Commit-Position: refs/heads/master@{#357056} |
