Fix user pod not always scroll to the center after VK shows up

Fix user pod not always scroll to the center after VK shows up

User pod at login/lock screen sometimes doesn't scroll to the center after
VK shows up. The problem was we need to set keyboard state before we invoke
the resize handler at the login/lock page. But this is not always the case
due to the observer for resize is added before observer for keyboard state.
This CL simply remove the dependency to keyboard state from resize logic. The dependency was unnecessary anymore.

BUG=491214
Committed: https://crrev.com/414c6e51f82e9f1b3285f9cb482865cde3ce2f32
Cr-Commit-Position: refs/heads/master@{#337230}

[bshe, 2015-06-24 14:34:56]

bshe@chromium.org changed reviewers:
+ flackr@chromium.org

[bshe, 2015-06-24 14:34:57]

Hi Rob. Do you mind to take a look at this CL?
Please see CL description for details. Thanks!

[flackr, 2015-06-24 22:08:42]

Hey, I'm not exactly clear on how this is failing, or what the changes happening
between OnKeyboardBoundsChanging and OnKeyboardBoundsChanged are, but do we ever
actually need to know Changing or should we just always notify after the bounds
have changed?

https://codereview.chromium.org/1205063002/diff/1/ui/keyboard/keyboard_contro...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/1205063002/diff/1/ui/keyboard/keyboard_contro...
ui/keyboard/keyboard_controller.cc:259: OnKeyboardBoundsChanging(new_bounds));
Do we ever need to get this Changing notification here or could we just have the
BoundsChanged observer?

https://codereview.chromium.org/1205063002/diff/1/ui/keyboard/keyboard_contro...
File ui/keyboard/keyboard_controller_observer.h (right):

https://codereview.chromium.org/1205063002/diff/1/ui/keyboard/keyboard_contro...
ui/keyboard/keyboard_controller_observer.h:23: virtual void
OnKeyboardBoundsChanging(const gfx::Rect& new_bounds) = 0;
Provide default implementation of this one as well for consistency.

[bshe, 2015-06-25 14:01:59]

Basically, the reason I add onKeyboardBoundsChanged is to make sure
LockLayoutManager's OnWindowResized is
called after the two implementations of onKeyboardBoundsChanging in
webui_screen_lock.cc and login_display_host_impl.cc
The latter two implementations will call a javascript function called
setKeyboardState in lock/login screen. And the implementation
in LockLayoutManager will dispatch a resize event to lock/login screen. To
scroll the user pod correctly, lock/login screen
assumed setKeyboardState is called before receiving resize event. But this is
not always the case. The reason is LockLayoutManager
was added to the observer list before WebUIScreenLock and LoginDisplayHost. So
LockLayoutManager's implementation get called before
WebUIScreenLock's and LoginDisplayHost's. And it looks like resize event is not
dispatched immediately after called OnWindowResized
in LockLayoutManager. So there was a race.

Adding the onChanged function seems like an easy way to fix this race without
the need to manually make sure the order of observers
is correct. But I am open to all suggestions.

https://codereview.chromium.org/1205063002/diff/1/ui/keyboard/keyboard_contro...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/1205063002/diff/1/ui/keyboard/keyboard_contro...
ui/keyboard/keyboard_controller.cc:259: OnKeyboardBoundsChanging(new_bounds));
On 2015/06/24 22:08:42, flackr wrote:
> Do we ever need to get this Changing notification here or could we just have
the
> BoundsChanged observer?

We probably do. Or we have to make sure the observer is added to observer list
in the right order. See my comment for detail.

https://codereview.chromium.org/1205063002/diff/1/ui/keyboard/keyboard_contro...
File ui/keyboard/keyboard_controller_observer.h (right):

https://codereview.chromium.org/1205063002/diff/1/ui/keyboard/keyboard_contro...
ui/keyboard/keyboard_controller_observer.h:23: virtual void
OnKeyboardBoundsChanging(const gfx::Rect& new_bounds) = 0;
On 2015/06/24 22:08:42, flackr wrote:
> Provide default implementation of this one as well for consistency.

Done.

[bshe, 2015-06-25 17:44:15]

Patchset #4 (id:60001) has been deleted

[bshe, 2015-06-25 17:44:27]

Patchset #3 (id:40001) has been deleted

[bshe, 2015-06-25 17:46:48]

discussed with flackr offline and it seems that it is safe to remove the
dependancy to keyboard state in login/lock page. I have updated this CL. It also
include a small code cleanup. Some of the variables no longer used anywhere, so
I removed them. Rob, do you mind to take another look?

+dpolukhin for review and owner for the new CL

[flackr, 2015-06-25 18:01:37]

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/login_common.js (right):

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/login_common.js:358:
Oobe.setKeyboardState = function(shown, width, height) {
Remove the function altogether.

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right):

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:383: if
(keyboard_controller) {
Is it possible for the keyboard_controller to go away? For example, if a
physical keyboard is plugged in and we have automatic keyboard detection? If so,
I'm guessing we should always update the control bar state, just set it to true
if there is no controller.

[bshe, 2015-06-25 21:15:07]

bshe@chromium.org changed reviewers:
+ dpolukhin@chromium.org

[bshe, 2015-06-25 21:15:07]

Thanks Rob. Can you take another look please?

actually +dpolukhin

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/login_common.js (right):

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/login_common.js:358:
Oobe.setKeyboardState = function(shown, width, height) {
On 2015/06/25 18:01:37, flackr wrote:
> Remove the function altogether.

Done.

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right):

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:383: if
(keyboard_controller) {
On 2015/06/25 18:01:37, flackr wrote:
> Is it possible for the keyboard_controller to go away? For example, if a
> physical keyboard is plugged in and we have automatic keyboard detection? If
so,
> I'm guessing we should always update the control bar state, just set it to
true
> if there is no controller.

I am not sure I understand you correctly. But if keyboard_controller is null
here, the control bar should already set to visible before this code. We
probably dont need to set it again here? This only called during initialization
stage, hot plug a keyboard wont trigger this code path I think.

[flackr, 2015-06-25 21:49:28]

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right):

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:383: if
(keyboard_controller) {
On 2015/06/25 21:15:07, bshe wrote:
> On 2015/06/25 18:01:37, flackr wrote:
> > Is it possible for the keyboard_controller to go away? For example, if a
> > physical keyboard is plugged in and we have automatic keyboard detection? If
> so,
> > I'm guessing we should always update the control bar state, just set it to
> true
> > if there is no controller.
> 
> I am not sure I understand you correctly. But if keyboard_controller is null
> here, the control bar should already set to visible before this code. We
> probably dont need to set it again here? This only called during
initialization
> stage, hot plug a keyboard wont trigger this code path I think. 

So my thoughts are we are on the lock screen with virtual keyboard showing so we
hide the control bar. Then the user plugs in a usb keyboard which calls
Shell::DeactivateKeyboard() which destroys the keyboard controller and we get in
here and don't reshow the control bar.

[Dmitry Polukhin, 2015-06-26 08:59:20]

dpolukhin@chromium.org changed reviewers:
+ dzhioev@chromium.org

[Dmitry Polukhin, 2015-06-26 08:59:21]

+ Pavel who reviewed CL that added this code
https://codereview.chromium.org/304153004

[bshe, 2015-06-26 17:22:48]

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right):

https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:383: if
(keyboard_controller) {
On 2015/06/25 21:49:28, flackr wrote:
> On 2015/06/25 21:15:07, bshe wrote:
> > On 2015/06/25 18:01:37, flackr wrote:
> > > Is it possible for the keyboard_controller to go away? For example, if a
> > > physical keyboard is plugged in and we have automatic keyboard detection?
If
> > so,
> > > I'm guessing we should always update the control bar state, just set it to
> > true
> > > if there is no controller.
> > 
> > I am not sure I understand you correctly. But if keyboard_controller is null
> > here, the control bar should already set to visible before this code. We
> > probably dont need to set it again here? This only called during
> initialization
> > stage, hot plug a keyboard wont trigger this code path I think. 
> 
> So my thoughts are we are on the lock screen with virtual keyboard showing so
we
> hide the control bar. Then the user plugs in a usb keyboard which calls
> Shell::DeactivateKeyboard() which destroys the keyboard controller and we get
in
> here and don't reshow the control bar.

I see what you mean. When shell deactivates keyboard, we send a bounds changed
notification before reset the keyboard controller instance.
(https://code.google.com/p/chromium/codesearch/#chromium/src/ash/root_window_c...)
After we send the notification, the visibility of control bar should be set
correctly by WebUIScreenLocker or LoginDisplayHostImpl in
onKeyboardBoundsChanging

[dzhioev (left Google), 2015-06-26 19:31:03]

Hello, 
generally LGTM. However I'm not too familiar wit this code. Could you please
double check that you CL works fine for the following cases:
1. Single pod case. In this case we focus the only pod automatically, right
after displaying it. I afraid there could be some issues with calculating
bounding rectangles during transition period.
2. Desktop user manager (user switching UI in desktop Chrome). This UI uses
user_pod_row.js for displaying user pods, but its html structure slightly
differs from Login/Lock UI, that's why I'm not sure that calling
scrollFocusedPodIntoView() will not break something there.

[flackr, 2015-06-26 19:31:59]

On 2015/06/26 17:22:48, bshe wrote:
>
https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right):
> 
>
https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:383: if
> (keyboard_controller) {
> On 2015/06/25 21:49:28, flackr wrote:
> > On 2015/06/25 21:15:07, bshe wrote:
> > > On 2015/06/25 18:01:37, flackr wrote:
> > > > Is it possible for the keyboard_controller to go away? For example, if a
> > > > physical keyboard is plugged in and we have automatic keyboard
detection?
> If
> > > so,
> > > > I'm guessing we should always update the control bar state, just set it
to
> > > true
> > > > if there is no controller.
> > > 
> > > I am not sure I understand you correctly. But if keyboard_controller is
null
> > > here, the control bar should already set to visible before this code. We
> > > probably dont need to set it again here? This only called during
> > initialization
> > > stage, hot plug a keyboard wont trigger this code path I think. 
> > 
> > So my thoughts are we are on the lock screen with virtual keyboard showing
so
> we
> > hide the control bar. Then the user plugs in a usb keyboard which calls
> > Shell::DeactivateKeyboard() which destroys the keyboard controller and we
get
> in
> > here and don't reshow the control bar.
> 
> I see what you mean. When shell deactivates keyboard, we send a bounds changed
> notification before reset the keyboard controller instance.
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/ash/root_window_c...)
> After we send the notification, the visibility of control bar should be set
> correctly by WebUIScreenLocker or LoginDisplayHostImpl in
> onKeyboardBoundsChanging

I see, makes sense :-). I don't suppose we have tests for this code do we?

[dzhioev (left Google), 2015-06-26 19:39:31]

Most likely we don't have tests for that =(

On Fri, Jun 26, 2015 at 12:32 PM <flackr@chromium.org> wrote:

> On 2015/06/26 17:22:48, bshe wrote:
>
>
>
https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
> > File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right):
>
>
>
>
https://codereview.chromium.org/1205063002/diff/80001/chrome/browser/ui/webui...
> > chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:383: if
> > (keyboard_controller) {
> > On 2015/06/25 21:49:28, flackr wrote:
> > > On 2015/06/25 21:15:07, bshe wrote:
> > > > On 2015/06/25 18:01:37, flackr wrote:
> > > > > Is it possible for the keyboard_controller to go away? For example,
> > if a
> > > > > physical keyboard is plugged in and we have automatic keyboard
> detection?
> > If
> > > > so,
> > > > > I'm guessing we should always update the control bar state, just
> > set it
> to
> > > > true
> > > > > if there is no controller.
> > > >
> > > > I am not sure I understand you correctly. But if keyboard_controller
> > is
> null
> > > > here, the control bar should already set to visible before this code.
> > We
> > > > probably dont need to set it again here? This only called during
> > > initialization
> > > > stage, hot plug a keyboard wont trigger this code path I think.
> > >
> > > So my thoughts are we are on the lock screen with virtual keyboard
> > showing
> so
> > we
> > > hide the control bar. Then the user plugs in a usb keyboard which calls
> > > Shell::DeactivateKeyboard() which destroys the keyboard controller and
> > we
> get
> > in
> > > here and don't reshow the control bar.
>
> > I see what you mean. When shell deactivates keyboard, we send a bounds
> > changed
> > notification before reset the keyboard controller instance.
>
> (
>
https://code.google.com/p/chromium/codesearch/#chromium/src/ash/root_window_c...
> )
> > After we send the notification, the visibility of control bar should be
> > set
> > correctly by WebUIScreenLocker or LoginDisplayHostImpl in
> > onKeyboardBoundsChanging
>
> I see, makes sense :-). I don't suppose we have tests for this code do we?
>
> https://codereview.chromium.org/1205063002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bshe, 2015-06-26 21:50:09]

On 2015/06/26 19:31:03, dzhioev wrote:
> Hello, 
> generally LGTM. However I'm not too familiar wit this code. Could you please
> double check that you CL works fine for the following cases:
> 1. Single pod case. In this case we focus the only pod automatically, right
> after displaying it. I afraid there could be some issues with calculating
> bounding rectangles during transition period.
> 2. Desktop user manager (user switching UI in desktop Chrome). This UI uses
> user_pod_row.js for displaying user pods, but its html structure slightly
> differs from Login/Lock UI, that's why I'm not sure that calling
> scrollFocusedPodIntoView() will not break something there.

I tested single pod case and it all seems working fine. I also did test screen
rotation on minnie. The user pod is positioned correctly after my change.
On desktop, I tested chromium on linux and it still looks good to me.
The code in scrollFocusedPodIntoView actually wont do anything if no VK is
presented,
as this check should be true
(https://code.google.com/p/chromium/codesearch/#chromium/src/ui/login/account_...).

[flackr, 2015-06-26 23:22:39]

LGTM

[bshe, 2015-07-02 15:34:46]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2015-07-02 15:35:14]

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

[commit-bot: I haz the power, 2015-07-02 16:32:11]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-07-02 16:33:12]

Patchset 4 (id:??) landed as
https://crrev.com/414c6e51f82e9f1b3285f9cb482865cde3ce2f32
Cr-Commit-Position: refs/heads/master@{#337230}