MacViews: Implement Full Keyboard Access.

ash/shell/lock_view.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 "ash/session/session_state_delegate.h"
 #include "ash/shell.h"
 #include "ash/shell/example_factory.h"
 #include "ash/shell_window_ids.h"
 #include "base/strings/utf_string_conversions.h"
 #include "ui/aura/window.h"
@@ skipping 12 matching lines @@
 namespace ash {
 namespace shell {
 
 class LockView : public views::WidgetDelegateView,
                  public views::ButtonListener {
  public:
   LockView() : unlock_button_(new views::LabelButton(
                                   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 {}
 
   // Overridden from views::View:
   gfx::Size GetPreferredSize() const override { return gfx::Size(500, 400); }
 
  private:
   // Overridden from views::View:
   void OnPaint(gfx::Canvas* canvas) override {
     canvas->FillRect(GetLocalBounds(), SK_ColorYELLOW);
@@ skipping 55 matching lines @@
   widget->GetNativeView()->SetName("LockView");
   widget->GetNativeView()->Focus();
 
   // TODO: it shouldn't be necessary to invoke UpdateTooltip() here.
   Shell::GetInstance()->tooltip_controller()->UpdateTooltip(
       widget->GetNativeView());
 }
 
 }  // namespace shell
 }  // namespace ash