MacViews: Implement Full Keyboard Access.

ui/views/view.h

 // 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.
 
 #ifndef UI_VIEWS_VIEW_H_
 #define UI_VIEWS_VIEW_H_
 
 #include <stddef.h>
 
 #include <algorithm>
@@ skipping 760 matching lines @@
   // makes the current view the precedent view of the specified one.
   // Note that by default views are linked in the order they have been added to
   // their container. Use this method if you want to modify the order.
   // IMPORTANT NOTE: loops in the focus hierarchy are not supported.
   void SetNextFocusableView(View* view);
 
   // Sets whether this view is capable of taking focus. It will clear focus if
   // 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.
   bool IsFocusable() const;
 
   // Return whether this view is focusable when the user requires full keyboard
   // access, even though it may not be normally focusable.
   bool IsAccessibilityFocusable() const;
 
   // Set whether this view can be made focusable if the user requires
   // 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
   // Widget that contains this view.  This can return NULL if this view is not
   // part of a view hierarchy with a Widget.
   virtual FocusManager* GetFocusManager();
   virtual const FocusManager* GetFocusManager() const;
 
   // Request keyboard focus. The receiving view will become the focused view.
   virtual void RequestFocus();
@@ skipping 770 matching lines @@
   // Belongs to this view, but it's reference-counted on some platforms
   // so we can't use a scoped_ptr. It's dereferenced in the destructor.
   NativeViewAccessibility* native_view_accessibility_;
 
   DISALLOW_COPY_AND_ASSIGN(View);
 };
 
 }  // namespace views
 
 #endif  // UI_VIEWS_VIEW_H_