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.

Other changes-
- Adds a static ConfigureDefaultFocus() method to
  ui/views/controls/button/button.h
- Adds ScopedFakeFullKeyboardAccess to ViewsTestHelperMac 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().

For prior discussion related to this CL, see crrev.com/1690543004.

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

BUG=564912
Committed: https://crrev.com/24c156252c11425af51d646cd0d405f018c70bb8
Cr-Commit-Position: refs/heads/master@{#391744}

[karandeepb, 2016-04-19 10:31:50]

Patchset #1 (id:1) has been deleted

[karandeepb, 2016-04-19 10:40:03]

Description was changed from

==========
MacViews-Final
==========

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. 

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-20 01:42:40]

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. 

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
==========

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. 

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().

For prior discussion related to this CL, see crrev.com/1690543004.

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

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

[karandeepb, 2016-04-20 01:43:06]

Patchset #1 (id:20001) has been deleted

[karandeepb, 2016-04-20 04:08:58]

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

[karandeepb, 2016-04-20 04:08:59]

PTAL sky@ and Trent.

@tapted - Can you confirm whether ui/views/controls/webview/webview.cc should
have FocusBehavior::ALWAYS or ACCESSIBLE_ONLY on macviews?

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

https://codereview.chromium.org/1894383002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:1239: if (focus_manager && IsAccessibilityFocusable())
@sky- Can you confirm whether this is ok to do?

[karandeepb, 2016-04-20 04:12:55]

@tapted - Also this causes 2 new views_unittests to fail on MacViews. Will fix
them in a subsequent CL.

[tapted, 2016-04-20 06:05:58]

On 2016/04/20 04:08:59, karandeepb wrote:
> @tapted - Can you confirm whether ui/views/controls/webview/webview.cc should
> have FocusBehavior::ALWAYS or ACCESSIBLE_ONLY on macviews?

I'd go with ALWAYS for webview.cc

On 2016/04/20 04:12:55, karandeepb wrote:
> @tapted - Also this causes 2 new views_unittests to fail on MacViews. Will fix
> them in a subsequent CL.

What are the details? True this CL is big... maybe you can prepare the CL to fix
them so I can have a peek :)

https://codereview.chromium.org/1894383002/diff/60001/ash/shell/lock_view.cc
File ash/shell/lock_view.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ash/shell/lock_view.cc#...
ash/shell/lock_view.cc:33: views::Button::ConfigureDefaultFocus(unlock_button_);
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?)

https://codereview.chromium.org/1894383002/diff/60001/ash/system/tray/tray_po...
File ash/system/tray/tray_popup_header_button.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ash/system/tray/tray_po...
ash/system/tray/tray_popup_header_button.cc:38:
views::Button::ConfigureDefaultFocus(this);
if |this| inherits from button, there shouldn't be a need for the
`views::Button::` prefix when calling static methods - more cases throughout.
But this comment is probably redundant if we switch to an instance method, which
I think will look nicer.

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
File ui/base/test/scoped_fake_full_keyboard_access.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_full_keyboard_access.mm:13: bool
g_full_keyboard_access_mode = false;
A lot of tests depend on traversing or setting focus to particular things - we
need to make sure that that behaviour doesn't depend on a system setting on the
particular machine a test is running on.

So I think this should default to "true" -- that will make it more consistent
across platforms. But also the ScopedFakeFullKeyboardAccess should be
"installed" by default for most of the UI tests -- probably into
ViewsTestHelperMac where the focus faker is already.

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_full_keyboard_access.mm:17: // Donates testing
implementation of [NSApp isFullKeyboardAccessEnabled].
nit: testing -> a testing

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_full_keyboard_access.mm:41: g_full_keyboard_access_mode
= new_state;
we need to test the notifications too. That is, when changing, this should
broadcast out to NSDistributedNotificationCenter similar to the way the
fullscreen faker does

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
File ui/base/test/scoped_fake_nswindow_focus.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_nswindow_focus.mm:73: - (void)orderOut:(id)sender {
What tests ended up needing this? It doesn't seem right to swizzle just orderOut
-- there are too many ways of making a window hidden/visible on Mac -- e.g.
there's `-[NSWindow orderWindow:NSWindowOut relateiveTo:nil]`. And ther should
maybe be a balanced orderIn.

Is there a way to resolve the problem by watching for -resignKeyWindow instead? 
(in theory, all the native OrderOut methods should be invoking that)

And if it's just for an added test (e.g.
BridgedNativeWidgetFullKeyboardAccess.something), we might want to restrict the
swizzling just to those tests, or just provide a way to call `ClearFocus`
explicitly via the ScopedFakeNSWindowFocus public API

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:250: [[NSDistributedNotificationCenter
defaultCenter] removeObserver:self];
nit: move up before [cursorTrackingArea_.get() clearOwner]; (should be reverse
order of initializer)

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:318: if
(hostedView_->GetWidget()->GetFocusManager())
nit: needs curlies, but also, the FocusManager should be cached in a local var,
which may or may not make the `if` body a single line

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:754: class
BridgedNativeWidgetFullKeyboardAccess
BridgedNativeWidgetFullKeyboardAccessTest

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:779:
~BridgedNativeWidgetFullKeyboardAccess() {}
these can be defined inline

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:786: // Don't use test_window()
since it has special handling when key.
It might be easier to use NativeWidgetMacTest -- BridgedNativeWidgetTestBase is
for really low-level / lifetime stuff -- it can be tricky to get the setup
right.  (and, e.g, we should try not to poke around too much with
views::internal::RootView if we don't need to)

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:817: [bridge()->ns_view()
updateFullKeyboardAccess];
(unless you run into problems, this should be triggered by the faker via
NSDistributedNotificationCenter)

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.h (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/custom_button.h:23: // part of the focus chain, unless
in accessibility mode. Call
Where's the "unless in accessibility mode" bit triggered? I was expecting to
find an ACCESSIBLE_ONLY somewhere...

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/image_button.h (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/image_button.h:21: 
this line should either be removed or be just `//` by itself

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/label...
ui/views/controls/label.cc:216: if (focusable() || accessibility_focusable()) {
oh butts - how many things call View::focusable()?  Can we replace it with
something like

bool View::CanFocus() const {
  return focus_behavior_ != FocusBehavior::NEVER;
}

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/table...
File ui/views/controls/table/table_view.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/table...
ui/views/controls/table/table_view.cc:150: // use ALWAYS focus behavior for
TableView.
This comment feels out of place. I think it's enough to say

// Always focusable, even on Mac (consistent with NSTableView).

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:298: // On Mac, only textfields and
list controls are focusable by default. Hence
Same here. E.g. saying "only <foo>" suggests we're about to make it _not_
focusable, because we're not a <foo>. But this is a <foo>.

This probably doesn't need a comment at all. A textfield without focus is a
weird concept on any platform.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/tree/...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:85: // represents a list, use ALWAYS focus
behavior.
This could be

// Always focusable, even on Mac (consistent with NS<whatever it is>).

https://codereview.chromium.org/1894383002/diff/60001/ui/views/focus/focus_tr...
File ui/views/focus/focus_traversal_unittest.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/focus/focus_tr...
ui/views/focus/focus_traversal_unittest.cc:609:
GetFocusManager()->SetKeyboardAccessible(false);
indenting is off here

https://codereview.chromium.org/1894383002/diff/60001/ui/views/focus/focus_tr...
ui/views/focus/focus_traversal_unittest.cc:618: // Let's traverse the whole
focus hierarchy (several times, to make sure it
I think this needs to be a helper function - it's too much code to copy-paste.
You could call it here with something like

SCOPED_TRACE("NormalTraversalMac");
AdvanceEntireFocusLoop(begin(kTraversalIDs), end(ktraversalIDs));

https://codereview.chromium.org/1894383002/diff/60001/ui/views/focus/focus_tr...
ui/views/focus/focus_traversal_unittest.cc:625: EXPECT_TRUE(focused_view !=
NULL);
I think 

EXPECT_NE(nullptr, focused_view)

is the latest fashion for these checks

https://codereview.chromium.org/1894383002/diff/60001/ui/views/focus/focus_tr...
ui/views/focus/focus_traversal_unittest.cc:675: #endif
nit:  // OS_MACOSX

https://codereview.chromium.org/1894383002/diff/60001/ui/views/test/focus_man...
File ui/views/test/focus_manager_test.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/test/focus_man...
ui/views/test/focus_manager_test.cc:45: // By default test with full keyboard
accessibility on Mac.
(should be unnecessary with a much earlier comment)

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

https://codereview.chromium.org/1894383002/diff/60001/ui/views/view.h#newcode...
ui/views/view.h:1152: // Returns last value passed to
SetAccessibilityFocusable(). Use
(ideally this is unnecessary with something like `CanFocus` -- depends how many
things access |focusable()|

[karandeepb, 2016-04-29 03:27:57]

Patchset #3 (id:80001) has been deleted

[karandeepb, 2016-05-03 00:44:17]

Patchset #4 (id:120001) has been deleted

[karandeepb, 2016-05-03 01:08:44]

Patchset #4 (id:140001) has been deleted

[karandeepb, 2016-05-03 02:45:20]

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. 

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().

For prior discussion related to this CL, see crrev.com/1690543004.

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. 

Other changes-
- 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().

For prior discussion related to this CL, see crrev.com/1690543004.

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

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

[karandeepb, 2016-05-03 02:46:17]

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. 

Other changes-
- 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().

For prior discussion related to this CL, see crrev.com/1690543004.

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. 

Other changes-
- Adds a static ConfigureDefaultFocus() method to 
  ui/views/controls/button/button.h
- Adds ScopedFakeFullKeyboardAccess to ViewsTestHelperMac 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().

For prior discussion related to this CL, see crrev.com/1690543004.

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

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

[karandeepb, 2016-05-03 02:46:41]

Patchset #4 (id:160001) has been deleted

[karandeepb, 2016-05-03 02:46:56]

Patchset #4 (id:180001) has been deleted

[karandeepb, 2016-05-03 02:47:13]

Patchset #4 (id:200001) has been deleted

[karandeepb, 2016-05-03 02:47:41]

Patchset #4 (id:220001) has been deleted

[karandeepb, 2016-05-03 02:54:13]

PTAL Trent. No new views_unittests fail on Mac now.

https://codereview.chromium.org/1894383002/diff/60001/ash/shell/lock_view.cc
File ash/shell/lock_view.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ash/shell/lock_view.cc#...
ash/shell/lock_view.cc:33: views::Button::ConfigureDefaultFocus(unlock_button_);
On 2016/04/20 06:05:57, tapted wrote:
> 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?)

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.

https://codereview.chromium.org/1894383002/diff/60001/ash/system/tray/tray_po...
File ash/system/tray/tray_popup_header_button.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ash/system/tray/tray_po...
ash/system/tray/tray_popup_header_button.cc:38:
views::Button::ConfigureDefaultFocus(this);
On 2016/04/20 06:05:57, tapted wrote:
> if |this| inherits from button, there shouldn't be a need for the
> `views::Button::` prefix when calling static methods - more cases throughout.
> But this comment is probably redundant if we switch to an instance method,
which
> I think will look nicer.

Done. But I have kept the Button:: prefix, since it makes it clear that we are
calling a static method.

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
File ui/base/test/scoped_fake_full_keyboard_access.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_full_keyboard_access.mm:13: bool
g_full_keyboard_access_mode = false;
On 2016/04/20 06:05:57, tapted wrote:
> A lot of tests depend on traversing or setting focus to particular things - we
> need to make sure that that behaviour doesn't depend on a system setting on
the
> particular machine a test is running on.
> 
> So I think this should default to "true" -- that will make it more consistent
> across platforms. But also the ScopedFakeFullKeyboardAccess should be
> "installed" by default for most of the UI tests -- probably into
> ViewsTestHelperMac where the focus faker is already.

Yeah I didn't take into account that these tests may depend on system setting.
Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_full_keyboard_access.mm:17: // Donates testing
implementation of [NSApp isFullKeyboardAccessEnabled].
On 2016/04/20 06:05:57, tapted wrote:
> nit: testing -> a testing

Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_full_keyboard_access.mm:41: g_full_keyboard_access_mode
= new_state;
On 2016/04/20 06:05:57, tapted wrote:
> we need to test the notifications too. That is, when changing, this should
> broadcast out to NSDistributedNotificationCenter similar to the way the
> fullscreen faker does

I can't get broadcasting notifications to NSDistributedNotificationCenter to
work. I am not sure why. The doc for NSDistributedNotificationCenter says - "The
NSDistributedNotificationCenter class provides a way to send notifications to
objects in other tasks. It takes NSNotification objects and broadcasts them to
any objects in other tasks that have registered for the notification with their
task’s default distributed notification center." I am guessing notifications
sent via NSDistributedNotificationCenter are not broadcasted to the broadcasting
application, but only to other interested applications. Maybe we can simulate an
actual Ctrl+F7 key press?

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
File ui/base/test/scoped_fake_nswindow_focus.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_nswindow_focus.mm:73: - (void)orderOut:(id)sender {
On 2016/04/20 06:05:57, tapted wrote:
> What tests ended up needing this? It doesn't seem right to swizzle just
orderOut
> -- there are too many ways of making a window hidden/visible on Mac -- e.g.
> there's `-[NSWindow orderWindow:NSWindowOut relateiveTo:nil]`. And ther should
> maybe be a balanced orderIn.
> 
> Is there a way to resolve the problem by watching for -resignKeyWindow
instead? 
> (in theory, all the native OrderOut methods should be invoking that)
> 
> And if it's just for an added test (e.g.
> BridgedNativeWidgetFullKeyboardAccess.something), we might want to restrict
the
> swizzling just to those tests, or just provide a way to call `ClearFocus`
> explicitly via the ScopedFakeNSWindowFocus public API

I needed this for NativeWidgetMacFullKeyboardAccessTest.Activation. I had tried 
swizzling -resignKeyWindow, but it's not being invoked. I guess this is expected
because the window is not actually key, since we are swizzling makeKeyWindow: 
and isKeyWindow:.

Still I think this is a good change if we can't find any other way to track the 
"unfocusing" of a window because- 
1) Since we are not swizzling any "unfocusing" methods currently, 
g_fake_focused_window is not completely correct. Hence if we do 
  widget->Show()
  widget->Hide()
g_fake_focused_window will still point to widget's window. Hence if we do a
Show() again, it will be ignored.

2) [window close] also calls orderOut. 

3) Current implementation can lead to subtle bugs. For example, if the currently
focused window is destroyed(say by invoking close), and then
ScopedFakeNSWindowFocus is
destroyed, it will cause an invalid memory reference, since
g_fake_focused_window
points to a destroyed window. Hence we have to be careful about destruction
sequence currently.

4) This still calls the original orderOut implementation, so I don't see this as
having any adverse effects.

Also, this will help generate windowDidResignKey notifications on widget->Hide()
which is not the case currently.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:250: [[NSDistributedNotificationCenter
defaultCenter] removeObserver:self];
On 2016/04/20 06:05:57, tapted wrote:
> nit: move up before [cursorTrackingArea_.get() clearOwner]; (should be reverse
> order of initializer)

Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:318: if
(hostedView_->GetWidget()->GetFocusManager())
On 2016/04/20 06:05:57, tapted wrote:
> nit: needs curlies, but also, the FocusManager should be cached in a local
var,
> which may or may not make the `if` body a single line

Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:754: class
BridgedNativeWidgetFullKeyboardAccess
On 2016/04/20 06:05:57, tapted wrote:
> BridgedNativeWidgetFullKeyboardAccessTest

Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:779:
~BridgedNativeWidgetFullKeyboardAccess() {}
On 2016/04/20 06:05:57, tapted wrote:
> these can be defined inline

Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:786: // Don't use test_window()
since it has special handling when key.
On 2016/04/20 06:05:57, tapted wrote:
> It might be easier to use NativeWidgetMacTest -- BridgedNativeWidgetTestBase
is
> for really low-level / lifetime stuff -- it can be tricky to get the setup
> right.  (and, e.g, we should try not to poke around too much with
> views::internal::RootView if we don't need to)

Done, though it feels like these tests should belong here.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:817: [bridge()->ns_view()
updateFullKeyboardAccess];
On 2016/04/20 06:05:57, tapted wrote:
> (unless you run into problems, this should be triggered by the faker via
> NSDistributedNotificationCenter)

Can't catch notifications generated via NSDistributedNotificationCenter. Maybe
we can simulate an actual Ctrl+F7 key press. Will this work -
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/test/ui_co...
? But it seems this will require creating an interactive ui test.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.h (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/custom_button.h:23: // part of the focus chain, unless
in accessibility mode. Call
On 2016/04/20 06:05:57, tapted wrote:
> Where's the "unless in accessibility mode" bit triggered? I was expecting to
> find an ACCESSIBLE_ONLY somewhere...

This inherits from Button, which has a
SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY); See
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/image_button.h (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/image_button.h:21: 
On 2016/04/20 06:05:58, tapted wrote:
> this line should either be removed or be just `//` by itself

Sorry it isn't clear to me which line you are talking about.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/label...
ui/views/controls/label.cc:216: if (focusable() || accessibility_focusable()) {
On 2016/04/20 06:05:58, tapted wrote:
> oh butts - how many things call View::focusable()?  Can we replace it with
> something like
> 
> bool View::CanFocus() const {
>   return focus_behavior_ != FocusBehavior::NEVER;
> }

Replaced this with focus_behavior() check. These are the only 2 instances in the
codebase.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/table...
File ui/views/controls/table/table_view.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/table...
ui/views/controls/table/table_view.cc:150: // use ALWAYS focus behavior for
TableView.
On 2016/04/20 06:05:58, tapted wrote:
> This comment feels out of place. I think it's enough to say
> 
> // Always focusable, even on Mac (consistent with NSTableView).

Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:298: // On Mac, only textfields and
list controls are focusable by default. Hence
On 2016/04/20 06:05:58, tapted wrote:
> Same here. E.g. saying "only <foo>" suggests we're about to make it _not_
> focusable, because we're not a <foo>. But this is a <foo>.
> 
> This probably doesn't need a comment at all. A textfield without focus is a
> weird concept on any platform.

Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/tree/...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:85: // represents a list, use ALWAYS focus
behavior.
On 2016/04/20 06:05:58, tapted wrote:
> This could be
> 
> // Always focusable, even on Mac (consistent with NS<whatever it is>).

Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/focus/focus_tr...
File ui/views/focus/focus_traversal_unittest.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/focus/focus_tr...
ui/views/focus/focus_traversal_unittest.cc:609:
GetFocusManager()->SetKeyboardAccessible(false);
On 2016/04/20 06:05:58, tapted wrote:
> indenting is off here

Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/focus/focus_tr...
ui/views/focus/focus_traversal_unittest.cc:618: // Let's traverse the whole
focus hierarchy (several times, to make sure it
On 2016/04/20 06:05:58, tapted wrote:
> I think this needs to be a helper function - it's too much code to copy-paste.
> You could call it here with something like
> 
> SCOPED_TRACE("NormalTraversalMac");
> AdvanceEntireFocusLoop(begin(kTraversalIDs), end(ktraversalIDs));
> 

Done. Turns out there were many other similar calls

https://codereview.chromium.org/1894383002/diff/60001/ui/views/focus/focus_tr...
ui/views/focus/focus_traversal_unittest.cc:625: EXPECT_TRUE(focused_view !=
NULL);
On 2016/04/20 06:05:58, tapted wrote:
> I think 
> 
> EXPECT_NE(nullptr, focused_view)
> 
> is the latest fashion for these checks

Done.

https://codereview.chromium.org/1894383002/diff/60001/ui/views/focus/focus_tr...
ui/views/focus/focus_traversal_unittest.cc:675: #endif
On 2016/04/20 06:05:58, tapted wrote:
> nit:  // OS_MACOSX

Done.

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

https://codereview.chromium.org/1894383002/diff/60001/ui/views/view.h#newcode...
ui/views/view.h:1152: // Returns last value passed to
SetAccessibilityFocusable(). Use
On 2016/04/20 06:05:58, tapted wrote:
> (ideally this is unnecessary with something like `CanFocus` -- depends how
many
> things access |focusable()|

Removed this.

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

https://codereview.chromium.org/1894383002/diff/240001/ui/views/view.cc#newco...
ui/views/view.cc:1229: if (focus_manager && IsAccessibilityFocusable())
Does this seem fine to you or should we do this only on Mac?

[tapted, 2016-05-03 08:08:26]

looks good - nits mostly.

https://codereview.chromium.org/1894383002/diff/60001/ash/shell/lock_view.cc
File ash/shell/lock_view.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ash/shell/lock_view.cc#...
ash/shell/lock_view.cc:33: views::Button::ConfigureDefaultFocus(unlock_button_);
On 2016/05/03 02:54:12, karandeepb wrote:
> On 2016/04/20 06:05:57, tapted wrote:
> > 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?)
> 
> 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. 

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

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
File ui/base/test/scoped_fake_full_keyboard_access.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_full_keyboard_access.mm:41: g_full_keyboard_access_mode
= new_state;
On 2016/05/03 02:54:12, karandeepb wrote:
> On 2016/04/20 06:05:57, tapted wrote:
> > we need to test the notifications too. That is, when changing, this should
> > broadcast out to NSDistributedNotificationCenter similar to the way the
> > fullscreen faker does
> 
> I can't get broadcasting notifications to NSDistributedNotificationCenter to
> work. I am not sure why. The doc for NSDistributedNotificationCenter says -
"The
> NSDistributedNotificationCenter class provides a way to send notifications to
> objects in other tasks. It takes NSNotification objects and broadcasts them to
> any objects in other tasks that have registered for the notification with
their
> task’s default distributed notification center." I am guessing notifications
> sent via NSDistributedNotificationCenter are not broadcasted to the
broadcasting
> application, but only to other interested applications. Maybe we can simulate
an
> actual Ctrl+F7 key press?

Hum - that starts to get into the realms of things that would need an
interactive_ui_test. I think this is OK - hopefully
NSDistributedNotificationCenter won't come up too often.

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
File ui/base/test/scoped_fake_nswindow_focus.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_nswindow_focus.mm:73: - (void)orderOut:(id)sender {
On 2016/05/03 02:54:12, karandeepb wrote:
> On 2016/04/20 06:05:57, tapted wrote:
> > What tests ended up needing this? It doesn't seem right to swizzle just
> orderOut
> > -- there are too many ways of making a window hidden/visible on Mac -- e.g.
> > there's `-[NSWindow orderWindow:NSWindowOut relateiveTo:nil]`. And ther
should
> > maybe be a balanced orderIn.
> > 
> > Is there a way to resolve the problem by watching for -resignKeyWindow
> instead? 
> > (in theory, all the native OrderOut methods should be invoking that)
> > 
> > And if it's just for an added test (e.g.
> > BridgedNativeWidgetFullKeyboardAccess.something), we might want to restrict
> the
> > swizzling just to those tests, or just provide a way to call `ClearFocus`
> > explicitly via the ScopedFakeNSWindowFocus public API
> 
> I needed this for NativeWidgetMacFullKeyboardAccessTest.Activation. I had
tried 
> swizzling -resignKeyWindow, but it's not being invoked. I guess this is
expected
> because the window is not actually key, since we are swizzling makeKeyWindow: 
> and isKeyWindow:.
> 
> Still I think this is a good change if we can't find any other way to track
the 
> "unfocusing" of a window because- 
> 1) Since we are not swizzling any "unfocusing" methods currently, 
> g_fake_focused_window is not completely correct. Hence if we do 
>   widget->Show()
>   widget->Hide()
> g_fake_focused_window will still point to widget's window. Hence if we do a
> Show() again, it will be ignored.
> 
> 2) [window close] also calls orderOut. 
> 
> 3) Current implementation can lead to subtle bugs. For example, if the
currently
> focused window is destroyed(say by invoking close), and then
> ScopedFakeNSWindowFocus is
> destroyed, it will cause an invalid memory reference, since
> g_fake_focused_window
> points to a destroyed window. Hence we have to be careful about destruction
> sequence currently.
> 
> 4) This still calls the original orderOut implementation, so I don't see this
as
> having any adverse effects.
> 
> Also, this will help generate windowDidResignKey notifications on
widget->Hide()
> which is not the case currently.

OK sounds good. That's lots of reasons :). For a test-helper, I guess it's OK
that there may be limitations, and this is leading to fewer limitations. (E.g.
[NSApp hide] probably doesn't call orderOut, but in a real app it should call
resignKeyWindow)

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/image_button.h (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/image_button.h:21: 
On 2016/05/03 02:54:12, karandeepb wrote:
> On 2016/04/20 06:05:58, tapted wrote:
> > this line should either be removed or be just `//` by itself
> 
> Sorry it isn't clear to me which line you are talking about.

The comment "// An image button." is currently 'detached' from its class because
line 21 is blank. It should be formatted like


// An image button.
//
// Note that....
class .. ImageButton

or

// An image button.
// Note that....
class .. ImageButton

https://codereview.chromium.org/1894383002/diff/240001/ui/base/test/scoped_fa...
File ui/base/test/scoped_fake_full_keyboard_access.mm (right):

https://codereview.chromium.org/1894383002/diff/240001/ui/base/test/scoped_fa...
ui/base/test/scoped_fake_full_keyboard_access.mm:15: bool
g_full_keyboard_access_mode = true;
this can be stored as a data member on ScopedFakeFullKeyboardAccess now -
usually best to minimise the number of globals

https://codereview.chromium.org/1894383002/diff/240001/ui/views/controls/butt...
File ui/views/controls/button/button.cc (right):

https://codereview.chromium.org/1894383002/diff/240001/ui/views/controls/butt...
ui/views/controls/button/button.cc:29: // On Mac, buttons are focusable only in
full keyboard access mode.
nit: move this into the #if (then clang-format will indent it - looks weird in
the first column)

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

https://codereview.chromium.org/1894383002/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:343: // IsFocusable() if |keyboard_accessible_|
is false on Mac).
nit: (this reads a bit funny). Maybe

// Whether |view| is currently focusable per the platform's interpretation of
|keyboard_accesible_|.

https://codereview.chromium.org/1894383002/diff/240001/ui/views/focus/focus_t...
File ui/views/focus/focus_traversal_unittest.cc (right):

https://codereview.chromium.org/1894383002/diff/240001/ui/views/focus/focus_t...
ui/views/focus/focus_traversal_unittest.cc:217: void
AdvanceEntireFocusLoop(Iterator begin, Iterator end, bool reverse) {
Hm it would be nice to avoid the template<> - I think it's the reversal
requirement that means we can't just use int*. And since callers change what
they do in between reversing (i.e. FocusManager::ClearFocus or
View::RequestFocus), we can't just put that in here.

An improvement might be

template <size_t SIZE>
void AdvanceEntireFocusLoop(const int (&traversal_ids)[SIZE], bool reverse) {
..
  for (int i...3
    for (int j = 0..N
      EXPECT_EQ(traversal_id[reverse ? N - j - 1 : j], focused_view->id())
}



Since we're not going in both directions, there should be a 

SCOPED_TRACE(testing::Message() << "In reverse: " << reverse);

at the start of the function too.

We could also pass in (possibly null) FocusManager* or View* arguments to do
both directions, calling ClearFocus or ReqeustFocus appropriately, and drop the
`reverse` argument. That might become less readable though

https://codereview.chromium.org/1894383002/diff/240001/ui/views/focus/focus_t...
ui/views/focus/focus_traversal_unittest.cc:609: bool reverse = false;
I probably wouldn't worry about this extra variable - the AdvanceEntireFocusLoop
prototype is close enough to see what a true/false argument is doing

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

https://codereview.chromium.org/1894383002/diff/240001/ui/views/view.cc#newco...
ui/views/view.cc:1229: if (focus_manager && IsAccessibilityFocusable())
On 2016/05/03 02:54:13, karandeepb wrote:
> Does this seem fine to you or should we do this only on Mac?

Isn't this needed to support the change to FocusManager::RestoreFocusedView()?
Otherwise it would regress the changes in http://crrev.com/53471 . I think this
came up on the other CL --
https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...

https://codereview.chromium.org/1894383002/diff/240001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1894383002/diff/240001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:1419: // Test that a widget's
associated focus manager is initialized with the correct
nit: widget -> Widget, focus manager -> FocusManager

[karandeepb, 2016-05-04 01:15:13]

Patchset #5 (id:260001) has been deleted

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

PTAL Trent. Can you also look at the IsFocusable() usages at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
and
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/accessibi...
and let me know whether they are correct for MacViews? (since a lot of views
will now return false for IsFocusable()). Like Views::RequestFocus(), I am not
sure whether these were IsFocusable() and not IsAccessibilityFocusable() for
some deliberate reason.

https://codereview.chromium.org/1894383002/diff/60001/ash/shell/lock_view.cc
File ash/shell/lock_view.cc (right):

https://codereview.chromium.org/1894383002/diff/60001/ash/shell/lock_view.cc#...
ash/shell/lock_view.cc:33: views::Button::ConfigureDefaultFocus(unlock_button_);
On 2016/05/03 08:08:25, tapted wrote:
> On 2016/05/03 02:54:12, karandeepb wrote:
> > On 2016/04/20 06:05:57, tapted wrote:
> > > 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?)
> > 
> > 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. 
> 
> 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

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.

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
File ui/base/test/scoped_fake_nswindow_focus.mm (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/base/test/scoped_fak...
ui/base/test/scoped_fake_nswindow_focus.mm:73: - (void)orderOut:(id)sender {
On 2016/05/03 08:08:25, tapted wrote:
> On 2016/05/03 02:54:12, karandeepb wrote:
> > On 2016/04/20 06:05:57, tapted wrote:
> > > What tests ended up needing this? It doesn't seem right to swizzle just
> > orderOut
> > > -- there are too many ways of making a window hidden/visible on Mac --
e.g.
> > > there's `-[NSWindow orderWindow:NSWindowOut relateiveTo:nil]`. And ther
> should
> > > maybe be a balanced orderIn.
> > > 
> > > Is there a way to resolve the problem by watching for -resignKeyWindow
> > instead? 
> > > (in theory, all the native OrderOut methods should be invoking that)
> > > 
> > > And if it's just for an added test (e.g.
> > > BridgedNativeWidgetFullKeyboardAccess.something), we might want to
restrict
> > the
> > > swizzling just to those tests, or just provide a way to call `ClearFocus`
> > > explicitly via the ScopedFakeNSWindowFocus public API
> > 
> > I needed this for NativeWidgetMacFullKeyboardAccessTest.Activation. I had
> tried 
> > swizzling -resignKeyWindow, but it's not being invoked. I guess this is
> expected
> > because the window is not actually key, since we are swizzling
makeKeyWindow: 
> > and isKeyWindow:.
> > 
> > Still I think this is a good change if we can't find any other way to track
> the 
> > "unfocusing" of a window because- 
> > 1) Since we are not swizzling any "unfocusing" methods currently, 
> > g_fake_focused_window is not completely correct. Hence if we do 
> >   widget->Show()
> >   widget->Hide()
> > g_fake_focused_window will still point to widget's window. Hence if we do a
> > Show() again, it will be ignored.
> > 
> > 2) [window close] also calls orderOut. 
> > 
> > 3) Current implementation can lead to subtle bugs. For example, if the
> currently
> > focused window is destroyed(say by invoking close), and then
> > ScopedFakeNSWindowFocus is
> > destroyed, it will cause an invalid memory reference, since
> > g_fake_focused_window
> > points to a destroyed window. Hence we have to be careful about destruction
> > sequence currently.
> > 
> > 4) This still calls the original orderOut implementation, so I don't see
this
> as
> > having any adverse effects.
> > 
> > Also, this will help generate windowDidResignKey notifications on
> widget->Hide()
> > which is not the case currently.
> 
> OK sounds good. That's lots of reasons :). For a test-helper, I guess it's OK
> that there may be limitations, and this is leading to fewer limitations. (E.g.
> [NSApp hide] probably doesn't call orderOut, but in a real app it should call
> resignKeyWindow)

Hehe yeah :) I realized that after writing this!

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/image_button.h (right):

https://codereview.chromium.org/1894383002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/image_button.h:21: 
On 2016/05/03 08:08:25, tapted wrote:
> On 2016/05/03 02:54:12, karandeepb wrote:
> > On 2016/04/20 06:05:58, tapted wrote:
> > > this line should either be removed or be just `//` by itself
> > 
> > Sorry it isn't clear to me which line you are talking about.
> 
> The comment "// An image button." is currently 'detached' from its class
because
> line 21 is blank. It should be formatted like
> 
> 
> // An image button.
> //
> // Note that....
> class .. ImageButton
> 
> or
> 
> // An image button.
> // Note that....
> class .. ImageButton

Done.

https://codereview.chromium.org/1894383002/diff/240001/ui/base/test/scoped_fa...
File ui/base/test/scoped_fake_full_keyboard_access.mm (right):

https://codereview.chromium.org/1894383002/diff/240001/ui/base/test/scoped_fa...
ui/base/test/scoped_fake_full_keyboard_access.mm:15: bool
g_full_keyboard_access_mode = true;
On 2016/05/03 08:08:25, tapted wrote:
> this can be stored as a data member on ScopedFakeFullKeyboardAccess now -
> usually best to minimise the number of globals

Done.

https://codereview.chromium.org/1894383002/diff/240001/ui/views/controls/butt...
File ui/views/controls/button/button.cc (right):

https://codereview.chromium.org/1894383002/diff/240001/ui/views/controls/butt...
ui/views/controls/button/button.cc:29: // On Mac, buttons are focusable only in
full keyboard access mode.
On 2016/05/03 08:08:25, tapted wrote:
> nit: move this into the #if (then clang-format will indent it - looks weird in
> the first column)

Done.

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

https://codereview.chromium.org/1894383002/diff/240001/ui/views/focus/focus_m...
ui/views/focus/focus_manager.h:343: // IsFocusable() if |keyboard_accessible_|
is false on Mac).
On 2016/05/03 08:08:25, tapted wrote:
> nit: (this reads a bit funny). Maybe
> 
> // Whether |view| is currently focusable per the platform's interpretation of
> |keyboard_accesible_|.

Done.

https://codereview.chromium.org/1894383002/diff/240001/ui/views/focus/focus_t...
File ui/views/focus/focus_traversal_unittest.cc (right):

https://codereview.chromium.org/1894383002/diff/240001/ui/views/focus/focus_t...
ui/views/focus/focus_traversal_unittest.cc:217: void
AdvanceEntireFocusLoop(Iterator begin, Iterator end, bool reverse) {
On 2016/05/03 08:08:25, tapted wrote:
> Hm it would be nice to avoid the template<> - I think it's the reversal
> requirement that means we can't just use int*. And since callers change what
> they do in between reversing (i.e. FocusManager::ClearFocus or
> View::RequestFocus), we can't just put that in here.
> 
> An improvement might be
> 
> template <size_t SIZE>
> void AdvanceEntireFocusLoop(const int (&traversal_ids)[SIZE], bool reverse) {
> ..
>   for (int i...3
>     for (int j = 0..N
>       EXPECT_EQ(traversal_id[reverse ? N - j - 1 : j], focused_view->id())
> }
> 
> 
> 
> Since we're not going in both directions, there should be a 
> 
> SCOPED_TRACE(testing::Message() << "In reverse: " << reverse);
> 
> at the start of the function too.
> 
> We could also pass in (possibly null) FocusManager* or View* arguments to do
> both directions, calling ClearFocus or ReqeustFocus appropriately, and drop
the
> `reverse` argument. That might become less readable though

Done.

https://codereview.chromium.org/1894383002/diff/240001/ui/views/focus/focus_t...
ui/views/focus/focus_traversal_unittest.cc:609: bool reverse = false;
On 2016/05/03 08:08:26, tapted wrote:
> I probably wouldn't worry about this extra variable - the
AdvanceEntireFocusLoop
> prototype is close enough to see what a true/false argument is doing

Done.

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

https://codereview.chromium.org/1894383002/diff/240001/ui/views/view.cc#newco...
ui/views/view.cc:1229: if (focus_manager && IsAccessibilityFocusable())
On 2016/05/03 08:08:26, tapted wrote:
> On 2016/05/03 02:54:13, karandeepb wrote:
> > Does this seem fine to you or should we do this only on Mac?
> 
> Isn't this needed to support the change to FocusManager::RestoreFocusedView()?
> Otherwise it would regress the changes in http://crrev.com/53471 . I think
this
> came up on the other CL --
>
https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...

Not really. 
This is needed on Mac, since if we don't do this, RequestFocus() would not work
on views like Combobox, for which focus behavior has been changed to
ACCESSIBLE_ONLY.

The changes to FocusManager::RestoreFocusedView() are caused by this change,.
not the other way round.

https://codereview.chromium.org/1894383002/diff/240001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1894383002/diff/240001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:1419: // Test that a widget's
associated focus manager is initialized with the correct
On 2016/05/03 08:08:26, tapted wrote:
> nit: widget -> Widget, focus manager -> FocusManager

Done.

[tapted, 2016-05-04 06:47:38]

lgtm

On 2016/05/04 01:56:38, karandeepb wrote:
> PTAL Trent. Can you also look at the IsFocusable() usages at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

this looks fine as is

> and
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/accessibi...

this one looks suspect. We should compare it with what the accessibility
inspector does on OSX for NSButtons when full keyboard access is/isn't enabled.
(If the accessibility inspector doesn't report differently when full keyboard
access is toggled, then that should probably be updated to
AccessibilityFocusable). But I think this is fine to explore in a follow-up.

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

https://codereview.chromium.org/1894383002/diff/240001/ui/views/view.cc#newco...
ui/views/view.cc:1229: if (focus_manager && IsAccessibilityFocusable())
On 2016/05/04 01:56:38, karandeepb wrote:
> On 2016/05/03 08:08:26, tapted wrote:
> > On 2016/05/03 02:54:13, karandeepb wrote:
> > > Does this seem fine to you or should we do this only on Mac?
> > 
> > Isn't this needed to support the change to
FocusManager::RestoreFocusedView()?
> > Otherwise it would regress the changes in http://crrev.com/53471 . I think
> this
> > came up on the other CL --
> >
>
https://codereview.chromium.org/1690543004/diff/240001/ui/views/focus/focus_m...
> 
> Not really. 
> This is needed on Mac, since if we don't do this, RequestFocus() would not
work
> on views like Combobox, for which focus behavior has been changed to
> ACCESSIBLE_ONLY.
> 
> The changes to FocusManager::RestoreFocusedView() are caused by this change,.
> not the other way round.

Acknowledged. I think this is fine to have on all platforms here.

https://codereview.chromium.org/1894383002/diff/300001/ui/views/focus/focus_t...
File ui/views/focus/focus_traversal_unittest.cc (right):

https://codereview.chromium.org/1894383002/diff/300001/ui/views/focus/focus_t...
ui/views/focus/focus_traversal_unittest.cc:7: #include <iterator>
nit: no longer needed. (maybe bring back stddef.h ? it's where size_t comes
from)

https://codereview.chromium.org/1894383002/diff/300001/ui/views/test/views_te...
File ui/views/test/views_test_helper_mac.h (right):

https://codereview.chromium.org/1894383002/diff/300001/ui/views/test/views_te...
ui/views/test/views_test_helper_mac.h:17: class ScopedFakeFullKeyboardAccess;
nit: sort

[karandeepb, 2016-05-04 07:13:03]

https://codereview.chromium.org/1894383002/diff/300001/ui/views/focus/focus_t...
File ui/views/focus/focus_traversal_unittest.cc (right):

https://codereview.chromium.org/1894383002/diff/300001/ui/views/focus/focus_t...
ui/views/focus/focus_traversal_unittest.cc:7: #include <iterator>
On 2016/05/04 06:47:38, tapted wrote:
> nit: no longer needed. (maybe bring back stddef.h ? it's where size_t comes
> from)

Done. stddef.h is still here. Had to move focus_manager.h to the bottom, since
it isn't recognized as a valid "related header" by PRESUBMIT.py.

https://codereview.chromium.org/1894383002/diff/300001/ui/views/test/views_te...
File ui/views/test/views_test_helper_mac.h (right):

https://codereview.chromium.org/1894383002/diff/300001/ui/views/test/views_te...
ui/views/test/views_test_helper_mac.h:17: class ScopedFakeFullKeyboardAccess;
On 2016/05/04 06:47:38, tapted wrote:
> nit: sort

Done.

[karandeepb, 2016-05-04 07:14:03]

On 2016/05/04 06:47:38, tapted wrote:
> lgtm
> 
> On 2016/05/04 01:56:38, karandeepb wrote:
> > PTAL Trent. Can you also look at the IsFocusable() usages at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> this looks fine as is
> 
> > and
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/accessibi...
> 
> this one looks suspect. We should compare it with what the accessibility
> inspector does on OSX for NSButtons when full keyboard access is/isn't
enabled.
> (If the accessibility inspector doesn't report differently when full keyboard
> access is toggled, then that should probably be updated to
> AccessibilityFocusable). But I think this is fine to explore in a follow-up.
> 

Thanks. Will file a bug to explore this in a follow-up.

[karandeepb, 2016-05-04 07:15:01]

PTAL sky@. Thanks.

[sky, 2016-05-04 17:30:26]

LGTM

[karandeepb, 2016-05-05 00:11:08]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2016-05-05 00:11:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1894383002/#ps340001
(title: "Rebased")

[commit-bot: I haz the power, 2016-05-05 00:11:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1894383002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1894383002/340001

[commit-bot: I haz the power, 2016-05-05 02:14:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-05 02:14:40]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[karandeepb, 2016-05-05 02:30:39]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2016-05-05 02:30:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1894383002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1894383002/340001

[commit-bot: I haz the power, 2016-05-05 03:28:56]

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. 

Other changes-
- Adds a static ConfigureDefaultFocus() method to 
  ui/views/controls/button/button.h
- Adds ScopedFakeFullKeyboardAccess to ViewsTestHelperMac 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().

For prior discussion related to this CL, see crrev.com/1690543004.

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. 

Other changes-
- Adds a static ConfigureDefaultFocus() method to 
  ui/views/controls/button/button.h
- Adds ScopedFakeFullKeyboardAccess to ViewsTestHelperMac 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().

For prior discussion related to this CL, see crrev.com/1690543004.

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

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

[commit-bot: I haz the power, 2016-05-05 03:28:58]

Committed patchset #8 (id:340001)

[commit-bot: I haz the power, 2016-05-05 03:30:27]

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. 

Other changes-
- Adds a static ConfigureDefaultFocus() method to 
  ui/views/controls/button/button.h
- Adds ScopedFakeFullKeyboardAccess to ViewsTestHelperMac 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().

For prior discussion related to this CL, see crrev.com/1690543004.

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.

Other changes-
- Adds a static ConfigureDefaultFocus() method to
  ui/views/controls/button/button.h
- Adds ScopedFakeFullKeyboardAccess to ViewsTestHelperMac 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().

For prior discussion related to this CL, see crrev.com/1690543004.

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

BUG=564912

Committed: https://crrev.com/24c156252c11425af51d646cd0d405f018c70bb8
Cr-Commit-Position: refs/heads/master@{#391744}
==========

[commit-bot: I haz the power, 2016-05-05 03:30:30]

Patchset 8 (id:??) landed as
https://crrev.com/24c156252c11425af51d646cd0d405f018c70bb8
Cr-Commit-Position: refs/heads/master@{#391744}

[Evan Stade, 2016-05-17 18:58:14]

estade@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2016-05-17 18:58:15]

https://codereview.chromium.org/1894383002/diff/340001/chrome/browser/ui/view...
File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
(right):

https://codereview.chromium.org/1894383002/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:112:
views::Button::ConfigureDefaultFocus(continue_signin_button_);
you don't need to do this after calling SetStyle. I think there are many places
that ConfigureDefaultFocus is now called somewhat redundantly (I'm removing this
one in https://codereview.chromium.org/1978403003/ )

[karandeepb, 2016-05-18 00:09:47]

https://codereview.chromium.org/1894383002/diff/340001/chrome/browser/ui/view...
File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
(right):

https://codereview.chromium.org/1894383002/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:112:
views::Button::ConfigureDefaultFocus(continue_signin_button_);
On 2016/05/17 18:58:15, Evan Stade wrote:
> you don't need to do this after calling SetStyle. I think there are many
places
> that ConfigureDefaultFocus is now called somewhat redundantly (I'm removing
this
> one in https://codereview.chromium.org/1978403003/ )

I had removed this in https://codereview.chromium.org/1977353002/. I only found
2 such places when I had last checked. Any reason for saying
ConfigureDefaultFocus is being called redundantly at several places?

[Evan Stade, 2016-05-18 00:16:32]

https://codereview.chromium.org/1894383002/diff/340001/chrome/browser/ui/view...
File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
(right):

https://codereview.chromium.org/1894383002/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:112:
views::Button::ConfigureDefaultFocus(continue_signin_button_);
On 2016/05/18 00:09:46, karandeepb wrote:
> On 2016/05/17 18:58:15, Evan Stade wrote:
> > you don't need to do this after calling SetStyle. I think there are many
> places
> > that ConfigureDefaultFocus is now called somewhat redundantly (I'm removing
> this
> > one in https://codereview.chromium.org/1978403003/ )
> 
> I had removed this in https://codereview.chromium.org/1977353002/. I only
found
> 2 such places when I had last checked. Any reason for saying
> ConfigureDefaultFocus is being called redundantly at several places? 

Ah, I see that you're already on it. Great. I just ran into two places and
assumed there would be more. (The other being DialogClientView.) 

FWIW, the name is a little confusing to me. The comment above the function is
"Make the |button| focusable as per the platform." which is clear, so why not
make the fn "MakeFocusable" or similar?

[karandeepb, 2016-05-18 00:22:26]

On 2016/05/18 00:16:32, Evan Stade wrote:
>
https://codereview.chromium.org/1894383002/diff/340001/chrome/browser/ui/view...
> File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
> (right):
> 
>
https://codereview.chromium.org/1894383002/diff/340001/chrome/browser/ui/view...
> chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:112:
> views::Button::ConfigureDefaultFocus(continue_signin_button_);
> On 2016/05/18 00:09:46, karandeepb wrote:
> > On 2016/05/17 18:58:15, Evan Stade wrote:
> > > you don't need to do this after calling SetStyle. I think there are many
> > places
> > > that ConfigureDefaultFocus is now called somewhat redundantly (I'm
removing
> > this
> > > one in https://codereview.chromium.org/1978403003/ )
> > 
> > I had removed this in https://codereview.chromium.org/1977353002/. I only
> found
> > 2 such places when I had last checked. Any reason for saying
> > ConfigureDefaultFocus is being called redundantly at several places? 
> 
> Ah, I see that you're already on it. Great. I just ran into two places and
> assumed there would be more. (The other being DialogClientView.) 
> 
> FWIW, the name is a little confusing to me. The comment above the function is
> "Make the |button| focusable as per the platform." which is clear, so why not
> make the fn "MakeFocusable" or similar?

Yeah others had also commented about the name not being that clear. I'll rename
it to SetPlatformFocusable in a subsequent CL.