Canvas2d: fix event.region being null on mouseleave/out events

Canvas2d: fix event.region being null on mouseleave/out events

In the current implementation of canvas hit region, the event.region always has
null in case of mouseleave/mouseout. Because the mouse cursor is already off
boundary of hit region when mouseleave/mouseout event happens.
To solve this problem, keeps the region in mouseenter/mouseover event and then
takes the value in mouseout/mouseleave event.

BUG=592992
Committed: https://crrev.com/282ff5518bc121cfe77a29ddd2b9a465b74daf37
Cr-Commit-Position: refs/heads/master@{#387915}

[zino, 2016-03-18 04:44:15]

Description was changed from

==========
Canvas2d: Fix the bug that event.region is always null.

BUG=582792
==========

to

==========
Canvas2d: Fix the bug that event.region is always null.

BUG=592992
==========

[zino, 2016-03-18 05:13:22]

Description was changed from

==========
Canvas2d: Fix the bug that event.region is always null.

BUG=592992
==========

to

==========
Canvas2d: Fix the bug that event.region is always null.

In the current implementation of canvas hit region, the event.region always has
null in case of mouseleave/mouseout. Because the mouse cursor is already off
boundary of hit region when mouseleave/mouseout event happens.
To solve this problem, keeps the region in mouseenter/mouseover event and then
takes the value in mouseout/mouseleave event.

BUG=592992
==========

[zino, 2016-03-18 05:13:46]

jinho.bang@samsung.com changed reviewers:
+ junov@chromium.org, rbyers@chromium.org

[zino, 2016-03-18 05:14:37]

PTAL (I'm not sure if this is a right way)

[Rick Byers, 2016-03-18 18:23:48]

rbyers@chromium.org changed reviewers:
+ dtapuska@chromium.org
- rbyers@chromium.org

[Rick Byers, 2016-03-18 18:31:51]

On 2016/03/18 05:14:37, zino wrote:
> PTAL (I'm not sure if this is a right way)

Hmm, unfortunately I don't think this is right.  When the mouse moves over a few
elements, it will send multiple mouseenter events, potentially with different
regions.  If all the mouseleave events have the region corresponding only to the
last mouseenter event there may be an imbalance.

Dave / Navid, can you guys take a look at this together and try to figure out
what the right behavior & implementation should be?  This is part of a new
feature "Canvas hit regions":
https://html.spec.whatwg.org/multipage/scripting.html#hit-regions which is sort
of like shadow DOM for canvas.  I think the DOM events that get sent to
"control" elements should be balanced (as they would be for normal DOM
elements).  It's possible there's really no need to have a Region in
enter/leave/over/out events, in which case we should file a spec issue and leave
it empty.  I'm not sure - some research is required I think...

[Navid Zolghadr, 2016-03-18 19:08:14]

On 2016/03/18 18:31:51, Rick Byers wrote:
> On 2016/03/18 05:14:37, zino wrote:
> > PTAL (I'm not sure if this is a right way)
> 
> Hmm, unfortunately I don't think this is right.  When the mouse moves over a
few
> elements, it will send multiple mouseenter events, potentially with different
> regions.  If all the mouseleave events have the region corresponding only to
the
> last mouseenter event there may be an imbalance.
> 
> Dave / Navid, can you guys take a look at this together and try to figure out
> what the right behavior & implementation should be?  This is part of a new
> feature "Canvas hit regions":
> https://html.spec.whatwg.org/multipage/scripting.html#hit-regions which is
sort
> of like shadow DOM for canvas.  I think the DOM events that get sent to
> "control" elements should be balanced (as they would be for normal DOM
> elements).  It's possible there's really no need to have a Region in
> enter/leave/over/out events, in which case we should file a spec issue and
leave
> it empty.  I'm not sure - some research is required I think...

After reading the link I'm still a bit fuzzy on the meaning of region here.
Based on your code review it seems that you want to save the region for when we
send mouseover/enter and send the same region when you send out/leave to the
same element. The question here is that what happens to the enter/leave that we
send to the element and it parents? Do they all have the same region as your
patch suggests? 

Although I highly doubt that I understood what you want to do here, assuming
what I said is what you wanted, I don't see the relation of region with every
pointer id (suggested by the map variable you have here). Don't you need one
region for mouse?  Maybe two: one for out and one for leave?

Mouse events are generated by pen, touch, and mouse device itself. The
m_nodeUnderMouse in EventHandler variable stores when the mouse is after dealing
with all different types of input. So I guess maybe keeping the record of one(or
two) regions beside that m_nodeUnderMouse would make sense for your use case and
using it when we created the PlatformMouseEvent in the first place in
EventHandler before passing it to the sendNodeTransitionEvents function in
PointerEventManager?

[zino, 2016-04-05 10:17:22]

jinho.bang@samsung.com changed reviewers:
+ rbyers@chromium.org

[zino, 2016-04-05 10:18:01]

I uploaded a new patch set.

What do you think, reviewers?

[dtapuska, 2016-04-05 15:57:28]

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:193: String
getIdFromControl(Element*);
Is there a requirement that Element be a ptr. Perhaps it would be better to be a
const reference like the isSupportedInteractiveCanvasFallback API..

Can this method be const as well?

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.h (right):

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.h:90:
PlatformMouseEvent mouseEventWithRegion(Node*, const PlatformMouseEvent&);
This seems like something that could just be in the anonymous namespace as it
doesn't require a reference to the PointerEventManager does it?

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:613:
HitRegion* hitRegion = m_hitRegionManager->getHitRegionByControl(element);
I'd make this all one line.

if (HitRegion* hitRegion = m_hitRegionManager->getHitRegionByControl(element))

[Rick Byers, 2016-04-05 16:01:35]

On 2016/04/05 10:18:01, zino wrote:
> I uploaded a new patch set.
> 
> What do you think, reviewers?

This design looks really nice to me and addresses my concerns, thanks! 
Basically rather than record extra state like in the previous patch, you're
using the knowledge of which element the event is going to to lookup the region.
 This makes sense as long as we don't need to worry about multiple hit-regions
with the same 'control' element having different IDs (looks like that's an
existing assumption, probably an edge case that's not really worth worrying
about at this point, should maybe be called out in the spec).

I'll leave the detailed review to Navid and Dave.

[zino, 2016-04-06 11:08:20]

Thank you for review.

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:193: String
getIdFromControl(Element*);
On 2016/04/05 15:57:28, dtapuska wrote:
> Is there a requirement that Element be a ptr. Perhaps it would be better to be
a
> const reference like the isSupportedInteractiveCanvasFallback API..
> 
> Can this method be const as well?

In the current implementation, we are saving the control elements as pointer
type. So, we will finally have to use pointer type to get a hit region
representing control. If needed, we can refactor the related codes to not use
pointer type in follow-up CL.

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.h (right):

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.h:90:
PlatformMouseEvent mouseEventWithRegion(Node*, const PlatformMouseEvent&);
On 2016/04/05 15:57:28, dtapuska wrote:
> This seems like something that could just be in the anonymous namespace as it
> doesn't require a reference to the PointerEventManager does it?

good point.

Done.

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:613:
HitRegion* hitRegion = m_hitRegionManager->getHitRegionByControl(element);
On 2016/04/05 15:57:28, dtapuska wrote:
> I'd make this all one line.
> 
> if (HitRegion* hitRegion = m_hitRegionManager->getHitRegionByControl(element))

Done.

[dtapuska, 2016-04-06 13:41:25]

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:193: String
getIdFromControl(Element*);
On 2016/04/06 11:08:20, zino wrote:
> On 2016/04/05 15:57:28, dtapuska wrote:
> > Is there a requirement that Element be a ptr. Perhaps it would be better to
be
> a
> > const reference like the isSupportedInteractiveCanvasFallback API..
> > 
> > Can this method be const as well?
> 
> In the current implementation, we are saving the control elements as pointer
> type. So, we will finally have to use pointer type to get a hit region
> representing control. If needed, we can refactor the related codes to not use
> pointer type in follow-up CL.

That doesn't preclude you from taking the address of the member down in the
HitRegionManager. I'm just looking for a consistent API. It the very least it
should be a const Element*

[zino, 2016-04-06 16:25:39]

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1815093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:193: String
getIdFromControl(Element*);
On 2016/04/06 13:41:25, dtapuska wrote:
> On 2016/04/06 11:08:20, zino wrote:
> > On 2016/04/05 15:57:28, dtapuska wrote:
> > > Is there a requirement that Element be a ptr. Perhaps it would be better
to
> be
> > a
> > > const reference like the isSupportedInteractiveCanvasFallback API..
> > > 
> > > Can this method be const as well?
> > 
> > In the current implementation, we are saving the control elements as pointer
> > type. So, we will finally have to use pointer type to get a hit region
> > representing control. If needed, we can refactor the related codes to not
use
> > pointer type in follow-up CL.
> 
> That doesn't preclude you from taking the address of the member down in the
> HitRegionManager. I'm just looking for a consistent API. It the very least it
> should be a const Element*

Done.

But I used |const_cast| when calling getHitRegionByControl() in
CanvasRenderingContext2D::getIdFromControl().

[Justin Novosad, 2016-04-06 18:18:26]

https://codereview.chromium.org/1815093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/1815093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:112: virtual
String getIdFromControl(const Element* element) { ASSERT_NOT_REACHED(); return
String(); }
I think the code would be cleaner if you removed the ASSERT_NOT_REACHED() here,
and then you could remove the is2d() check in
HTMLCanvasElement::getIdFromControl.

[zino, 2016-04-07 02:08:16]

Thank you for review.

https://codereview.chromium.org/1815093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/1815093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:112: virtual
String getIdFromControl(const Element* element) { ASSERT_NOT_REACHED(); return
String(); }
On 2016/04/06 18:18:26, Justin Novosad wrote:
> I think the code would be cleaner if you removed the ASSERT_NOT_REACHED()
here,
> and then you could remove the is2d() check in
> HTMLCanvasElement::getIdFromControl.

Done.

But I'm a little bit confused.
The following thing is your old comment in other patch.
https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...

What's difference between this patch and old patch?

[zino, 2016-04-08 15:14:46]

Ping junov@ and dtapuska@

[dtapuska, 2016-04-08 15:18:31]

https://codereview.chromium.org/1815093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1815093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:613: if
(HitRegion* hitRegion =
m_hitRegionManager->getHitRegionByControl(const_cast<Element*>(element)))
ugh this const_cast is ugly. Why does the hit region manager need a non-const
Element* does it mutate the object?

[zino, 2016-04-08 16:34:56]

On 2016/04/08 15:18:31, dtapuska wrote:
>
https://codereview.chromium.org/1815093002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
> (right):
> 
>
https://codereview.chromium.org/1815093002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:613:
if
> (HitRegion* hitRegion =
> m_hitRegionManager->getHitRegionByControl(const_cast<Element*>(element)))
> ugh this const_cast is ugly. Why does the hit region manager need a non-const
> Element* does it mutate the object?

Yes, I know it’s ugly. I misunderstood something.
To take const parameter, I thought I had to change ‘control’ member of HitRegion
into const.
It can’t be const because element can be updated by accessibility apis.
On the other hand, 'controlMap’ member of HitRegionManager can have const key
because it is only used for querying HitRegion. (In other words, updating
element doesn’t happen.)
So, your comment is reasonable.
Thank you. I’ll upload a new patch set soon.

[Justin Novosad, 2016-04-08 19:46:28]

On 2016/04/07 02:08:16, zino wrote:
> Thank you for review.
> 
>
https://codereview.chromium.org/1815093002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
> (right):
> 
>
https://codereview.chromium.org/1815093002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:112:
virtual
> String getIdFromControl(const Element* element) { ASSERT_NOT_REACHED(); return
> String(); }
> On 2016/04/06 18:18:26, Justin Novosad wrote:
> > I think the code would be cleaner if you removed the ASSERT_NOT_REACHED()
> here,
> > and then you could remove the is2d() check in
> > HTMLCanvasElement::getIdFromControl.
> 
> Done.
> 
> But I'm a little bit confused.
> The following thing is your old comment in other patch.
>
https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
> 
> What's difference between this patch and old patch?

In that old patch, the base class implementation was not meant as fallback. The
method is deliberately not pure virtual to avoid having to implement it in
classes where it is not relevant. The only reason it was necessary to define the
base implementation is to respect syntax, to prevent a compile error from not
defining a method that is not pure virtual.  Since the method was not meant to
be called, adding ASSERT_NOT_REACHED() made sense.  We should always put
ASSERT_NOT_REACH in places that are not reachable, and therefore have no test
coverage.

In this case, having a base class implementation that is meant to be called
makes sense and simplifies the code.

[zino, 2016-04-11 06:20:09]

Thank you for review.

PTAL.

[zino, 2016-04-12 23:58:30]

ping reviewers

[dtapuska, 2016-04-13 01:02:13]

lgtm % I don't see a test case that asserts region is null for mouse events
under certain conditions. It always seems to be asserting it is button.  Is
there one?

[Rick Byers, 2016-04-13 17:55:35]

RS LGTM

junov@ any other concerns from your side?

[Rick Byers, 2016-04-13 17:56:05]

Description was changed from

==========
Canvas2d: Fix the bug that event.region is always null.

In the current implementation of canvas hit region, the event.region always has
null in case of mouseleave/mouseout. Because the mouse cursor is already off
boundary of hit region when mouseleave/mouseout event happens.
To solve this problem, keeps the region in mouseenter/mouseover event and then
takes the value in mouseout/mouseleave event.

BUG=592992
==========

to

==========
Canvas2d: fix event.region being null on mouseleave/out events

In the current implementation of canvas hit region, the event.region always has
null in case of mouseleave/mouseout. Because the mouse cursor is already off
boundary of hit region when mouseleave/mouseout event happens.
To solve this problem, keeps the region in mouseenter/mouseover event and then
takes the value in mouseout/mouseleave event.

BUG=592992
==========

[zino, 2016-04-14 11:04:02]

On 2016/04/13 01:02:13, dtapuska wrote:
> lgtm % I don't see a test case that asserts region is null for mouse events
> under certain conditions. It always seems to be asserting it is button.  Is
> there one?

Done.

Thank you for review.

[zino, 2016-04-15 17:31:44]

Ping Justin

[Justin Novosad, 2016-04-18 13:53:04]

On 2016/04/15 17:31:44, zino wrote:
> Ping Justin

lgtm

[zino, 2016-04-18 14:53:47]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2016-04-18 14:53:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1815093002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-04-18 14:54:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815093002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815093002/120001

[commit-bot: I haz the power, 2016-04-18 15:52:09]

Description was changed from

==========
Canvas2d: fix event.region being null on mouseleave/out events

In the current implementation of canvas hit region, the event.region always has
null in case of mouseleave/mouseout. Because the mouse cursor is already off
boundary of hit region when mouseleave/mouseout event happens.
To solve this problem, keeps the region in mouseenter/mouseover event and then
takes the value in mouseout/mouseleave event.

BUG=592992
==========

to

==========
Canvas2d: fix event.region being null on mouseleave/out events

In the current implementation of canvas hit region, the event.region always has
null in case of mouseleave/mouseout. Because the mouse cursor is already off
boundary of hit region when mouseleave/mouseout event happens.
To solve this problem, keeps the region in mouseenter/mouseover event and then
takes the value in mouseout/mouseleave event.

BUG=592992
==========

[commit-bot: I haz the power, 2016-04-18 15:52:10]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-18 15:53:10]

Description was changed from

==========
Canvas2d: fix event.region being null on mouseleave/out events

In the current implementation of canvas hit region, the event.region always has
null in case of mouseleave/mouseout. Because the mouse cursor is already off
boundary of hit region when mouseleave/mouseout event happens.
To solve this problem, keeps the region in mouseenter/mouseover event and then
takes the value in mouseout/mouseleave event.

BUG=592992
==========

to

==========
Canvas2d: fix event.region being null on mouseleave/out events

In the current implementation of canvas hit region, the event.region always has
null in case of mouseleave/mouseout. Because the mouse cursor is already off
boundary of hit region when mouseleave/mouseout event happens.
To solve this problem, keeps the region in mouseenter/mouseover event and then
takes the value in mouseout/mouseleave event.

BUG=592992

Committed: https://crrev.com/282ff5518bc121cfe77a29ddd2b9a465b74daf37
Cr-Commit-Position: refs/heads/master@{#387915}
==========

[commit-bot: I haz the power, 2016-04-18 15:53:11]

Patchset 7 (id:??) landed as
https://crrev.com/282ff5518bc121cfe77a29ddd2b9a465b74daf37
Cr-Commit-Position: refs/heads/master@{#387915}