(Mac)Views: Widgets focus first View in traversal order if initial focus fails.

ui/views/window/dialog_delegate_unittest.cc

Index: ui/views/window/dialog_delegate_unittest.cc
diff --git a/ui/views/window/dialog_delegate_unittest.cc b/ui/views/window/dialog_delegate_unittest.cc
index 648175d8d23eb5dca090e03ed87a97c4d3fa4972..ab9db3b1560db840640ebaf8c03f3579650db2e2 100644
--- a/ui/views/window/dialog_delegate_unittest.cc
+++ b/ui/views/window/dialog_delegate_unittest.cc
@@ -49,6 +49,8 @@ class TestDialog : public DialogDelegateView, public ButtonListener {
     return !title_.empty();
   }
 
+  bool ShouldShowCloseButton() const override { return false; }

[tapted, 2017/01/06 11:45:15]

This makes the test a bit "unauthentic". That is, the real reason the changes is
needed is that view that should be focused depends on the users's current `Full
Keyboard Access` setting. That limits the options for fixing it (e.g. we can't
just #ifdef(mac), and we don't want to litter knowledge of that setting
throughout the codebase).

I think we want something like

#if defined(OS_MACOSX)
#include "ui/base/test/scoped_fake_full_keyboard_access.h"
#endif

..

#if defined(OS_MACOSX)
ScopedFakeFullKeyboardAccess::GetInstance()->set_full_keyboard_access_state(false);
#endif

(alternatively we could update ViewsTestBase with a method to tweak it and just
have a no-op implementation in ViewsTestHelperAura, but I'm slightly in favour
of the above for now)

Of course then the test will have different results on other platforms. I think
that's OK (more #ifdefs..)

[Patti Lor, 2017/01/12 02:16:22]

Apparently you have to call [bridge_->ns_view() updateFullKeyboardAccess] after
setting the full keyboard access state, according to
NativeWidgetMacFullKeyboardAccessTest.FullKeyboardToggle. I think the reason for
that was here:
https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_mac_unitte...,
but you or Karan will have to confirm this.

I had a look at your second suggestion of adding a method to ViewsTestBase, but
it's really gross. There seems like there are too many layers between
ViewsTestBase and ViewsTestHelper* - I had to add a method to
ScopedViewsTestHelper which will call the proper implementation on
ViewsTestHelperMac? I might be misunderstanding something though. I went ahead
and did this anyway, but I'm not sure if you would prefer, something else, e.g.
an approach similar to
https://cs.chromium.org/chromium/src/ui/base/test/user_interactive_test_case....

+
   // DialogDelegateView overrides:
   bool Cancel() override {
     canceled_ = true;
@@ -128,7 +130,7 @@ class DialogTest : public ViewsTestBase {
     ViewsTestBase::SetUp();
     dialog_ = new TestDialog();
     dialog_->Init();
-    DialogDelegate::CreateDialogWidget(dialog_, GetContext(), nullptr)->Show();
+    DialogDelegate::CreateDialogWidget(dialog_, GetContext(), nullptr);
   }
 
   void TearDown() override {
@@ -142,6 +144,8 @@ class DialogTest : public ViewsTestBase {
       dialog()->GetWidget()->OnKeyEvent(&event_copy);
   }
 
+  void ShowDialog() { dialog()->GetWidget()->Show(); }
+
   TestDialog* dialog() const { return dialog_; }
 
  private:
@@ -153,6 +157,7 @@ class DialogTest : public ViewsTestBase {
 }  // namespace
 
 TEST_F(DialogTest, AcceptAndCancel) {
+  ShowDialog();
   DialogClientView* client_view = dialog()->GetDialogClientView();
   LabelButton* ok_button = client_view->ok_button();
   LabelButton* cancel_button = client_view->cancel_button();
@@ -187,12 +192,14 @@ TEST_F(DialogTest, AcceptAndCancel) {
 }
 
 TEST_F(DialogTest, RemoveDefaultButton) {
+  ShowDialog();
   // Removing buttons from the dialog here should not cause a crash on close.
   delete dialog()->GetDialogClientView()->ok_button();
   delete dialog()->GetDialogClientView()->cancel_button();
 }
 
 TEST_F(DialogTest, HitTest_HiddenTitle) {
+  ShowDialog();
   // Ensure that BubbleFrameView hit-tests as expected when the title is hidden.
   const NonClientView* view = dialog()->GetWidget()->non_client_view();
   BubbleFrameView* frame = static_cast<BubbleFrameView*>(view->frame_view());
@@ -219,6 +226,7 @@ TEST_F(DialogTest, HitTest_HiddenTitle) {
 }
 
 TEST_F(DialogTest, HitTest_WithTitle) {
+  ShowDialog();
   // Ensure that BubbleFrameView hit-tests as expected when the title is shown.
   const NonClientView* view = dialog()->GetWidget()->non_client_view();
   dialog()->set_title(base::ASCIIToUTF16("Title"));
@@ -246,6 +254,7 @@ TEST_F(DialogTest, HitTest_WithTitle) {
 }
 
 TEST_F(DialogTest, BoundsAccommodateTitle) {
+  ShowDialog();
   TestDialog* dialog2(new TestDialog());
   dialog2->set_title(base::ASCIIToUTF16("Title"));
   DialogDelegate::CreateDialogWidget(dialog2, GetContext(), nullptr);
@@ -267,8 +276,26 @@ TEST_F(DialogTest, BoundsAccommodateTitle) {
 
 // Tests default focus is assigned correctly when showing a new dialog.
 TEST_F(DialogTest, InitialFocus) {
+  ShowDialog();
   EXPECT_TRUE(dialog()->input()->HasFocus());
   EXPECT_EQ(dialog()->input(), dialog()->GetFocusManager()->GetFocusedView());
 }
 
+// If the initially focused View provided is unfocusable, check the next
+// available focusable View is focused.
+TEST_F(DialogTest, UnfocusableInitialFocus) {
+  // Make the View to focus by default unfocusable, and add a second View that
+  // will be the first in the focus traversal order.
+  dialog()->input()->SetFocusBehavior(View::FocusBehavior::NEVER);

[tapted, 2017/01/06 11:45:15]

My approach here would be something like do this line _only_ on not-mac (and do
it for the close button too). Then remove the ShouldShowCloseButton() override.
That lets us check something on each platform -- the authentic scenario on Mac,
and a contrived one on other platforms that gives us some cross-platform
coverage.

[Patti Lor, 2017/01/12 02:16:22]

With keyboard-focus off on Mac, I think we don't need to worry about the close
button now. It turns out the close button isn't part of the focus traversal
order on non-Mac - I'm not sure why this is the case, any ideas? If that's not
intentional, though, I'm not sure if using ShouldShowCloseButton() can be
avoided, because there doesn't seem to be a way to retrieve the close button out
of the dialog, AFAICT.

[tapted, 2017/01/12 02:25:43]

yep - that makes sense - buttons are skipped with full keyboard access off, so
that's the right behaviour.
ah, i see. Can we add #ifdefs around  ShouldShowCloseButton() then, so it's only
overridden on not-mac? (the override should have a comment too)

[Patti Lor, 2017/01/12 06:12:37]

Sorry, by the close button I meant the little 'x' in the top-right. You can get
to it pressing tab in MacViews but not on Linux (dunno about Windows, I'm
assuming it's the same). Originally I put in the ShouldShowCloseButton()
override because the behaviour was the same as Mac with keyboard access on,
which would always hit the close button instead of |textfield|. But this test
passes on non-Mac without hiding the 'x'. I wasn't sure if this was a bug. If
yes, then we might need the ifdefs.

+  Textfield* textfield = new Textfield();
+  dialog()->AddChildView(textfield);
+
+  // On showing the dialog, the default focus item shouldn't have focus, but
+  // |textfield| should.
+  ShowDialog();
+  EXPECT_FALSE(dialog()->input()->HasFocus());
+  EXPECT_TRUE(textfield->HasFocus());
+  EXPECT_EQ(textfield, dialog()->GetFocusManager()->GetFocusedView());
+}
+
 }  // namespace views