Signin screen and locked screen status area focus advancing

ash/common/system/status_area_widget.cc

Index: ash/common/system/status_area_widget.cc
diff --git a/ash/common/system/status_area_widget.cc b/ash/common/system/status_area_widget.cc
index cf5ae68e1a7ff2b35187053b56b02bf069553dae..584f2bd37373e562296b595b589a8d1939167803 100644
--- a/ash/common/system/status_area_widget.cc
+++ b/ash/common/system/status_area_widget.cc
@@ -11,6 +11,7 @@
 #include "ash/common/system/status_area_widget_delegate.h"
 #include "ash/common/system/tray/system_tray.h"
 #include "ash/common/system/tray/system_tray_delegate.h"
+#include "ash/common/system/tray/system_tray_notifier.h"
 #include "ash/common/system/web_notification/web_notification_tray.h"
 #include "ash/common/wm_lookup.h"
 #include "ash/common/wm_root_window_controller.h"
@@ -18,6 +19,7 @@
 #include "ash/common/wm_window.h"
 #include "base/i18n/time_formatting.h"
 #include "ui/native_theme/native_theme_dark_aura.h"
+#include "ui/views/focus/focus_manager.h"
 
 #if defined(OS_CHROMEOS)
 #include "ash/common/system/chromeos/ime_menu/ime_menu_tray.h"
@@ -219,6 +221,53 @@ void StatusAreaWidget::OnNativeWidgetActivationChanged(bool active) {
     status_area_widget_delegate_->SetPaneFocusAndFocusDefault();
 }
 
+void StatusAreaWidget::OnKeyEvent(ui::KeyEvent* event) {
+  WmShell* wm_shell = WmShell::Get();
+  SystemTrayDelegate* delegate = wm_shell->system_tray_delegate();
+  LoginStatus status = delegate->GetUserLoginStatus();
+  if (status == LoginStatus::NOT_LOGGED_IN || status == LoginStatus::LOCKED) {
+    if (event->type() == ui::ET_KEY_PRESSED &&
+        views::FocusManager::IsTabTraversalKeyEvent(*event)) {

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

Combine these ifs, invert the logic, call Widget::OnKeyEvent(event), and early
exit.

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

Done.

+      bool reverse = event->IsShiftDown();
+      views::FocusManager* focus_manager = GetFocusManager();
+      if (!focus_manager) {
+        Widget::OnKeyEvent(event);
+        return;
+      }

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

Like this :)

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

Done.

+      views::View* next_focusable_view = focus_manager->GetNextFocusableView(
+          focus_manager->GetFocusedView(), nullptr, false, false);
+#if defined(OS_CHROMEOS)
+      // When |system_tray_|, |web_notification_tray_|, |virtual_keyboard_tray_|
+      // are all visible, the reverse sequence needs special processing as
+      // |virtual_keyboard_tray_|'s next focusable view is |system_tray_|,
+      // however, |virtual_keyboard_tray_|'s reverse next view should be
+      // |web_notification_tray_|.
+      if (reverse && web_notification_tray_ &&
+          web_notification_tray_->visible() && virtual_keyboard_tray_ &&
+          virtual_keyboard_tray_->visible() && system_tray_ &&
+          system_tray_->visible()) {
+        if (virtual_keyboard_tray_->HasFocus()) {
+          Widget::OnKeyEvent(event);
+          return;

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

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]

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

Done for HandleFocusKeyEvent().

+        } else if (web_notification_tray_->HasFocus()) {
+          wm_shell->system_tray_notifier()->NotifyWillReturnFocusToWebContents(
+              reverse);
+          event->SetHandled();
+        }
+      }
+#endif
+      // At most two visible trays that need focus on status area:
+      if (system_tray_ && system_tray_->visible() &&
+          system_tray_->Contains(next_focusable_view)) {
+        wm_shell->system_tray_notifier()->NotifyWillReturnFocusToWebContents(
+            reverse);
+        event->SetHandled();
+      }
+    }
+  }
+  Widget::OnKeyEvent(event);
+}
+
 void StatusAreaWidget::UpdateShelfItemBackground(int alpha) {
   web_notification_tray_->UpdateShelfItemBackground(alpha);
   system_tray_->UpdateShelfItemBackground(alpha);