Views: Don't advance focus in Widgets when the stored focus view is not null.

ui/views/widget/widget.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/widget/widget.h"
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 1289 matching lines @@
     // If not focusing the window now, tell the focus manager which view to
     // focus when the window is restored.
     if (v && focus_manager)
       focus_manager->SetStoredFocusView(v);
     return true;
   }
   if (v) {
     v->RequestFocus();
     // If the request for focus was unsuccessful, fall back to using the first
     // focusable View instead.
-    if (focus_manager && focus_manager->GetFocusedView() == nullptr)
+    if (focus_manager && focus_manager->GetFocusedView() == nullptr &&
+        focus_manager->GetStoredFocusView() == nullptr) {

[sky, 2017/02/10 18:18:18]

Can you clarify why we end up here? I'm specifically wondering why the
RequestFocus() on 1307 fails? If the request focus fails, then we should use the
stored focus view if null and if focus is till null, then try to advance focus.
Please add test coverage of this.

[Patti Lor, 2017/02/15 03:15:21]

The RequestFocus() call doesn't really fail, it just got called while the Widget
was deactivated. When that's the case, FocusManager::SetFocusedViewWithReason()
stores the focused view instead so that it can be focused later when the Widget
becomes activated. But my previous change (r443827) didn't account for this
scenario, so it would advance the focus anyway to the next View. All of this
happens while the Widget is still deactivated, so the focus manager gets updated
with an incorrect stored focus view.

The change I made should do what you've suggested (use the originally stored
focus view on Widget activation).

[sky, 2017/02/15 16:28:28]

In that case can you check the active status rather than the stored focused
view? Checking the 'stored focus view' which is a side effect of not being
active is subtle and easily missed. Also, please update the comment above.

[Patti Lor, 2017/02/16 22:58:49]

Done.

       focus_manager->AdvanceFocus(false);
+    }
   }
   return !!focus_manager->GetFocusedView();
 }
 
 bool Widget::ShouldDescendIntoChildForEventHandling(
     ui::Layer* root_layer,
     gfx::NativeView child,
     ui::Layer* child_layer,
     const gfx::Point& location) {
   if (widget_delegate_ &&
@@ skipping 222 matching lines @@
 
 ////////////////////////////////////////////////////////////////////////////////
 // internal::NativeWidgetPrivate, NativeWidget implementation:
 
 internal::NativeWidgetPrivate* NativeWidgetPrivate::AsNativeWidgetPrivate() {
   return this;
 }
 
 }  // namespace internal
 }  // namespace views