Indicate focused state on text buttons with blue outline.

Indicate focused state on text buttons with blue outline.

BUG=118040
TEST=Visit http://www.pagetutor.com/keeper/mystash/secretstuff.html and focus the native views text buttons. They should get a blue outline to indicate focus. Examine native web buttons which should be unaffected by this patch.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140711

[flackr, 2012-06-04 18:55:38]

PTAL, thanks!

[tony, 2012-06-04 19:20:44]

I think xiyuan was the original author of this code.

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/base/native_theme/n...
File ui/base/native_theme/native_theme_base.cc (right):

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/base/native_theme/n...
ui/base/native_theme/native_theme_base.cc:490: State state,
Can you fix the indenting while you're here?

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/base/native_theme/n...
ui/base/native_theme/native_theme_base.cc:535: state == kHovered ? 0x80 : 0x55;
Nit: Nested ternary operators are hard to read.  Can we use an 'if' for the
is_focused check?  You could merge it with the if below.

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/views/controls/butt...
File ui/views/controls/button/text_button.cc (left):

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/views/controls/butt...
ui/views/controls/button/text_button.cc:759: if (HasFocus() && (focusable() ||
IsAccessibilityFocusable())) {
Is it OK to lose the focusable() || IsAccessibilityFocusable() checks?

https://chromiumcodereview.appspot.com/10513009/diff/1/webkit/glue/webthemeen...
File webkit/glue/webthemeengine_impl_android.cc (right):

https://chromiumcodereview.appspot.com/10513009/diff/1/webkit/glue/webthemeen...
webkit/glue/webthemeengine_impl_android.cc:112: false;
Nit: Can you put |false| on the same line as the assignment?

https://chromiumcodereview.appspot.com/10513009/diff/1/webkit/glue/webthemeen...
File webkit/glue/webthemeengine_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/10513009/diff/1/webkit/glue/webthemeen...
webkit/glue/webthemeengine_impl_linux.cc:109: false;
Nit: false on previous line.

[xiyuan, 2012-06-04 19:46:53]

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/views/controls/butt...
File ui/views/controls/button/text_button.cc (left):

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/views/controls/butt...
ui/views/controls/button/text_button.cc:759: if (HasFocus() && (focusable() ||
IsAccessibilityFocusable())) {
On 2012/06/04 19:20:45, tony wrote:
> Is it OK to lose the focusable() || IsAccessibilityFocusable() checks?

I have the same question.
Should we do "params->button.is_focused = HasFocus() && (focusable() ||
IsAccessibilityFocusable())" in TextButtonBase::GetExtraParams as in here?

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/views/controls/butt...
ui/views/controls/button/text_button.cc:846: #if defined(OS_WIN)
Would this break Windows?

[flackr, 2012-06-05 14:13:18]

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/base/native_theme/n...
File ui/base/native_theme/native_theme_base.cc (right):

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/base/native_theme/n...
ui/base/native_theme/native_theme_base.cc:490: State state,
On 2012/06/04 19:20:45, tony wrote:
> Can you fix the indenting while you're here?

Done.

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/base/native_theme/n...
ui/base/native_theme/native_theme_base.cc:535: state == kHovered ? 0x80 : 0x55;
On 2012/06/04 19:20:45, tony wrote:
> Nit: Nested ternary operators are hard to read.  Can we use an 'if' for the
> is_focused check?  You could merge it with the if below.

Done.

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/views/controls/butt...
File ui/views/controls/button/text_button.cc (left):

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/views/controls/butt...
ui/views/controls/button/text_button.cc:759: if (HasFocus() && (focusable() ||
IsAccessibilityFocusable())) {
On 2012/06/04 19:46:54, xiyuan wrote:
> On 2012/06/04 19:20:45, tony wrote:
> > Is it OK to lose the focusable() || IsAccessibilityFocusable() checks?
> 
> I have the same question.
> Should we do "params->button.is_focused = HasFocus() && (focusable() ||
> IsAccessibilityFocusable())" in TextButtonBase::GetExtraParams as in here?

You're absolutely right, I originally had this and it got lost somewhere. Added
these. Thanks

https://chromiumcodereview.appspot.com/10513009/diff/1/ui/views/controls/butt...
ui/views/controls/button/text_button.cc:846: #if defined(OS_WIN)
On 2012/06/04 19:46:54, xiyuan wrote:
> Would this break Windows?

I tried in Windows (7) and it draws a similar looking blue colored focus border
drawn by NativeThemeWin::PaintButton I believe. Nevertheless to be safe I'll
keep the existing functionality for windows and the base TextButton (since this
doesn't use NativeTheme I believe), and only override NativeTextButton's
behaviour.

https://chromiumcodereview.appspot.com/10513009/diff/1/webkit/glue/webthemeen...
File webkit/glue/webthemeengine_impl_android.cc (right):

https://chromiumcodereview.appspot.com/10513009/diff/1/webkit/glue/webthemeen...
webkit/glue/webthemeengine_impl_android.cc:112: false;
On 2012/06/04 19:20:45, tony wrote:
> Nit: Can you put |false| on the same line as the assignment?

Done.

https://chromiumcodereview.appspot.com/10513009/diff/1/webkit/glue/webthemeen...
File webkit/glue/webthemeengine_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/10513009/diff/1/webkit/glue/webthemeen...
webkit/glue/webthemeengine_impl_linux.cc:109: false;
On 2012/06/04 19:20:45, tony wrote:
> Nit: false on previous line.

Done.

[xiyuan, 2012-06-05 17:12:53]

LGTM with one nit.

http://codereview.chromium.org/10513009/diff/8001/ui/base/native_theme/native...
File ui/base/native_theme/native_theme_base.cc (right):

http://codereview.chromium.org/10513009/diff/8001/ui/base/native_theme/native...
ui/base/native_theme/native_theme_base.cc:536: kBorderAlpha = 0xff;
nit: Change kBorderAlpha to border_alpha since this is no longer a const.

[tony, 2012-06-05 17:15:44]

LGTM2

[flackr, 2012-06-05 18:20:03]

http://codereview.chromium.org/10513009/diff/8001/ui/base/native_theme/native...
File ui/base/native_theme/native_theme_base.cc (right):

http://codereview.chromium.org/10513009/diff/8001/ui/base/native_theme/native...
ui/base/native_theme/native_theme_base.cc:536: kBorderAlpha = 0xff;
On 2012/06/05 17:12:53, xiyuan wrote:
> nit: Change kBorderAlpha to border_alpha since this is no longer a const.

Done.

[sky, 2012-06-05 19:48:57]

http://codereview.chromium.org/10513009/diff/15002/ui/views/controls/button/t...
File ui/views/controls/button/text_button.cc (right):

http://codereview.chromium.org/10513009/diff/15002/ui/views/controls/button/t...
ui/views/controls/button/text_button.cc:863: params->button.is_focused =
HasFocus() &&
Does this result in a double indicator on windows?

[flackr, 2012-06-05 20:11:47]

http://codereview.chromium.org/10513009/diff/15002/ui/views/controls/button/t...
File ui/views/controls/button/text_button.cc (right):

http://codereview.chromium.org/10513009/diff/15002/ui/views/controls/button/t...
ui/views/controls/button/text_button.cc:863: params->button.is_focused =
HasFocus() &&
On 2012/06/05 19:48:57, sky wrote:
> Does this result in a double indicator on windows?

In my own build Windows had it's own style (from NativeThemeWin::PaintButton)
and didn't even use this class's OnPaintFocusBorder. In case this is used on
some versions of windows though I have added an if !defined to only set it on
non-windows.

[sky, 2012-06-05 20:25:36]

LGTM

http://codereview.chromium.org/10513009/diff/19002/ui/views/controls/button/t...
File ui/views/controls/button/text_button.cc (right):

http://codereview.chromium.org/10513009/diff/19002/ui/views/controls/button/t...
ui/views/controls/button/text_button.cc:864: params->button.is_focused =
HasFocus() &&
Add a comment as to why this is ifdef'd.

[commit-bot: I haz the power, 2012-06-06 03:52:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/10513009/20002

[commit-bot: I haz the power, 2012-06-06 05:29:45]

Change committed as 140711