MacViews: Implement Full Keyboard Access.

MacViews: Implement Full Keyboard Access.

Currently MacViews does not respect the full keyboard accessibility setting.
In the normal mode, as per Full Keyboard Access, only text boxes and lists
should get focus. However, currently, views like comboboxes, buttons, links
etc. get focus in the normal mode.

This CL implements Full Keyboard access by adding a platform level keyboard
accessibility setting to the FocusManager class. It also adds a new method
SetFocusBehavior to the views class to replace the existing SetFocusable() and
SetAccessibilityFocusable(), which have been made private.

Other changes-
- Adds a accessibility_focusable() method to ui/views/view.h.
- Adds a static ConfigureDefaultFocus() method to 
  ui/views/controls/button/button.h
- Adds ScopedFakeFullKeyboardAccess to swizzle 
  [NSAppl isFullKeyboardAccessEnabled] for use in tests.
- ScopedFakeNSWindowFocus now also swizzles orderOut: to determine if the 
  currently focused window is ordered-out.
- View::RequestToFocus now uses IsAccessibilityFocusable() instead of 
  IsFocusable().

Doc - https://docs.google.com/document/d/16A_kZHCIaDEGZGIIZfEJ7sfFJD4829GcPxycuMBysW0/edit

BUG=564912

[karandeepb, 2016-02-12 00:28:12]

Description was changed from

==========
Fullkeyboard access Mac.
==========

to

==========
MacViews: Implement Full Keyboard Access.

Currently MacViews does not respect the full keyboard accessibility setting.
In the normal mode, as per Full Keyboard Access, only text boxes and lists
should get focus. However, currently, views like comboboxes, buttons, links
etc. get focus in the normal mode.

This CL implements Full Keyboard access by overriding the
FocusSearch::IsFocusable method on Mac and setting the correct focusable and
accessibility focusable states for the various view classes.

BUG=564912
==========

[karandeepb, 2016-02-12 00:28:21]

Patchset #1 (id:1) has been deleted

[karandeepb, 2016-02-12 00:33:56]

karandeepb@chromium.org changed reviewers:
+ tapted@chromium.org

[karandeepb, 2016-02-12 00:33:57]

PTAL Trent. This is just an initial CL and I still need to research the correct
focus settings for various views. 

Let me know if this approach looks reasonable to you or whether I should look at
implementing this in another way.

[tapted, 2016-02-12 02:56:45]

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/butto...
ui/views/controls/button/checkbox.cc:32: #if !defined(OS_MACOSX)
It's usually better to avoid negatives in conditions if you can neatly. I think
the SetFocusable(false) should be redundant too, so this could be

#if defined(OS_MACOSX)
SetAccessibilityFocusable(true);
#else
SetFocusable(false);
#endif

But also button.cc already has `SetAccessibilityFocusable(true);` so perhaps we
just need 

#if !defined(OS_MACOSX)
SetFocusable(false);
#endif


But!

I nicer solution might be to do something like

PlatformStyle::SetButtonStyleFocus(View* view);

replace all the current SetFocusable(true) calls with
PlatformStyle::SetButtonStyleFocus(this) and provide platform implementations in
platform_style.cc and platform_style_mac.cc. I think this is the nicest option.

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:94: SetAccessibilityFocusable(false);
perhaps DCHECK(!IsFocusable());  after this? It's subtle that LabelButtons are
by-default focusable, but not CustomButtons

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/link.cc
File ui/views/controls/link.cc (right):

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/link....
ui/views/controls/link.cc:169: // On Mac, only allow links to have focus, when
the accessibility mode is on.
this isn't what Chrome does in the content area -- links are focused when
eaccessibility mode is off, so I think the UI should be consistent with that (..
even if it's not what Safari does..)

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/slide...
File ui/views/controls/slider.cc (right):

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/slide...
ui/views/controls/slider.cc:70: #if !defined(OS_MACOSX)
I think this can be

#if defined(OS_MACOSX)
SetAccessibilityFocusable(true);
#else
SetFocusable(true);
#endif

https://codereview.chromium.org/1690543004/diff/20001/ui/views/focus/focus_se...
File ui/views/focus/focus_search.cc (right):

https://codereview.chromium.org/1690543004/diff/20001/ui/views/focus/focus_se...
ui/views/focus/focus_search.cc:101: // On Mac, we also need to read system
preferences for "Full Keyboard Access" to
Can we instead expose the |accessibility_mode_| data member?

Then BridgedNativeWidget can simply call
native_widget_mac_->GetWidget()->GetFocusSearch()->SetAccessibilityMode([NSApp
isFullKeyboardAccessEnabled]);

(probably at the end of SetRootView)

There also should be a system notification that we listen for when the user
presses Ctrl+F7, so that we can toggle this on the fly. SetAccessibilityMode(..)
should then advance focus if it's currently on a non-focusable view, to match
OSX behaviour.

[karandeepb, 2016-02-22 07:15:37]

PTAL Trent. This CL still needs to be refined and there are still several
remaining SetFocusable() call sites which need to be changed. Let me know what
you think of the general structure of the CL.

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/butto...
ui/views/controls/button/checkbox.cc:32: #if !defined(OS_MACOSX)
On 2016/02/12 02:56:44, tapted wrote:
> It's usually better to avoid negatives in conditions if you can neatly. I
think
> the SetFocusable(false) should be redundant too, so this could be
> 
> #if defined(OS_MACOSX)
> SetAccessibilityFocusable(true);
> #else
> SetFocusable(false);
> #endif
> 
> But also button.cc already has `SetAccessibilityFocusable(true);` so perhaps
we
> just need 
> 
> #if !defined(OS_MACOSX)
> SetFocusable(false);
> #endif
> 
> 
> But!
> 
> I nicer solution might be to do something like
> 
> PlatformStyle::SetButtonStyleFocus(View* view);
> 
> replace all the current SetFocusable(true) calls with
> PlatformStyle::SetButtonStyleFocus(this) and provide platform implementations
in
> platform_style.cc and platform_style_mac.cc. I think this is the nicest
option.

I have added a PlatformStyle::SetControlStyleFocus instead, since all views
which represent controls(except textfields and lists) should have this focus
setting.

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:94: SetAccessibilityFocusable(false);
On 2016/02/12 02:56:44, tapted wrote:
> perhaps DCHECK(!IsFocusable());  after this? It's subtle that LabelButtons are
> by-default focusable, but not CustomButtons

Done.

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/link.cc
File ui/views/controls/link.cc (right):

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/link....
ui/views/controls/link.cc:169: // On Mac, only allow links to have focus, when
the accessibility mode is on.
On 2016/02/12 02:56:44, tapted wrote:
> this isn't what Chrome does in the content area -- links are focused when
> eaccessibility mode is off, so I think the UI should be consistent with that
(..
> even if it's not what Safari does..)

As discussed, keeping the links not focusable by default on Mac.

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/slide...
File ui/views/controls/slider.cc (right):

https://codereview.chromium.org/1690543004/diff/20001/ui/views/controls/slide...
ui/views/controls/slider.cc:70: #if !defined(OS_MACOSX)
On 2016/02/12 02:56:44, tapted wrote:
> I think this can be
> 
> #if defined(OS_MACOSX)
> SetAccessibilityFocusable(true);
> #else
> SetFocusable(true);
> #endif
> 

Using PlatformStyle::SetControlStyleFocus() now.

https://codereview.chromium.org/1690543004/diff/20001/ui/views/focus/focus_se...
File ui/views/focus/focus_search.cc (right):

https://codereview.chromium.org/1690543004/diff/20001/ui/views/focus/focus_se...
ui/views/focus/focus_search.cc:101: // On Mac, we also need to read system
preferences for "Full Keyboard Access" to
On 2016/02/12 02:56:45, tapted wrote:
> Can we instead expose the |accessibility_mode_| data member?
> 
> Then BridgedNativeWidget can simply call
> native_widget_mac_->GetWidget()->GetFocusSearch()->SetAccessibilityMode([NSApp
> isFullKeyboardAccessEnabled]);
> 
> (probably at the end of SetRootView)
> 
> There also should be a system notification that we listen for when the user
> presses Ctrl+F7, so that we can toggle this on the fly.
SetAccessibilityMode(..)
> should then advance focus if it's currently on a non-focusable view, to match
> OSX behaviour.

Done. Instead of setting the accessibility mode on the FocusSearch, I am setting
it on the FocusManager. This is because, each widget in the widget hierarchy
will have its own separate FocusSearch (corresponding to its root view) but all
of them will have the same FocusManager.

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

There's a lot more calling SetFocusable(true) than I was expecting.. I think we
can integrate this into views::View rather than using PlatformStyle, but we
should consult sky@ for opinions on the design

https://codereview.chromium.org/1690543004/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_shelf_view.cc (left):

https://codereview.chromium.org/1690543004/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_shelf_view.cc:168:
show_all_view->SetFocusable(true);
SetControlStyleFocus instead?

https://codereview.chromium.org/1690543004/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/permission_selector_view.cc
(right):

https://codereview.chromium.org/1690543004/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_selector_view.cc:181:
menu_button_->SetEnabled(button_enabled);
View::IsFocusable() has a check for |enabled_|, so this should just be
SetFocusable(true)

https://codereview.chromium.org/1690543004/diff/120001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1690543004/diff/120001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:2119: '../ui/views/style/platform_style.cc',
these don't belong here - they should come in via the views dependency

https://codereview.chromium.org/1690543004/diff/120001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:355: // Todo(karandeepb): See how to
store constant string.
You can just declare a

NSString* const kFullKeyboardAccessChangedNotification =
    @"com.apple.KeyboardUIModeDidChange";

in an anonymous namespace

https://codereview.chromium.org/1690543004/diff/120001/ui/views/cocoa/views_n...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/cocoa/views_n...
ui/views/cocoa/views_nswindow_delegate.mm:47: parent_->native_widget_mac()
I think nicer would be a new method on BridgedContentView, perhaps
`UpdateFullKeyboardAccess()` (i.e. rather than On...Changed, since we want to
call it when we don't know if it's changing).

Then collapse all the places you have a `[NSApp isFullKeyboardAccessEnabled]`
into that single function.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/controls/link.cc
File ui/views/controls/link.cc (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/controls/link...
ui/views/controls/link.cc:164: // Disable focusability for empty links. 
Otherwise Label::GetInsets() will
This comment about Label::GetInsets() concerns me. Does Label::GetInsets() need
to be changed to use IsAccessibilityFocusable()?  (or, perhaps neater, should
the text.empty() check move to Label::GetInsets()? then leave off the
SetAccessibilityFocusable(false); calls

https://codereview.chromium.org/1690543004/diff/120001/ui/views/controls/link...
ui/views/controls/link.cc:210: if (!text().empty()) {
This could also just call SetText again, but we should be able to simplify it so
there's not as much copy-paste

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.cc:302: keyboard_accessibility_ =
keyboard_accessibility;
this should early-exit if there is no change

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.cc:555: bool FocusManager::IsFocusable(View* v) {
v -> view

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.cc:557: return v && v->IsAccessibilityFocusable();
I think the null checks are redundant - perhaps DCHECK(view) at the start

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:330: bool IsKeyboardAccessibilityEnabled() {
return keyboard_accessibility_; }
for accessors like this, we just do

bool keyboard_accessibility() const { return keyboard_accessibility_; }

it doesn't need a comment

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:332: // Updates keyboard_accessibility_ to the
given value and advances focus if
keyboard_accessibility_ -> |keyboard_accessibility_|

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:349: // Returns true if a view is not NULL and is
focusable (checking
NULL -> null

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:350: // IsAccessibilityFocusable() if
keyboard_accessibility_ is true).
keyboard_accessibility_ -> |keyboard_accessibility_|

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:351: bool IsFocusable(View* view);
const method?

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:385: // This is true if full keyboard
accessibility is needed and you want to
`you want to check` sounds funny. Perhaps `FocusSearch should check`

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:387: // set depending on platform constraints,
for example, on Mac, this is true
`constraints, for example,` -> `constraints. For Example,`

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:391: bool keyboard_accessibility_;
maybe `keyboard_accessible_` is a better name

https://codereview.chromium.org/1690543004/diff/120001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/view.h#newcod...
ui/views/view.h:784: void SetFocusable(bool focusable);
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 01:57:48]

Description was changed from

==========
MacViews: Implement Full Keyboard Access.

Currently MacViews does not respect the full keyboard accessibility setting.
In the normal mode, as per Full Keyboard Access, only text boxes and lists
should get focus. However, currently, views like comboboxes, buttons, links
etc. get focus in the normal mode.

This CL implements Full Keyboard access by overriding the
FocusSearch::IsFocusable method on Mac and setting the correct focusable and
accessibility focusable states for the various view classes.

BUG=564912
==========

to

==========
MacViews: Implement Full Keyboard Access.

Currently MacViews does not respect the full keyboard accessibility setting.
In the normal mode, as per Full Keyboard Access, only text boxes and lists
should get focus. However, currently, views like comboboxes, buttons, links
etc. get focus in the normal mode.

This CL implements Full Keyboard access by adding a platform level keyboard 
accessibility setting to the FocusManager class. It also adds a new method 
SetFocusBehavior to the views class to replace the existing SetFocusable() and 
SetAccessibilityFocusable().

BUG=564912
==========

[karandeepb, 2016-03-15 02:10:57]

Patchset #9 (id:180001) has been deleted

[karandeepb, 2016-03-15 02:11:06]

Patchset #9 (id:200001) has been deleted

[karandeepb, 2016-03-15 02:12:04]

Patchset #9 (id:220001) has been deleted

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

PTAL Trent. I have removed the PlatformStyle::SetControlStyleFocus calls and
replaced them with calls to SetFocusBehavior(..), a new method on the views
class. I have made the patch leaner, currently only modifying the call sites in
ui/views/controls, so that we can first focus on the core changes. Later, all
call sites using SetFocusable() and SetAccessibilityFocusable() can be changed
to use SetFocusBehavior, and SetFocusable() and SetAccessibilityFocusable() can
be made private.

However, there are a few outstanding issues -

Now that we are thinking of adding a platform level keyboard accessibility, I
think it would be incorrect if the View::IsFocusable() method did not take it
into account. I think the IsFocusable() and IsAccessibilityFocusable() can be
made private, and replaced with a public method like -

bool View::CanGetFocus() const {
  const FocusManager* focus_manager = GetFocusManager();
  return focus_manager && focus_manager->keyboard_accessible() ?
IsAccessibilityFocusable() : IsFocusable();
}

Also an accessibility_focusable() method can be added to the views class, to
complement the already existing focusable() method.

However, I am not sure how to handle the FocusSearch class, which manages its
own accessibility mode and needs access to IsFocusable() and
IsAccessibilityFocusable(). Maybe it can be made a friend of the view class,
though it seems slightly ugly.

I have also created a small preliminary doc, giving some background for the
changes -
https://docs.google.com/a/google.com/document/d/16A_kZHCIaDEGZGIIZfEJ7sfFJD48....
Let me know if any of it is incorrect.

https://codereview.chromium.org/1690543004/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_shelf_view.cc (left):

https://codereview.chromium.org/1690543004/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_shelf_view.cc:168:
show_all_view->SetFocusable(true);
On 2016/02/23 03:01:19, tapted wrote:
> SetControlStyleFocus instead?

This call is redundant, since SetStyle(..) on the next line calls
SetFocusable(true).

https://codereview.chromium.org/1690543004/diff/120001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:355: // Todo(karandeepb): See how to
store constant string.
On 2016/02/23 03:01:19, tapted wrote:
> You can just declare a
> 
> NSString* const kFullKeyboardAccessChangedNotification =
>     @"com.apple.KeyboardUIModeDidChange";
> 
> in an anonymous namespace

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/cocoa/views_n...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/cocoa/views_n...
ui/views/cocoa/views_nswindow_delegate.mm:47: parent_->native_widget_mac()
On 2016/02/23 03:01:19, tapted wrote:
> I think nicer would be a new method on BridgedContentView, perhaps
> `UpdateFullKeyboardAccess()` (i.e. rather than On...Changed, since we want to
> call it when we don't know if it's changing).
> 
> Then collapse all the places you have a `[NSApp isFullKeyboardAccessEnabled]`
> into that single function.

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/controls/link.cc
File ui/views/controls/link.cc (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/controls/link...
ui/views/controls/link.cc:164: // Disable focusability for empty links. 
Otherwise Label::GetInsets() will
On 2016/02/23 03:01:19, tapted wrote:
> This comment about Label::GetInsets() concerns me. Does Label::GetInsets()
need
> to be changed to use IsAccessibilityFocusable()?  (or, perhaps neater, should
> the text.empty() check move to Label::GetInsets()? then leave off the
> SetAccessibilityFocusable(false); calls 

Yeah GetInsets() is currently using focusable() to determine whether to add
insets or not. This way insets won't be added to links while in Full Keyboard
access mode on Mac. This can be corrected by using IsAccessibilityFocusable().
However, using focusable() instead of IsFocusable() seems to have been a
conscious decision -  https://codereview.chromium.org/111023004/#msg4

https://codereview.chromium.org/1690543004/diff/120001/ui/views/controls/link...
ui/views/controls/link.cc:210: if (!text().empty()) {
On 2016/02/23 03:01:19, tapted wrote:
> This could also just call SetText again, but we should be able to simplify it
so
> there's not as much copy-paste

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.cc:302: keyboard_accessibility_ =
keyboard_accessibility;
On 2016/02/23 03:01:19, tapted wrote:
> this should early-exit if there is no change

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.cc:555: bool FocusManager::IsFocusable(View* v) {
On 2016/02/23 03:01:19, tapted wrote:
> v -> view

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.cc:557: return v && v->IsAccessibilityFocusable();
On 2016/02/23 03:01:19, tapted wrote:
> I think the null checks are redundant - perhaps DCHECK(view) at the start

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:330: bool IsKeyboardAccessibilityEnabled() {
return keyboard_accessibility_; }
On 2016/02/23 03:01:19, tapted wrote:
> for accessors like this, we just do
> 
> bool keyboard_accessibility() const { return keyboard_accessibility_; }
> 
> it doesn't need a comment

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:332: // Updates keyboard_accessibility_ to the
given value and advances focus if
On 2016/02/23 03:01:19, tapted wrote:
> keyboard_accessibility_ -> |keyboard_accessibility_|

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:349: // Returns true if a view is not NULL and is
focusable (checking
On 2016/02/23 03:01:20, tapted wrote:
> NULL -> null

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:350: // IsAccessibilityFocusable() if
keyboard_accessibility_ is true).
On 2016/02/23 03:01:20, tapted wrote:
> keyboard_accessibility_ -> |keyboard_accessibility_|

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:351: bool IsFocusable(View* view);
On 2016/02/23 03:01:19, tapted wrote:
> const method?

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:385: // This is true if full keyboard
accessibility is needed and you want to
On 2016/02/23 03:01:20, tapted wrote:
> `you want to check` sounds funny. Perhaps `FocusSearch should check`

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:387: // set depending on platform constraints,
for example, on Mac, this is true
On 2016/02/23 03:01:20, tapted wrote:
> `constraints, for example,` -> `constraints. For Example,`

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:391: bool keyboard_accessibility_;
On 2016/02/23 03:01:20, tapted wrote:
> maybe `keyboard_accessible_` is a better name

Done.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/view.h#newcod...
ui/views/view.h:784: void SetFocusable(bool focusable);
On 2016/02/23 03:01:20, tapted wrote:
> 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 http://chromium.org

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.

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.cc:393: if (!view->IsFocusable() &&
view->IsAccessibilityFocusable()) {
I can't understand the rationale behind this line of code. It is introduced here
- https://codereview.chromium.org/3013023. Any insights?

[tapted, 2016-03-15 05:05:32]

nice - i'm liking how this looks.

we should add some mac-specific tests that Swizzle `NSApp
isFullKeyboardAccessEnabled` so we can test the logic that switches between the
focusable modes.

https://codereview.chromium.org/1690543004/diff/120001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1690543004/diff/120001/ui/views/view.h#newcod...
ui/views/view.h:784: void SetFocusable(bool focusable);
On 2016/03/15 02:19:51, karandeepb wrote:
> On 2016/02/23 03:01:20, tapted wrote:
> > 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 http://chromium.org
> 
> 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.

sgtm

https://codereview.chromium.org/1690543004/diff/240001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.mm:315: if (!hostedView_) {
nit: no curlies

https://codereview.chromium.org/1690543004/diff/240001/ui/views/controls/butt...
File ui/views/controls/button/image_button.h (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/controls/butt...
ui/views/controls/button/image_button.h:24: 
(while you're here - this blank line should be deleted)

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.cc:393: if (!view->IsFocusable() &&
view->IsAccessibilityFocusable()) {
On 2016/03/15 02:19:51, karandeepb wrote:
> I can't understand the rationale behind this line of code. It is introduced
here
> - https://codereview.chromium.org/3013023. Any insights?

So I think `RequestFocus` can be overridden by a View. Before that CL you link
to, the logic here would have bypassed any RequestFocus override, which is bad
[some things rely on that override]. So, where possible, RequstFocus should be
used rather than SetFocusedViewWithReason. 

But since the default implementation of View::RequestFocus checks only
`IsFocusable`, it can't be called for views that are only
`AccessibilityFocusable`, so for that case only, it falls back to calling
SetFocusedView directly.

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:204: // successfully refocused. In case, the
stored view is no longer focusable,
nit: no comma after 'case'

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:349: // IsAccessibilityFocusable() if
|keyboard_accessible_ | is true). |view|
nit: spacing after keyboard_accessible_

Also I think the view-cant-be-null comment isn't necessary for a private method

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:385: // This is true if full keyboard
accessibility is needed which causes
nit: True if full keyboard accessibility is needed. This causes..

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.cc#newco...
ui/views/view.cc:1219: #if defined(OS_MACOSX)
This might be more readable with something like (at the top)

#if defined(OS_MACOSX)
if (focus_behaviour == CONTROL)
  focus_behavior = ACCESSIBLE_ONLY;
#endif

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.h#newcod...
ui/views/view.h:754: enum FocusBehavior {
enums are always declared before methods in the class declaration order, so this
should be at the top.

Also I think we should use an `enum class` and refer to these as
FocusBehaviour::ALWAYS  (otherwise we would prefix like FOCUS_ALWAYS, since
ALWAYS/NEVER are a bit too vague to be in the subclass' default namespace)

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.h#newcod...
ui/views/view.h:755: ALWAYS,  // Use when the view is to be focusable both in
default and full
nit: if we can't fit in a line, we usually just comment above and leave a blank
line after the declaration

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.h#newcod...
ui/views/view.h:760: CONTROL  // Use for controls. On Mac, this is the same as
ACCESSIBLE_ONLY
nit: CONTROL,

[karandeepb, 2016-03-17 07:24:47]

https://codereview.chromium.org/1690543004/diff/240001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.mm:315: if (!hostedView_) {
On 2016/03/15 05:05:31, tapted wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/1690543004/diff/240001/ui/views/controls/butt...
File ui/views/controls/button/image_button.h (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/controls/butt...
ui/views/controls/button/image_button.h:24: 
On 2016/03/15 05:05:31, tapted wrote:
> (while you're here - this blank line should be deleted)

Done.

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.cc:393: if (!view->IsFocusable() &&
view->IsAccessibilityFocusable()) {
On 2016/03/15 05:05:31, tapted wrote:
> On 2016/03/15 02:19:51, karandeepb wrote:
> > I can't understand the rationale behind this line of code. It is introduced
> here
> > - https://codereview.chromium.org/3013023. Any insights?
> 
> So I think `RequestFocus` can be overridden by a View. Before that CL you link
> to, the logic here would have bypassed any RequestFocus override, which is bad
> [some things rely on that override]. So, where possible, RequstFocus should be
> used rather than SetFocusedViewWithReason. 
> 
> But since the default implementation of View::RequestFocus checks only
> `IsFocusable`, it can't be called for views that are only
> `AccessibilityFocusable`, so for that case only, it falls back to calling
> SetFocusedView directly.

Thanks!

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:204: // successfully refocused. In case, the
stored view is no longer focusable,
On 2016/03/15 05:05:32, tapted wrote:
> nit: no comma after 'case'

Done.

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:349: // IsAccessibilityFocusable() if
|keyboard_accessible_ | is true). |view|
On 2016/03/15 05:05:32, tapted wrote:
> nit: spacing after keyboard_accessible_
> 
> Also I think the view-cant-be-null comment isn't necessary for a private
method

Done.

https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:385: // This is true if full keyboard
accessibility is needed which causes
On 2016/03/15 05:05:32, tapted wrote:
> nit: True if full keyboard accessibility is needed. This causes..

Done.

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.cc#newco...
ui/views/view.cc:1219: #if defined(OS_MACOSX)
On 2016/03/15 05:05:32, tapted wrote:
> This might be more readable with something like (at the top)
> 
> #if defined(OS_MACOSX)
> if (focus_behaviour == CONTROL)
>   focus_behavior = ACCESSIBLE_ONLY;
> #endif
> 

Done.

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.h#newcod...
ui/views/view.h:754: enum FocusBehavior {
On 2016/03/15 05:05:32, tapted wrote:
> enums are always declared before methods in the class declaration order, so
this
> should be at the top.
> 
> Also I think we should use an `enum class` and refer to these as
> FocusBehaviour::ALWAYS  (otherwise we would prefix like FOCUS_ALWAYS, since
> ALWAYS/NEVER are a bit too vague to be in the subclass' default namespace)

Done.

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.h#newcod...
ui/views/view.h:755: ALWAYS,  // Use when the view is to be focusable both in
default and full
On 2016/03/15 05:05:32, tapted wrote:
> nit: if we can't fit in a line, we usually just comment above and leave a
blank
> line after the declaration

Done.

https://codereview.chromium.org/1690543004/diff/240001/ui/views/view.h#newcod...
ui/views/view.h:760: CONTROL  // Use for controls. On Mac, this is the same as
ACCESSIBLE_ONLY
On 2016/03/15 05:05:32, tapted wrote:
> nit: CONTROL,

Done.

https://codereview.chromium.org/1690543004/diff/260001/ui/views/controls/webv...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/1690543004/diff/260001/ui/views/controls/webv...
ui/views/controls/webview/webview.cc:70: SetFocusBehavior(web_contents() ?
FocusBehavior::CONTROL
I am not sure what's the correct value here - CONTROL or ALWAYS.

[karandeepb, 2016-03-17 07:26:52]

Description was changed from

==========
MacViews: Implement Full Keyboard Access.

Currently MacViews does not respect the full keyboard accessibility setting.
In the normal mode, as per Full Keyboard Access, only text boxes and lists
should get focus. However, currently, views like comboboxes, buttons, links
etc. get focus in the normal mode.

This CL implements Full Keyboard access by adding a platform level keyboard 
accessibility setting to the FocusManager class. It also adds a new method 
SetFocusBehavior to the views class to replace the existing SetFocusable() and 
SetAccessibilityFocusable().

BUG=564912
==========

to

==========
MacViews: Implement Full Keyboard Access.

Currently MacViews does not respect the full keyboard accessibility setting.
In the normal mode, as per Full Keyboard Access, only text boxes and lists
should get focus. However, currently, views like comboboxes, buttons, links
etc. get focus in the normal mode.

This CL implements Full Keyboard access by adding a platform level keyboard
accessibility setting to the FocusManager class. It also adds a new method
SetFocusBehavior to the views class to replace the existing SetFocusable() and
SetAccessibilityFocusable(), which shall be made private.

BUG=564912
==========

[karandeepb, 2016-03-17 07:31:13]

Description was changed from

==========
MacViews: Implement Full Keyboard Access.

Currently MacViews does not respect the full keyboard accessibility setting.
In the normal mode, as per Full Keyboard Access, only text boxes and lists
should get focus. However, currently, views like comboboxes, buttons, links
etc. get focus in the normal mode.

This CL implements Full Keyboard access by adding a platform level keyboard
accessibility setting to the FocusManager class. It also adds a new method
SetFocusBehavior to the views class to replace the existing SetFocusable() and
SetAccessibilityFocusable(), which shall be made private.

BUG=564912
==========

to

==========
MacViews: Implement Full Keyboard Access.

Currently MacViews does not respect the full keyboard accessibility setting.
In the normal mode, as per Full Keyboard Access, only text boxes and lists
should get focus. However, currently, views like comboboxes, buttons, links
etc. get focus in the normal mode.

This CL implements Full Keyboard access by adding a platform level keyboard
accessibility setting to the FocusManager class. It also adds a new method
SetFocusBehavior to the views class to replace the existing SetFocusable() and
SetAccessibilityFocusable(), which shall be made private.

Doc -
https://docs.google.com/document/d/16A_kZHCIaDEGZGIIZfEJ7sfFJD4829GcPxycuMBys...

BUG=564912
==========

[karandeepb, 2016-03-17 07:33:09]

karandeepb@chromium.org changed reviewers:
+ sky@chromium.org

[karandeepb, 2016-03-17 07:33:10]

PTAL sky@. Let me know what you think of the proposed api changes, before I
begin refactoring the code.

[sky, 2016-03-17 19:52:43]

On 2016/03/17 07:33:10, karandeepb wrote:
> PTAL sky@. Let me know what you think of the proposed api changes, before I
> begin refactoring the code.

My concern with an API like you've outlined is how clients know which constant
to use. Lets say I create a new view subclass, how do I know if it should use
CONTROL vs ALWAYS vs ACCESSIBLE_ONLY? Even more confusing is something like
table_view, where you've made it ALWAYS because of platform specific behavior.

The API we have currently is simple, which I like. Less easy to get wrong. Seems
like what you really want is a master list of which controls should be focusable
depending upon the platform. Could we keep the current API and change call sites
to call off to a function given an enum for the control type so that we can
inject platform specific behavior?

[karandeepb, 2016-03-18 07:38:59]

On 2016/03/17 19:52:43, sky wrote:
> On 2016/03/17 07:33:10, karandeepb wrote:
> > PTAL sky@. Let me know what you think of the proposed api changes, before I
> > begin refactoring the code.
> 
> My concern with an API like you've outlined is how clients know which constant
> to use. Lets say I create a new view subclass, how do I know if it should use
> CONTROL vs ALWAYS vs ACCESSIBLE_ONLY? Even more confusing is something like
> table_view, where you've made it ALWAYS because of platform specific behavior.
> 
> The API we have currently is simple, which I like. Less easy to get wrong.
Seems
> like what you really want is a master list of which controls should be
focusable
> depending upon the platform. Could we keep the current API and change call
sites
> to call off to a function given an enum for the control type so that we can
> inject platform specific behavior?

Thanks sky@. Can you elaborate a bit more on the approach you propose? 

If we use a master list approach, then would we still need the
SetFocusable/SetAccessibilityFocusable functions, since the master list will
describe the focusability settings for us, and the
IsFocusable/IsAccessibilityFocusable functions can use the list. 

Or are you proposing something similar to the approach in Patch Set 6, where for
views which represented controls, we called off to a function
views::PlatformStyle::SetControlStyleFocus(view) which injected the necessary
platform specific behavior. 

However, any such approach which keeps SetFocusable and
SetAccessibilityFocusable around will have the drawback of clients not becoming
aware of a function like views::PlatformStyle::SetControlStyleFocus(view) which
injects platform specific behavior. The current approach will at least force the
clients to think about the current focusability settings for the different
platforms.

[karandeepb, 2016-03-18 07:40:47]

On 2016/03/18 07:38:59, karandeepb wrote:
> On 2016/03/17 19:52:43, sky wrote:
> > On 2016/03/17 07:33:10, karandeepb wrote:
> > > PTAL sky@. Let me know what you think of the proposed api changes, before
I
> > > begin refactoring the code.
> > 
> > My concern with an API like you've outlined is how clients know which
constant
> > to use. Lets say I create a new view subclass, how do I know if it should
use
> > CONTROL vs ALWAYS vs ACCESSIBLE_ONLY? Even more confusing is something like
> > table_view, where you've made it ALWAYS because of platform specific
behavior.
> > 
> > The API we have currently is simple, which I like. Less easy to get wrong.
> Seems
> > like what you really want is a master list of which controls should be
> focusable
> > depending upon the platform. Could we keep the current API and change call
> sites
> > to call off to a function given an enum for the control type so that we can
> > inject platform specific behavior?
> 
> Thanks sky@. Can you elaborate a bit more on the approach you propose? 
> 
> If we use a master list approach, then would we still need the
> SetFocusable/SetAccessibilityFocusable functions, since the master list will
> describe the focusability settings for us, and the
> IsFocusable/IsAccessibilityFocusable functions can use the list. 
> 
> Or are you proposing something similar to the approach in Patch Set 6, where
for
> views which represented controls, we called off to a function
> views::PlatformStyle::SetControlStyleFocus(view) which injected the necessary
> platform specific behavior. 
> 
> However, any such approach which keeps SetFocusable and
> SetAccessibilityFocusable around will have the drawback of clients not
becoming
> aware of a function like views::PlatformStyle::SetControlStyleFocus(view)
which
> injects platform specific behavior. The current approach will at least force
the
> clients to think about the current focusability settings for the different
> platforms.

The current approach will at least force the clients to think about the *correct
focusability settings for the different platforms.

[sky, 2016-03-18 17:11:42]

On Fri, Mar 18, 2016 at 12:38 AM,  <karandeepb@chromium.org> wrote:
> On 2016/03/17 19:52:43, sky wrote:
>> On 2016/03/17 07:33:10, karandeepb wrote:
>> > PTAL sky@. Let me know what you think of the proposed api changes,
>> > before I
>> > begin refactoring the code.
>>
>> My concern with an API like you've outlined is how clients know which
>> constant
>> to use. Lets say I create a new view subclass, how do I know if it should
>> use
>> CONTROL vs ALWAYS vs ACCESSIBLE_ONLY? Even more confusing is something
>> like
>> table_view, where you've made it ALWAYS because of platform specific
>> behavior.
>>
>> The API we have currently is simple, which I like. Less easy to get wrong.
> Seems
>> like what you really want is a master list of which controls should be
> focusable
>> depending upon the platform. Could we keep the current API and change call
> sites
>> to call off to a function given an enum for the control type so that we
>> can
>> inject platform specific behavior?
>
> Thanks sky@. Can you elaborate a bit more on the approach you propose?
>
> If we use a master list approach, then would we still need the
> SetFocusable/SetAccessibilityFocusable functions, since the master list will
> describe the focusability settings for us, and the
> IsFocusable/IsAccessibilityFocusable functions can use the list.
>
> Or are you proposing something similar to the approach in Patch Set 6, where
> for
> views which represented controls, we called off to a function
> views::PlatformStyle::SetControlStyleFocus(view) which injected the
> necessary
> platform specific behavior.

Somewhat. I was thinking that in views any code that changes focus
calls off to PlatformStyle to determine focus state. For example,
Checkbox has code like:

SetFocusable(true);

It would change to:

if (PlatformStyle::GetFocusState(Control::BUTTON))
  SetFocusable(true);

or perhaps

PlatformStyle::ConfigureFocus(Control::BUTTON, this);

The enum of controls is only the set in views.

> However, any such approach which keeps SetFocusable and
> SetAccessibilityFocusable around will have the drawback of clients not
> becoming
> aware of a function like views::PlatformStyle::SetControlStyleFocus(view)
> which
> injects platform specific behavior. The current approach will at least force
> the
> clients to think about the current focusability settings for the different
> platforms.

What I find confusing is when should FocusBehavior::CONTROL be used? I
would have expected a table to be a control, but that isn't the case.

Seems like there are two problems:
. How do we configure leaf type views in views for focus behavior per
platform. What I outlined addresses that.
. How do leaf type views outside of views get focus behavior. I don't
think we have that many of these. Do we?

  -Scott

>
> https://codereview.chromium.org/1690543004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[karandeepb, 2016-04-08 07:22:42]

On 2016/03/18 17:11:42, sky wrote:
> On Fri, Mar 18, 2016 at 12:38 AM,  <mailto:karandeepb@chromium.org> wrote:
> > On 2016/03/17 19:52:43, sky wrote:
> >> On 2016/03/17 07:33:10, karandeepb wrote:
> >> > PTAL sky@. Let me know what you think of the proposed api changes,
> >> > before I
> >> > begin refactoring the code.
> >>
> >> My concern with an API like you've outlined is how clients know which
> >> constant
> >> to use. Lets say I create a new view subclass, how do I know if it should
> >> use
> >> CONTROL vs ALWAYS vs ACCESSIBLE_ONLY? Even more confusing is something
> >> like
> >> table_view, where you've made it ALWAYS because of platform specific
> >> behavior.
> >>
> >> The API we have currently is simple, which I like. Less easy to get wrong.
> > Seems
> >> like what you really want is a master list of which controls should be
> > focusable
> >> depending upon the platform. Could we keep the current API and change call
> > sites
> >> to call off to a function given an enum for the control type so that we
> >> can
> >> inject platform specific behavior?
> >
> > Thanks sky@. Can you elaborate a bit more on the approach you propose?
> >
> > If we use a master list approach, then would we still need the
> > SetFocusable/SetAccessibilityFocusable functions, since the master list will
> > describe the focusability settings for us, and the
> > IsFocusable/IsAccessibilityFocusable functions can use the list.
> >
> > Or are you proposing something similar to the approach in Patch Set 6, where
> > for
> > views which represented controls, we called off to a function
> > views::PlatformStyle::SetControlStyleFocus(view) which injected the
> > necessary
> > platform specific behavior.
> 
> Somewhat. I was thinking that in views any code that changes focus
> calls off to PlatformStyle to determine focus state. For example,
> Checkbox has code like:
> 
> SetFocusable(true);
> 
> It would change to:
> 
> if (PlatformStyle::GetFocusState(Control::BUTTON))
>   SetFocusable(true);
> 
> or perhaps
> 
> PlatformStyle::ConfigureFocus(Control::BUTTON, this);
> 
> The enum of controls is only the set in views.
> 
> > However, any such approach which keeps SetFocusable and
> > SetAccessibilityFocusable around will have the drawback of clients not
> > becoming
> > aware of a function like views::PlatformStyle::SetControlStyleFocus(view)
> > which
> > injects platform specific behavior. The current approach will at least force
> > the
> > clients to think about the current focusability settings for the different
> > platforms.
> 
> What I find confusing is when should FocusBehavior::CONTROL be used? I
> would have expected a table to be a control, but that isn't the case.
> 
> Seems like there are two problems:
> . How do we configure leaf type views in views for focus behavior per
> platform. What I outlined addresses that.
> . How do leaf type views outside of views get focus behavior. I don't
> think we have that many of these. Do we?
> 
>   -Scott
> 
> >
> > https://codereview.chromium.org/1690543004/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

>Somewhat. I was thinking that in views any code that changes focus
>calls off to PlatformStyle to determine focus state. For example,
>Checkbox has code like:
>
>SetFocusable(true);
>
>It would change to:
>
>if (PlatformStyle::GetFocusState(Control::BUTTON))
  >SetFocusable(true);
>
>or perhaps
>
>PlatformStyle::ConfigureFocus(Control::BUTTON, this);
>
>The enum of controls is only the set in views.

I have explored a patch using this approach in
https://codereview.chromium.org/1870573003/. Let me know how you feel about it.
Although I don't think I understood what you meant by "The enum of controls is
only the set in views."

>What I find confusing is when should FocusBehavior::CONTROL be used? I
>would have expected a table to be a control, but that isn't the case.

While not in full keyboard access mode, only textfields and lists should be
focusable on Mac, while other view types should not be focusable. Hence while a
table or a textfield is a control, I had used FocusBehavior::ALWAYS, since these
types are to be always focusable on Mac. Probably FocusBehavior::CONTROL was a
misnomer. What we need is just a FocusBehavior setting which defaults to
FocusBehavior::ALWAYS on Non-Mac platforms and FocusBehavior::ACCESSIBLE_ONLY on
Mac.  

>Seems like there are two problems:
>. How do we configure leaf type views in views for focus behavior per
>platform. What I outlined addresses that.
>. How do leaf type views outside of views get focus behavior. I don't
>think we have that many of these. Do we?

I am assuming that by outside of views, you mean code outside of ui/views. As
can be seen in https://codereview.chromium.org/1870573003/, there are multiple
places in chrome/browser/ui/views which need platform specific focus behavior.
Most of these are button types.

I have also updated the design doc
https://docs.google.com/document/d/16A_kZHCIaDEGZGIIZfEJ7sfFJD4829GcPxycuMBys...
in case you need context for the changes.

[karandeepb, 2016-04-12 00:03:59]

On 2016/04/08 07:22:42, karandeepb wrote:
> On 2016/03/18 17:11:42, sky wrote:
> > On Fri, Mar 18, 2016 at 12:38 AM,  <mailto:karandeepb@chromium.org> wrote:
> > > On 2016/03/17 19:52:43, sky wrote:
> > >> On 2016/03/17 07:33:10, karandeepb wrote:
> > >> > PTAL sky@. Let me know what you think of the proposed api changes,
> > >> > before I
> > >> > begin refactoring the code.
> > >>
> > >> My concern with an API like you've outlined is how clients know which
> > >> constant
> > >> to use. Lets say I create a new view subclass, how do I know if it should
> > >> use
> > >> CONTROL vs ALWAYS vs ACCESSIBLE_ONLY? Even more confusing is something
> > >> like
> > >> table_view, where you've made it ALWAYS because of platform specific
> > >> behavior.
> > >>
> > >> The API we have currently is simple, which I like. Less easy to get
wrong.
> > > Seems
> > >> like what you really want is a master list of which controls should be
> > > focusable
> > >> depending upon the platform. Could we keep the current API and change
call
> > > sites
> > >> to call off to a function given an enum for the control type so that we
> > >> can
> > >> inject platform specific behavior?
> > >
> > > Thanks sky@. Can you elaborate a bit more on the approach you propose?
> > >
> > > If we use a master list approach, then would we still need the
> > > SetFocusable/SetAccessibilityFocusable functions, since the master list
will
> > > describe the focusability settings for us, and the
> > > IsFocusable/IsAccessibilityFocusable functions can use the list.
> > >
> > > Or are you proposing something similar to the approach in Patch Set 6,
where
> > > for
> > > views which represented controls, we called off to a function
> > > views::PlatformStyle::SetControlStyleFocus(view) which injected the
> > > necessary
> > > platform specific behavior.
> > 
> > Somewhat. I was thinking that in views any code that changes focus
> > calls off to PlatformStyle to determine focus state. For example,
> > Checkbox has code like:
> > 
> > SetFocusable(true);
> > 
> > It would change to:
> > 
> > if (PlatformStyle::GetFocusState(Control::BUTTON))
> >   SetFocusable(true);
> > 
> > or perhaps
> > 
> > PlatformStyle::ConfigureFocus(Control::BUTTON, this);
> > 
> > The enum of controls is only the set in views.
> > 
> > > However, any such approach which keeps SetFocusable and
> > > SetAccessibilityFocusable around will have the drawback of clients not
> > > becoming
> > > aware of a function like views::PlatformStyle::SetControlStyleFocus(view)
> > > which
> > > injects platform specific behavior. The current approach will at least
force
> > > the
> > > clients to think about the current focusability settings for the different
> > > platforms.
> > 
> > What I find confusing is when should FocusBehavior::CONTROL be used? I
> > would have expected a table to be a control, but that isn't the case.
> > 
> > Seems like there are two problems:
> > . How do we configure leaf type views in views for focus behavior per
> > platform. What I outlined addresses that.
> > . How do leaf type views outside of views get focus behavior. I don't
> > think we have that many of these. Do we?
> > 
> >   -Scott
> > 
> > >
> > > https://codereview.chromium.org/1690543004/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> 
> >Somewhat. I was thinking that in views any code that changes focus
> >calls off to PlatformStyle to determine focus state. For example,
> >Checkbox has code like:
> >
> >SetFocusable(true);
> >
> >It would change to:
> >
> >if (PlatformStyle::GetFocusState(Control::BUTTON))
>   >SetFocusable(true);
> >
> >or perhaps
> >
> >PlatformStyle::ConfigureFocus(Control::BUTTON, this);
> >
> >The enum of controls is only the set in views.
> 
> I have explored a patch using this approach in
> https://codereview.chromium.org/1870573003/. Let me know how you feel about
it.
> Although I don't think I understood what you meant by "The enum of controls is
> only the set in views."
> 
> >What I find confusing is when should FocusBehavior::CONTROL be used? I
> >would have expected a table to be a control, but that isn't the case.
> 
> While not in full keyboard access mode, only textfields and lists should be
> focusable on Mac, while other view types should not be focusable. Hence while
a
> table or a textfield is a control, I had used FocusBehavior::ALWAYS, since
these
> types are to be always focusable on Mac. Probably FocusBehavior::CONTROL was a
> misnomer. What we need is just a FocusBehavior setting which defaults to
> FocusBehavior::ALWAYS on Non-Mac platforms and FocusBehavior::ACCESSIBLE_ONLY
on
> Mac.  
> 
> >Seems like there are two problems:
> >. How do we configure leaf type views in views for focus behavior per
> >platform. What I outlined addresses that.
> >. How do leaf type views outside of views get focus behavior. I don't
> >think we have that many of these. Do we?
> 
> I am assuming that by outside of views, you mean code outside of ui/views. As
> can be seen in https://codereview.chromium.org/1870573003/, there are multiple
> places in chrome/browser/ui/views which need platform specific focus behavior.
> Most of these are button types.
> 
> I have also updated the design doc
>
https://docs.google.com/document/d/16A_kZHCIaDEGZGIIZfEJ7sfFJD4829GcPxycuMBys...
> in case you need context for the changes.

ping sky@.

[sky, 2016-04-12 16:39:52]

https://codereview.chromium.org/1690543004/diff/300001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1690543004/diff/300001/ui/views/view.h#newcod...
ui/views/view.h:131: CONTROL,
CONTROL sounds good, but in practice it is really CONTROL_AS_UNDERSTOOD_BY_OSX,
which is hardly satisfying. The table approach I outlined and you implemented
here https://codereview.chromium.org/1870573003 suffers from needing a
centralized list of all possible controls. I'm ok with that approach, but the
ui/views code shouldn't contain a list of leaf view nodes defined else where,
e.g. DESKTOP_MEDIA_SOURCE_VIEW should be in chrome. That means you need both a
platformstyle in ui/views and one in other places defined their own leaf view
nodes (such as chrome, but likely other places as well).

I'm leaning toward ifdefs in the controls themselves. This avoids needing
multiple lists, and it avoids CONTROL_AS_UNDERSTOOD_BY_OSX.

More concretely I'm saying keep the FocusBehavior you have here, but not
CONTROL. Code like that in button becomes:

namespace {
View::FocusBehavior GetButtonFocusBehavior() {
#if defined(OS_MACOSX)
  return FocusBehavior::ACCESSIBLE_ONLY;
#else
  return FocusBehavior::ALWAYS;
#endif
}
}  // namespace

SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);

Thoughts?

[karandeepb, 2016-04-13 02:01:22]

On 2016/04/12 16:39:52, sky wrote:
> https://codereview.chromium.org/1690543004/diff/300001/ui/views/view.h
> File ui/views/view.h (right):
> 
>
https://codereview.chromium.org/1690543004/diff/300001/ui/views/view.h#newcod...
> ui/views/view.h:131: CONTROL,
> CONTROL sounds good, but in practice it is really
CONTROL_AS_UNDERSTOOD_BY_OSX,
> which is hardly satisfying. The table approach I outlined and you implemented
> here https://codereview.chromium.org/1870573003 suffers from needing a
> centralized list of all possible controls. I'm ok with that approach, but the
> ui/views code shouldn't contain a list of leaf view nodes defined else where,
> e.g. DESKTOP_MEDIA_SOURCE_VIEW should be in chrome. That means you need both a
> platformstyle in ui/views and one in other places defined their own leaf view
> nodes (such as chrome, but likely other places as well).
> 
> I'm leaning toward ifdefs in the controls themselves. This avoids needing
> multiple lists, and it avoids CONTROL_AS_UNDERSTOOD_BY_OSX.
> 
> More concretely I'm saying keep the FocusBehavior you have here, but not
> CONTROL. Code like that in button becomes:
> 
> namespace {
> View::FocusBehavior GetButtonFocusBehavior() {
> #if defined(OS_MACOSX)
>   return FocusBehavior::ACCESSIBLE_ONLY;
> #else
>   return FocusBehavior::ALWAYS;
> #endif
> }
> }  // namespace
> 
> SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
> 
> Thoughts?

I personally liked the SetFocusBehavior with CONTROL better, but I agree CONTROL
as a concept isn't well defined semantically. Though if we name it something
like CONTROL_DEFAULT, it probably starts to make more sense.

I don't think GetButtonFocusBehavior() can reside in an anon namespace within
button, since there are a lot of call sites doing 
  button->SetFocusable(true);

We can add a public method to button itself like
  button->ConfigureDefaultFocus();
which injects platform specific behavior. I don't think there are other cases
where we'll need to do this, since most, if not all view classes have there
focus configured within the class implementation itself.

[sky, 2016-04-13 16:31:14]

On Tue, Apr 12, 2016 at 7:01 PM,  <karandeepb@chromium.org> wrote:
> On 2016/04/12 16:39:52, sky wrote:
>> https://codereview.chromium.org/1690543004/diff/300001/ui/views/view.h
>> File ui/views/view.h (right):
>>
>>
>
https://codereview.chromium.org/1690543004/diff/300001/ui/views/view.h#newcod...
>> ui/views/view.h:131: CONTROL,
>> CONTROL sounds good, but in practice it is really
> CONTROL_AS_UNDERSTOOD_BY_OSX,
>> which is hardly satisfying. The table approach I outlined and you
>> implemented
>> here https://codereview.chromium.org/1870573003 suffers from needing a
>> centralized list of all possible controls. I'm ok with that approach, but
>> the
>> ui/views code shouldn't contain a list of leaf view nodes defined else
>> where,
>> e.g. DESKTOP_MEDIA_SOURCE_VIEW should be in chrome. That means you need
>> both a
>> platformstyle in ui/views and one in other places defined their own leaf
>> view
>> nodes (such as chrome, but likely other places as well).
>>
>> I'm leaning toward ifdefs in the controls themselves. This avoids needing
>> multiple lists, and it avoids CONTROL_AS_UNDERSTOOD_BY_OSX.
>>
>> More concretely I'm saying keep the FocusBehavior you have here, but not
>> CONTROL. Code like that in button becomes:
>>
>> namespace {
>> View::FocusBehavior GetButtonFocusBehavior() {
>> #if defined(OS_MACOSX)
>> return FocusBehavior::ACCESSIBLE_ONLY;
>> #else
>> return FocusBehavior::ALWAYS;
>> #endif
>> }
>> } // namespace
>>
>> SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
>>
>> Thoughts?
>
> I personally liked the SetFocusBehavior with CONTROL better, but I agree
> CONTROL
> as a concept isn't well defined semantically. Though if we name it something
> like CONTROL_DEFAULT, it probably starts to make more sense.

I think CONTROL is super confusing because it's really
CONTROL_AS_UNDERSTOOD_BY_OSX. Do you disagree with that? If not, why
is table not a control?

> I don't think GetButtonFocusBehavior() can reside in an anon namespace
> within
> button, since there are a lot of call sites doing
> button->SetFocusable(true);
>
> We can add a public method to button itself like
> button->ConfigureDefaultFocus();
> which injects platform specific behavior. I don't think there are other
> cases
> where we'll need to do this, since most, if not all view classes have there
> focus configured within the class implementation itself.

I'm fine with button->ConfigureDefaultFocus().

  -Scott

>
>
> https://codereview.chromium.org/1690543004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[karandeepb, 2016-04-14 09:58:26]

> I think CONTROL is super confusing because it's really
> CONTROL_AS_UNDERSTOOD_BY_OSX. Do you disagree with that? If not, why
> is table not a control?
 
Yeah, I agree CONTROL is not that well defined a concept. I'll work on
implementing the #ifdef with FocusBehavior approach.

[sky, 2016-04-14 16:11:25]

SGTM

On Thu, Apr 14, 2016 at 2:58 AM,  <karandeepb@chromium.org> wrote:
>
>> I think CONTROL is super confusing because it's really
>> CONTROL_AS_UNDERSTOOD_BY_OSX. Do you disagree with that? If not, why
>> is table not a control?
>
> Yeah, I agree CONTROL is not that well defined a concept. I'll work on
> implementing the #ifdef with FocusBehavior approach.
>
> https://codereview.chromium.org/1690543004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[karandeepb, 2016-04-18 10:47:05]

Description was changed from

==========
MacViews: Implement Full Keyboard Access.

Currently MacViews does not respect the full keyboard accessibility setting.
In the normal mode, as per Full Keyboard Access, only text boxes and lists
should get focus. However, currently, views like comboboxes, buttons, links
etc. get focus in the normal mode.

This CL implements Full Keyboard access by adding a platform level keyboard
accessibility setting to the FocusManager class. It also adds a new method
SetFocusBehavior to the views class to replace the existing SetFocusable() and
SetAccessibilityFocusable(), which shall be made private.

Doc -
https://docs.google.com/document/d/16A_kZHCIaDEGZGIIZfEJ7sfFJD4829GcPxycuMBys...

BUG=564912
==========

to

==========
MacViews: Implement Full Keyboard Access.

Currently MacViews does not respect the full keyboard accessibility setting.
In the normal mode, as per Full Keyboard Access, only text boxes and lists
should get focus. However, currently, views like comboboxes, buttons, links
etc. get focus in the normal mode.

This CL implements Full Keyboard access by adding a platform level keyboard
accessibility setting to the FocusManager class. It also adds a new method
SetFocusBehavior to the views class to replace the existing SetFocusable() and
SetAccessibilityFocusable(), which have been made private.

Other changes-
- Adds a accessibility_focusable() method to ui/views/view.h.
- Adds a static ConfigureDefaultFocus() method to 
  ui/views/controls/button/button.h
- Adds ScopedFakeFullKeyboardAccess to swizzle 
  [NSAppl isFullKeyboardAccessEnabled] for use in tests.
- ScopedFakeNSWindowFocus now also swizzles orderOut: to determine if the 
  currently focused window is ordered-out.
- View::RequestToFocus now uses IsAccessibilityFocusable() instead of 
  IsFocusable().

Doc -
https://docs.google.com/document/d/16A_kZHCIaDEGZGIIZfEJ7sfFJD4829GcPxycuMBys...

BUG=564912
==========

[karandeepb, 2016-04-18 11:22:22]

Patchset #14 (id:340001) has been deleted

[karandeepb, 2016-04-18 11:22:59]

Patchset #13 (id:320001) has been deleted

[karandeepb, 2016-04-18 11:29:02]

PTAL sky@ and Trent. I have implemented the SetFocusBehavior with #ifdef
approach. I have also added relevant tests in 
bridged_native_widget_unittest.mm, focus_manager_unittest.cc and
focus_traversal_unittest.cc. 

The patch would be a lot smaller if we use SetFocusable and
SetAccessibilityFocusable directly, so we should use SetFocusBehavior only if
you feel that it provides a strictly better API.

The core logic of the patch is in the following files-
-ui/views/cocoa/bridged_content_view.*
-ui/views/cocoa/bridged_native_widget.mm
-ui/views/focus/focus_search.*
-ui/views/focus/focus_manager.*
-ui/views/view.*

https://codereview.chromium.org/1690543004/diff/360001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1690543004/diff/360001/ui/views/view.cc#newco...
ui/views/view.cc:1239: if (focus_manager && IsAccessibilityFocusable())
Does changing IsFocusable to IsAccessibilityFocusable here seem ok, or should
this be done only on Mac?

[sky, 2016-04-18 15:40:24]

https://codereview.chromium.org/1690543004/diff/360001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1690543004/diff/360001/ui/views/view.h#newcod...
ui/views/view.h:119: enum class FocusBehavior {
Can you please separate out the FocusBehavior into it's own patch?

[karandeepb, 2016-04-29 03:02:36]

Closing this since this was split into -
https://codereview.chromium.org/1898633004/ and
https://codereview.chromium.org/1894383002/.