A keyboard widget that manages itself (the animation and all that).

A keyboard widget that manages itself (the animation and all that).

At the moment, the keyboard widget manages itself pretty well. But external components can still control its visibility (and it is necessary at the moment too); the usage is simple: ShowKeyboardForWidget and Hide. The widget currently does its own animation, but eventually, perhaps the window-manager will take care of it.

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97740

[Ian Vollick, 2011-07-04 20:12:00]

http://codereview.chromium.org/7302015/diff/2001/chrome/browser/ui/touch/keyb...
File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right):

http://codereview.chromium.org/7302015/diff/2001/chrome/browser/ui/touch/keyb...
chrome/browser/ui/touch/keyboard/keyboard_manager.cc:72: 
If the UI is rotated on its side, will setting the transform here clobber the
rotation (or is the rotation inherited)? If so, you might want to combine the
translation w/ an InterpolatedConstantTransform (for the old transformation).

Nit: 0.0 - 1.0 is the default, so you should be able to omit the last two
params.

[sadrul, 2011-07-04 20:21:19]

http://codereview.chromium.org/7302015/diff/2001/chrome/browser/ui/touch/keyb...
File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right):

http://codereview.chromium.org/7302015/diff/2001/chrome/browser/ui/touch/keyb...
chrome/browser/ui/touch/keyboard/keyboard_manager.cc:72: 
On 2011/07/04 20:12:00, vollick wrote:
> If the UI is rotated on its side, will setting the transform here clobber the
> rotation (or is the rotation inherited)? If so, you might want to combine the
> translation w/ an InterpolatedConstantTransform (for the old transformation).

The way orientation will happen, in my mind, is that the transform will not be
set on the individual Widgets' RootViews, but on the very rooty RootView, i.e.
the RootView of the views::DesktopWindow. So setting a transform on individual
Widget's (this one and others) will not conflict with the transform used for the
orientation.

> Nit: 0.0 - 1.0 is the default, so you should be able to omit the last two
> params.

Cool. Done for next revision of the CL.

[sadrul, 2011-07-05 06:03:38]

@bryeung: There are a couple of comments in patchset 6 that I would like you to
look at.

@sky: Please take a look at the whole thing.

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/touch_login_view.cc (right):

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/touch_login_view.cc:15:
KeyboardManager::GetInstance();
Perhaps this can just be done in WebUILoginView with an ifdef, so we can remove
TouchLoginView?

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/fram...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/fram...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:67:
GetFocusManager()->RemoveFocusChangeListener(this);
@sky: I would really love to have a way to add/remove FocusChangeListener to a
FocusManager for any arbitrary Widget, and not just a Widget that contains a
TouchBrowserFrameView. Can you suggest a way of doing this? Perhaps have
ViewsDelegate::OnWidgetCreated or something? (somewhat equivalent to
NotificationType::BROWSER_OPENED, only in views parlance)

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/keyb...
File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right):

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/keyb...
chrome/browser/ui/touch/keyboard/keyboard_manager.cc:250:
views::TextInputClient* input =
@bryeung: We used to look at the View::GetClassName to decide whether or not to
show the keyboard. I am using the TextInputClient here instead; I think this is
better. Would you agree?

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/keyb...
File chrome/browser/ui/touch/keyboard/keyboard_manager.h (right):

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/keyb...
chrome/browser/ui/touch/keyboard/keyboard_manager.h:54: // TODO(sad): Allow
specifying the type of keyboard to show.
@bryeung: I have not added VirtualKeyboardType from TouchBrowserFrameView in
here. This is intentional, as I think it'd be better to add that if and when we
need it (or perhaps we can just use ui::TextInputType to decide what kind of
keyboard to display). This sounds OK to you?

[bryeung, 2011-07-05 14:23:12]

Added Peng to the reviewers, as he's in the middle of refactoring how keyboard
visibility is managed.

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/touch_login_view.cc (right):

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/touch_login_view.cc:15:
KeyboardManager::GetInstance();
On 2011/07/05 06:03:38, sadrul wrote:
> Perhaps this can just be done in WebUILoginView with an ifdef, so we can
remove
> TouchLoginView?

That sounds better to me: seems silly to be subclassing for this single line
now.

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/keyb...
File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right):

http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/keyb...
chrome/browser/ui/touch/keyboard/keyboard_manager.cc:250:
views::TextInputClient* input =
On 2011/07/05 06:03:38, sadrul wrote:
> @bryeung: We used to look at the View::GetClassName to decide whether or not
to
> show the keyboard. I am using the TextInputClient here instead; I think this
is
> better. Would you agree?

Actually: all of this logic is going to change/go away in the near future.  Peng
is working on refactoring things so that the IME has full control over keyboard
visibility.

http://codereview.chromium.org/7217008/

I'm going to add him to this review.

[Peng, 2011-07-05 22:13:24]

I tested this CL and found several problems.

1. The keyboard widget is transparent, and it is almost invisible.
2. When the keyboard is visible, the web page will not re layout. And the entry
control may be covered by the keyboard widget.

And I like this CL. It makes the show/hide logic simpler. If this CL is landed,
I  will modify my CL base on it.

On 2011/07/05 14:23:12, bryeung wrote:
> Added Peng to the reviewers, as he's in the middle of refactoring how keyboard
> visibility is managed.
> 
>
http://codereview.chromium.org/7302015/diff/8005/chrome/browser/chromeos/logi...
> File chrome/browser/chromeos/login/touch_login_view.cc (right):
> 
>
http://codereview.chromium.org/7302015/diff/8005/chrome/browser/chromeos/logi...
> chrome/browser/chromeos/login/touch_login_view.cc:15:
> KeyboardManager::GetInstance();
> On 2011/07/05 06:03:38, sadrul wrote:
> > Perhaps this can just be done in WebUILoginView with an ifdef, so we can
> remove
> > TouchLoginView?
> 
> That sounds better to me: seems silly to be subclassing for this single line
> now.
> 
>
http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/keyb...
> File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right):
> 
>
http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/keyb...
> chrome/browser/ui/touch/keyboard/keyboard_manager.cc:250:
> views::TextInputClient* input =
> On 2011/07/05 06:03:38, sadrul wrote:
> > @bryeung: We used to look at the View::GetClassName to decide whether or not
> to
> > show the keyboard. I am using the TextInputClient here instead; I think this
> is
> > better. Would you agree?
> 
> Actually: all of this logic is going to change/go away in the near future. 
Peng
> is working on refactoring things so that the IME has full control over
keyboard
> visibility.
> 
> http://codereview.chromium.org/7217008/
> 
> I'm going to add him to this review.

[sadrul, 2011-07-05 22:24:23]

On 2011/07/05 22:13:24, Peng wrote:
> I tested this CL and found several problems.
> 
> 1. The keyboard widget is transparent, and it is almost invisible.

Can you please run with '--views-desktop' and see if that helps?

> 2. When the keyboard is visible, the web page will not re layout.

Indeed. That is the desired behaviour.

> And the entry
> control may be covered by the keyboard widget.

We (Fady, Rob, myself) discussed this a bit; some changes on the WebKit side
will be necessary to fix this (the necessary change is in the works). I will add
a TODO noting that in this CL.

> And I like this CL. It makes the show/hide logic simpler. If this CL is
landed,
> I  will modify my CL base on it.

That's awesome. Thanks!

[sadrul, 2011-07-06 02:35:15]

[snip]
> 
> > And the entry
> > control may be covered by the keyboard widget.
> 
> We (Fady, Rob, myself) discussed this a bit; some changes on the WebKit side
> will be necessary to fix this (the necessary change is in the works). I will
add
> a TODO noting that in this CL.

Come to think of it, the notifications added in
http://codereview.chromium.org/7212029/ should be sufficient to send the
necessary message back to webkit, so the KeyboardManager wouldn't need to do
anything else.

[sky, 2011-07-06 15:08:57]

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:25:
KeyboardManager::GetInstance();
This looks unused. Add a comment as to why it is here.

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:36: views::Widget*
widget = focused_now ? focused_now->GetWidget() : NULL;
Did you consider having KeyboardManager track this too?

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/key...
File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right):

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/key...
chrome/browser/ui/touch/keyboard/keyboard_manager.cc:39: bool
TabContentsCanAffectKeyboard(const TabContents* tab_contents) {
Add a description.

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/key...
chrome/browser/ui/touch/keyboard/keyboard_manager.cc:208: // TODO(sad): Find a
better way. Perhaps just return NULL, and fix
Do you want BrowserList::GetLastActive here?

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/key...
chrome/browser/ui/touch/keyboard/keyboard_manager.cc:334: case
NotificationType::BROWSER_OPENED: {
I think KeyboardManager should be a BrowserList::Observer rather than listening
for opened/closing.

[sadrul, 2011-07-06 15:38:35]

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:25:
KeyboardManager::GetInstance();
On 2011/07/06 15:08:57, sky wrote:
> This looks unused. Add a comment as to why it is here.

Done.

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:36: views::Widget*
widget = focused_now ? focused_now->GetWidget() : NULL;
On 2011/07/06 15:08:57, sky wrote:
> Did you consider having KeyboardManager track this too?

I did. Unfortunately, I couldn't see a way to automatically connect to the
focusmanager of the Widgets
(http://codereview.chromium.org/7302015/diff/8005/chrome/browser/ui/touch/fram...).

This will, however, be removed shortly, with
http://codereview.chromium.org/7217008/, when a singleton InputMethodManager
object will dictate the visibility of the keyboard, and the singleton
KeyboardManager will track only that.

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/key...
File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right):

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/key...
chrome/browser/ui/touch/keyboard/keyboard_manager.cc:39: bool
TabContentsCanAffectKeyboard(const TabContents* tab_contents) {
On 2011/07/06 15:08:57, sky wrote:
> Add a description.

Done.

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/key...
chrome/browser/ui/touch/keyboard/keyboard_manager.cc:208: // TODO(sad): Find a
better way. Perhaps just return NULL, and fix
On 2011/07/06 15:08:57, sky wrote:
> Do you want BrowserList::GetLastActive here?

Cool. I think that'd be better. Done.

http://codereview.chromium.org/7302015/diff/10001/chrome/browser/ui/touch/key...
chrome/browser/ui/touch/keyboard/keyboard_manager.cc:334: case
NotificationType::BROWSER_OPENED: {
On 2011/07/06 15:08:57, sky wrote:
> I think KeyboardManager should be a BrowserList::Observer rather than
listening
> for opened/closing.

Ah, nice. Didn't know about BrowserList::Observer. Made that change. Thanks!

[sadrul, 2011-07-06 15:41:19]

http://codereview.chromium.org/7302015/diff/14001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/touch_login_view.cc (right):

http://codereview.chromium.org/7302015/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/touch_login_view.cc:15:
KeyboardManager::GetInstance();
@sky: I agree with the bryeung that having TouchLoginView just for this looks
like an overkill. Should I just initialize the KeyboardManager with an ifdef
inside WebUILoginView and remove TouchLoginView?

[sky, 2011-07-06 16:07:01]

http://codereview.chromium.org/7302015/diff/14001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/touch_login_view.cc (right):

http://codereview.chromium.org/7302015/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/touch_login_view.cc:15:
KeyboardManager::GetInstance();
On 2011/07/06 15:41:19, sadrul wrote:
> @sky: I agree with the bryeung that having TouchLoginView just for this looks
> like an overkill. Should I just initialize the KeyboardManager with an ifdef
> inside WebUILoginView and remove TouchLoginView?

SGTM

[sadrul, 2011-07-06 16:40:12]

http://codereview.chromium.org/7302015/diff/14001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/touch_login_view.cc (right):

http://codereview.chromium.org/7302015/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/touch_login_view.cc:15:
KeyboardManager::GetInstance();
On 2011/07/06 16:07:01, sky wrote:
> On 2011/07/06 15:41:19, sadrul wrote:
> > @sky: I agree with the bryeung that having TouchLoginView just for this
looks
> > like an overkill. Should I just initialize the KeyboardManager with an ifdef
> > inside WebUILoginView and remove TouchLoginView?
> 
> SGTM

Done. Thanks!

[sky, 2011-07-06 16:50:12]

LGTM

[Peng, 2011-07-06 17:50:34]

http://codereview.chromium.org/7302015/diff/16002/chrome/browser/ui/touch/key...
File chrome/browser/ui/touch/keyboard/keyboard_manager.h (right):

http://codereview.chromium.org/7302015/diff/16002/chrome/browser/ui/touch/key...
chrome/browser/ui/touch/keyboard/keyboard_manager.h:115: DOMView* dom_view_;
Maybe should use scoped_ptr here? Or delete it in destructor? Or maybe just use
a class instance instead of pointer?

[sadrul, 2011-07-06 18:54:18]

http://codereview.chromium.org/7302015/diff/16002/chrome/browser/ui/touch/key...
File chrome/browser/ui/touch/keyboard/keyboard_manager.h (right):

http://codereview.chromium.org/7302015/diff/16002/chrome/browser/ui/touch/key...
chrome/browser/ui/touch/keyboard/keyboard_manager.h:115: DOMView* dom_view_;
On 2011/07/06 17:50:34, Peng wrote:
> Maybe should use scoped_ptr here? Or delete it in destructor? Or maybe just
use
> a class instance instead of pointer?

The Widget/RootView will take ownership of the DOMView after SetContentsView,
and it should get destroyed with the Widget/RootView. Does that not happen?

[Peng, 2011-07-06 19:47:07]

LGTM.