Add support for rect-based event targeting in views

Add support for rect-based event targeting in views

Views currently only supports point-based event targeting,
meaning that the center point is the only location used
when determining the target of a touch region. This can
lead to a poor experience for touchscreen users because
a touch can overlap many more possible targets than a
mouse cursor and it is difficult to ensure that the center
of the touch region is over the intended target (especially
if that target is small, such as the tab close button).

The goal of this patch is to add support for rect-based
event targeting ("touch fuzzing") by using a heuristic
to determine the most probable target of a rectangular
region.

This heuristic is implemented in
View::GetEventHandlerForRect() by recursively looking for
targets among the descendants of |this| which the
touch region overlaps by at least 60% (and then returning
the one that is closest to the location of the touch). If
no such targets exist, instead return the target that would
have been returned if point-based event targeting were
used with the center point of the touch. The idea behind
this approach is that small targets are more difficult to
touch reliably than large targets, so small targets
should get priority when determining the most probable
target of a touch.

In this patch, all of the overrides of
View::GetEventHandlerForRect() (formerly
View::GetEventHandlerForPoint()) simply call
View::GetEventHandlerForPoint() directly if rect-based
event targeting is to be used. The two exceptions are
NotificationView and AutofillDialogViews::SectionContainer,
for which I have preserved the existing point-based
behavior for the time being (and have added TODOs to
implement rect-based targeting in a follow up patch).

BUG=129794, 131208
TEST=ViewTest.GetEventHandlerForRect
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232891

[tdanderson, 2013-09-04 21:48:12]

@sky, would you mind taking a look at Patch Set 5? I'm not looking for a
thorough or formal review just yet. At this point I am just interested in your
thoughts about:

- My implementation of View::GetEventHandlerForRect() (which is the core of the
touch fuzzing logic), and
- My two comments under "my main concerns", especially the one about the tab
strip.

[danakj, 2013-09-05 19:10:07]

https://codereview.chromium.org/22891016/diff/26001/ui/gfx/point_conversions.h
File ui/gfx/point_conversions.h (right):

https://codereview.chromium.org/22891016/diff/26001/ui/gfx/point_conversions....
ui/gfx/point_conversions.h:19: UI_EXPORT Rect ToFlooredRect(const RectF& rect);
not lgtm on this. See rect_conversions for the various valid ways to convert a
rect.

[sky, 2013-09-06 19:56:39]

On Wed, Sep 4, 2013 at 2:48 PM,  <tdanderson@chromium.org> wrote:
> A large code review. Reviewers: sky,
>
> Message:
> @sky, would you mind taking a look at Patch Set 5? I'm not looking for a
> thorough or formal review just yet. At this point I am just interested in
> your
> thoughts about:
>
> - My implementation of View::GetEventHandlerForRect() (which is the core of
> the
> touch fuzzing logic), and
> - My two comments under "my main concerns", especially the one about the tab
> strip.
>
> Description:
> Add support for rect-based event targeting in views
>
> Still a WIP. See "my main concerns" and "left to do" at
> the bottom of the issue description.
>
> Views currently only supports point-based event targeting,
> meaning that the center point is the only location used
> when determining the target of a touch region. This can
> lead to a poor experience for touchscreen users because
> a touch can overlap many more possible targets than a
> mouse cursor and it is difficult to ensure that the center
> of the touch region is over the intended target (especially
> if that target is small, such as the tab close button).
>
> The goal of this patch is to add support for rect-based
> event targeting ("touch fuzzing") by using a heuristic
> to determine the most probable target of a rectangular
> region.
>
> This heuristic is implemented in
> View::GetEventHandlerForRect() by recursively looking for
> targets among the descendants of |this| which the
> touch region overlaps by at least 60% (and then returning
> the one that is closest to the location of the touch). If
> no such targets exist, instead return the target that would
> have been returned if point-based event targeting were
> used with the center point of the touch. The idea behind
> this approach is that small targets are more difficult to
> touch reliably than large targets, so small targets
> should get priority when determining the most probable
> target of a touch.
>
> In this patch, all of the overrides of
> View::GetEventHandlerForRect() (formerly
> View::GetEventHandlerForPoint()) simply call
> View::GetEventHandlerForPoint() directly if rect-based
> event targeting is to be used.
>
>
> My main concerns:
> - As a result of patch Set 5 using the logic in
> View::GetEventHandler() for the tab strip, it is possible
> to tap the tab close buttons of occluded tabs in
> touch layout / stacked tab mode. This is because the
> tab close buttons report themselves as being visible even
> if they are behind another tab. Two possible approaches
> to solving this problem are a) modifying the hit test
> mask for a tab when in stacked tab mode, or b) implement
> the overriden TabStrip::GetEventHandlerForRect() to ignore
> "mini" tabs (i.e., completely occluded) and non-visible
> tab close buttons when in stacked tab mode.

Isn't this just a bug in how you've implemented TabStrip's
GetEventHandlerForRect? It has enough information to know whether the
close button is obscured or not.


> - Does NotificationView::GetEventHandlerForRect() need
> a special implementation, or is it correct to call back
> to View::GetEventHandlerForPoint()?

Seems like it wants your View implementation but over the specified
views rather than the children.

  -Scott

> Left to do:
> - Add a flag to be able to switch fuzzing on/off.
> - Address the few comments starting with "Terry:"
> in Patch Set 5.
> - Split this into smaller patches and add testing.
>
> BUG=129794
>
> Please review this at https://codereview.chromium.org/22891016/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files +333, -106:
>   M chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
>   M chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
>   M chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h
>   M chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc
>   M chrome/browser/ui/views/tabs/tab.cc
>   M chrome/browser/ui/views/tabs/tab_strip.h
>   M chrome/browser/ui/views/tabs/tab_strip.cc
>   M chrome/browser/ui/views/toolbar_view.cc
>   M ui/gfx/point_conversions.h
>   M ui/gfx/point_conversions.cc
>   M ui/message_center/views/notification_view.h
>   M ui/message_center/views/notification_view.cc
>   M ui/views/view.h
>   M ui/views/view.cc
>   M ui/views/widget/root_view.cc
>   M ui/views/window/non_client_view.h
>   M ui/views/window/non_client_view.cc
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-09-06 19:56:47]

Here's an additional couple of comments for you.

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode859
ui/views/view.cc:859: View* View::GetEventHandlerForPoint(const gfx::Point&
point) {
Is there a reason to keep this? Keep it is error prone in so far as subclasses
that override one really need to override both.

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode863
ui/views/view.cc:863: View* View::GetEventHandlerForRect(const gfx::Rect& rect)
{
I kind of think the return value should be changed so that it returns not only
the view but also the hit region, or maybe a percent. That way you can end early
and you know the region of the view considered hit.

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode894
ui/views/view.cc:894: if (PercentCoveredBy(cur_rect, rect) >=
kViewsFuzzingOverlap) {
Doesn't this assume the view completely fills its space, which is not
necessarily true? For example, tabs definitely don't fill their space.

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode897
ui/views/view.cc:897: gfx::Point touch_center(rect.CenterPoint());
Can't you return some where in here if the view completely covers the area?

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode910
ui/views/view.cc:910: if (child->HitTestPoint(point_in_child_coords))
HitTestPoint isn't enough, you really need to be invoking
GetEventHandlerForPoint here.

[tdanderson, 2013-09-09 22:15:39]

My replies to sky's and danakj's comments along with an updated patch set 6. I
am still working on fully addressing the comment in
View::GetEventHandlerForRect(), line 894.

https://codereview.chromium.org/22891016/diff/26001/ui/gfx/point_conversions.h
File ui/gfx/point_conversions.h (right):

https://codereview.chromium.org/22891016/diff/26001/ui/gfx/point_conversions....
ui/gfx/point_conversions.h:19: UI_EXPORT Rect ToFlooredRect(const RectF& rect);
On 2013/09/05 19:10:07, danakj wrote:
> not lgtm on this. See rect_conversions for the various valid ways to convert a
> rect.

Done.

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode859
ui/views/view.cc:859: View* View::GetEventHandlerForPoint(const gfx::Point&
point) {
On 2013/09/06 19:56:47, sky wrote:
> Is there a reason to keep this? Keep it is error prone in so far as subclasses
> that override one really need to override both.

I am keeping it as a convenience so that existing callers of
GetEventHandlerForPoint() don't need to do the work of converting a point to a
1x1 rect themselves. I have made View::GetEventHandlerForPoint() non-virtual, so
subclasses will only need to override View::GetEventHandlerForRect().

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode863
ui/views/view.cc:863: View* View::GetEventHandlerForRect(const gfx::Rect& rect)
{
On 2013/09/06 19:56:47, sky wrote:
> I kind of think the return value should be changed so that it returns not only
> the view but also the hit region, or maybe a percent. That way you can end
early
> and you know the region of the view considered hit.

Please see my reply to your comment at line 897.

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode894
ui/views/view.cc:894: if (PercentCoveredBy(cur_rect, rect) >=
kViewsFuzzingOverlap) {
On 2013/09/06 19:56:47, sky wrote:
> Doesn't this assume the view completely fills its space, which is not
> necessarily true? For example, tabs definitely don't fill their space.

Yes, good point... I am going to try and use the hit test mask (if applicable)
instead of GetLocalBounds(). Hopefully doing so will also resolve the problem I
have about occluded tab close buttons in the tab  strip being targeted.

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode897
ui/views/view.cc:897: gfx::Point touch_center(rect.CenterPoint());
On 2013/09/06 19:56:47, sky wrote:
> Can't you return some where in here if the view completely covers the area?

I don't think an early return is possible by just looking at the overlap
percentages because:

- It is possible for more than one view to completely cover |rect| if one of
these views is enclosed by the other, so an early return is not guaranteed to
give the correct result.
- It is possible for |rect| to completely cover more than one view, so even if
PercentCoveredBy(cur_rect, rect) == 1.0, that does not mean that |cur_view| is
the one that should be returned (distance is used to break 'ties' among views of
this type).

For this reason I don't think it would be helpful to change the return type of
View::GetEventHandlerForRect() as you suggested on line 863.

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode910
ui/views/view.cc:910: if (child->HitTestPoint(point_in_child_coords))
On 2013/09/06 19:56:47, sky wrote:
> HitTestPoint isn't enough, you really need to be invoking
> GetEventHandlerForPoint here.

Done.

[danakj, 2013-09-09 22:17:33]

Thanks Terry. Removing myself from reviewers in hopes that undoes my previous
comment.

[danakj, 2013-09-09 22:18:00]

Seems not, so LGTM on the zero changes to ui/gfx/

[tdanderson, 2013-09-09 22:30:51]

> >
> > My main concerns:
> > - As a result of patch Set 5 using the logic in
> > View::GetEventHandler() for the tab strip, it is possible
> > to tap the tab close buttons of occluded tabs in
> > touch layout / stacked tab mode. This is because the
> > tab close buttons report themselves as being visible even
> > if they are behind another tab. Two possible approaches
> > to solving this problem are a) modifying the hit test
> > mask for a tab when in stacked tab mode, or b) implement
> > the overriden TabStrip::GetEventHandlerForRect() to ignore
> > "mini" tabs (i.e., completely occluded) and non-visible
> > tab close buttons when in stacked tab mode.
> 
> Isn't this just a bug in how you've implemented TabStrip's
> GetEventHandlerForRect? It has enough information to know whether the
> close button is obscured or not.

I am hoping that I can keep the implementation of
TabStrip::GetEventHandlerForRect() from Patch Set 5 and that
using the hit test mask for hit testing instead of the local
bounds will solve this problem (see my comment in view.cc, line 894).

Can you clarify what information you mean (in "has enough information")?
The only way I could (possibly) find to know whether or not the tab close
button is obscured in stacked tab mode was to perform computations
based on the tab's position in the stacked tab strip, the number of
non-stacked tabs, the size of each non-stacked tab, etc. Is there
an easier way that I have missed?

> 
> 
> > - Does NotificationView::GetEventHandlerForRect() need
> > a special implementation, or is it correct to call back
> > to View::GetEventHandlerForPoint()?
> 
> Seems like it wants your View implementation but over the specified
> views rather than the children.
> 

Right, and looking at this again I don't think the implementation in
Patch Set 5 will be correct. Would you have any objections to 
addressing fuzzing in NotificationView in a follow-up patch, and 
have this patch just focus on the views located above contents? That's
where my UMA stats have been put in place to measure, anyway
(specifically on or around the tab strip). In the meanwhile I can
have NotificationView::GetEventHandlerForRect() just use the
center point of the rect so there will be no change in behavior
there.

[sky, 2013-09-10 16:47:41]

AFAICT this approach has a couple of flaws for tabs, which if I understand is
one of the main things you care about. It isn't taking the shape of the tab into
consideration when considering the percent overlapping and at the end of the day
you're using the center of the touch to determine hit test.

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/22891016/diff/26001/ui/views/view.cc#newcode897
ui/views/view.cc:897: gfx::Point touch_center(rect.CenterPoint());
On 2013/09/09 22:15:39, tdanderson wrote:
> On 2013/09/06 19:56:47, sky wrote:
> > Can't you return some where in here if the view completely covers the area?
> 
> I don't think an early return is possible by just looking at the overlap
> percentages because:
> 
> - It is possible for more than one view to completely cover |rect| if one of
> these views is enclosed by the other, so an early return is not guaranteed to
> give the correct result.
> - It is possible for |rect| to completely cover more than one view, so even if
> PercentCoveredBy(cur_rect, rect) == 1.0, that does not mean that |cur_view| is
> the one that should be returned (distance is used to break 'ties' among views
of
> this type).
> 
> For this reason I don't think it would be helpful to change the return type of
> View::GetEventHandlerForRect() as you suggested on line 863.

99% of the views don't allow overlapping children, meaning you could early out
if you knew this. But I'm hesitant to add in API to determine if a view allows
overlapping children unless we identify this as a hotspot (which I doubt it is).
So, ok, make sense.

That said, your implementation does need to early out for the 1x1 case (the
mouse case), as we get way more mouse events than touch events.

https://codereview.chromium.org/22891016/diff/35001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/22891016/diff/35001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip.cc:2590: bool TabStrip::IsRectInTab(Tab*
tab,
You don't seem to use this.

[tdanderson, 2013-10-03 23:16:07]

> AFAICT this approach has a couple of flaws for tabs, which if I understand is
> one of the main things you care about. It isn't taking the shape of the tab
into
> consideration when considering the percent overlapping and at the end of the
day
> you're using the center of the touch to determine hit test.

I agree that using the bounding rect of the tab is not ideal. But since the area
covered by a touch is not as exact as the location of a mouse cursor, I think
that using rects as an approximation is a reasonable place to start (similar to
how we approximate an elliptical touch region as a rect).

> 99% of the views don't allow overlapping children, meaning you could early out
> if you knew this. But I'm hesitant to add in API to determine if a view allows
> overlapping children unless we identify this as a hotspot (which I doubt it
is).
> So, ok, make sense.
> 
> That said, your implementation does need to early out for the 1x1 case (the
> mouse case), as we get way more mouse events than touch events.

I think that I'm already doing this (line 870 in view.cc). I introduced
UsePointBasedTargeting() to detect when we're dealing with a mouse event,
in which case View::GetEventHandlerForRect() should behave exactly the
same as the old implementation of View::GetEventHandlerForPoint().

[tdanderson, 2013-10-03 23:17:42]

Scott, can you please have a look at my changes in patch set 8 (specifically the
hit test mask changes in tab.cc)?

https://codereview.chromium.org/22891016/diff/35001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/22891016/diff/35001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip.cc:2590: bool TabStrip::IsRectInTab(Tab*
tab,
On 2013/09/10 16:47:41, sky wrote:
> You don't seem to use this.

Removed.

https://codereview.chromium.org/22891016/diff/45001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/22891016/diff/45001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab.cc:444: #if defined(USE_ASH)
If this code stays, I think the #if defined(USE_ASH) code in
TabCloseButton::GetEventHandlerForRect() can be removed.

[sky, 2013-10-04 16:57:02]

On Thu, Oct 3, 2013 at 4:16 PM,  <tdanderson@chromium.org> wrote:
>> AFAICT this approach has a couple of flaws for tabs, which if I understand
>> is
>> one of the main things you care about. It isn't taking the shape of the
>> tab
>
> into
>>
>> consideration when considering the percent overlapping and at the end of
>> the
>
> day
>>
>> you're using the center of the touch to determine hit test.
>
>
> I agree that using the bounding rect of the tab is not ideal. But since the
> area
> covered by a touch is not as exact as the location of a mouse cursor, I
> think
> that using rects as an approximation is a reasonable place to start (similar
> to
> how we approximate an elliptical touch region as a rect).

Won't this mean clicks above the tabstrip would incorrectly favor the
tabstrip as you're not considering the shape?

  -Scott

>
>> 99% of the views don't allow overlapping children, meaning you could early
>> out
>> if you knew this. But I'm hesitant to add in API to determine if a view
>> allows
>> overlapping children unless we identify this as a hotspot (which I doubt
>> it
>
> is).
>>
>> So, ok, make sense.
>
>
>> That said, your implementation does need to early out for the 1x1 case
>> (the
>> mouse case), as we get way more mouse events than touch events.
>
>
> I think that I'm already doing this (line 870 in view.cc). I introduced
> UsePointBasedTargeting() to detect when we're dealing with a mouse event,
> in which case View::GetEventHandlerForRect() should behave exactly the
> same as the old implementation of View::GetEventHandlerForPoint().
>
> https://codereview.chromium.org/22891016/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-10-04 17:03:48]

You need to write some tests too.

I go back and forth on whether it's best to have both GetEventHandlerForPoint
and GetEventHandlerForRect. I like a single one as that way it's easy to make
sure folks override both, but then I dislike implementations that only do
something for points or rects (as TabStrip does).

+ben in case he has opinion on View API.

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode873
ui/views/view.cc:873: gfx::Rect cur_rect(cur_view->GetLocalBounds());
cur_rect -> cur_view_bounds

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode879
ui/views/view.cc:879: // Terry: change this to just use distance between center
points.
TODO(tdanderson):

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode889
ui/views/view.cc:889: gfx::Point point_in_child_coords(rect.CenterPoint());
Can you use the center point of rect_in_child_coords?

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode1341
ui/views/view.cc:1341: float intersection_area = intersection.size().GetArea();
Should this and 1342 be kept as ints and then cased on 1343?

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.h#newcode985
ui/views/view.h:985: static bool UsePointBasedTargeting(const gfx::Rect& rect);
Other than UsePointBasedTargeting does any of this need to be exposed? I think
they should be moved into their file as functions in the views namespace. Then
you can write tests for them.

[sky, 2013-10-04 17:05:13]

https://codereview.chromium.org/22891016/diff/45001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/22891016/diff/45001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab.cc:444: #if defined(USE_ASH)
On 2013/10/03 23:17:42, tdanderson wrote:
> If this code stays, I think the #if defined(USE_ASH) code in
> TabCloseButton::GetEventHandlerForRect() can be removed.

I don't like this ifdef. It implies the API should contain a bit more
information than is passed.

[tdanderson, 2013-10-07 21:22:44]

On 2013/10/04 16:57:02, sky wrote:
> On Thu, Oct 3, 2013 at 4:16 PM,  <mailto:tdanderson@chromium.org> wrote:
> >> AFAICT this approach has a couple of flaws for tabs, which if I understand
> >> is
> >> one of the main things you care about. It isn't taking the shape of the
> >> tab
> >
> > into
> >>
> >> consideration when considering the percent overlapping and at the end of
> >> the
> >
> > day
> >>
> >> you're using the center of the touch to determine hit test.
> >
> >
> > I agree that using the bounding rect of the tab is not ideal. But since the
> > area
> > covered by a touch is not as exact as the location of a mouse cursor, I
> > think
> > that using rects as an approximation is a reasonable place to start (similar
> > to
> > how we approximate an elliptical touch region as a rect).
> 
> Won't this mean clicks above the tabstrip would incorrectly favor the
> tabstrip as you're not considering the shape?
> 
>   -Scott

I don't think so. I am making use of UsePointBasedTargeting() in
View::GetEventHandlerForRect() (and also in all of its overrides) so that
targeting of clicks remains unchanged. A click will never reach the
call to PercentCoveredBy() in View::GetEventHandlerForRect().

> 
> >
> >> 99% of the views don't allow overlapping children, meaning you could early
> >> out
> >> if you knew this. But I'm hesitant to add in API to determine if a view
> >> allows
> >> overlapping children unless we identify this as a hotspot (which I doubt
> >> it
> >
> > is).
> >>
> >> So, ok, make sense.
> >
> >
> >> That said, your implementation does need to early out for the 1x1 case
> >> (the
> >> mouse case), as we get way more mouse events than touch events.
> >
> >
> > I think that I'm already doing this (line 870 in view.cc). I introduced
> > UsePointBasedTargeting() to detect when we're dealing with a mouse event,
> > in which case View::GetEventHandlerForRect() should behave exactly the
> > same as the old implementation of View::GetEventHandlerForPoint().
> >
> > https://codereview.chromium.org/22891016/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[tdanderson, 2013-10-07 21:28:52]

On 2013/10/04 17:03:48, sky wrote:
> You need to write some tests too.

I definitely agree.

Before I started writing tests, I wanted to get a general LGTM for the
approach of this patch and then split it into smaller pieces instead
of landing everything all at once. Do you have a preference?

> 
> I go back and forth on whether it's best to have both GetEventHandlerForPoint
> and GetEventHandlerForRect. I like a single one as that way it's easy to make
> sure folks override both, but then I dislike implementations that only do
> something for points or rects (as TabStrip does).
> 
> +ben in case he has opinion on View API.

Unless I am misunderstanding, I don't think having both functions will introduce
any burden of overriding because I have made GetEventHandlerForPoint
non-virtual.

> 
> https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc
> File ui/views/view.cc (right):
> 
>
https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode873
> ui/views/view.cc:873: gfx::Rect cur_rect(cur_view->GetLocalBounds());
> cur_rect -> cur_view_bounds
> 
>
https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode879
> ui/views/view.cc:879: // Terry: change this to just use distance between
center
> points.
> TODO(tdanderson):
> 
>
https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode889
> ui/views/view.cc:889: gfx::Point point_in_child_coords(rect.CenterPoint());
> Can you use the center point of rect_in_child_coords?
> 
>
https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode1341
> ui/views/view.cc:1341: float intersection_area =
intersection.size().GetArea();
> Should this and 1342 be kept as ints and then cased on 1343?
> 
> https://codereview.chromium.org/22891016/diff/45001/ui/views/view.h
> File ui/views/view.h (right):
> 
> https://codereview.chromium.org/22891016/diff/45001/ui/views/view.h#newcode985
> ui/views/view.h:985: static bool UsePointBasedTargeting(const gfx::Rect&
rect);
> Other than UsePointBasedTargeting does any of this need to be exposed? I think
> they should be moved into their file as functions in the views namespace. Then
> you can write tests for them.

[tdanderson, 2013-10-07 21:35:20]

Please have a look at my changes and comments in Patch Set 9.

https://codereview.chromium.org/22891016/diff/45001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/22891016/diff/45001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab.cc:444: #if defined(USE_ASH)
On 2013/10/04 17:05:14, sky wrote:
> On 2013/10/03 23:17:42, tdanderson wrote:
> > If this code stays, I think the #if defined(USE_ASH) code in
> > TabCloseButton::GetEventHandlerForRect() can be removed.
> 
> I don't like this ifdef. It implies the API should contain a bit more
> information than is passed.

I'm also not a fan of having this ifdef where it is, but my understanding is
that we only want to include the padding if USE_ASH is true, so we have to have
the ifdef somewhere (similar to what we're already doing in
TabCloseButton::GetEventHandlerForRect()). Would moving it to a new function
such as bool TabCloseButton::IncludePadding() be better?

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode873
ui/views/view.cc:873: gfx::Rect cur_rect(cur_view->GetLocalBounds());
On 2013/10/04 17:03:49, sky wrote:
> cur_rect -> cur_view_bounds

Done.

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode879
ui/views/view.cc:879: // Terry: change this to just use distance between center
points.
On 2013/10/04 17:03:49, sky wrote:
> TODO(tdanderson):

On second thought, I'm going to leave this as is since I believe it will perform
better for selecting wide views elements (e.g., tabs, omnibox) in situations
where they are considered candidates for fuzzing.

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode889
ui/views/view.cc:889: gfx::Point point_in_child_coords(rect.CenterPoint());
On 2013/10/04 17:03:49, sky wrote:
> Can you use the center point of rect_in_child_coords?

Done.

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.cc#newcode1341
ui/views/view.cc:1341: float intersection_area = intersection.size().GetArea();
On 2013/10/04 17:03:49, sky wrote:
> Should this and 1342 be kept as ints and then cased on 1343?

Done.

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/22891016/diff/45001/ui/views/view.h#newcode985
ui/views/view.h:985: static bool UsePointBasedTargeting(const gfx::Rect& rect);
On 2013/10/04 17:03:49, sky wrote:
> Other than UsePointBasedTargeting does any of this need to be exposed? I think
> they should be moved into their file as functions in the views namespace. Then
> you can write tests for them.

Moved these into ui/views/rect_based_targeting_utils.{h,cc}.

https://codereview.chromium.org/22891016/diff/45001/ui/views/widget/root_view.cc
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/22891016/diff/45001/ui/views/widget/root_view...
ui/views/widget/root_view.cc:281: // Terry: A runtime flag will specify whether
or not fuzzing should be
I still have to define a --use-rect-based-views-targeting flag and replace
|use_fuzzing| with a check for the existence of this flag.

Since there is no *_switches.{h,cc} files in ui/views/widget/ or any of its
parent directories, is it appropriate to create the new files
ui/views/views_switches.{h,cc} to define the new flag? Or would you prefer
another way?

[Ben Goodger (Google), 2013-10-07 22:59:55]

i'd like some of the utility functions here to be relocated to ui/events, since
I think it'll be needed for aura also.

[sky, 2013-10-07 23:01:24]

On Mon, Oct 7, 2013 at 2:22 PM,  <tdanderson@chromium.org> wrote:
> On 2013/10/04 16:57:02, sky wrote:
>
>> On Thu, Oct 3, 2013 at 4:16 PM,  <mailto:tdanderson@chromium.org> wrote:
>> >> AFAICT this approach has a couple of flaws for tabs, which if I
>> >> understand
>> >> is
>> >> one of the main things you care about. It isn't taking the shape of the
>> >> tab
>> >
>> > into
>> >>
>> >> consideration when considering the percent overlapping and at the end
>> >> of
>> >> the
>> >
>> > day
>> >>
>> >> you're using the center of the touch to determine hit test.
>> >
>> >
>> > I agree that using the bounding rect of the tab is not ideal. But since
>> > the
>> > area
>> > covered by a touch is not as exact as the location of a mouse cursor, I
>> > think
>> > that using rects as an approximation is a reasonable place to start
>> > (similar
>> > to
>> > how we approximate an elliptical touch region as a rect).
>
>
>> Won't this mean clicks above the tabstrip would incorrectly favor the
>> tabstrip as you're not considering the shape?
>
>
>>    -Scott
>
>
> I don't think so. I am making use of UsePointBasedTargeting() in
> View::GetEventHandlerForRect() (and also in all of its overrides) so that
> targeting of clicks remains unchanged. A click will never reach the
> call to PercentCoveredBy() in View::GetEventHandlerForRect().

Sorry, I mean touch. That is, won't touch above the tabstrip
incorrectly favor the tabstrip as you're not considering the shape?

  -Scott

>
>
>
>> >
>> >> 99% of the views don't allow overlapping children, meaning you could
>> >> early
>> >> out
>> >> if you knew this. But I'm hesitant to add in API to determine if a view
>> >> allows
>> >> overlapping children unless we identify this as a hotspot (which I
>> >> doubt
>> >> it
>> >
>> > is).
>> >>
>> >> So, ok, make sense.
>> >
>> >
>> >> That said, your implementation does need to early out for the 1x1 case
>> >> (the
>> >> mouse case), as we get way more mouse events than touch events.
>> >
>> >
>> > I think that I'm already doing this (line 870 in view.cc). I introduced
>> > UsePointBasedTargeting() to detect when we're dealing with a mouse
>> > event,
>> > in which case View::GetEventHandlerForRect() should behave exactly the
>> > same as the old implementation of View::GetEventHandlerForPoint().
>> >
>> > https://codereview.chromium.org/22891016/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
>
> https://codereview.chromium.org/22891016/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-10-08 15:39:06]

https://codereview.chromium.org/22891016/diff/45001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/22891016/diff/45001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab.cc:444: #if defined(USE_ASH)
On 2013/10/07 21:35:21, tdanderson wrote:
> On 2013/10/04 17:05:14, sky wrote:
> > On 2013/10/03 23:17:42, tdanderson wrote:
> > > If this code stays, I think the #if defined(USE_ASH) code in
> > > TabCloseButton::GetEventHandlerForRect() can be removed.
> > 
> > I don't like this ifdef. It implies the API should contain a bit more
> > information than is passed.
> 
> I'm also not a fan of having this ifdef where it is, but my understanding is
> that we only want to include the padding if USE_ASH is true, so we have to
have
> the ifdef somewhere (similar to what we're already doing in
> TabCloseButton::GetEventHandlerForRect()). Would moving it to a new function
> such as bool TabCloseButton::IncludePadding() be better?

My point is that if a view needs to know if touch is down to change the behavior
of this GetHitTestMask it implies to me the state should be passed in, eg
GetHitTestMask(HitTestSource source, path).

HitTestSource would be: touch, mouse or whatever we use.

[sky, 2013-10-08 20:01:11]

https://codereview.chromium.org/22891016/diff/57001/ui/views/rect_based_targe...
File ui/views/rect_based_targeting_utils.cc (right):

https://codereview.chromium.org/22891016/diff/57001/ui/views/rect_based_targe...
ui/views/rect_based_targeting_utils.cc:33: return rect_1_area ?
(float)intersection_area / (float)rect_1_area : 0;
use c++ casts, eg static_cast<float> ...

https://codereview.chromium.org/22891016/diff/57001/ui/views/rect_based_targe...
File ui/views/rect_based_targeting_utils.h (right):

https://codereview.chromium.org/22891016/diff/57001/ui/views/rect_based_targe...
ui/views/rect_based_targeting_utils.h:5: #ifndef
UI_RECT_BASED_TARGETING_UTILS_H_
Since this is all touch based, how about touch_targetting_utils or
touch_hit_test_utils?

[tdanderson, 2013-10-09 21:20:36]

On 2013/10/07 22:59:55, Ben Goodger (Google) wrote:
> i'd like some of the utility functions here to be relocated to ui/events,
since
> I think it'll be needed for aura also.

Rect-based targeting in views is still considered experimental, which is why I
am
landing it behind a flag and will be looking at collected metrics data and any 
other feedback to determine if it should be enabled by default. For this reason,
I'd rather leave the utility functions alone for the time being but I agree
they should be shared with Aura if needed. I have filed issue 305795 to make
sure
I keep this in mind.

[tdanderson, 2013-10-09 21:31:05]

On 2013/10/07 23:01:24, sky wrote:
> On Mon, Oct 7, 2013 at 2:22 PM,  <mailto:tdanderson@chromium.org> wrote:
> > On 2013/10/04 16:57:02, sky wrote:
> >
> >> On Thu, Oct 3, 2013 at 4:16 PM,  <mailto:tdanderson@chromium.org> wrote:
> >> >> AFAICT this approach has a couple of flaws for tabs, which if I
> >> >> understand
> >> >> is
> >> >> one of the main things you care about. It isn't taking the shape of the
> >> >> tab
> >> >
> >> > into
> >> >>
> >> >> consideration when considering the percent overlapping and at the end
> >> >> of
> >> >> the
> >> >
> >> > day
> >> >>
> >> >> you're using the center of the touch to determine hit test.
> >> >
> >> >
> >> > I agree that using the bounding rect of the tab is not ideal. But since
> >> > the
> >> > area
> >> > covered by a touch is not as exact as the location of a mouse cursor, I
> >> > think
> >> > that using rects as an approximation is a reasonable place to start
> >> > (similar
> >> > to
> >> > how we approximate an elliptical touch region as a rect).
> >
> >
> >> Won't this mean clicks above the tabstrip would incorrectly favor the
> >> tabstrip as you're not considering the shape?
> >
> >
> >>    -Scott
> >
> >
> > I don't think so. I am making use of UsePointBasedTargeting() in
> > View::GetEventHandlerForRect() (and also in all of its overrides) so that
> > targeting of clicks remains unchanged. A click will never reach the
> > call to PercentCoveredBy() in View::GetEventHandlerForRect().
> 
> Sorry, I mean touch. That is, won't touch above the tabstrip
> incorrectly favor the tabstrip as you're not considering the shape?
> 
>   -Scott

The bounding box of a tab has a larger area than the tab itself, so if
anything it will have the opposite effect (since it would be harder
to reach the 60% threshold needed for fuzzing to take effect).

To address the concern about considering the tab's shape in the
overlap calculations, I see no easy / inexpensive way to be
able to either a) determine the region of intersection between a 
rect and a region of arbitrary shape or b) calculate the area
of such a region.

The bounding box is a reasonable approximation for the time being but
can always be changed if needed as part of refining this feature.


> 
> >
> >
> >
> >> >
> >> >> 99% of the views don't allow overlapping children, meaning you could
> >> >> early
> >> >> out
> >> >> if you knew this. But I'm hesitant to add in API to determine if a view
> >> >> allows
> >> >> overlapping children unless we identify this as a hotspot (which I
> >> >> doubt
> >> >> it
> >> >
> >> > is).
> >> >>
> >> >> So, ok, make sense.
> >> >
> >> >
> >> >> That said, your implementation does need to early out for the 1x1 case
> >> >> (the
> >> >> mouse case), as we get way more mouse events than touch events.
> >> >
> >> >
> >> > I think that I'm already doing this (line 870 in view.cc). I introduced
> >> > UsePointBasedTargeting() to detect when we're dealing with a mouse
> >> > event,
> >> > in which case View::GetEventHandlerForRect() should behave exactly the
> >> > same as the old implementation of View::GetEventHandlerForPoint().
> >> >
> >> > https://codereview.chromium.org/22891016/
> >
> >
> >> To unsubscribe from this group and stop receiving emails from it, send an
> >
> > email
> >>
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> >
> > https://codereview.chromium.org/22891016/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[tdanderson, 2013-10-09 21:32:23]

On 2013/10/08 15:39:06, sky wrote:
>
https://codereview.chromium.org/22891016/diff/45001/chrome/browser/ui/views/t...
> File chrome/browser/ui/views/tabs/tab.cc (right):
> 
>
https://codereview.chromium.org/22891016/diff/45001/chrome/browser/ui/views/t...
> chrome/browser/ui/views/tabs/tab.cc:444: #if defined(USE_ASH)
> On 2013/10/07 21:35:21, tdanderson wrote:
> > On 2013/10/04 17:05:14, sky wrote:
> > > On 2013/10/03 23:17:42, tdanderson wrote:
> > > > If this code stays, I think the #if defined(USE_ASH) code in
> > > > TabCloseButton::GetEventHandlerForRect() can be removed.
> > > 
> > > I don't like this ifdef. It implies the API should contain a bit more
> > > information than is passed.
> > 
> > I'm also not a fan of having this ifdef where it is, but my understanding is
> > that we only want to include the padding if USE_ASH is true, so we have to
> have
> > the ifdef somewhere (similar to what we're already doing in
> > TabCloseButton::GetEventHandlerForRect()). Would moving it to a new function
> > such as bool TabCloseButton::IncludePadding() be better?
> 
> My point is that if a view needs to know if touch is down to change the
behavior
> of this GetHitTestMask it implies to me the state should be passed in, eg
> GetHitTestMask(HitTestSource source, path).
> 
> HitTestSource would be: touch, mouse or whatever we use.

I have added this in Patch Set 10.

[tdanderson, 2013-10-09 21:42:06]

I will now start the process of splitting this up into smaller patches and
adding tests.

https://codereview.chromium.org/22891016/diff/57001/ui/views/rect_based_targe...
File ui/views/rect_based_targeting_utils.cc (right):

https://codereview.chromium.org/22891016/diff/57001/ui/views/rect_based_targe...
ui/views/rect_based_targeting_utils.cc:33: return rect_1_area ?
(float)intersection_area / (float)rect_1_area : 0;
On 2013/10/08 20:01:12, sky wrote:
> use c++ casts, eg static_cast<float> ...

Done.

https://codereview.chromium.org/22891016/diff/57001/ui/views/rect_based_targe...
File ui/views/rect_based_targeting_utils.h (right):

https://codereview.chromium.org/22891016/diff/57001/ui/views/rect_based_targe...
ui/views/rect_based_targeting_utils.h:5: #ifndef
UI_RECT_BASED_TARGETING_UTILS_H_
On 2013/10/08 20:01:12, sky wrote:
> Since this is all touch based, how about touch_targetting_utils or
> touch_hit_test_utils?

My preference would be to use point/rect instead of mouse/touch where possible
since it stays consistent with the naming of HitTestPoint()/HitTestRect() and
GetEventHandlerForPoint()/GetEventHandlerForRect(). 

I recognize that for the time being "rect-based targeting" and "touch-based
targeting" are the same thing (which is why the HitTestSource enum in view.h
makes sense) but my opinion for the longer term is that the size/shape of the
region used for the hit test shouldn't necessarily imply that the hit test was
requested as a result of using a specific input device.

[tdanderson, 2013-10-29 23:36:56]

Scott, would you mind having a look at Patch Set 19? Specifically, please look
at:

- The tests I added (would you like any more testing?)
- The flag I added
- The change I made to the main fuzzing algorithm (I marked this with a
comment).

Note that you should ignore all code related to hit test masks for tabs / tab
close buttons. I left that in just to be able to run the trybots, but all of
that code will land separately in https://codereview.chromium.org/32463003/
(pending Ben's LGTM).

Thank you.

https://codereview.chromium.org/22891016/diff/537001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/22891016/diff/537001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1835: "views-use-rect-based-targeting",  //
FLAGS:RECORD_UMA
This is the flag I am landing everything behind. I have only done manual testing
on ChromeOS, but if I have time before the M32 branch point I will see how this
code behaves on Windows. If it works, then I will also make the flag available
on Win (also turned off by default, of course).

https://codereview.chromium.org/22891016/diff/537001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/22891016/diff/537001/ui/views/view.cc#newcode904
ui/views/view.cc:904: gfx::Rect local_bounds(GetLocalBounds());
Lines 904-911 are a new addition since the last time you looked at this
algorithm (I realized that I needed to perform this check while writing new test
cases).

Would you prefer that I factor out the common code here and at lines 883-888?

[sky, 2013-10-30 14:03:47]

https://codereview.chromium.org/22891016/diff/537001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/22891016/diff/537001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1833: #if defined(OS_CHROMEOS)
This should be TOOLKIT_VIEWS

https://codereview.chromium.org/22891016/diff/537001/ui/views/views_switches.h
File ui/views/views_switches.h (right):

https://codereview.chromium.org/22891016/diff/537001/ui/views/views_switches....
ui/views/views_switches.h:17: #if defined(OS_CHROMEOS)
I wouldn't bother ifdefing this now.

[tdanderson, 2013-10-30 20:55:47]

Scott, can you please take another look? I have addressed your latest comments
and have also tested this on Win Aura. After making a small change in
root_view.cc, it appears to work fine.

https://codereview.chromium.org/22891016/diff/537001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/22891016/diff/537001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1833: #if defined(OS_CHROMEOS)
On 2013/10/30 14:03:49, sky wrote:
> This should be TOOLKIT_VIEWS

Done.

https://codereview.chromium.org/22891016/diff/537001/ui/views/views_switches.h
File ui/views/views_switches.h (right):

https://codereview.chromium.org/22891016/diff/537001/ui/views/views_switches....
ui/views/views_switches.h:17: #if defined(OS_CHROMEOS)
On 2013/10/30 14:03:49, sky wrote:
> I wouldn't bother ifdefing this now.

Done.

[sky, 2013-10-30 20:57:57]

I'm sure these are fine now. The bigger issue is addressing the
concerns Ben has.

On Wed, Oct 30, 2013 at 1:55 PM,  <tdanderson@chromium.org> wrote:
> Scott, can you please take another look? I have addressed your latest
> comments
> and have also tested this on Win Aura. After making a small change in
> root_view.cc, it appears to work fine.
>
>
>
>
https://codereview.chromium.org/22891016/diff/537001/chrome/browser/about_fla...
> File chrome/browser/about_flags.cc (right):
>
>
https://codereview.chromium.org/22891016/diff/537001/chrome/browser/about_fla...
> chrome/browser/about_flags.cc:1833: #if defined(OS_CHROMEOS)
> On 2013/10/30 14:03:49, sky wrote:
>>
>> This should be TOOLKIT_VIEWS
>
>
> Done.
>
>
>
https://codereview.chromium.org/22891016/diff/537001/ui/views/views_switches.h
> File ui/views/views_switches.h (right):
>
>
https://codereview.chromium.org/22891016/diff/537001/ui/views/views_switches....
> ui/views/views_switches.h:17: #if defined(OS_CHROMEOS)
> On 2013/10/30 14:03:49, sky wrote:
>>
>> I wouldn't bother ifdefing this now.
>
>
> Done.
>
> https://codereview.chromium.org/22891016/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-11-04 22:16:21]

LGTM

[commit-bot: I haz the power, 2013-11-04 23:22:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/22891016/917001

[tdanderson, 2013-11-05 03:11:30]

Committed patchset #23 manually as r232891 (presubmit successful).