Change focus indicator for TableView and TreeView.

Change focus indicator for TableView and TreeView.

This affects surfaces such as the task manager, the bookmark bubble
editor, and the cookies view.

The indicator for selection (the background of the row) is unchanged.
When the window is focused, instead of drawing a dashed focus rect
which looks dated and especially bad at fractional scales, we draw the
table/tree's border in blue. This matches textfields and the general
trend of blue outlines/rings for focus indication.

This will change in Harmony to be a focus ring so it's mainly just an
interim solution that makes the current state of the world look less
bad while we wait for Harmony to arrive.

BUG=695540
Review-Url: https://codereview.chromium.org/2715813002
Cr-Commit-Position: refs/heads/master@{#452876}
Committed: https://chromium.googlesource.com/chromium/src/+/f084da03407bcb848d8ef8f232c28db56883d648

[Evan Stade, 2017-02-23 21:00:10]

estade@chromium.org changed reviewers:
+ ellyjones@chromium.org, sky@chromium.org, tapted@chromium.org

[Evan Stade, 2017-02-23 21:00:11]

+tapted, ellyjones for Harmony angle.
+sky for Views/OWNER.

bettes@ gave the OK for this change.

https://codereview.chromium.org/2715813002/diff/1/ui/views/style/platform_sty...
File ui/views/style/platform_style.h (left):

https://codereview.chromium.org/2715813002/diff/1/ui/views/style/platform_sty...
ui/views/style/platform_style.h:60: static const bool kTreeViewHasFocusRing;
not sure why this was limited to Mac. Trent? Elly?

[tapted, 2017-02-23 22:55:00]

https://codereview.chromium.org/2715813002/diff/1/ui/views/style/platform_sty...
File ui/views/style/platform_style.h (left):

https://codereview.chromium.org/2715813002/diff/1/ui/views/style/platform_sty...
ui/views/style/platform_style.h:60: static const bool kTreeViewHasFocusRing;
On 2017/02/23 21:00:11, Evan Stade wrote:
> not sure why this was limited to Mac. Trent? Elly?

Just so that other platforms didn't "regress". A TreeView on Mac has a fancy
focus ring around the whole TreeView whereas Views at the time only showed focus
on the selected node.

(Also note that on Mac a *table* view does not have a focus ring around it, but
we're not planning to roll out a toolkit-views TaskManager in the first phase of
Harmony .. although I think the device picker(s) use(s) them..)

[sky, 2017-02-24 00:28:05]

LGTM

[Evan Stade, 2017-02-24 07:31:37]

On 2017/02/23 22:55:00, tapted wrote:
>
https://codereview.chromium.org/2715813002/diff/1/ui/views/style/platform_sty...
> File ui/views/style/platform_style.h (left):
> 
>
https://codereview.chromium.org/2715813002/diff/1/ui/views/style/platform_sty...
> ui/views/style/platform_style.h:60: static const bool kTreeViewHasFocusRing;
> On 2017/02/23 21:00:11, Evan Stade wrote:
> > not sure why this was limited to Mac. Trent? Elly?
> 
> Just so that other platforms didn't "regress". A TreeView on Mac has a fancy
> focus ring around the whole TreeView whereas Views at the time only showed
focus
> on the selected node.

But that change to trees is a Harmony change. I don't understand why it would be
gated on Harmony *and* platform. Other platforms wouldn't "regress" because
Harmony is not default on any platform.

> 
> (Also note that on Mac a *table* view does not have a focus ring around it,

well, that changes with this CL, at least for MacViews. Is that a problem? Alan
approved the idea and the screenshots but maybe there's a reason we're missing
not to do the same thing to tables as trees?

[tapted, 2017-02-24 11:18:59]

On 2017/02/24 07:31:37, Evan Stade wrote:
> On 2017/02/23 22:55:00, tapted wrote:
> >
>
https://codereview.chromium.org/2715813002/diff/1/ui/views/style/platform_sty...
> > File ui/views/style/platform_style.h (left):
> > 
> >
>
https://codereview.chromium.org/2715813002/diff/1/ui/views/style/platform_sty...
> > ui/views/style/platform_style.h:60: static const bool kTreeViewHasFocusRing;
> > On 2017/02/23 21:00:11, Evan Stade wrote:
> > > not sure why this was limited to Mac. Trent? Elly?
> > 
> > Just so that other platforms didn't "regress". A TreeView on Mac has a fancy
> > focus ring around the whole TreeView whereas Views at the time only showed
> focus
> > on the selected node.
> 
> But that change to trees is a Harmony change. I don't understand why it would
be
> gated on Harmony *and* platform. Other platforms wouldn't "regress" because
> Harmony is not default on any platform.

perhaps just caution then? It may not have been intentional (I wasn't involved
in this particular case) - git blame may have the answer. There was a time
before Harmony where there was just MacViews and the bugs were mac-only.

> > (Also note that on Mac a *table* view does not have a focus ring around it,
> 
> well, that changes with this CL, at least for MacViews. Is that a problem?
Alan
> approved the idea and the screenshots but maybe there's a reason we're missing
> not to do the same thing to tables as trees?

It might be worth confirming with Alan. I haven't seen the screenshots so don't
really have an opinion :). I don't think we have Harmony mocks for
tableview/treeview. Someone familiar with Cocoa might think a focus ring around
a table view looks weird, but can always remove it later.

[Elly Fong-Jones, 2017-02-24 14:41:36]

On 2017/02/24 11:18:59, tapted wrote:
> On 2017/02/24 07:31:37, Evan Stade wrote:
> > On 2017/02/23 22:55:00, tapted wrote:
> > >
> >
>
https://codereview.chromium.org/2715813002/diff/1/ui/views/style/platform_sty...
> > > File ui/views/style/platform_style.h (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2715813002/diff/1/ui/views/style/platform_sty...
> > > ui/views/style/platform_style.h:60: static const bool
kTreeViewHasFocusRing;
> > > On 2017/02/23 21:00:11, Evan Stade wrote:
> > > > not sure why this was limited to Mac. Trent? Elly?
> > > 
> > > Just so that other platforms didn't "regress". A TreeView on Mac has a
fancy
> > > focus ring around the whole TreeView whereas Views at the time only showed
> > focus
> > > on the selected node.
> > 
> > But that change to trees is a Harmony change. I don't understand why it
would
> be
> > gated on Harmony *and* platform. Other platforms wouldn't "regress" because
> > Harmony is not default on any platform.
> 
> perhaps just caution then? It may not have been intentional (I wasn't involved
> in this particular case) - git blame may have the answer. There was a time
> before Harmony where there was just MacViews and the bugs were mac-only.

I think this one was my fault. That code probably dates to the "shiny modern"
days, so it's obsolete now.

> > > (Also note that on Mac a *table* view does not have a focus ring around
it,
> > 
> > well, that changes with this CL, at least for MacViews. Is that a problem?
> Alan
> > approved the idea and the screenshots but maybe there's a reason we're
missing
> > not to do the same thing to tables as trees?
> 
> It might be worth confirming with Alan. I haven't seen the screenshots so
don't
> really have an opinion :). I don't think we have Harmony mocks for
> tableview/treeview. Someone familiar with Cocoa might think a focus ring
around
> a table view looks weird, but can always remove it later.

I have no objections to this. Cocoa tableviews do not usually get focus rings,
but I don't care too much whether the Harmony ones do or not.

[Elly Fong-Jones, 2017-02-24 14:44:30]

lgtm

thanks for killing off another PlatformStyle constant :)

https://codereview.chromium.org/2715813002/diff/20001/ui/views/controls/scrol...
File ui/views/controls/scroll_view.h (left):

https://codereview.chromium.org/2715813002/diff/20001/ui/views/controls/scrol...
ui/views/controls/scroll_view.h:92: void SetHasFocusRing(bool has_focus_ring);
might be good to rename this to SetHasFocusIndicator() instead

[Evan Stade, 2017-02-24 16:55:56]

thanks Elly and Trent.

https://codereview.chromium.org/2715813002/diff/20001/ui/views/controls/scrol...
File ui/views/controls/scroll_view.h (left):

https://codereview.chromium.org/2715813002/diff/20001/ui/views/controls/scrol...
ui/views/controls/scroll_view.h:92: void SetHasFocusRing(bool has_focus_ring);
On 2017/02/24 14:44:30, Elly Fong-Jones wrote:
> might be good to rename this to SetHasFocusIndicator() instead

the reason I didn't do that is because FocusRing will be accurate again when
Harmony is default. Of course SetHasFocusIndicator will still be accurate in
that case. I guess I'll change it then.

[Evan Stade, 2017-02-24 16:56:22]

On 2017/02/24 16:55:56, Evan Stade wrote:
> thanks Elly and Trent.

... and Scott :)

[Evan Stade, 2017-02-24 17:03:33]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-02-24 17:03:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
ellyjones@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2715813002/#ps60001
(title: "rename")

[commit-bot: I haz the power, 2017-02-24 17:04:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 18:22:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487955813875960,
"parent_rev": "683f1569e2d2503b4d297fcb6c7bc5cd1aef78fa", "commit_rev":
"f084da03407bcb848d8ef8f232c28db56883d648"}

[commit-bot: I haz the power, 2017-02-24 18:22:43]

Description was changed from

==========
Change focus indicator for TableView and TreeView.

This affects surfaces such as the task manager, the bookmark bubble
editor, and the cookies view.

The indicator for selection (the background of the row) is unchanged.
When the window is focused, instead of drawing a dashed focus rect
which looks dated and especially bad at fractional scales, we draw the
table/tree's border in blue. This matches textfields and the general
trend of blue outlines/rings for focus indication.

This will change in Harmony to be a focus ring so it's mainly just an
interim solution that makes the current state of the world look less
bad while we wait for Harmony to arrive.

BUG=695540
==========

to

==========
Change focus indicator for TableView and TreeView.

This affects surfaces such as the task manager, the bookmark bubble
editor, and the cookies view.

The indicator for selection (the background of the row) is unchanged.
When the window is focused, instead of drawing a dashed focus rect
which looks dated and especially bad at fractional scales, we draw the
table/tree's border in blue. This matches textfields and the general
trend of blue outlines/rings for focus indication.

This will change in Harmony to be a focus ring so it's mainly just an
interim solution that makes the current state of the world look less
bad while we wait for Harmony to arrive.

BUG=695540

Review-Url: https://codereview.chromium.org/2715813002
Cr-Commit-Position: refs/heads/master@{#452876}
Committed:
https://chromium.googlesource.com/chromium/src/+/f084da03407bcb848d8ef8f232c2...
==========

[commit-bot: I haz the power, 2017-02-24 18:22:46]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f084da03407bcb848d8ef8f232c2...

[lazyboy, 2017-02-24 19:01:58]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2716793004/ by lazyboy@chromium.org.

The reason for reverting is: Breaking build with [chromium-style] error.
../../ui/views/controls/scroll_view.cc:184:3: error: [chromium-style] auto
variable type must not deduce to a raw pointer type.
  auto scroll_view = new ScrollView();
  ^~~~
  auto*
.