Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2290133002:
Make WebView keep keyboard when losing focus (Closed)
DescriptionMake WebView keep keyboard when losing focus
WebView should not hide keyboard when losing focus. As for Chrome, it
should keep current behavior. These cases include when the ContentView is
hidden (closing current tab, opening a new tab, switching to an existing
tab), and when imeAdapter is attached to its native counterpart(navigation).
Note that this CL will change one behavior of Chrome: keyboard will not be
hidden when focus moves from ContentView to UrlBar.
BUG=636237
Committed: https://crrev.com/66d5ae12299e85269251cd655d46a7b5710b7c80
Cr-Commit-Position: refs/heads/master@{#429531}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add some tests #
Total comments: 9
Patch Set 3 : Only for Test. Use CallbackHelper, but still flaky. #
Total comments: 3
Patch Set 4 : Restrict tabstest to tablet. #Patch Set 5 #
Total comments: 14
Patch Set 6 : Address changwan@'s review #Patch Set 7 #Patch Set 8 : Rebase #Patch Set 9 #
Total comments: 2
Patch Set 10 : rebase #Messages
Total messages: 71 (45 generated)
The CQ bit was checked by yabinh@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...
Description was changed from ========== Make WebView keep keyboard when losing focus WebView should not hide keyboard when losing focus. As for Chrome, it should keep current behavior. These cases include when the ContentView is hidden (closing current tab, opening a new tab, switching to an existing tab), and when imeAdapter is attached to its native counterpart(navigation). Note that this CL will change one behavior of Chrome: keyboard will not be hidden when WebView loses focus to TextEditor in the same page. ========== to ========== Make WebView keep keyboard when losing focus WebView should not hide keyboard when losing focus. As for Chrome, it should keep current behavior. These cases include when the ContentView is hidden (closing current tab, opening a new tab, switching to an existing tab), and when imeAdapter is attached to its native counterpart(navigation). Note that this CL will change one behavior of Chrome: keyboard will not be hidden when WebView loses focus to TextEditor in the same page. BUG=636237 ==========
yabinh@chromium.org changed reviewers: + changwan@chromium.org
changwan@, PTAL. Thanks~ https://codereview.chromium.org/2290133002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2290133002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:349: resetAndHideKeyboard(); For navigation.
Could you add to TabsTest some tests to verify tab management cases and to ImeTest some tests to verify navigation cases? These are quite sensitive cases, so I think automated tests are required.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yabinh@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...
yabinh@chromium.org changed reviewers: + aelias@chromium.org
On 2016/08/30 07:13:44, Changwan Ryu wrote: > Could you add to TabsTest some tests to verify tab management cases and to > ImeTest some tests to verify navigation cases? These are quite sensitive cases, > so I think automated tests are required. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yabinh@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2290133002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwImeTest.java (right): https://codereview.chromium.org/2290133002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwImeTest.java:115: CriteriaHelper.pollUiThread(new Criteria() { FYI, this test is based on CriteriaHelper, and is not in a good state. Can you try to change this to CallbackHelper? https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java (right): https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java:54: public void testHideImeOnLosingFocus() throws Throwable { testDoNotHideImeOnLosingFocus https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java:56: // view loses its focus Tests that the IME window does not get hidden when the content view loses its focus. https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java:72: // and the input method window should be hidden When another view (URL box) takes focus, we do not hide the IME window.
https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1311: mImeAdapter.resetAndHideKeyboard(); As we talked offline, I'm still not convinced that we can call this on hiding a tab. There is no guarantee that either previous tab's view or new tab's view is active at this point. There is a chance that keyboard hiding may not be effective. Could it be one of the causes of test flakiness?
On 2016/09/21 01:22:45, Changwan Ryu wrote: > https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1311: > mImeAdapter.resetAndHideKeyboard(); > As we talked offline, I'm still not convinced that we can call this on hiding a > tab. There is no guarantee that either previous tab's view or new tab's view is > active at this point. There is a chance that keyboard hiding may not be > effective. Could it be one of the causes of test flakiness? I tried another way, but tabsTest is still flaky. I ran it 600 times, and got 5 failures because keyboard behavior was not as expected. But I got even more failures because tabsTest is flaky in setUp(). (As a result, many tests in tabsTest were disabled.) The other failures are: 13/600: Tab never selected/initialized (in ChromeActivityTestCaseBase.setUp) 6/600: Deferred startup never completed (in ChromeActivityTestCaseBase.setUp) 1/600: ChromeActivity did not start (in startMainActivityWithURL)
On 2016/09/30 06:03:35, yabinh wrote: > On 2016/09/21 01:22:45, Changwan Ryu wrote: > > > https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... > > File > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > (right): > > > > > https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1311: > > mImeAdapter.resetAndHideKeyboard(); > > As we talked offline, I'm still not convinced that we can call this on hiding > a > > tab. There is no guarantee that either previous tab's view or new tab's view > is > > active at this point. There is a chance that keyboard hiding may not be > > effective. Could it be one of the causes of test flakiness? > > I tried another way, but tabsTest is still flaky. I ran it 600 times, and got 5 > failures because keyboard behavior was not as expected. But I got even more > failures because tabsTest is flaky in setUp(). (As a result, many tests in > tabsTest were disabled.) > > The other failures are: > 13/600: Tab never selected/initialized (in ChromeActivityTestCaseBase.setUp) > 6/600: Deferred startup never completed (in ChromeActivityTestCaseBase.setUp) > 1/600: ChromeActivity did not start (in startMainActivityWithURL) FYI, devastating 44.2% of chrome public tests are flaky now. I don't think you need to worry about other test flakiness as long as you know that they are not newly introduced. But as we talked offline, let's not try to add a new test to TabsTest as there is a dependency on the real keyboard behavior which may be the cause of the flakiness. We can just add a test in ImeTest about onDetachedFromWindow() and manually verify the tab case now. I think ultimately we may need Chrome level IME test utilities such that we can run TabsTest against mock input connection.
https://codereview.chromium.org/2290133002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2290133002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:293: ImeAdapter imeAdapter = So the flakiness we talked about was caused because by the time onHide() is called, the current tab's ContentViewCore() points to the new tab's CVC? If so we may see the real failure in production code because the previous tab's view is already ejected by this point and isActive(view) may return false. Tab#swapWebContents seems to initiate CVC#onHide() but the comment says: /* This is currently called when committing a pre-rendered page. */ I'm afraid that this is not the right hook to have hideSoftInput().
https://codereview.chromium.org/2290133002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2290133002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:293: ImeAdapter imeAdapter = On 2016/10/12 12:15:49, Changwan Ryu wrote: > So the flakiness we talked about was caused because by the time onHide() is > called, > the current tab's ContentViewCore() points to the new tab's CVC? > If so we may see the real failure in production code because the previous tab's > view is already ejected > by this point and isActive(view) may return false. > > Tab#swapWebContents seems to initiate CVC#onHide() but the comment says: > /* This is currently called when committing a pre-rendered page. */ > > I'm afraid that this is not the right hook to have hideSoftInput(). How about listening to TabModelObserver#didSelectTab()? Would it make any difference?
https://codereview.chromium.org/2290133002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2290133002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:293: ImeAdapter imeAdapter = On 2016/10/12 12:15:49, Changwan Ryu wrote: > So the flakiness we talked about was caused because by the time onHide() is > called, > the current tab's ContentViewCore() points to the new tab's CVC? > If so we may see the real failure in production code because the previous tab's > view is already ejected > by this point and isActive(view) may return false. > > Tab#swapWebContents seems to initiate CVC#onHide() but the comment says: > /* This is currently called when committing a pre-rendered page. */ > > I'm afraid that this is not the right hook to have hideSoftInput(). Actually we use "onViewDetachedFromWindow()" instead of "onHide()" in this patch. The reason for the flakiness is that, we only set InputMethodManagerWrapper for the old tab. So when we open a new tab, its InputMethodManagerWrapper is the old one, not the overrided one. By the time we open the new tab, if the old tab is still active, then it will call the overrided hideSoftInputFromWindow(), and notifyCalled(), then everything is OK. If the old tab is not active, the new tab will be active, and it will call the old hideSoftInputFromWindow() from the new tab. It will also hide keyboard, but we will not get notified, because there is no notifyCalled() in the old InputMethodManagerWrapper.
The CQ bit was checked by yabinh@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...
The CQ bit was checked by yabinh@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/2290133002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwImeTest.java (right): https://codereview.chromium.org/2290133002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwImeTest.java:115: CriteriaHelper.pollUiThread(new Criteria() { On 2016/09/09 01:07:29, Changwan Ryu wrote: > FYI, this test is based on CriteriaHelper, and is not in a good state. Can you > try to change this to CallbackHelper? When we use CallbackHelper, we expect it should send notification if something happens. In this test, we need to test the keyboard is not hidden. The problem is, how do we test that nothing should happen? It seems not obvious, so I'll keep using CriteriaHelper. https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java (right): https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java:54: public void testHideImeOnLosingFocus() throws Throwable { On 2016/09/09 01:07:29, Changwan Ryu wrote: > testDoNotHideImeOnLosingFocus Done. https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java:56: // view loses its focus On 2016/09/09 01:07:29, Changwan Ryu wrote: > Tests that the IME window does not get hidden when the content view loses its > focus. Done. https://codereview.chromium.org/2290133002/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java:72: // and the input method window should be hidden On 2016/09/09 01:07:29, Changwan Ryu wrote: > When another view (URL box) takes focus, we do not hide the IME window. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In the description of this CL, could you replace "WebView loses focus to TextEditor in the same page" by "focus moves from ContentView to UrlBar" https://codereview.chromium.org/2290133002/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwImeTest.java (right): https://codereview.chromium.org/2290133002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwImeTest.java:150: waitForNonNullInputConnection(); Is this really testing something? Does this test fail against the original production logic? https://codereview.chromium.org/2290133002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2290133002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:327: final CallbackHelper showSoftInputHelper1 = new CallbackHelper(); How about naming these variables as showCallback1 / hideCallback1 / showCallback2 / hideCallback2 for short? Also, is it possible to avoid keeping callback counts and only asserts when necessary? That way we can also test that the minimum number of show and hide calls are invoked. (Only if it works, that is.) assertEquals(0, showCallback1.getCallCount()); // Click node in the 1st tab. showCallback1.waitForCallback(0, 1); // Open a new tab (2nd tab). assertEquals(0, hideCallback1.getCallCount()); hideCallback1.waitForCallback(0, 1); ... // Click node in the 2nd tab. assertEquals(0, showCallback2.getCallCount()); ... showCallback2.waitForCallback(0, 1); ... https://codereview.chromium.org/2290133002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:333: boolean result = DOMUtils.clickNode( Could you make the two lines into one? assertTrue(DOMUtils.clickNode(...)); https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (left): https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:371: if (mInputMethodManagerWrapper.isActive(view)) { Is this really causing flakiness in test with onDetachedFromWindow approach as well? If so in what way? In CompositorViewHolder, we remove a view first and then add a new view. I don't understand why this is needed. Could you look into it a bit more? At the minimum, please make a separate method instead of using this. There's some risk in not checking activeness of the view. https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:429: resetAndHideKeyboard(); As we talked offline, I now prefer this approach because it may cover unexpected scenarios such as removing container view for some UX effects. Could you add some comment, though? Something like the following should suffice: // Currently, the view is removed when you select another tab in CompositorViewHolder. InputMethodManager does not automatically hide keyboard even when view is removed, so we add additional logic here. https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java (right): https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java:71: assertFalse(immw.isHidden()); Hmm... This test depends on the real behavior of the keyboard app. Unless we change this into CallbackHelper approach, can we just remove this file? https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:138: fullyLoadNewUrl(UrlUtils.getIsolatedTestFileUrl(INPUT_FORM_HTML)); Isn't reloadPage() enough? If not, could you fix reloadPage() instead? https://codereview.chromium.org/2290133002/diff/80001/content/test/data/andro... File content/test/data/android/input/input_forms.html (right): https://codereview.chromium.org/2290133002/diff/80001/content/test/data/andro... content/test/data/android/input/input_forms.html:25: <a id="link" href="https://www.google.com">google</a> Does the test pass even when network is disabled? How about replacing it by about:blank?
Description was changed from ========== Make WebView keep keyboard when losing focus WebView should not hide keyboard when losing focus. As for Chrome, it should keep current behavior. These cases include when the ContentView is hidden (closing current tab, opening a new tab, switching to an existing tab), and when imeAdapter is attached to its native counterpart(navigation). Note that this CL will change one behavior of Chrome: keyboard will not be hidden when WebView loses focus to TextEditor in the same page. BUG=636237 ========== to ========== Make WebView keep keyboard when losing focus WebView should not hide keyboard when losing focus. As for Chrome, it should keep current behavior. These cases include when the ContentView is hidden (closing current tab, opening a new tab, switching to an existing tab), and when imeAdapter is attached to its native counterpart(navigation). Note that this CL will change one behavior of Chrome: keyboard will not be hidden when focus moves from ContentView to UrlBar. BUG=636237 ==========
The CQ bit was checked by yabinh@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...
The CQ bit was checked by yabinh@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/2290133002/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwImeTest.java (right): https://codereview.chromium.org/2290133002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwImeTest.java:150: waitForNonNullInputConnection(); On 2016/10/19 05:10:21, Changwan Ryu wrote: > Is this really testing something? Does this test fail against the original > production logic? Yes. If will fail if we keep the old logic. As we talked offline, this test doesn't make sense. I'll remove it. https://codereview.chromium.org/2290133002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2290133002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:333: boolean result = DOMUtils.clickNode( On 2016/10/19 05:10:21, Changwan Ryu wrote: > Could you make the two lines into one? > > assertTrue(DOMUtils.clickNode(...)); Done. https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (left): https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:371: if (mInputMethodManagerWrapper.isActive(view)) { On 2016/10/19 05:10:21, Changwan Ryu wrote: > Is this really causing flakiness in test with onDetachedFromWindow approach as > well? If so in what way? In CompositorViewHolder, we remove a view first and > then add a new view. I don't understand why this is needed. Could you look into > it a bit more? > > At the minimum, please make a separate method instead of using this. There's > some risk in not checking activeness of the view. Still flaky, but it's because of the test itself, not the product. If we use CriteriaHelper instead of CallbackHelper, we can solve that. https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java (right): https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java:71: assertFalse(immw.isHidden()); On 2016/10/19 05:10:21, Changwan Ryu wrote: > Hmm... This test depends on the real behavior of the keyboard app. Unless we > change this into CallbackHelper approach, can we just remove this file? Done. https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2290133002/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:138: fullyLoadNewUrl(UrlUtils.getIsolatedTestFileUrl(INPUT_FORM_HTML)); On 2016/10/19 05:10:21, Changwan Ryu wrote: > Isn't reloadPage() enough? If not, could you fix reloadPage() instead? Done. https://codereview.chromium.org/2290133002/diff/80001/content/test/data/andro... File content/test/data/android/input/input_forms.html (right): https://codereview.chromium.org/2290133002/diff/80001/content/test/data/andro... content/test/data/android/input/input_forms.html:25: <a id="link" href="https://www.google.com">google</a> On 2016/10/19 05:10:21, Changwan Ryu wrote: > Does the test pass even when network is disabled? How about replacing it by > about:blank? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yabinh@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...
Rebased patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by yabinh@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still lgtm from my POV; changwan@, is this ready to go? I'd like to get this into 55 before it starts to get late for cherry-picks.
lgtm https://codereview.chromium.org/2290133002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2290133002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:287: == org.chromium.ui.UiUtils.isKeyboardShowing( no need for line separation here
yabinh@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@: Please review changes in chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java. Thanks~ https://codereview.chromium.org/2290133002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2290133002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:287: == org.chromium.ui.UiUtils.isKeyboardShowing( On 2016/11/01 03:49:31, Changwan Ryu wrote: > no need for line separation here It formats automatically after running "git cl format" because it's too long.
ping - nyquist@, could you take a look at TabsTest?
TabsTest.java lgtm
The CQ bit was checked by yabinh@chromium.org
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
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yabinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, aelias@chromium.org, changwan@chromium.org Link to the patchset: https://codereview.chromium.org/2290133002/#ps180001 (title: "rebase")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by yabinh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make WebView keep keyboard when losing focus WebView should not hide keyboard when losing focus. As for Chrome, it should keep current behavior. These cases include when the ContentView is hidden (closing current tab, opening a new tab, switching to an existing tab), and when imeAdapter is attached to its native counterpart(navigation). Note that this CL will change one behavior of Chrome: keyboard will not be hidden when focus moves from ContentView to UrlBar. BUG=636237 ========== to ========== Make WebView keep keyboard when losing focus WebView should not hide keyboard when losing focus. As for Chrome, it should keep current behavior. These cases include when the ContentView is hidden (closing current tab, opening a new tab, switching to an existing tab), and when imeAdapter is attached to its native counterpart(navigation). Note that this CL will change one behavior of Chrome: keyboard will not be hidden when focus moves from ContentView to UrlBar. BUG=636237 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Make WebView keep keyboard when losing focus WebView should not hide keyboard when losing focus. As for Chrome, it should keep current behavior. These cases include when the ContentView is hidden (closing current tab, opening a new tab, switching to an existing tab), and when imeAdapter is attached to its native counterpart(navigation). Note that this CL will change one behavior of Chrome: keyboard will not be hidden when focus moves from ContentView to UrlBar. BUG=636237 ========== to ========== Make WebView keep keyboard when losing focus WebView should not hide keyboard when losing focus. As for Chrome, it should keep current behavior. These cases include when the ContentView is hidden (closing current tab, opening a new tab, switching to an existing tab), and when imeAdapter is attached to its native counterpart(navigation). Note that this CL will change one behavior of Chrome: keyboard will not be hidden when focus moves from ContentView to UrlBar. BUG=636237 Committed: https://crrev.com/66d5ae12299e85269251cd655d46a7b5710b7c80 Cr-Commit-Position: refs/heads/master@{#429531} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/66d5ae12299e85269251cd655d46a7b5710b7c80 Cr-Commit-Position: refs/heads/master@{#429531} |
