Views: Flip default value of CustomButton::request_focus_on_press_ to false.

Views: Flip default value of CustomButton::request_focus_on_press_ to false.

CustomButton::request_focus_on_press_ has a default value of true currently.
Since View::RequestFocus uses IsFocusable() when keyboard accessibility is off
(i.e. on Non-Mac platforms), hence only buttons with FocusBehavior::ALWAYS can
actually gain focus if they have request_focus_on_press_ set to true. Most
CustomButton subclasses like say ToolbarButton or TabCloseButton, which have
FocusBehavior::ACCESSIBLE_ONLY, rely on this behavior and do not explicitly
override request_focus_on_press_ to false.

This CL corrects the request_focus_on_press_ value on the CustomButton
subclasses by flipping the default value to false. Since only buttons with
request_focus_on_press_ set to true and having FocusBehavior::ALWAYS are
intended to gain focus on a mouse press, all ConfigureDefaultFocus call sites
are examined and set_request_focus_on_press is set to the appropriate value.

BUG=NONE

Committed: https://crrev.com/2380985ecba0f27f2a6c7406121c76dd59ed711b
Cr-Commit-Position: refs/heads/master@{#394020}

[karandeepb, 2016-05-09 10:43:48]

Description was changed from

==========
---


---


---

BUG=
==========

to

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to true.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). On a mouse click on a CustomButton, 
CustomButton::MousePressed() requests focus on the button, if it has 
request_focus_on_press_ set to true. It turns out that most button subclasses, 
did not explicitly set request_focus_on_press_ to false, which has a default 
value of true. These custom buttons which are accessibility focusable, can now 
gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate.

BUG=609701,610186,564912
==========

[karandeepb, 2016-05-09 10:45:55]

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

[karandeepb, 2016-05-09 10:45:56]

Trent, can you PTAL before I send it for owner's review. Thanks.

[karandeepb, 2016-05-10 01:37:22]

Description was changed from

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to true.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). On a mouse click on a CustomButton, 
CustomButton::MousePressed() requests focus on the button, if it has 
request_focus_on_press_ set to true. It turns out that most button subclasses, 
did not explicitly set request_focus_on_press_ to false, which has a default 
value of true. These custom buttons which are accessibility focusable, can now 
gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate.

BUG=609701,610186,564912
==========

to

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to true.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). On a mouse click on a CustomButton, 
CustomButton::MousePressed() requests focus on the button, if it has 
request_focus_on_press_ set to true. It turns out that most button subclasses, 
did not explicitly set request_focus_on_press_ to false, which has a default 
value of true. These custom buttons which are accessibility focusable, can now 
gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate.

BUG=609701,610186,610235,564912
==========

[karandeepb, 2016-05-10 02:35:52]

Description was changed from

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to true.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). On a mouse click on a CustomButton, 
CustomButton::MousePressed() requests focus on the button, if it has 
request_focus_on_press_ set to true. It turns out that most button subclasses, 
did not explicitly set request_focus_on_press_ to false, which has a default 
value of true. These custom buttons which are accessibility focusable, can now 
gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate.

BUG=609701,610186,610235,564912
==========

to

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). On a mouse click on a CustomButton, 
CustomButton::MousePressed() requests focus on the button, if it has 
request_focus_on_press_ set to true. It turns out that most button subclasses, 
did not explicitly set request_focus_on_press_ to false, which has a default 
value of true. These custom buttons which are accessibility focusable, can now 
gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate.

BUG=609701,610186,610235,564912
==========

[tapted, 2016-05-10 02:57:20]

lgtm - seems reasonable. Perhaps include a comment in the CL description for the
rationale behind flipping the default versus updating the [X] places that should
call request_focus_on_press(true) and don't currently

[karandeepb, 2016-05-10 03:35:07]

Description was changed from

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). On a mouse click on a CustomButton, 
CustomButton::MousePressed() requests focus on the button, if it has 
request_focus_on_press_ set to true. It turns out that most button subclasses, 
did not explicitly set request_focus_on_press_ to false, which has a default 
value of true. These custom buttons which are accessibility focusable, can now 
gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate.

BUG=609701,610186,610235,564912
==========

to

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). 

On a mouse click on a CustomButton, CustomButton::MousePressed() requests focus 
on the button, if it has request_focus_on_press_ set to true. It turns out that 
most button subclasses, do not explicitly set request_focus_on_press_ to false,
which has a default value of true. These custom buttons which are accessibility
focusable, can now gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate. 

This approach is better as compared to not flipping the default, since it would 
be quite difficult to identify all the CustomButton subclasses which need 
set_request_focus_on_press(false). 

BUG=609701, 610186, 610235, 564912
==========

[karandeepb, 2016-05-10 05:10:53]

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

[karandeepb, 2016-05-10 05:10:53]

PTAL sky@. Thanks.

[karandeepb, 2016-05-11 00:30:41]

Description was changed from

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). 

On a mouse click on a CustomButton, CustomButton::MousePressed() requests focus 
on the button, if it has request_focus_on_press_ set to true. It turns out that 
most button subclasses, do not explicitly set request_focus_on_press_ to false,
which has a default value of true. These custom buttons which are accessibility
focusable, can now gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate. 

This approach is better as compared to not flipping the default, since it would 
be quite difficult to identify all the CustomButton subclasses which need 
set_request_focus_on_press(false). 

BUG=609701, 610186, 610235, 564912
==========

to

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). 

On a mouse click on a CustomButton, CustomButton::MousePressed() requests focus 
on the button, if it has request_focus_on_press_ set to true. It turns out that 
most button subclasses, do not explicitly set request_focus_on_press_ to false,
which has a default value of true. These custom buttons which are accessibility
focusable, can now gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate. 

This approach is better as compared to not flipping the default, since it would 
be quite difficult to identify all the CustomButton subclasses which need 
set_request_focus_on_press(false). 

BUG=609701, 610186, 610235, 610740, 564912
==========

[karandeepb, 2016-05-11 00:50:46]

Description was changed from

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). 

On a mouse click on a CustomButton, CustomButton::MousePressed() requests focus 
on the button, if it has request_focus_on_press_ set to true. It turns out that 
most button subclasses, do not explicitly set request_focus_on_press_ to false,
which has a default value of true. These custom buttons which are accessibility
focusable, can now gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate. 

This approach is better as compared to not flipping the default, since it would 
be quite difficult to identify all the CustomButton subclasses which need 
set_request_focus_on_press(false). 

BUG=609701, 610186, 610235, 610740, 564912
==========

to

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). 

On a mouse click on a CustomButton, CustomButton::MousePressed() requests focus 
on the button, if it has request_focus_on_press_ set to true. It turns out that 
most button subclasses, do not explicitly set request_focus_on_press_ to false,
which has a default value of true. These custom buttons which are accessibility
focusable, can now gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate. 

This approach is better as compared to not flipping the default, since it would 
be quite difficult to identify all the CustomButton subclasses which need 
set_request_focus_on_press(false). 

BUG=609701, 610186, 610235, 610740, 610802, 564912
==========

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

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

[karandeepb, 2016-05-11 01:00:17]

karandeepb@chromium.org changed reviewers:
+ msw@chromium.org, oshima@chromium.org, sadrul@chromium.org

[karandeepb, 2016-05-11 01:00:18]

+sadrul@ for ui/ review.
+oshima@ for ash/ and chrome/browser/chromeos review.
+msw@ for chrome/broswer/ui/views review.

Let me know if you need more context.

[karandeepb, 2016-05-11 01:01:09]

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

[karandeepb, 2016-05-11 02:43:14]

Description was changed from

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). 

On a mouse click on a CustomButton, CustomButton::MousePressed() requests focus 
on the button, if it has request_focus_on_press_ set to true. It turns out that 
most button subclasses, do not explicitly set request_focus_on_press_ to false,
which has a default value of true. These custom buttons which are accessibility
focusable, can now gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate. 

This approach is better as compared to not flipping the default, since it would 
be quite difficult to identify all the CustomButton subclasses which need 
set_request_focus_on_press(false). 

BUG=609701, 610186, 610235, 610740, 610802, 564912
==========

to

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). 

On a mouse click on a CustomButton, CustomButton::MousePressed() requests focus 
on the button, if it has request_focus_on_press_ set to true. It turns out that 
most button subclasses, do not explicitly set request_focus_on_press_ to false,
which has a default value of true. These custom buttons which are accessibility
focusable, can now gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate. 

This approach is better as compared to not flipping the default, since it would 
be quite difficult to identify all the CustomButton subclasses which need 
set_request_focus_on_press(false). 

BUG=609701, 610186, 610235, 610740, 610802, 610664, 564912
==========

[oshima, 2016-05-11 14:58:28]

ash lgtm

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

https://codereview.chromium.org/1963563002/diff/40001/ash/shell/lock_view.cc#...
ash/shell/lock_view.cc:33: views::Button::ConfigureDefaultFocus(unlock_button_);
It wasn't obvious to me that this will enable focus on button.
Consider name like

Button::SetPlatformFocusable(unlock_button_);
Button::EnablePlatformFocus(unlock_button_);

[msw, 2016-05-11 19:08:47]

Generally the change lgtm, but I agree with oshima's renaming suggestions. Also,
should Button::ConfigureDefaultFocus() automatically
set_request_focus_on_press(true)? That seems to be the common case. Then, very
few places would need to explicitly call set_request_focus_on_press(false)
afterwards, perhaps with comments justifying that behavior.

[karandeepb, 2016-05-12 00:13:18]

Patchset #4 (id:60001) has been deleted

[karandeepb, 2016-05-12 00:43:00]

On 2016/05/11 19:08:47, msw wrote:
> Generally the change lgtm, but I agree with oshima's renaming suggestions.
Also,
> should Button::ConfigureDefaultFocus() automatically
> set_request_focus_on_press(true)? That seems to be the common case. Then, very
> few places would need to explicitly call set_request_focus_on_press(false)
> afterwards, perhaps with comments justifying that behavior.

Will rename ConfigureDefaultFocus() to SetPlatformFocusable() in a subsequent
CL. 

I also think that ConfigureDefaultFocus() calling
set_request_focus_on_press(true) is a good idea but currently
ConfigureDefaultFocus() is defined on the Button class while
set_request_focus_on_press() is defined on CustomButton. CustomButton is the
only subclass of Button which is not used in tests. Hence it'll probably make
sense to move ConfigureDefaultFocus() from Button to CustomButton while
renaming.

[karandeepb, 2016-05-12 00:45:28]

ping sadrul@.

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

https://codereview.chromium.org/1963563002/diff/40001/ash/shell/lock_view.cc#...
ash/shell/lock_view.cc:33: views::Button::ConfigureDefaultFocus(unlock_button_);
On 2016/05/11 14:58:27, oshima wrote:
> It wasn't obvious to me that this will enable focus on button.
> Consider name like
> 
> Button::SetPlatformFocusable(unlock_button_);
> Button::EnablePlatformFocus(unlock_button_);
> 
> 
> 
Will rename this in a subsequent CL. Thanks.

[karandeepb, 2016-05-13 09:09:16]

I am exploring a simpler patch at https://codereview.chromium.org/1973073003/.
Nevertheless, I think this also improves the code since request_focus_on_press_
is currently incorrectly set on many CustomButton subclasses.

[karandeepb, 2016-05-16 04:42:53]

Description was changed from

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in 
https://codereview.chromium.org/1894383002/. This regression is caused due to 
the change in View::RequestFocus() from IsFocusable() to 
IsAccessibilityFocusable(). 

On a mouse click on a CustomButton, CustomButton::MousePressed() requests focus 
on the button, if it has request_focus_on_press_ set to true. It turns out that 
most button subclasses, do not explicitly set request_focus_on_press_ to false,
which has a default value of true. These custom buttons which are accessibility
focusable, can now gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since 
earlier only buttons with FocusBehavior::ALWAYS could gain focus from 
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and 
set_request_focus_on_press is set to true, wherever appropriate. 

This approach is better as compared to not flipping the default, since it would 
be quite difficult to identify all the CustomButton subclasses which need 
set_request_focus_on_press(false). 

BUG=609701, 610186, 610235, 610740, 610802, 610664, 564912
==========

to

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in
https://codereview.chromium.org/1894383002/. This regression is caused due to
the change in View::RequestFocus() from IsFocusable() to
IsAccessibilityFocusable().

On a mouse click on a CustomButton, CustomButton::MousePressed() requests focus
on the button, if it has request_focus_on_press_ set to true. It turns out that
most button subclasses, do not explicitly set request_focus_on_press_ to false,
which has a default value of true. These custom buttons which are accessibility
focusable, can now gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since
earlier only buttons with FocusBehavior::ALWAYS could gain focus from
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and
set_request_focus_on_press is set to true, wherever appropriate.

This approach is better as compared to not flipping the default, since it would
be quite difficult to identify all the CustomButton subclasses which need
set_request_focus_on_press(false).

BUG=609701, 610186, 610235, 610740, 610802, 610664, 564912
==========

[karandeepb, 2016-05-16 04:46:43]

Description was changed from

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

This CL fixes regression crbug.com/609701 introduced in
https://codereview.chromium.org/1894383002/. This regression is caused due to
the change in View::RequestFocus() from IsFocusable() to
IsAccessibilityFocusable().

On a mouse click on a CustomButton, CustomButton::MousePressed() requests focus
on the button, if it has request_focus_on_press_ set to true. It turns out that
most button subclasses, do not explicitly set request_focus_on_press_ to false,
which has a default value of true. These custom buttons which are accessibility
focusable, can now gain focus on a mouse press, hence the bug.

This CL flips the default value of request_focus_on_press_ to false. Since
earlier only buttons with FocusBehavior::ALWAYS could gain focus from
View::RequestFocus, all ConfigureDefaultFocus call sites are examined, and
set_request_focus_on_press is set to true, wherever appropriate.

This approach is better as compared to not flipping the default, since it would
be quite difficult to identify all the CustomButton subclasses which need
set_request_focus_on_press(false).

BUG=609701, 610186, 610235, 610740, 610802, 610664, 564912
==========

to

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

CustomButton::request_focus_on_press_ has a default value of true currently.
Since View::RequestFocus uses IsFocusable() when keyboard accessibility is off
(i.e. on Non-Mac platforms), hence only buttons with FocusBehavior::ALWAYS can
actually gain focus if they have request_focus_on_press_ set to true. Most
CustomButton subclasses like say ToolbarButton or TabCloseButton, which have
FocusBehavior::ACCESSIBLE_ONLY, rely on this behavior and do not explicitly
override request_focus_on_press_ to false.

This CL corrects the request_focus_on_press_ value on the CustomButton
subclasses by flipping the default value to false. Since only buttons with
request_focus_on_press_ set to true and having FocusBehavior::ALWAYS are
intended to gain focus on a mouse press, all ConfigureDefaultFocus call sites
are examined and set_request_focus_on_press is set to the appropriate value.

BUG=NONE
==========

[karandeepb, 2016-05-16 05:43:30]

-

[karandeepb, 2016-05-16 05:44:34]

=PTAL

[karandeepb, 2016-05-16 05:51:34]

=PTAL sky@ for ui/ review.

[sky, 2016-05-16 17:54:33]

I can't for the life of me LGTM this on rietveld today. Hopefully the
email LGTM is enough.

On Sun, May 15, 2016 at 10:51 PM, karandeepb@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
wrote:
> =PTAL sky@ for ui/ review.
>
> https://codereview.chromium.org/1963563002/

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

[maxbogue, 2016-05-16 18:52:31]

On 2016/05/16 17:54:33, sky wrote:
> I can't for the life of me LGTM this on rietveld today. Hopefully the
> email LGTM is enough.

That issue should be resolved now (side effect of http://crbug.com/601521).

[sky, 2016-05-16 19:07:43]

LGTM

[sky, 2016-05-16 19:07:56]

Yay, it works!

On Mon, May 16, 2016 at 11:52 AM,  <maxbogue@chromium.org> wrote:
> On 2016/05/16 17:54:33, sky wrote:
>> I can't for the life of me LGTM this on rietveld today. Hopefully the
>> email LGTM is enough.
>
> That issue should be resolved now (side effect of http://crbug.com/601521).
>
> https://codereview.chromium.org/1963563002/

-- 
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-05-17 00:49:03]

On 2016/05/16 19:07:56, sky wrote:
> Yay, it works!
> 
> On Mon, May 16, 2016 at 11:52 AM,  <mailto:maxbogue@chromium.org> wrote:
> > On 2016/05/16 17:54:33, sky wrote:
> >> I can't for the life of me LGTM this on rietveld today. Hopefully the
> >> email LGTM is enough.
> >
> > That issue should be resolved now (side effect of http://crbug.com/601521).
> >
> > https://codereview.chromium.org/1963563002/
> 
> -- 
> 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.
> 

Yeah, I also had to use the command line to post the replies, hence the
redundant replies from me!

[karandeepb, 2016-05-17 00:49:20]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2016-05-17 00:49:21]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
oshima@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1963563002/#ps80001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-05-17 00:49:43]

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

[commit-bot: I haz the power, 2016-05-17 02:19:49]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-05-17 02:21:10]

Description was changed from

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

CustomButton::request_focus_on_press_ has a default value of true currently.
Since View::RequestFocus uses IsFocusable() when keyboard accessibility is off
(i.e. on Non-Mac platforms), hence only buttons with FocusBehavior::ALWAYS can
actually gain focus if they have request_focus_on_press_ set to true. Most
CustomButton subclasses like say ToolbarButton or TabCloseButton, which have
FocusBehavior::ACCESSIBLE_ONLY, rely on this behavior and do not explicitly
override request_focus_on_press_ to false.

This CL corrects the request_focus_on_press_ value on the CustomButton
subclasses by flipping the default value to false. Since only buttons with
request_focus_on_press_ set to true and having FocusBehavior::ALWAYS are
intended to gain focus on a mouse press, all ConfigureDefaultFocus call sites
are examined and set_request_focus_on_press is set to the appropriate value.

BUG=NONE
==========

to

==========
Views: Flip default value of CustomButton::request_focus_on_press_ to false.

CustomButton::request_focus_on_press_ has a default value of true currently.
Since View::RequestFocus uses IsFocusable() when keyboard accessibility is off
(i.e. on Non-Mac platforms), hence only buttons with FocusBehavior::ALWAYS can
actually gain focus if they have request_focus_on_press_ set to true. Most
CustomButton subclasses like say ToolbarButton or TabCloseButton, which have
FocusBehavior::ACCESSIBLE_ONLY, rely on this behavior and do not explicitly
override request_focus_on_press_ to false.

This CL corrects the request_focus_on_press_ value on the CustomButton
subclasses by flipping the default value to false. Since only buttons with
request_focus_on_press_ set to true and having FocusBehavior::ALWAYS are
intended to gain focus on a mouse press, all ConfigureDefaultFocus call sites
are examined and set_request_focus_on_press is set to the appropriate value.

BUG=NONE

Committed: https://crrev.com/2380985ecba0f27f2a6c7406121c76dd59ed711b
Cr-Commit-Position: refs/heads/master@{#394020}
==========

[commit-bot: I haz the power, 2016-05-17 02:21:12]

Patchset 4 (id:??) landed as
https://crrev.com/2380985ecba0f27f2a6c7406121c76dd59ed711b
Cr-Commit-Position: refs/heads/master@{#394020}