Signin screen and locked screen status area focus advancing

Signin screen and locked screen status area focus advancing

This CL is aimed at solving the bug described in comment 7 in crbug.com/468969. The original method only considered the system tray alone.

After the fix:

On signin screen, tabing focus order is:
three dots -> system tray -> (virtual keyboard) -> user login
shift-tabing focus order is:
user login -> (virtual keyboard) -> system tray -> three dots

On locked screen, tabing focus order is:
signout -> system tray -> notification tray -> (virtual keyboard) -> user login
reverse tabing focus order is:
user login -> (virtual keyboard) -> notification tray -> system tray -> signout

BUG=468969
TEST=device test, works good on the above description.

Committed: https://crrev.com/e9224222f960f7ab2406abf05a3bb95289994f98
Cr-Commit-Position: refs/heads/master@{#419234}

[Qiang(Joe) Xu, 2016-09-02 05:01:39]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 05:02:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2016-09-02 05:12:04]

Description was changed from

==========
Signin screen and locked screen status area focus advancing

BUG=468969
==========

to

==========
Signin screen and locked screen status area focus advancing

This CL is aimed at solving the bug described in comment 7 in crbug.com/468969.
The original method only considered the system tray alone.

After the fix:

On signin screen, tabing focus order is:
three dots -> system tray -> (virtual keyboard) -> user login
shift-tabing focus order is:
user login -> system tray -> (virtual keyboard) -> three dots

On locked screen, tabing focus order is:
signout -> system tray -> notification tray -> (virtual keyboard) -> user login
reverse tabing focus order is:
user login -> system tray -> (virtual keyboard) -> notification tray -> signout

system tray is always the first tab stop because it is the default focus.

BUG=468969
TEST=device test, works good on the above description.
==========

[Qiang(Joe) Xu, 2016-09-02 05:12:04]

warx@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Qiang(Joe) Xu, 2016-09-02 05:15:59]

Hi Steven, could you please take a look? Thanks!

I am not sure if there is any better way to solve this problem. I proposed using
observer to notify WebUILoginView. It is better that we can call
WebUILoginView's method in status_area_widget, but ash/ could not depend on
chrome/.

[commit-bot: I haz the power, 2016-09-02 05:45:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 05:45:52]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-09-02 16:29:53]

stevenjb@chromium.org changed reviewers:
+ xiyuan@chromium.org

[stevenjb, 2016-09-02 16:29:53]

xiyuan@, can you also take a look? The approach seems reasonable to me, but I
haven't been in Views code in a while...

https://codereview.chromium.org/2295843006/diff/1/ash/common/system/status_ar...
File ash/common/system/status_area_widget.cc (right):

https://codereview.chromium.org/2295843006/diff/1/ash/common/system/status_ar...
ash/common/system/status_area_widget.cc:230:
views::FocusManager::IsTabTraversalKeyEvent(*event)) {
Combine these ifs, invert the logic, call Widget::OnKeyEvent(event), and early
exit.

https://codereview.chromium.org/2295843006/diff/1/ash/common/system/status_ar...
ash/common/system/status_area_widget.cc:236: }
Like this :)

https://codereview.chromium.org/2295843006/diff/1/ash/common/system/status_ar...
ash/common/system/status_area_widget.cc:251: return;
And this. Also no else after a return.

Also, you may want to put all of this in a helper function, e.g.:

void StatusAreaWidget::OnKeyEvent(ui::KeyEvent* event) {
  HandleFocusKeyEvent(event);
  Widget::OnKeyEvent(event);
}

Then HandleFocusKeyEvent() can just return without having to call
Widget::OnKeyEvent().

[Qiang(Joe) Xu, 2016-09-02 19:32:51]

New patch is uploaded based on Steven's comments. Let us wait and see how xiyuan
thinks. Thanks!

https://codereview.chromium.org/2295843006/diff/1/ash/common/system/status_ar...
File ash/common/system/status_area_widget.cc (right):

https://codereview.chromium.org/2295843006/diff/1/ash/common/system/status_ar...
ash/common/system/status_area_widget.cc:230:
views::FocusManager::IsTabTraversalKeyEvent(*event)) {
On 2016/09/02 16:29:53, stevenjb wrote:
> Combine these ifs, invert the logic, call Widget::OnKeyEvent(event), and early
> exit.
> 

Done.

https://codereview.chromium.org/2295843006/diff/1/ash/common/system/status_ar...
ash/common/system/status_area_widget.cc:236: }
On 2016/09/02 16:29:53, stevenjb wrote:
> Like this :)

Done.

https://codereview.chromium.org/2295843006/diff/1/ash/common/system/status_ar...
ash/common/system/status_area_widget.cc:251: return;
On 2016/09/02 16:29:53, stevenjb wrote:
> And this. Also no else after a return.
> 
> Also, you may want to put all of this in a helper function, e.g.:
> 
> void StatusAreaWidget::OnKeyEvent(ui::KeyEvent* event) {
>   HandleFocusKeyEvent(event);
>   Widget::OnKeyEvent(event);
> }
> 
> Then HandleFocusKeyEvent() can just return without having to call
> Widget::OnKeyEvent().
> 

Combine this return seems to add code lines for OS_CHROMEOS macros and if
condition. I leave it here.

Done for HandleFocusKeyEvent().

[stevenjb, 2016-09-02 20:05:32]

This lgtm w/ nits, but please wait for xiyuan@ to also take a look next wee when
he is back, thanks.

https://codereview.chromium.org/2295843006/diff/20001/ash/common/system/statu...
File ash/common/system/status_area_widget.cc (right):

https://codereview.chromium.org/2295843006/diff/20001/ash/common/system/statu...
ash/common/system/status_area_widget.cc:308: return;
{}

https://codereview.chromium.org/2295843006/diff/20001/ash/common/system/statu...
ash/common/system/status_area_widget.cc:333: notify_return_focus = true;
{}

[xiyuan, 2016-09-06 22:02:44]

Is it possible to fix the issue by setting WebUILoginView as focus traversable
parent of StatusAreaWidget ? If that works, then we would not need to mess with
keyboard events and knows how many tray views are there.

[Qiang(Joe) Xu, 2016-09-09 00:11:15]

Hi xiyuan, I investigated this bug today. SetFocusTraversableParent doesn't
really help. If we could define/override the behavior of
FocusManager::FindFocusableView for status_area_widget, that might help, maybe
overriding FocusSearch for status_area_widget_delegate. But I still don't have a
clear idea now.

I send this update because I clean the logic a bit. Now, we don't rely on how
many tray views there are. But still need the help from keyboard event. How does
this sound to you?

https://codereview.chromium.org/2295843006/diff/20001/ash/common/system/statu...
File ash/common/system/status_area_widget.cc (right):

https://codereview.chromium.org/2295843006/diff/20001/ash/common/system/statu...
ash/common/system/status_area_widget.cc:308: return;
On 2016/09/02 20:05:32, stevenjb wrote:
> {}

Done.

https://codereview.chromium.org/2295843006/diff/20001/ash/common/system/statu...
ash/common/system/status_area_widget.cc:333: notify_return_focus = true;
On 2016/09/02 20:05:32, stevenjb wrote:
> {}

Done.

[xiyuan, 2016-09-12 18:19:09]

I have created a CL that is based on focus traversable parent:
  https://codereview.chromium.org/2332863002/

It requires more work though,

e.g. shift-tab on lock screen:
  password -> System Tray -> Signout

The notification tray is skipped because the CL uses a FocusSearch that does not
cycle for StatusAreaWidget so that focus traversal parent is used at the end of
focus chain.

https://codereview.chromium.org/2295843006/diff/40001/ash/common/system/statu...
File ash/common/system/status_area_focus_observer.h (right):

https://codereview.chromium.org/2295843006/diff/40001/ash/common/system/statu...
ash/common/system/status_area_focus_observer.h:19: virtual void
OnWillReturnFocusToWebContents(bool reverse) = 0;
nit: OnWillReturnFocusToWebContents -> OnFocusOut

The thinking is that StatusAreaFocusObserver should be generic and does not need
to know how it is used. The comment could be something like this:
  // Invoked when the FocusManager of StatusAreaWidget has done
  // tabbing through its children.

https://codereview.chromium.org/2295843006/diff/40001/ash/common/system/statu...
File ash/common/system/status_area_widget.cc (right):

https://codereview.chromium.org/2295843006/diff/40001/ash/common/system/statu...
ash/common/system/status_area_widget.cc:227: Widget::OnKeyEvent(event);
nit: Skip if event->handled() is true.

[Qiang(Joe) Xu, 2016-09-13 00:08:34]

Hi, xiyuan@, I patched your CL and figured out a way to fixing shift-tab
skipping notification tray and virtual keyboard tray, etc.

Solution is when status area widget is going to take focus from web contents,
depending reverse, the default focus child is either the first focusable child
(when !reverse) or the last focusable child (when reverse).

This changes the original signin/locked screen default focus, which is always
system tray. This change may need the PM's confirm. What do you think?

The logged screen behavior is not changed. As user presses Ctrl-F1, we want the
default focus to be system tray always I believe.

[Qiang(Joe) Xu, 2016-09-13 00:08:44]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 00:09:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2016-09-13 00:11:42]

Description was changed from

==========
Signin screen and locked screen status area focus advancing

This CL is aimed at solving the bug described in comment 7 in crbug.com/468969.
The original method only considered the system tray alone.

After the fix:

On signin screen, tabing focus order is:
three dots -> system tray -> (virtual keyboard) -> user login
shift-tabing focus order is:
user login -> system tray -> (virtual keyboard) -> three dots

On locked screen, tabing focus order is:
signout -> system tray -> notification tray -> (virtual keyboard) -> user login
reverse tabing focus order is:
user login -> system tray -> (virtual keyboard) -> notification tray -> signout

system tray is always the first tab stop because it is the default focus.

BUG=468969
TEST=device test, works good on the above description.
==========

to

==========
Signin screen and locked screen status area focus advancing

This CL is aimed at solving the bug described in comment 7 in crbug.com/468969.
The original method only considered the system tray alone.

After the fix:

On signin screen, tabing focus order is:
three dots -> system tray -> (virtual keyboard) -> user login
shift-tabing focus order is:
user login -> (virtual keyboard) -> system tray -> three dots

On locked screen, tabing focus order is:
signout -> system tray -> notification tray -> (virtual keyboard) -> user login
reverse tabing focus order is:
user login -> (virtual keyboard) -> notification tray -> system tray -> signout

BUG=468969
TEST=device test, works good on the above description.
==========

[commit-bot: I haz the power, 2016-09-13 00:45:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 00:45:29]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2016-09-13 18:00:57]

lgtm

stevenjb@, you might want to take another look since the CL is changed
significantly. :p It is now using customized focus traversable/search rather
than messing with keyboard events.

> This changes the original signin/locked screen default focus, which is always
> system tray. This change may need the PM's confirm. What do you think?

I think this is okay. But get a PM to approve before submitting.

https://codereview.chromium.org/2295843006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2295843006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/webui_login_view.cc:353:
tray_widget->GetContentsView());
nit: Let's create a helper function to get this StatusAreaWidgetDelegate since
the code is used in two places.

[stevenjb, 2016-09-13 21:41:38]

For the new changes I will defer to xiyuan@, but at a glance this lgtm.

[stevenjb, 2016-09-13 21:42:22]

(Assuming tbuckley@ or another PM approves the change in focus behavior)

[Qiang(Joe) Xu, 2016-09-13 21:44:57]

I updated the patch with the helper method GetStatusAreaWidgetDelegate. I
contacted lpalmaro@, once she confirmed. I think it is ready to be landed.

https://codereview.chromium.org/2295843006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2295843006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/webui_login_view.cc:353:
tray_widget->GetContentsView());
On 2016/09/13 18:00:57, xiyuan wrote:
> nit: Let's create a helper function to get this StatusAreaWidgetDelegate since
> the code is used in two places.

Done.

[xiyuan, 2016-09-13 21:48:08]

https://codereview.chromium.org/2295843006/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/webui_login_view.h (right):

https://codereview.chromium.org/2295843006/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/webui_login_view.h:149:
ash::StatusAreaWidgetDelegate* GetStatusAreaWidgetDelegate();
Move this to cc file in anonymous namespace since it is impl detail and outside
world does not need to know about it.

[Qiang(Joe) Xu, 2016-09-13 21:54:57]

https://codereview.chromium.org/2295843006/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/webui_login_view.h (right):

https://codereview.chromium.org/2295843006/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/webui_login_view.h:149:
ash::StatusAreaWidgetDelegate* GetStatusAreaWidgetDelegate();
On 2016/09/13 21:48:08, xiyuan wrote:
> Move this to cc file in anonymous namespace since it is impl detail and
outside
> world does not need to know about it.

done

[xiyuan, 2016-09-13 21:56:46]

Sorry for the spamming. I should have looked more closely last time. :p

https://codereview.chromium.org/2295843006/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2295843006/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/webui_login_view.cc:522: if
(status_area_widget_delegate && status_area_widget_delegate->visible()) {
status_area_widget_delegate->visible() is not the same thing as its Widget's
visibility. We should do

  if (status_area_widget_delegate &&
status_area_widget_delegate->GetWidget()->IsVisible())

[Qiang(Joe) Xu, 2016-09-13 22:07:01]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[Qiang(Joe) Xu, 2016-09-13 22:07:21]

Thanks, it is updated.

https://codereview.chromium.org/2295843006/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2295843006/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/webui_login_view.cc:522: if
(status_area_widget_delegate && status_area_widget_delegate->visible()) {
On 2016/09/13 21:56:46, xiyuan wrote:
> status_area_widget_delegate->visible() is not the same thing as its Widget's
> visibility. We should do
> 
>   if (status_area_widget_delegate &&
> status_area_widget_delegate->GetWidget()->IsVisible())

done. Thanks, good to know this.

[commit-bot: I haz the power, 2016-09-13 22:07:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2016-09-13 22:08:39]

lgtm!

[commit-bot: I haz the power, 2016-09-13 22:54:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 22:54:50]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2016-09-16 17:39:38]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2016-09-16 17:39:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2295843006/#ps160001
(title: "GetWidget->IsVisible()")

[commit-bot: I haz the power, 2016-09-16 17:40:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 18:56:50]

Description was changed from

==========
Signin screen and locked screen status area focus advancing

This CL is aimed at solving the bug described in comment 7 in crbug.com/468969.
The original method only considered the system tray alone.

After the fix:

On signin screen, tabing focus order is:
three dots -> system tray -> (virtual keyboard) -> user login
shift-tabing focus order is:
user login -> (virtual keyboard) -> system tray -> three dots

On locked screen, tabing focus order is:
signout -> system tray -> notification tray -> (virtual keyboard) -> user login
reverse tabing focus order is:
user login -> (virtual keyboard) -> notification tray -> system tray -> signout

BUG=468969
TEST=device test, works good on the above description.
==========

to

==========
Signin screen and locked screen status area focus advancing

This CL is aimed at solving the bug described in comment 7 in crbug.com/468969.
The original method only considered the system tray alone.

After the fix:

On signin screen, tabing focus order is:
three dots -> system tray -> (virtual keyboard) -> user login
shift-tabing focus order is:
user login -> (virtual keyboard) -> system tray -> three dots

On locked screen, tabing focus order is:
signout -> system tray -> notification tray -> (virtual keyboard) -> user login
reverse tabing focus order is:
user login -> (virtual keyboard) -> notification tray -> system tray -> signout

BUG=468969
TEST=device test, works good on the above description.
==========

[commit-bot: I haz the power, 2016-09-16 18:56:52]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-16 18:59:01]

Description was changed from

==========
Signin screen and locked screen status area focus advancing

This CL is aimed at solving the bug described in comment 7 in crbug.com/468969.
The original method only considered the system tray alone.

After the fix:

On signin screen, tabing focus order is:
three dots -> system tray -> (virtual keyboard) -> user login
shift-tabing focus order is:
user login -> (virtual keyboard) -> system tray -> three dots

On locked screen, tabing focus order is:
signout -> system tray -> notification tray -> (virtual keyboard) -> user login
reverse tabing focus order is:
user login -> (virtual keyboard) -> notification tray -> system tray -> signout

BUG=468969
TEST=device test, works good on the above description.
==========

to

==========
Signin screen and locked screen status area focus advancing

This CL is aimed at solving the bug described in comment 7 in crbug.com/468969.
The original method only considered the system tray alone.

After the fix:

On signin screen, tabing focus order is:
three dots -> system tray -> (virtual keyboard) -> user login
shift-tabing focus order is:
user login -> (virtual keyboard) -> system tray -> three dots

On locked screen, tabing focus order is:
signout -> system tray -> notification tray -> (virtual keyboard) -> user login
reverse tabing focus order is:
user login -> (virtual keyboard) -> notification tray -> system tray -> signout

BUG=468969
TEST=device test, works good on the above description.

Committed: https://crrev.com/e9224222f960f7ab2406abf05a3bb95289994f98
Cr-Commit-Position: refs/heads/master@{#419234}
==========

[commit-bot: I haz the power, 2016-09-16 18:59:02]

Patchset 9 (id:??) landed as
https://crrev.com/e9224222f960f7ab2406abf05a3bb95289994f98
Cr-Commit-Position: refs/heads/master@{#419234}