MacViews: Implement Full Keyboard Access.

ui/views/view.h

Index: ui/views/view.h
diff --git a/ui/views/view.h b/ui/views/view.h
index a5ef00dae1dc83cdaeccd314cc80e239ff475c47..d8792d3f16053cd5b39e193de567cd9902d6cee9 100644
--- a/ui/views/view.h
+++ b/ui/views/view.h
@@ -778,6 +778,9 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   // the focused view is set to be non-focusable.
   // Note that this is false by default so that a view used as a container does
   // not get the focus.
+  // Subclasses which represent controls should call
+  // PlatformStyle::SetControlStyleFocus to set focus correctly for different
+  // platforms.
   void SetFocusable(bool focusable);

[tapted, 2016/02/23 03:01:20]

I think it's confusing to keep this method around -- we need a way for people to
`notice` that calling SetFocusable might not be the right thing to do. And in
fact, it seems like there would be very few callers to SetFocusable after this.
But I wonder whether SetFocusable(true) is now in the minority.

Also, since we're at the point of modifying view.h, anyway, I think we should
integrate this into views::View rather than populating files with
platform_style.h

I'm not sure what the best approach is though. And the current way
`accessibility focusable` works is confusing.


I'm thinking something like

SetAlwaysFocusable();  // Called for text fields, does SetFocusable(true) on all
platforms.
SetNeverFocusable();  // Called for things to override default focus, calls
SetFocusable(false) && SetAccessibilityFocusable(false); on all platforms.

Then


void View::SetFocusable(bool focusable) {
  if (!focusable)
    SetNeverFocusable();
  SetAccessibilityFocusable(true);
#if !defined(OS_MACOSX)
  SetFocusable(true);
#endif
}

With 

// TODO(..): Convert callers to SetFocusable(false) to call SetNeverFocusable
directly, then remove the `bool` argument to SetFocusable (and/or rename it to
SetControlFocusable());


Then SetAccessibilityFocusable also needs an adjustment. For example there are
*very* few callers that pass `false` -- those that do probably aren't doing
anything. And we need to ensure the things calling
SetAccessibilityFocusable(true) will get the correct behaviour on Mac --- I'm
not sure if we are still supporting their use-case properly by overloading
IsAccessibilityFocusable like this. E.g. maybe the nicer API is something like

enum FocusBehaviour {
  ALWAYS,
  NEVER,  // Default.
  CONTROL_STYLE,  // Mac specific change here.
  ACCESSIBLE_ONLY,  // ??? do we support this use case on Mac - can we say "same
as CONTROL_STYLE on Mac" and keep the users happy?
}

But I think we need some expert eyes on this design. i.e. we should should
neaten up this CL a bit, then send it off to sky@, or perhaps summarise some of
the findings / background / rationale in a google doc on chromium.org

[karandeepb, 2016/03/15 02:19:51]

I like the enum idea. I have created a new function SetFocusBehavior. I think we
can replace all SetFocusable and SetAccessibilityFocusable calls to use
SetFocusBehavior and make SetFocusable and SetAccessibilityFocusable private.

[tapted, 2016/03/15 05:05:31]

sgtm

 
   // Returns true if this view is |focusable_|, |enabled_| and drawn.
@@ -791,6 +794,9 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   // full keyboard access, even though it's not normally focusable. It will
   // clear focus if the focused view is set to be non-focusable.
   // Note that this is false by default.
+  // Subclasses which represent controls should call
+  // PlatformStyle::SetControlStyleFocus to set focus correctly for different
+  // platforms.
   void SetAccessibilityFocusable(bool accessibility_focusable);
 
   // Convenience method to retrieve the FocusManager associated with the