views: add focus ring to TreeView

views: add focus ring to TreeView

When TreeView is focused, there should be a focus ring around the control,
at least on Mac. This is mildly complicated because TreeViews are often
hosted in a ScrollView, and in that case the focus ring should go around
the ScrollView instead of the TreeView so the focus ring does not scroll.

BUG=605589, 647515
Committed: https://crrev.com/8cbab2b4e38a3eeb48d76f7cf839d18d6a0fc433
Cr-Commit-Position: refs/heads/master@{#425662}

[Elly Fong-Jones, 2016-10-11 13:14:36]

Description was changed from

==========
views: add focus ring to TreeView

When TreeView is focused, there should be a focus ring around the control,
at least on Mac. This is mildly complicated because TreeViews are often
hosted in a ScrollView, and in that case the focus ring should go around
the ScrollView instead of the TreeView so the focus ring does not scroll.

BUG=605589,647515
==========

to

==========
views: add focus ring to TreeView

When TreeView is focused, there should be a focus ring around the control,
at least on Mac. This is mildly complicated because TreeViews are often
hosted in a ScrollView, and in that case the focus ring should go around
the ScrollView instead of the TreeView so the focus ring does not scroll.

BUG=605589,647515
==========

[Elly Fong-Jones, 2016-10-11 13:14:36]

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

[Elly Fong-Jones, 2016-10-11 13:14:41]

sky: ptal? :)

[sky, 2016-10-11 18:17:15]

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

https://codereview.chromium.org/2411693002/diff/20001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:1082: if (grandparent &&
grandparent->GetClassName() == ScrollView::kViewClassName)
Did you verify we want this behavior everywhere? On other platforms its more
common to draw focus around the selected row(s), not the whole container. For
mac, do the indicators typically extend around the scrollbars too? I would
expect it to stay in the viewport, but then I haven't paid close attention to
the behavior on a mac.

[Elly Fong-Jones, 2016-10-12 12:42:37]

On 2016/10/11 18:17:15, sky wrote:
>
https://codereview.chromium.org/2411693002/diff/20001/ui/views/controls/tree/...
> File ui/views/controls/tree/tree_view.cc (right):
> 
>
https://codereview.chromium.org/2411693002/diff/20001/ui/views/controls/tree/...
> ui/views/controls/tree/tree_view.cc:1082: if (grandparent &&
> grandparent->GetClassName() == ScrollView::kViewClassName)
> Did you verify we want this behavior everywhere? On other platforms its more
> common to draw focus around the selected row(s), not the whole container. For
> mac, do the indicators typically extend around the scrollbars too? I would
> expect it to stay in the viewport, but then I haven't paid close attention to
> the behavior on a mac.

Hm, we definitely do not want this on non-Mac platforms, but on Mac the focus
ring does go outside the scrollbars and on the whole control:
https://bugs.chromium.org/p/chromium/issues/detail?id=647515#c8

I eyeballed the result on Mac and it looks basically correct compared to that.

[sky, 2016-10-12 16:13:47]

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

https://codereview.chromium.org/2411693002/diff/40001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:360: int width = preferred_size_.width();
I believe you'll need to force a layout on the focusring if the size changes.

[Elly Fong-Jones, 2016-10-12 19:36:06]

sky: ptal? :)

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

https://codereview.chromium.org/2411693002/diff/40001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:360: int width = preferred_size_.width();
On 2016/10/12 16:13:47, sky wrote:
> I believe you'll need to force a layout on the focusring if the size changes.

Done.

[sky, 2016-10-12 20:08:53]

https://codereview.chromium.org/2411693002/diff/60001/ui/views/controls/scrol...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2411693002/diff/60001/ui/views/controls/scrol...
ui/views/controls/scroll_view.cc:299: for (int i = 0; i < child_count(); ++i)
I suspect tree isn't the only place that wants scrollview to have a focusring.
How about adding explicit functions to ScrollView for install/uninstall the
focusring and then Scrollview can cache the focus_ring_ and call Layout on it?
That matches what we do in Textfield.

[Elly Fong-Jones, 2016-10-12 21:10:22]

sky: ptal? :)

https://codereview.chromium.org/2411693002/diff/60001/ui/views/controls/scrol...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2411693002/diff/60001/ui/views/controls/scrol...
ui/views/controls/scroll_view.cc:299: for (int i = 0; i < child_count(); ++i)
On 2016/10/12 20:08:53, sky wrote:
> I suspect tree isn't the only place that wants scrollview to have a focusring.
> How about adding explicit functions to ScrollView for install/uninstall the
> focusring and then Scrollview can cache the focus_ring_ and call Layout on it?
> That matches what we do in Textfield.

Done.

[sky, 2016-10-12 23:58:01]

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/focus...
File ui/views/controls/focus_ring.h (right):

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/focus...
ui/views/controls/focus_ring.h:19: FocusRing();
Move definition to match new position (style guide says declaration and
definition orders should match).

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/scrol...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/scrol...
ui/views/controls/scroll_view.cc:276: if (has_focus_ring == bool(focus_ring_))
Why not just focus_ring_?

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

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:641: focus_ring_view->SchedulePaint();
Are you sure you need a SchedulePaint here? If so, I think it should be in
ScrollView.

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:651: if
(ui::MaterialDesignController::IsSecondaryUiMaterial() &&
As this is nearly the same as the above and only differs in the parameter to
SetHasFocusRing, how about a function like SetShowFocusRing(bool)?

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/tree/...
File ui/views/controls/tree/tree_view.h (right):

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.h:355: ScrollView* FindFocusRingView();
This function is mildly confusing as based on name I expect it to find the
FocusRing, but that isn't the case. I think either name it
GetFocusRingContainerView() or possibly GetScrollView() as that is what it does
too.

[Elly Fong-Jones, 2016-10-13 11:14:56]

sky: ptal? :)

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/focus...
File ui/views/controls/focus_ring.h (right):

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/focus...
ui/views/controls/focus_ring.h:19: FocusRing();
On 2016/10/12 23:58:01, sky wrote:
> Move definition to match new position (style guide says declaration and
> definition orders should match).

Done.

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/scrol...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/scrol...
ui/views/controls/scroll_view.cc:276: if (has_focus_ring == bool(focus_ring_))
On 2016/10/12 23:58:01, sky wrote:
> Why not just focus_ring_?

It doesn't compile:

../../ui/views/controls/scroll_view.cc:276:22: error: comparison between pointer
and integer ('int' and 'views::View *')

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

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:641: focus_ring_view->SchedulePaint();
On 2016/10/12 23:58:01, sky wrote:
> Are you sure you need a SchedulePaint here? If so, I think it should be in
> ScrollView.

I believe so - FocusRing::Install() and FocusRing::Uninstall() do it, anyway. I
moved it to ScrollView.

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.cc:651: if
(ui::MaterialDesignController::IsSecondaryUiMaterial() &&
On 2016/10/12 23:58:01, sky wrote:
> As this is nearly the same as the above and only differs in the parameter to
> SetHasFocusRing, how about a function like SetShowFocusRing(bool)?

Done.

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/tree/...
File ui/views/controls/tree/tree_view.h (right):

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/tree/...
ui/views/controls/tree/tree_view.h:355: ScrollView* FindFocusRingView();
On 2016/10/12 23:58:01, sky wrote:
> This function is mildly confusing as based on name I expect it to find the
> FocusRing, but that isn't the case. I think either name it
> GetFocusRingContainerView() or possibly GetScrollView() as that is what it
does
> too.

It is gone now and its logic moved into TreeView::SetHasFocusRing().

[sky, 2016-10-13 17:39:45]

LGTM

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/scrol...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2411693002/diff/80001/ui/views/controls/scrol...
ui/views/controls/scroll_view.cc:276: if (has_focus_ring == bool(focus_ring_))
On 2016/10/13 11:14:56, Elly Fong-Jones wrote:
> On 2016/10/12 23:58:01, sky wrote:
> > Why not just focus_ring_?
> 
> It doesn't compile:
> 
> ../../ui/views/controls/scroll_view.cc:276:22: error: comparison between
pointer
> and integer ('int' and 'views::View *')

My mistake.

[Elly Fong-Jones, 2016-10-13 17:45:51]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2016-10-13 17:46:10]

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, 2016-10-13 18:21:01]

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

[commit-bot: I haz the power, 2016-10-13 18:21:03]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Evan Stade, 2016-10-14 15:07:00]

why can't you just use install/uninstall like any other client of FocusRing?
It's confusing to have two ways to use that class.

If you're just adding that because you want a focus_ring_ member, you could make
Install return a View, or make GetFocusRing a public function, or something like
just iterating through all children views and call layout on them (like
views::View::Layout)

[sky, 2016-10-14 16:27:56]

On Fri, Oct 14, 2016 at 8:07 AM,  <estade@chromium.org> wrote:
> why can't you just use install/uninstall like any other client of FocusRing?
> It's confusing to have two ways to use that class.

If the tree has focus and you resize it, the focusring needs to be
layed out as well. I suspect other places that are using FocusRing
need to deal with.

> If you're just adding that because you want a focus_ring_ member, you could
> make
> Install return a View, or make GetFocusRing a public function, or something
> like
> just iterating through all children views and call layout on them (like
> views::View::Layout)

That will certainly work. On advantage of being explicit is that the
caller *knows* what is going on (and remembers to do the layout).

  -Scott

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

[Elly Fong-Jones, 2016-10-14 18:22:40]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-10-14 18:22:40]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2411693002/#ps120001
(title: "make ::Install return the FocusRing")

[commit-bot: I haz the power, 2016-10-14 18:23:58]

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, 2016-10-14 18:27:50]

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

[commit-bot: I haz the power, 2016-10-14 18:27:51]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Evan Stade, 2016-10-15 01:43:58]

On 2016/10/14 16:27:56, sky wrote:
> On Fri, Oct 14, 2016 at 8:07 AM,  <mailto:estade@chromium.org> wrote:
> > why can't you just use install/uninstall like any other client of FocusRing?
> > It's confusing to have two ways to use that class.
> 
> If the tree has focus and you resize it, the focusring needs to be
> layed out as well. I suspect other places that are using FocusRing
> need to deal with.

The other places that use FocusRing rely on views::View::Layout() laying out all
children. (Also, I don't think they actually change size while focused
frequently or ever.)

[sky, 2016-10-16 01:49:40]

It's very unusual for FocusRing to reposition itself in Layout. That
works for some things, but not all.

  -Scott

On Fri, Oct 14, 2016 at 6:43 PM,  <estade@chromium.org> wrote:
> On 2016/10/14 16:27:56, sky wrote:
>> On Fri, Oct 14, 2016 at 8:07 AM, <mailto:estade@chromium.org> wrote:
>> > why can't you just use install/uninstall like any other client of
>> > FocusRing?
>> > It's confusing to have two ways to use that class.
>>
>> If the tree has focus and you resize it, the focusring needs to be
>> layed out as well. I suspect other places that are using FocusRing
>> need to deal with.
>
> The other places that use FocusRing rely on views::View::Layout() laying out
> all
> children. (Also, I don't think they actually change size while focused
> frequently or ever.)
>
> https://codereview.chromium.org/2411693002/

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

[Elly Fong-Jones, 2016-10-17 11:40:31]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-10-17 11:40:31]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2411693002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-17 11:40:43]

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, 2016-10-17 12:22:04]

Description was changed from

==========
views: add focus ring to TreeView

When TreeView is focused, there should be a focus ring around the control,
at least on Mac. This is mildly complicated because TreeViews are often
hosted in a ScrollView, and in that case the focus ring should go around
the ScrollView instead of the TreeView so the focus ring does not scroll.

BUG=605589,647515
==========

to

==========
views: add focus ring to TreeView

When TreeView is focused, there should be a focus ring around the control,
at least on Mac. This is mildly complicated because TreeViews are often
hosted in a ScrollView, and in that case the focus ring should go around
the ScrollView instead of the TreeView so the focus ring does not scroll.

BUG=605589,647515
==========

[commit-bot: I haz the power, 2016-10-17 12:22:05]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-17 12:25:38]

Description was changed from

==========
views: add focus ring to TreeView

When TreeView is focused, there should be a focus ring around the control,
at least on Mac. This is mildly complicated because TreeViews are often
hosted in a ScrollView, and in that case the focus ring should go around
the ScrollView instead of the TreeView so the focus ring does not scroll.

BUG=605589,647515
==========

to

==========
views: add focus ring to TreeView

When TreeView is focused, there should be a focus ring around the control,
at least on Mac. This is mildly complicated because TreeViews are often
hosted in a ScrollView, and in that case the focus ring should go around
the ScrollView instead of the TreeView so the focus ring does not scroll.

BUG=605589,647515

Committed: https://crrev.com/8cbab2b4e38a3eeb48d76f7cf839d18d6a0fc433
Cr-Commit-Position: refs/heads/master@{#425662}
==========

[commit-bot: I haz the power, 2016-10-17 12:25:39]

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