MacViews: Implement Full Keyboard Access.

ash/shell/lock_view.cc

Index: ash/shell/lock_view.cc
diff --git a/ash/shell/lock_view.cc b/ash/shell/lock_view.cc
index 8fa39711e455de3dfe0b9d154c50e676c7bb188c..67b5571a5155ed1baf97f68401e0020d1b7f29e1 100644
--- a/ash/shell/lock_view.cc
+++ b/ash/shell/lock_view.cc
@@ -30,7 +30,7 @@ class LockView : public views::WidgetDelegateView,
                                   this, base::ASCIIToUTF16("Unlock"))) {
     unlock_button_->SetStyle(views::Button::STYLE_BUTTON);
     AddChildView(unlock_button_);
-    unlock_button_->SetFocusBehavior(FocusBehavior::ALWAYS);
+    views::Button::ConfigureDefaultFocus(unlock_button_);

[tapted, 2016/04/20 06:05:57]

It feels inconsistent to flip things around when configuring a view. I feel like
maybe something like

unlock_button_->SetButtonFocusablePerPlatform();

might give a better indication to what's happening, and not flip the variable
being configured onto the right-hand side of the line.

(i.e. with SetButtonFocusablePerPlatform is declared on views::Button)

Maybe see what sky thinks before running a `sed` script over your checkout
though :)


It might also be an interesting experiment to move this into the Button
constructor.. (e.g., of all the buttons, is it just ToolbarButton that shouldn't
be focusable by default?)

[karandeepb, 2016/05/03 02:54:12]

I used a static method because I thought it was better encapsulation, since we
are only using public methods within the function, though I agree, a normal
instance method does look nicer. Will wait for sky's review before changing
this.

So with this CL, there are about 40 odd occurrences of
Button::ConfigureDefaultFocus(..). LabelButton and ImageButton are the 2 most
widely used button classes and they both have FocusBehavior::ACCESSIBLE_ONLY by
default. There are a total of 150 and 54 instantiations of LabelButton and
ImageButton respectively. This seems to suggest that the defaults as they
currently stand are good enough. 

[tapted, 2016/05/03 08:08:25]

Ack. Thanks for investigating that! I wonder if it's unusual that we have
relatively fewer focusable buttons ... That is, it's possible they *should* be
focusable, someone just forgot to add in a call to SetFocusable(..) and never
tested with keyboard navigation

[karandeepb, 2016/05/04 01:56:38]

Yup I was also a bit surprised by this. But LabelButton::SetStyle() also has a
SetFocusBehavior/ConfigureDefaultFocus() and there are are 91 calls to it. As
far as ImageButton is concerned, I am not sure, but maybe since a lot of these
reside in a AccessiblePaneView(which has FocusSearch::accessibility_mode set to
true), we probably want them to be accessibility focusable anyway. Also, if
there are cases where buttons are not focusable when they should be, we won't be
able to catch these while transitioning to MacViews since we want all buttons on
MacViews to be accessibility focusable, which is the default.

   }
   ~LockView() override {}