Harmony - Don't paint focus ring on unless border requests it.

Harmony - Don't paint focus ring on textfield unless border requests it.

This fixes both the omnibox and the find in page textfield.

BUG=653361
Committed: https://crrev.com/e0077f601a2907c931e932d68ae2149b25e9ff3c
Cr-Commit-Position: refs/heads/master@{#424239}

[Evan Stade, 2016-10-06 02:05:20]

Description was changed from

==========
Don't paint focus ring on unless border requests it.

BUG=653361
==========

to

==========
Harmony - Don't paint focus ring on unless border requests it.

This fixes both the omnibox and the find in page textfield.

BUG=653361
==========

[Evan Stade, 2016-10-06 02:05:20]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org, sky@chromium.org

[Evan Stade, 2016-10-06 02:05:26]

https://codereview.chromium.org/2395033002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/2395033002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:132:
SetBorder(views::Border::NullBorder());
+pkasting for this change

this didn't have any effect as far as I could tell because location_bar_view_
also sets a border which overrides this.

[Peter Kasting, 2016-10-06 07:12:11]

LGTM for c/b/ui/

https://codereview.chromium.org/2395033002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/2395033002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:132:
SetBorder(views::Border::NullBorder());
On 2016/10/06 02:05:25, Evan Stade wrote:
> +pkasting for this change
> 
> this didn't have any effect as far as I could tell because location_bar_view_
> also sets a border which overrides this.

I agree.

[Evan Stade, 2016-10-06 15:42:13]

Description was changed from

==========
Harmony - Don't paint focus ring on unless border requests it.

This fixes both the omnibox and the find in page textfield.

BUG=653361
==========

to

==========
Harmony - Don't paint focus ring on textfield unless border requests it.

This fixes both the omnibox and the find in page textfield.

BUG=653361
==========

[sky, 2016-10-06 19:34:42]

https://codereview.chromium.org/2395033002/diff/1/ui/views/border.h
File ui/views/border.h (right):

https://codereview.chromium.org/2395033002/diff/1/ui/views/border.h#newcode84
ui/views/border.h:84: virtual bool ShouldAddFocusRing() const;
Adding functions specific to certain controls doesn't scale well. Can this be a
property of TextField? By that I mean can TextField get a specific setter as to
whether the ring should be installed?

[Evan Stade, 2016-10-06 20:14:19]

https://codereview.chromium.org/2395033002/diff/1/ui/views/border.h
File ui/views/border.h (right):

https://codereview.chromium.org/2395033002/diff/1/ui/views/border.h#newcode84
ui/views/border.h:84: virtual bool ShouldAddFocusRing() const;
On 2016/10/06 19:34:42, sky wrote:
> Adding functions specific to certain controls doesn't scale well. Can this be
a
> property of TextField? By that I mean can TextField get a specific setter as
to
> whether the ring should be installed?

It could, but I strayed from that approach because it would be too easy to get
wrong. Not only do we have to hunt down every present textfield with a custom
border, but also in the future people have to realize the need for both calls.

Another option would be to add OnFocus and OnBlur to Border, then have
FocusableBorder be the one to install the focus ring. This would also allow us
to remove some SchedulePaint calls which are only there to refresh the border
after focus or blur.

[sky, 2016-10-06 22:19:51]

On Thu, Oct 6, 2016 at 1:14 PM,  <estade@chromium.org> wrote:
>
> https://codereview.chromium.org/2395033002/diff/1/ui/views/border.h
> File ui/views/border.h (right):
>
> https://codereview.chromium.org/2395033002/diff/1/ui/views/border.h#newcode84
> ui/views/border.h:84: virtual bool ShouldAddFocusRing() const;
> On 2016/10/06 19:34:42, sky wrote:
>> Adding functions specific to certain controls doesn't scale well. Can
> this be a
>> property of TextField? By that I mean can TextField get a specific
> setter as to
>> whether the ring should be installed?
>
> It could, but I strayed from that approach because it would be too easy
> to get wrong. Not only do we have to hunt down every present textfield
> with a custom border, but also in the future people have to realize the
> need for both calls.

What about assuming if a custom border is set the focus ring isn't installed?

> Another option would be to add OnFocus and OnBlur to Border, then have
> FocusableBorder be the one to install the focus ring. This would also
> allow us to remove some SchedulePaint calls which are only there to
> refresh the border after focus or blur.

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

[Evan Stade, 2016-10-06 22:38:29]

On 2016/10/06 22:19:51, sky wrote:
> On Thu, Oct 6, 2016 at 1:14 PM,  <mailto:estade@chromium.org> wrote:
> >
> > https://codereview.chromium.org/2395033002/diff/1/ui/views/border.h
> > File ui/views/border.h (right):
> >
> >
https://codereview.chromium.org/2395033002/diff/1/ui/views/border.h#newcode84
> > ui/views/border.h:84: virtual bool ShouldAddFocusRing() const;
> > On 2016/10/06 19:34:42, sky wrote:
> >> Adding functions specific to certain controls doesn't scale well. Can
> > this be a
> >> property of TextField? By that I mean can TextField get a specific
> > setter as to
> >> whether the ring should be installed?
> >
> > It could, but I strayed from that approach because it would be too easy
> > to get wrong. Not only do we have to hunt down every present textfield
> > with a custom border, but also in the future people have to realize the
> > need for both calls.
> 
> What about assuming if a custom border is set the focus ring isn't installed?

You mean overriding SetBorder? I thought of that, but if you SetBorder with a
FocusableBorder it should reinstate the focus ring. That said, I don't see
anywhere that actually does that, so I guess it will work and we don't need to
worry overly much about hypotheticals. Also, I thought I remembered someone
disliking that SetBorder was virtual and overriding it would make it harder to
fix that. Anyway, updated.

> 
> > Another option would be to add OnFocus and OnBlur to Border, then have
> > FocusableBorder be the one to install the focus ring. This would also
> > allow us to remove some SchedulePaint calls which are only there to
> > refresh the border after focus or blur.
> 
> -- 
> 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.
>

[Evan Stade, 2016-10-06 22:38:42]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 22:39:16]

Dry run: 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-06 23:33:13]

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

[commit-bot: I haz the power, 2016-10-06 23:33:13]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-07 03:10:48]

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

https://codereview.chromium.org/2395033002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:564: View::SetBorder(std::move(b));
Should you also call FocusRing::Uninstall?

[Evan Stade, 2016-10-10 17:08:29]

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

https://codereview.chromium.org/2395033002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:564: View::SetBorder(std::move(b));
On 2016/10/07 03:10:48, sky wrote:
> Should you also call FocusRing::Uninstall?

I didn't add that because it's dead code, but if you prefer it be here, I can
add it (done in latest patchset).

[Evan Stade, 2016-10-10 17:08:33]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 17:08:48]

Dry run: 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-10 17:51:58]

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

[commit-bot: I haz the power, 2016-10-10 17:51:59]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-10 19:55:19]

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

https://codereview.chromium.org/2395033002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:564: View::SetBorder(std::move(b));
On 2016/10/10 17:08:28, Evan Stade wrote:
> On 2016/10/07 03:10:48, sky wrote:
> > Should you also call FocusRing::Uninstall?
> 
> I didn't add that because it's dead code, but if you prefer it be here, I can
> add it (done in latest patchset).

Can you clarify what you mean by dead code? If there was a focus ring installed
and someone called SetBorder(nullptr) wouldn't that mean the focus ring was
never uninstalled? Maybe I'm missing something?

[Evan Stade, 2016-10-10 20:19:17]

On 2016/10/10 19:55:19, sky wrote:
>
https://codereview.chromium.org/2395033002/diff/20001/ui/views/controls/textf...
> File ui/views/controls/textfield/textfield.cc (right):
> 
>
https://codereview.chromium.org/2395033002/diff/20001/ui/views/controls/textf...
> ui/views/controls/textfield/textfield.cc:564: View::SetBorder(std::move(b));
> On 2016/10/10 17:08:28, Evan Stade wrote:
> > On 2016/10/07 03:10:48, sky wrote:
> > > Should you also call FocusRing::Uninstall?
> > 
> > I didn't add that because it's dead code, but if you prefer it be here, I
can
> > add it (done in latest patchset).
> 
> Can you clarify what you mean by dead code? If there was a focus ring
installed
> and someone called SetBorder(nullptr) wouldn't that mean the focus ring was
> never uninstalled?

yes. I just don't think that currently ever happens (calling SetBorder after
focus).

[sky, 2016-10-10 21:03:09]

LGTM

[Evan Stade, 2016-10-10 21:03:23]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-10-10 21:03:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2395033002/#ps40001
(title: "uninstall")

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

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-10 21:11:04]

Description was changed from

==========
Harmony - Don't paint focus ring on textfield unless border requests it.

This fixes both the omnibox and the find in page textfield.

BUG=653361
==========

to

==========
Harmony - Don't paint focus ring on textfield unless border requests it.

This fixes both the omnibox and the find in page textfield.

BUG=653361
==========

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

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-10 21:12:51]

Description was changed from

==========
Harmony - Don't paint focus ring on textfield unless border requests it.

This fixes both the omnibox and the find in page textfield.

BUG=653361
==========

to

==========
Harmony - Don't paint focus ring on textfield unless border requests it.

This fixes both the omnibox and the find in page textfield.

BUG=653361

Committed: https://crrev.com/e0077f601a2907c931e932d68ae2149b25e9ff3c
Cr-Commit-Position: refs/heads/master@{#424239}
==========

[commit-bot: I haz the power, 2016-10-10 21:12:52]

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