Keyboard should not be displayed in tab switcher mode.

Keyboard should not be displayed in tab switcher mode.

Currently in content when editbox is tapped and keyborad appears then if tab switcher is opened, keyboard remains.These changes hides the soft keyboard in
the tab switcher mode.

BUG=432049
Committed: https://crrev.com/2d8391c6f62277cd6319fb53bfb36c5ccf395392
Cr-Commit-Position: refs/heads/master@{#304189}

[divya.bansal, 2014-11-13 07:39:24]

divya.bansal@samsung.com changed reviewers:
+ bauerb@chromium.org, tedchoc@chromium.org

[divya.bansal, 2014-11-13 07:39:36]

PTAL

[Bernhard Bauer, 2014-11-13 09:35:09]

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
(right):

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:272:
setKeyboardVisibilityForUrl(mHasFocus);
I don't understand this. If |mHasFocus| is false at this point, it means that we
must have executed line 185 to set it to false, but then the next line would
have hidden the keyboard anyway. Under what circumstances does the visibility of
the keyboard get out of sync with the focus of the text field?

[divya.bansal, 2014-11-13 09:49:07]

So in tab manager window, if |hasFocus| is false keyboard will not appear.
Please suggest your reviews.

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
(right):

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:272:
setKeyboardVisibilityForUrl(mHasFocus);
On 2014/11/13 09:35:09, Bernhard Bauer wrote:
> I don't understand this. If |mHasFocus| is false at this point, it means that
we
> must have executed line 185 to set it to false, but then the next line would
> have hidden the keyboard anyway. Under what circumstances does the visibility
of
> the keyboard get out of sync with the focus of the text field?

@Bernhard this case is when focus is not on url field. if focus is on text field
then keyboard appears but hasFocus is false then if we switch on to tab manager
then keyboard should go away. Because focus is not on url field and in tab
manager window only edit field is url field.

[Bernhard Bauer, 2014-11-13 10:06:30]

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
(right):

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:272:
setKeyboardVisibilityForUrl(mHasFocus);
On 2014/11/13 09:49:07, divya.bansal wrote:
> On 2014/11/13 09:35:09, Bernhard Bauer wrote:
> > I don't understand this. If |mHasFocus| is false at this point, it means
that
> we
> > must have executed line 185 to set it to false, but then the next line would
> > have hidden the keyboard anyway. Under what circumstances does the
visibility
> of
> > the keyboard get out of sync with the focus of the text field?
> 
> @Bernhard this case is when focus is not on url field. if focus is on text
field
> then keyboard appears but hasFocus is false then if we switch on to tab
manager
> then keyboard should go away. Because focus is not on url field and in tab
> manager window only edit field is url field.

Could we achieve this by unfocusing the text field when opening the tab
switcher? I don't see why it should be the responsibility of the toolbar to hide
the keyboard if a text field in the content area is focused, or why it should do
that in this method (which is supposed to show or hide the add tab button).

[divya.bansal, 2014-11-13 10:34:00]

Please suggest

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
(right):

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:272:
setKeyboardVisibilityForUrl(mHasFocus);
On 2014/11/13 10:06:30, Bernhard Bauer wrote:
> On 2014/11/13 09:49:07, divya.bansal wrote:
> > On 2014/11/13 09:35:09, Bernhard Bauer wrote:
> > > I don't understand this. If |mHasFocus| is false at this point, it means
> that
> > we
> > > must have executed line 185 to set it to false, but then the next line
would
> > > have hidden the keyboard anyway. Under what circumstances does the
> visibility
> > of
> > > the keyboard get out of sync with the focus of the text field?
> > 
> > @Bernhard this case is when focus is not on url field. if focus is on text
> field
> > then keyboard appears but hasFocus is false then if we switch on to tab
> manager
> > then keyboard should go away. Because focus is not on url field and in tab
> > manager window only edit field is url field.
> 
> Could we achieve this by unfocusing the text field when opening the tab
> switcher? I don't see why it should be the responsibility of the toolbar to
hide
> the keyboard if a text field in the content area is focused, or why it should
do
> that in this method (which is supposed to show or hide the add tab button).

But if we unfocus the text field on opening the tab switcher then that focus
will be lost when we come back on that URL. Currently in Chrome Beta also, after
going to tab switcher and coming back focus is on same text box.

[Bernhard Bauer, 2014-11-13 10:45:35]

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
(right):

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:272:
setKeyboardVisibilityForUrl(mHasFocus);
On 2014/11/13 10:34:00, divya.bansal wrote:
> On 2014/11/13 10:06:30, Bernhard Bauer wrote:
> > On 2014/11/13 09:49:07, divya.bansal wrote:
> > > On 2014/11/13 09:35:09, Bernhard Bauer wrote:
> > > > I don't understand this. If |mHasFocus| is false at this point, it means
> > that
> > > we
> > > > must have executed line 185 to set it to false, but then the next line
> would
> > > > have hidden the keyboard anyway. Under what circumstances does the
> > visibility
> > > of
> > > > the keyboard get out of sync with the focus of the text field?
> > > 
> > > @Bernhard this case is when focus is not on url field. if focus is on text
> > field
> > > then keyboard appears but hasFocus is false then if we switch on to tab
> > manager
> > > then keyboard should go away. Because focus is not on url field and in tab
> > > manager window only edit field is url field.
> > 
> > Could we achieve this by unfocusing the text field when opening the tab
> > switcher? I don't see why it should be the responsibility of the toolbar to
> hide
> > the keyboard if a text field in the content area is focused, or why it
should
> do
> > that in this method (which is supposed to show or hide the add tab button).
> 
> But if we unfocus the text field on opening the tab switcher then that focus
> will be lost when we come back on that URL. Currently in Chrome Beta also,
after
> going to tab switcher and coming back focus is on same text box.

Okay, I see.

Could we simply hide the keyboard when activating the tab switcher? The URL
field is bit useless when the tab switcher is visible anyway.

The other thing that is a little dirty is that setKeyboardVisibilityForUrl()
assumes that the URL field is currently focused and uses its window token, which
is not the case here. Technically, we should use the window token from the
content view, which is also why I'm thinking this might be better done in the
TabManager or so.

[divya.bansal, 2014-11-13 11:47:51]

PTAL

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
(right):

https://codereview.chromium.org/724683002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:272:
setKeyboardVisibilityForUrl(mHasFocus);
On 2014/11/13 10:45:35, Bernhard Bauer wrote:
> On 2014/11/13 10:34:00, divya.bansal wrote:
> > On 2014/11/13 10:06:30, Bernhard Bauer wrote:
> > > On 2014/11/13 09:49:07, divya.bansal wrote:
> > > > On 2014/11/13 09:35:09, Bernhard Bauer wrote:
> > > > > I don't understand this. If |mHasFocus| is false at this point, it
means
> > > that
> > > > we
> > > > > must have executed line 185 to set it to false, but then the next line
> > would
> > > > > have hidden the keyboard anyway. Under what circumstances does the
> > > visibility
> > > > of
> > > > > the keyboard get out of sync with the focus of the text field?
> > > > 
> > > > @Bernhard this case is when focus is not on url field. if focus is on
text
> > > field
> > > > then keyboard appears but hasFocus is false then if we switch on to tab
> > > manager
> > > > then keyboard should go away. Because focus is not on url field and in
tab
> > > > manager window only edit field is url field.
> > > 
> > > Could we achieve this by unfocusing the text field when opening the tab
> > > switcher? I don't see why it should be the responsibility of the toolbar
to
> > hide
> > > the keyboard if a text field in the content area is focused, or why it
> should
> > do
> > > that in this method (which is supposed to show or hide the add tab
button).
> > 
> > But if we unfocus the text field on opening the tab switcher then that focus
> > will be lost when we come back on that URL. Currently in Chrome Beta also,
> after
> > going to tab switcher and coming back focus is on same text box.
> 
> Okay, I see.
> 
> Could we simply hide the keyboard when activating the tab switcher? The URL
> field is bit useless when the tab switcher is visible anyway.
> 
> The other thing that is a little dirty is that setKeyboardVisibilityForUrl()
> assumes that the URL field is currently focused and uses its window token,
which
> is not the case here. Technically, we should use the window token from the
> content view, which is also why I'm thinking this might be better done in the
> TabManager or so.

Done.

[Bernhard Bauer, 2014-11-13 11:52:55]

Thanks! I think this is a lot nicer.

https://codereview.chromium.org/724683002/diff/20001/chrome/android/shell/jav...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://codereview.chromium.org/724683002/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:223:
mImm.hideSoftInputFromWindow(mContentViewHolder.getWindowToken(), 0);
Just get the InputMethodManager directly here?

[divya.bansal, 2014-11-13 13:53:51]

PTAL

https://codereview.chromium.org/724683002/diff/20001/chrome/android/shell/jav...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://codereview.chromium.org/724683002/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:223:
mImm.hideSoftInputFromWindow(mContentViewHolder.getWindowToken(), 0);
On 2014/11/13 11:52:54, Bernhard Bauer wrote:
> Just get the InputMethodManager directly here?

Done.

[Bernhard Bauer, 2014-11-13 13:59:46]

Please also update the CL subject and description now that the CL has nothing to
do with the URL bar anymore.

[divya.bansal, 2014-11-14 03:54:25]

On 2014/11/13 13:59:46, Bernhard Bauer wrote:
> Please also update the CL subject and description now that the CL has nothing
to
> do with the URL bar anymore.

@Bernhard CL subject description is changed.
PTAL

[Bernhard Bauer, 2014-11-14 09:08:10]

LGTM!

[divya.bansal, 2014-11-14 09:09:25]

The CQ bit was checked by divya.bansal@samsung.com

[divya.bansal, 2014-11-14 09:09:28]

On 2014/11/14 09:08:10, Bernhard Bauer wrote:
> LGTM!

@Bernhard Thanks for the review.

[commit-bot: I haz the power, 2014-11-14 09:09:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/724683002/40001

[commit-bot: I haz the power, 2014-11-14 09:59:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-14 09:59:45]

Patchset 3 (id:??) landed as
https://crrev.com/2d8391c6f62277cd6319fb53bfb36c5ccf395392
Cr-Commit-Position: refs/heads/master@{#304189}