Canvas2d: Implement rerouting event by hit region's control.

Canvas2d: Implement rerouting event by hit region's control.

According to the spec[1], should support rerouting event if there is a control
on hit region. This CL is also includes fixing two spec issues[2][3] and test.

[1] https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerouting-steps
[2] https://github.com/whatwg/html/issues/547
[3] https://github.com/whatwg/html/issues/548

BUG=579076, 579614, 582792
Committed: https://crrev.com/f18571e9097d0533f6fc18d62b3ab006e1d6c6d4
Cr-Commit-Position: refs/heads/master@{#380870}

[zino, 2016-01-31 15:51:34]

Description was changed from

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,582792
==========

to

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,582792
==========

[zino, 2016-01-31 15:51:35]

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

[zino, 2016-01-31 15:58:42]

I'm working on implementing rerouting event part in hit region feature.
This is still WIP CL but welcome your comments.
If I'm going in the wrong direction, please correct me.

[zino, 2016-01-31 16:02:38]

Description was changed from

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,582792
==========

to

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: If the region is empty, should return null.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,582792
==========

[zino, 2016-01-31 16:03:40]

Description was changed from

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: If the region is empty, should return null.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,582792
==========

to

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: If the region is empty, should return null.
TODO: Remove legacy codes.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,582792
==========

[philipj_slow, 2016-02-01 04:05:20]

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1023: bool
HTMLCanvasElement::dispatchEventInternal(PassRefPtrWillBeRawPtr<Event> event)
Will this happen for both UA-created and script-created events? I'm trying to
understand how this will be expressed in the spec, and if this will really
correspond to patching hit testing, or if we're now inside DOM's
dispatchEvent().

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1027: target =
m_context->getControlAndUpdateEvent(*event);
Might it be more clear to just return the target here and modify the event
outside of getControlAndUpdateEvent? I think so :)

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1029: target = this;
Shouldn't this fall back to the parent class's dispatchEventInternal? Seems like
the easiest way to guarantee no change in behavior if there is no region.

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1805:
MouseEvent& e = toMouseEvent(event);
https://www.chromium.org/blink/coding-style#TOC-Names asks for full names in
general.

[Justin Novosad, 2016-02-01 16:51:43]

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1808:
document.updateLayoutTreeForNodeIfNeeded(canvas());
Don't forget write a test that requires this update.

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1814:
localPos.scale(canvas()->width() / box->contentWidth(), canvas()->height() /
box->contentHeight());
don't forget test cases that cover CSS zooming and hidpi (use
testRunner.setBackingScaleFactor)

[zino, 2016-02-04 13:32:13]

Description was changed from

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: If the region is empty, should return null.
TODO: Remove legacy codes.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,582792
==========

to

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,582792
==========

[zino, 2016-02-04 13:32:53]

Patchset #2 (id:20001) has been deleted

[zino, 2016-02-04 14:21:46]

Patchset #2 (id:40001) has been deleted

[zino, 2016-02-04 14:46:09]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-04 14:47:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1654653002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1654653002/80001

[zino, 2016-02-04 14:48:42]

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

[zino, 2016-02-04 14:50:07]

Description was changed from

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,582792
==========

to

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,579614,582792
==========

[zino, 2016-02-05 06:46:48]

Reviewers,

Sorry for delay. I'm writing layout tests but
I'm staying my hometown for now(Lunar new year's day in our country).
So, my work progress is slow. (Because I'm working at public hosting server
without my own pc)
Please execuse me.
Anyway, I'll finish this work(only remaining tests) until this weekend.

Thank you.

[zino, 2016-02-05 06:48:54]

Description was changed from

==========
[WIP] Canvas2d: Implement rerouting mouse event by hit region's control.

Should support rerouting mouse event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,579614,582792
==========

to

==========
[WIP] Canvas2d: Implement rerouting event by hit region's control.

Should support rerouting event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,579614,582792
==========

[zino, 2016-02-05 06:59:48]

I have to write tests for this change.

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1023: bool
HTMLCanvasElement::dispatchEventInternal(PassRefPtrWillBeRawPtr<Event> event)
On 2016/02/01 04:05:19, philipj_UTC7 wrote:
> Will this happen for both UA-created and script-created events? I'm trying to
> understand how this will be expressed in the spec, and if this will really
> correspond to patching hit testing, or if we're now inside DOM's
> dispatchEvent().

In the new patch set, I filtered script-created events out.
I think we avoid hit-testing in case of script-created events. (It can be
initialized from MouseEventInit dictionary).

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1027: target =
m_context->getControlAndUpdateEvent(*event);
On 2016/02/01 04:05:19, philipj_UTC7 wrote:
> Might it be more clear to just return the target here and modify the event
> outside of getControlAndUpdateEvent? I think so :)

I agree with you but performance is also important.
So, I thought it is better to avoid to get hit-region if possible.
If we split the function into two parts(getControl and updateEvent), we will
have to get hit-region twice.
So, Instead, I introduced getControlAndIDIfHitRegionExists() in the new patch
set.

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1029: target = this;
On 2016/02/01 04:05:19, philipj_UTC7 wrote:
> Shouldn't this fall back to the parent class's dispatchEventInternal? Seems
like
> the easiest way to guarantee no change in behavior if there is no region.

Sure!

Thank you.

Done.

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1805:
MouseEvent& e = toMouseEvent(event);
On 2016/02/01 04:05:20, philipj_UTC7 wrote:
> https://www.chromium.org/blink/coding-style#TOC-Names asks for full names in
> general.

Done.

[philipj_slow, 2016-02-05 07:30:06]

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1654653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1027: target =
m_context->getControlAndUpdateEvent(*event);
On 2016/02/05 06:59:48, zino wrote:
> On 2016/02/01 04:05:19, philipj_UTC7 wrote:
> > Might it be more clear to just return the target here and modify the event
> > outside of getControlAndUpdateEvent? I think so :)
> 
> I agree with you but performance is also important.
> So, I thought it is better to avoid to get hit-region if possible.
> If we split the function into two parts(getControl and updateEvent), we will
> have to get hit-region twice.
> So, Instead, I introduced getControlAndIDIfHitRegionExists() in the new patch
> set.

Ack, std::pair looks like a nice fit.

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1040:
std::pair<Element*, String>
HTMLCanvasElement::getControlAndIDIfHitRegionExists(const LayoutPoint& location)
Should probably be Id (not ID) for consistency. I defer to canvas owners,
though.

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1042: if (m_context &&
m_context->is2d() && m_context->hitRegionsCount() > 0)
It's a bit odd to both have an is2d() test and a virtual call, is this a
necessary optimization or could the m_context check be moved out to
dispatchEventInternal and the gitRegionsCount() check into the implementation,
so that this helper vanishes?

[Justin Novosad, 2016-02-05 15:22:53]

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

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:112: virtual
std::pair<Element*, String> getControlAndIDIfHitRegionExists(const LayoutPoint&
location) { return std::make_pair(nullptr, String()); }
I think this default implementation (and the all the other ones that should
never be invoked on a WebGL canvas) should have ASSERT_NOT_REACHED();  As was
done in the WebGL section below.

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

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1803:
document.updateLayoutTreeForNodeIfNeeded(canvas());
Updating layout... Is this the right thing to do?  My concerns are that the
updated layout is less likely to represent the layout that was presented on
screen when mouse or touch event was registered.  I am also concerned about how
this might affect performance because it may trigger multiple layout passes per
animation frame. On the other hand, I think updating the layout maybe the
desired behavior when dealing with synthetic events. For example if a we have
some JS code that makes DOM or style changes before dispatching a synthetic
mouse event, one would probably expect the event to take the DOM changes into
account.

@philipj, rbyers: What do you think?

[Rick Byers, 2016-02-05 16:01:20]

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

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1803:
document.updateLayoutTreeForNodeIfNeeded(canvas());
On 2016/02/05 15:22:53, Justin Novosad wrote:
> Updating layout... Is this the right thing to do?  My concerns are that the
> updated layout is less likely to represent the layout that was presented on
> screen when mouse or touch event was registered.  I am also concerned about
how
> this might affect performance because it may trigger multiple layout passes
per
> animation frame. On the other hand, I think updating the layout maybe the
> desired behavior when dealing with synthetic events. For example if a we have
> some JS code that makes DOM or style changes before dispatching a synthetic
> mouse event, one would probably expect the event to take the DOM changes into
> account.
> 
> @philipj, rbyers: What do you think?

I agree we absolutely don't want to force any extra layouts - that could be
disasterous for performance.  We're working to try to align input with the
document lifecycle to make input-event-induced layouts more predictable (ideally
unnecessary) also.

I haven't looked over the entire CL in detail yet, but I _think_ this code
should only need to be called when creating a real mouse or touch event, just
after that does a hit test (which also requires layout to be up-to-date).

We could try to assert that layout isn't dirty, but I think a better approach is
to update the signature to make it clear we're not really going to re-do any
sort of hit-test here.  In particular, instead of taking a LayoutPoint, we could
take a HitTestResult (and then use it's m_localPoint for getting the region).

Doing that would require a bit more plumbing, but it looks easy in the
TouchEvent case.  The MouseEvent case looks pretty hacky right now (plugging
into event dispatch), which would make taking a HitTestResult harder.  Ideally
that should be reworked to be similar to the TouchEvent case.  That is, hooked
in at the point of event generation in EventHandler, not trying to modify the
event at dispatch time.  If we can do that, then again taking a HitTestResult
should be easy.

Then the case for JS dispatching a synthetic result is very analogous.  Either
the JS code already has some context from which to get the 'target', 'region'
etc. for the event constructor, or it does it's own hit-test
(document.elementFromPoint) and computes all the event constructor arguments
based on that.

[zino, 2016-02-10 16:32:03]

Description was changed from

==========
[WIP] Canvas2d: Implement rerouting event by hit region's control.

Should support rerouting event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,579614,582792
==========

to

==========
[WIP] Canvas2d: Implement rerouting event by hit region's control.

Should support rerouting event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: Write tests.
TODO: Remove updating layout if possible.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,579614,582792
==========

[zino, 2016-02-11 03:38:51]

Patchset #6 (id:140001) has been deleted

[zino, 2016-02-11 18:01:10]

Description was changed from

==========
[WIP] Canvas2d: Implement rerouting event by hit region's control.

Should support rerouting event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: Write tests.
TODO: Remove updating layout if possible.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,579614,582792
==========

to

==========
[WIP] Canvas2d: Implement rerouting event by hit region's control.

Should support rerouting event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,579614,582792
==========

[zino, 2016-02-12 14:59:12]

Description was changed from

==========
[WIP] Canvas2d: Implement rerouting event by hit region's control.

Should support rerouting event if there is a control on hit region.

** These TODO list will be removed when this is finished once. **
TODO: If the control is not canvas fallback element, do not allow rerouting
event.
TODO: Write tests.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,579614,582792
==========

to

==========
Canvas2d: Implement rerouting event by hit region's control.

Should support rerouting event if there is a control on hit region.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,579614,582792
==========

[zino, 2016-02-12 15:21:40]

Description was changed from

==========
Canvas2d: Implement rerouting event by hit region's control.

Should support rerouting event if there is a control on hit region.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...

BUG=579076,579614,582792
==========

to

==========
Canvas2d: Implement rerouting event by hit region's control.

According to the spec[1], should support rerouting event if there is a control
on hit region. This CL is also includes fixing two spec issues[2][3] and test.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...
[2] https://github.com/whatwg/html/issues/547
[3] https://github.com/whatwg/html/issues/548

BUG=579076,579614,582792
==========

[zino, 2016-02-12 15:22:22]

Patchset #4 (id:100001) has been deleted

[zino, 2016-02-12 15:25:30]

Patchset #4 (id:120001) has been deleted

[zino, 2016-02-12 15:25:37]

Patchset #4 (id:160001) has been deleted

[zino, 2016-02-12 15:25:45]

Patchset #4 (id:180001) has been deleted

[zino, 2016-02-12 15:26:19]

Please take a look again.

Thank you.

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1040:
std::pair<Element*, String>
HTMLCanvasElement::getControlAndIDIfHitRegionExists(const LayoutPoint& location)
On 2016/02/05 07:30:06, philipj_OOO_til_Feb15 wrote:
> Should probably be Id (not ID) for consistency. I defer to canvas owners,
> though.

I think it's better to "Id" than "ID".

Done.

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

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:112: virtual
std::pair<Element*, String> getControlAndIDIfHitRegionExists(const LayoutPoint&
location) { return std::make_pair(nullptr, String()); }
On 2016/02/05 15:22:53, Justin Novosad wrote:
> I think this default implementation (and the all the other ones that should
> never be invoked on a WebGL canvas) should have ASSERT_NOT_REACHED();  As was
> done in the WebGL section below.

Done.

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

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1803:
document.updateLayoutTreeForNodeIfNeeded(canvas());
Probably done.

[Justin Novosad, 2016-02-12 15:41:59]

lgtm for modules/canvas2d and core/html

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

https://codereview.chromium.org/1654653002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1803:
document.updateLayoutTreeForNodeIfNeeded(canvas());
On 2016/02/12 15:26:19, zino wrote:
> Probably done.

I think this is correct (layout "IfNeeded"). But I would like to hear from other
reviewers before committing.

[philipj_slow, 2016-02-15 10:45:25]

Rick said "In particular, instead of taking a LayoutPoint, we could take a
HitTestResult (and then use it's m_localPoint for getting the region)"

I don't think what's currently in EventTarget quite matches that idea, have you
looked into it?

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Touch.h (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Touch.h:51: const FloatSize& radius, float
rotationAngle, float force, String region = String())
Why give region a default when no other argument has a default? Can you update
the callers instead?

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/MouseEvent.h (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/MouseEvent.h:113: const String& region =
String());
Is the default needed? Update callers?

[zino, 2016-02-15 12:35:20]

On 2016/02/15 10:45:25, philipj_UTC7 wrote:
> Rick said "In particular, instead of taking a LayoutPoint, we could take a
> HitTestResult (and then use it's m_localPoint for getting the region)"
> 
> I don't think what's currently in EventTarget quite matches that idea, have
you
> looked into it?
> 

Yes,

We can use the localPoint() in some place but it's hard to apply in some place.
For example, the dispatchMouseEvent() and updatePointerTargetAndDispatchEvents()
don't take HitTestResult currently.
Then we have to pass it from caller (or caller of caller .. caller). I'm not
sure if the work is valuable.

Even if we could pass the HitTestResult, we would still have to convert the
point to canvas coordinates and do hit-test to check whether the point is in
canvas hit region.

[zino, 2016-02-15 12:54:35]

On 2016/02/15 12:35:20, zino wrote:
> On 2016/02/15 10:45:25, philipj_UTC7 wrote:
> > Rick said "In particular, instead of taking a LayoutPoint, we could take a
> > HitTestResult (and then use it's m_localPoint for getting the region)"
> > 
> > I don't think what's currently in EventTarget quite matches that idea, have
> you
> > looked into it?
> > 
> 
> Yes,
> 
> We can use the localPoint() in some place but it's hard to apply in some
place.
> For example, the dispatchMouseEvent() and
updatePointerTargetAndDispatchEvents()
> don't take HitTestResult currently.
> Then we have to pass it from caller (or caller of caller .. caller). I'm not
> sure if the work is valuable.
> 
> Even if we could pass the HitTestResult, we would still have to convert the
> point to canvas coordinates and do hit-test to check whether the point is in
> canvas hit region.

And I also moved old code from dispatchEventInternal() to EventHandler.cpp.
The change is based on Rick's idea and we can avoid unnecessary hit-test from
now. (e.g. generated mouse events from gesturetapevent)

[philipj_slow, 2016-02-16 09:52:27]

On 2016/02/15 12:54:35, zino wrote:
> On 2016/02/15 12:35:20, zino wrote:
> > On 2016/02/15 10:45:25, philipj_UTC7 wrote:
> > > Rick said "In particular, instead of taking a LayoutPoint, we could take a
> > > HitTestResult (and then use it's m_localPoint for getting the region)"
> > > 
> > > I don't think what's currently in EventTarget quite matches that idea,
have
> > you
> > > looked into it?
> > > 
> > 
> > Yes,
> > 
> > We can use the localPoint() in some place but it's hard to apply in some
> place.
> > For example, the dispatchMouseEvent() and
> updatePointerTargetAndDispatchEvents()
> > don't take HitTestResult currently.
> > Then we have to pass it from caller (or caller of caller .. caller). I'm not
> > sure if the work is valuable.
> > 
> > Even if we could pass the HitTestResult, we would still have to convert the
> > point to canvas coordinates and do hit-test to check whether the point is in
> > canvas hit region.
> 
> And I also moved old code from dispatchEventInternal() to EventHandler.cpp.
> The change is based on Rick's idea and we can avoid unnecessary hit-test from
> now. (e.g. generated mouse events from gesturetapevent)

Thanks! This all looks reasonable to me, but if you don't mind I'd like Rick to
have the final say, and he'll be back in two days it looks like.

[zino, 2016-02-16 09:55:03]

On 2016/02/16 09:52:27, philipj_UTC7 wrote:
> Thanks! This all looks reasonable to me, but if you don't mind I'd like Rick
to
> have the final say, and he'll be back in two days it looks like.

Sure!

Thank you for your review again.

and I'll wait for his review.

Thanks.

[Rick Byers, 2016-02-23 20:26:09]

Sorry it took me awhile to review this - finally spent a few hours on it today
(had to get up-to-speed on some of the pre-existing code).

In general this seems really good - now fits with the model defined by the spec.
 Thank you!

Just a few suggestions, hopefully they're relatively straightforward to
implement.

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-event-test.html
(right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-event-test.html:29:
context.addHitRegion({ id : 'button' });
Can you add a test which adds a hit region for an element that isn't a
descendant of the canvas element?  Probably also one that is a descendant at the
time addHitRegion is called but is then moved to some other place in the DOM?

I think those cases are important because it could be really confusing /
breaking to non-hit-region-aware scripts if we ever accidentally sent events
that occurred on the canvas to some unrelated element elsewhere in the DOM
(where they wouldn't bubble up to the canvas element).

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-event-test.html:39:
actual.push(e.type + e.region);
I think you also want to test the event target, right?  'e.target' should be
#button not the canvas element.

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-event-test.html:80:
button.addEventListener('mouseup', function(e) {
You probably want to also check mouseover, mouseout, mouseenter and mouseleave
events.  They may be a little tricky to get right (feel free to file a separate
bug for any that aren't working rather than try to fix it in this CL).

In particular, what is the 'region' of a mouseout/mouseleave event supposed to
be?  The region that was just left?

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Touch.h (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Touch.h:51: const FloatSize& radius, float
rotationAngle, float force, String region = String())
On 2016/02/15 10:45:24, philipj_UTC7 wrote:
> Why give region a default when no other argument has a default? Can you update
> the callers instead?

Yes updating callers is much preferred (helps us remember not to forget to
propagate new bits like this), I don't think there are many...

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/MouseEventInit.idl (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/MouseEventInit.idl:24: // IDL parser can't
support it yet. Please see: http://crbug.com/579896
Since I don't think this is going to happen anytime soon, how about we just make
everything nice and consistent and move the trivial
MouseHitRegion/TouchHitRegion code directly into MouseEvent and TouchEvent?  Now
that it's a simple accessor the implementation is really trivial, probably more
confusion than benefit to trying to have it separated out into modules/canvas2d

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1393:
const_cast<PlatformMouseEvent&>(mouseEvent).setRegion(regionInfo.second);
It's unfortunate to need to rely on a const_cast here - conceptually the
PlatformMouseEvent really should be immutable and I can imagine how we might
have bugs as a result of violating that expectation.

Also you really don't want to have to duplicate this code at each place that
generates a MouseEvent (I think you're missing a few).

Happily I think there's a nice clean solution to both of these issues.  This
step is conceptually part of hit-testing, I think you can do it in a more
centralized place as part of the hit test - Document::prepareMouseEvent.  That
code already creates and returns a copy of the PlatformMouseEvent, so you can
modify it there without violating const.

There are a couple places that use prepareMouseEvent that'll have to be updated
to use the PlatformMouseEvent from the return value (instead of continuing to
use the value supplied) but that looks trivial to me.

In terms of the control, hopefully you can just change the innerNode of the
HitTestResults.  Then everything downstream of hit-testing will behave as if the
input device actually hit that node.

BTW, the spec says: "The control is ignored when it is not a descendant of the
canvas element.)".  Have you enforced that somewhere already?  Do you need to
reverify it every time it's used (in case it has moved)?

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/PlatformMouseEvent.h (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/PlatformMouseEvent.h:110: String m_region;
Conceptually this would probably make more sense to store in HitTestResults
(which is the other half of MouseEventWithHitTestResults).  But after spending
some time looking into the details of implementing that it seems like it's
probably more work than just stuffing it here (there are still a lot of places
we pass a PlatformMouseEvent instead of a MouseEventWithHitTestResults).

So I'm OK with taking a bit of a short-cut here to make this CL more tractable. 
I think it's worth tracking with a comment here though, maybe something like:
"This might make more sense to put in HitTestResults or some other part of
MouseEventWithHitTestResults, but for now it's most convenient to stash it
here.".

[Rick Byers, 2016-02-24 23:16:05]

Oh also I suspect there are a few more cases that may be broken and need tests:
 1) wheel events
 2) gesture events (in particular the mouse events generated as a result of a
gesturetap)
 3) pointer events

I'd suggest you land this CL first though to get the basics.  Those cases should
be a pretty easy extension of this CL.  No need to worry much about them until
you've got the basics working and landed.

[zino, 2016-02-25 17:00:57]

On 2016/02/24 23:16:05, Rick Byers wrote:
> Oh also I suspect there are a few more cases that may be broken and need
tests:
>  1) wheel events
>  2) gesture events (in particular the mouse events generated as a result of a
> gesturetap)
>  3) pointer events
> 
> I'd suggest you land this CL first though to get the basics.  Those cases
should
> be a pretty easy extension of this CL.  No need to worry much about them until
> you've got the basics working and landed.

Hi Rick,

Thank you for your review first.

I have a question. Do we need to support for wheel events, gesture events and
pointer events?
I think your saying is reasonable but I'm not sure if we really need it. (I mean
that use-case looks low?) as well as the spec doesn't mention all of input
events.
If you are saying the mouse events generated as a result of other events (e.g.
gesture tap), I already considered them.

Thank you.!

[zino, 2016-03-02 14:36:42]

On 2016/02/25 17:00:57, zino (OOO until Mar 1) wrote:
> On 2016/02/24 23:16:05, Rick Byers wrote:
> > Oh also I suspect there are a few more cases that may be broken and need
> tests:
> >  1) wheel events
> >  2) gesture events (in particular the mouse events generated as a result of
a
> > gesturetap)
> >  3) pointer events
> > 
> > I'd suggest you land this CL first though to get the basics.  Those cases
> should
> > be a pretty easy extension of this CL.  No need to worry much about them
until
> > you've got the basics working and landed.
> 
> Hi Rick,
> 
> Thank you for your review first.
> 
> I have a question. Do we need to support for wheel events, gesture events and
> pointer events?
> I think your saying is reasonable but I'm not sure if we really need it. (I
mean
> that use-case looks low?) as well as the spec doesn't mention all of input
> events.
> If you are saying the mouse events generated as a result of other events (e.g.
> gesture tap), I already considered them.
> 
> Thank you.!

Ping Rick@

[Rick Byers, 2016-03-02 17:08:31]

On 2016/03/02 14:36:42, zino wrote:
> On 2016/02/25 17:00:57, zino (OOO until Mar 1) wrote:
> > On 2016/02/24 23:16:05, Rick Byers wrote:
> > > Oh also I suspect there are a few more cases that may be broken and need
> > tests:
> > >  1) wheel events
> > >  2) gesture events (in particular the mouse events generated as a result
of
> a
> > > gesturetap)
> > >  3) pointer events
> > > 
> > > I'd suggest you land this CL first though to get the basics.  Those cases
> > should
> > > be a pretty easy extension of this CL.  No need to worry much about them
> until
> > > you've got the basics working and landed.
> > 
> > Hi Rick,
> > 
> > Thank you for your review first.
> > 
> > I have a question. Do we need to support for wheel events, gesture events
and
> > pointer events?
> > I think your saying is reasonable but I'm not sure if we really need it. (I
> mean
> > that use-case looks low?) as well as the spec doesn't mention all of input
> > events.
> > If you are saying the mouse events generated as a result of other events
(e.g.
> > gesture tap), I already considered them.
> > 
> > Thank you.!
> 
> Ping Rick@

Sorry for the delay.  Wheel events and pointer events ARE instances of
MouseEvent and so are covered (IMHO) by the spec.  Maybe they'd just work (or
would require very little effort), but we need tests at least.  You could argue
that they'd be low priority bugs.

I'm not sure how important 'wheel' is - eg. are there accessibility scenarios
that will rely on it for scrolling?  I'm not an expert there, but it at least
seems possible.

Pointer events are definitely important - if mousedown gets re-routed then
pointerdown must too or it may break some scripts.  But then pointer events in
blink haven't shipped yet so this isn't as urgent - doesn't need to block your
shipping (but at least needs a bug assigned to someone, blocking the shipping of
pointer events).  

As for gesture events, yes I mean only the mouse events generated as a result of
a GestureTap.  If that already works correctly that's great (IIRC from reviewing
the code I didn't think your changes were sufficient but I could easily be
wrong), but we need at least a basic test that ensures it's covered.

But again all of this is probably best done in a follow-up CL.  I didn't want to
derail this CL for it, just wanted to set expectations that I don't think it's
quite ready to ship yet (but getting very close).

[zino, 2016-03-08 12:33:48]

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-event-test.html
(right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-event-test.html:29:
context.addHitRegion({ id : 'button' });
On 2016/02/23 20:26:08, Rick Byers wrote:
> Can you add a test which adds a hit region for an element that isn't a
> descendant of the canvas element?  Probably also one that is a descendant at
the
> time addHitRegion is called but is then moved to some other place in the DOM?
> 
> I think those cases are important because it could be really confusing /
> breaking to non-hit-region-aware scripts if we ever accidentally sent events
> that occurred on the canvas to some unrelated element elsewhere in the DOM
> (where they wouldn't bubble up to the canvas element).

Done.

There was a basic test when calling addHitRegion():
fast/canvas/canvas-hit-regions-fallback-element-test.html

I also added a test when event happen if its event target is not descendant of
canvas.

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-event-test.html:39:
actual.push(e.type + e.region);
On 2016/02/23 20:26:08, Rick Byers wrote:
> I think you also want to test the event target, right?  'e.target' should be
> #button not the canvas element.

Done.

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-event-test.html:80:
button.addEventListener('mouseup', function(e) {
On 2016/02/23 20:26:08, Rick Byers wrote:
> You probably want to also check mouseover, mouseout, mouseenter and mouseleave
> events.  They may be a little tricky to get right (feel free to file a
separate
> bug for any that aren't working rather than try to fix it in this CL).
> 
> In particular, what is the 'region' of a mouseout/mouseleave event supposed to
> be?  The region that was just left?

I added the test cases.
But mouseout/mouseleave don't work correctly. I've filed a separate bug and I'll
fix it in follow-up CL.

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Touch.h (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Touch.h:51: const FloatSize& radius, float
rotationAngle, float force, String region = String())
On 2016/02/23 20:26:08, Rick Byers wrote:
> On 2016/02/15 10:45:24, philipj_UTC7 wrote:
> > Why give region a default when no other argument has a default? Can you
update
> > the callers instead?
> 
> Yes updating callers is much preferred (helps us remember not to forget to
> propagate new bits like this), I don't think there are many...

Done.

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/MouseEvent.h (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/MouseEvent.h:113: const String& region =
String());
On 2016/02/15 10:45:24, philipj_UTC7 wrote:
> Is the default needed? Update callers?

Done.

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/MouseEventInit.idl (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/MouseEventInit.idl:24: // IDL parser can't
support it yet. Please see: http://crbug.com/579896
On 2016/02/23 20:26:08, Rick Byers wrote:
> Since I don't think this is going to happen anytime soon, how about we just
make
> everything nice and consistent and move the trivial
> MouseHitRegion/TouchHitRegion code directly into MouseEvent and TouchEvent? 
Now
> that it's a simple accessor the implementation is really trivial, probably
more
> confusion than benefit to trying to have it separated out into
modules/canvas2d

Done.

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1393:
const_cast<PlatformMouseEvent&>(mouseEvent).setRegion(regionInfo.second);
On 2016/02/23 20:26:08, Rick Byers wrote:
> It's unfortunate to need to rely on a const_cast here - conceptually the
> PlatformMouseEvent really should be immutable and I can imagine how we might
> have bugs as a result of violating that expectation.
> 
> Also you really don't want to have to duplicate this code at each place that
> generates a MouseEvent (I think you're missing a few).
> 
> Happily I think there's a nice clean solution to both of these issues.  This
> step is conceptually part of hit-testing, I think you can do it in a more
> centralized place as part of the hit test - Document::prepareMouseEvent.  That
> code already creates and returns a copy of the PlatformMouseEvent, so you can
> modify it there without violating const.

I couldn't find the part that you mentioned "That code already creates and
returns a copy of the PlatformMouseEvent".
In Document::prepareMouseEvent, the PlatformMouseEvent also looks const?

> 
> There are a couple places that use prepareMouseEvent that'll have to be
updated
> to use the PlatformMouseEvent from the return value (instead of continuing to
> use the value supplied) but that looks trivial to me.
> 
> In terms of the control, hopefully you can just change the innerNode of the
> HitTestResults.  Then everything downstream of hit-testing will behave as if
the
> input device actually hit that node.
> 
> BTW, the spec says: "The control is ignored when it is not a descendant of the
> canvas element.)".  Have you enforced that somewhere already?  Do you need to
> reverify it every time it's used (in case it has moved)?

Done.

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/PlatformMouseEvent.h (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/PlatformMouseEvent.h:110: String m_region;
On 2016/02/23 20:26:08, Rick Byers wrote:
> Conceptually this would probably make more sense to store in HitTestResults
> (which is the other half of MouseEventWithHitTestResults).  But after spending
> some time looking into the details of implementing that it seems like it's
> probably more work than just stuffing it here (there are still a lot of places
> we pass a PlatformMouseEvent instead of a MouseEventWithHitTestResults).
> 
> So I'm OK with taking a bit of a short-cut here to make this CL more
tractable. 
> I think it's worth tracking with a comment here though, maybe something like:
> "This might make more sense to put in HitTestResults or some other part of
> MouseEventWithHitTestResults, but for now it's most convenient to stash it
> here.".

Done.

I agree with you. I'll move the region data to HitTestResults or some other
parts in follow-up CL.
I left a comment and filed a bug for now.

[Rick Byers, 2016-03-08 15:57:35]

Did you intend to upload a new CL with the changes you marked as 'Done'?

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1393:
const_cast<PlatformMouseEvent&>(mouseEvent).setRegion(regionInfo.second);
On 2016/03/08 12:33:48, zino wrote:
> On 2016/02/23 20:26:08, Rick Byers wrote:
> > It's unfortunate to need to rely on a const_cast here - conceptually the
> > PlatformMouseEvent really should be immutable and I can imagine how we might
> > have bugs as a result of violating that expectation.
> > 
> > Also you really don't want to have to duplicate this code at each place that
> > generates a MouseEvent (I think you're missing a few).
> > 
> > Happily I think there's a nice clean solution to both of these issues.  This
> > step is conceptually part of hit-testing, I think you can do it in a more
> > centralized place as part of the hit test - Document::prepareMouseEvent. 
That
> > code already creates and returns a copy of the PlatformMouseEvent, so you
can
> > modify it there without violating const.
> 
> I couldn't find the part that you mentioned "That code already creates and
> returns a copy of the PlatformMouseEvent".
> In Document::prepareMouseEvent, the PlatformMouseEvent also looks const?

This code: 
  return MouseEventWithHitTestResults(event, result);
Makes a copy of the PlatformMouseEvent and stores that copy inside the
MouseEventWithHitTestResults.  So this is your chance to modify that copy to be
different from the input.  i.e. replace the above with:
  PlatformMouseEvent eventWithRegion = event;
  eventWithRegion.setRegion(region);
  return MouseEventWithHitTestResults(eventWithRegion, result);
> 
> > 
> > There are a couple places that use prepareMouseEvent that'll have to be
> updated
> > to use the PlatformMouseEvent from the return value (instead of continuing
to
> > use the value supplied) but that looks trivial to me.
> > 
> > In terms of the control, hopefully you can just change the innerNode of the
> > HitTestResults.  Then everything downstream of hit-testing will behave as if
> the
> > input device actually hit that node.
> > 
> > BTW, the spec says: "The control is ignored when it is not a descendant of
the
> > canvas element.)".  Have you enforced that somewhere already?  Do you need
to
> > reverify it every time it's used (in case it has moved)?
> 
> Done.

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/PlatformMouseEvent.h (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/PlatformMouseEvent.h:110: String m_region;
On 2016/03/08 12:33:48, zino wrote:
> On 2016/02/23 20:26:08, Rick Byers wrote:
> > Conceptually this would probably make more sense to store in HitTestResults
> > (which is the other half of MouseEventWithHitTestResults).  But after
spending
> > some time looking into the details of implementing that it seems like it's
> > probably more work than just stuffing it here (there are still a lot of
places
> > we pass a PlatformMouseEvent instead of a MouseEventWithHitTestResults).
> > 
> > So I'm OK with taking a bit of a short-cut here to make this CL more
> tractable. 
> > I think it's worth tracking with a comment here though, maybe something
like:
> > "This might make more sense to put in HitTestResults or some other part of
> > MouseEventWithHitTestResults, but for now it's most convenient to stash it
> > here.".
> 
> Done.
> 
> I agree with you. I'll move the region data to HitTestResults or some other
> parts in follow-up CL.
> I left a comment and filed a bug for now.

Thanks.  Note that I'm not convinced it's worth the effort / code churn to make
that change.  It would be nice to look into / experiment with, see if it's
practical to do cleanly without a TON of work.  But I don't consider it required
- storing it in PlatformMouseEvent isn't THAT bad, just a little bit of a hack.

[zino, 2016-03-09 04:52:24]

Patchset #5 (id:220001) has been deleted

[zino, 2016-03-09 04:55:01]

@Rick,

PTAL.

(Sorry, I tried to upload a new patch set yesterday but unknown network error
happened.)

[Rick Byers, 2016-03-10 21:36:29]

LGTM with nits

Thanks for all the hard work on this - it's a substantial feature and it's
looking quite good to me now!

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-event-test.html
(right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-event-test.html:29:
context.addHitRegion({ id : 'button' });
On 2016/03/08 12:33:48, zino wrote:
> On 2016/02/23 20:26:08, Rick Byers wrote:
> > Can you add a test which adds a hit region for an element that isn't a
> > descendant of the canvas element?  Probably also one that is a descendant at
> the
> > time addHitRegion is called but is then moved to some other place in the
DOM?
> > 
> > I think those cases are important because it could be really confusing /
> > breaking to non-hit-region-aware scripts if we ever accidentally sent events
> > that occurred on the canvas to some unrelated element elsewhere in the DOM
> > (where they wouldn't bubble up to the canvas element).
> 
> Done.
> 
> There was a basic test when calling addHitRegion():
> fast/canvas/canvas-hit-regions-fallback-element-test.html
> 
> I also added a test when event happen if its event target is not descendant of
> canvas.

The tests look excellent now, thanks for the improvements!

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1654653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1393:
const_cast<PlatformMouseEvent&>(mouseEvent).setRegion(regionInfo.second);
On 2016/03/08 15:57:34, Rick Byers wrote:
> On 2016/03/08 12:33:48, zino wrote:
> > On 2016/02/23 20:26:08, Rick Byers wrote:
> > > It's unfortunate to need to rely on a const_cast here - conceptually the
> > > PlatformMouseEvent really should be immutable and I can imagine how we
might
> > > have bugs as a result of violating that expectation.
> > > 
> > > Also you really don't want to have to duplicate this code at each place
that
> > > generates a MouseEvent (I think you're missing a few).
> > > 
> > > Happily I think there's a nice clean solution to both of these issues. 
This
> > > step is conceptually part of hit-testing, I think you can do it in a more
> > > centralized place as part of the hit test - Document::prepareMouseEvent. 
> That
> > > code already creates and returns a copy of the PlatformMouseEvent, so you
> can
> > > modify it there without violating const.
> > 
> > I couldn't find the part that you mentioned "That code already creates and
> > returns a copy of the PlatformMouseEvent".
> > In Document::prepareMouseEvent, the PlatformMouseEvent also looks const?
> 
> This code: 
>   return MouseEventWithHitTestResults(event, result);
> Makes a copy of the PlatformMouseEvent and stores that copy inside the
> MouseEventWithHitTestResults.  So this is your chance to modify that copy to
be
> different from the input.  i.e. replace the above with:
>   PlatformMouseEvent eventWithRegion = event;
>   eventWithRegion.setRegion(region);
>   return MouseEventWithHitTestResults(eventWithRegion, result);
> > 
> > > 
> > > There are a couple places that use prepareMouseEvent that'll have to be
> > updated
> > > to use the PlatformMouseEvent from the return value (instead of continuing
> to
> > > use the value supplied) but that looks trivial to me.
> > > 
> > > In terms of the control, hopefully you can just change the innerNode of
the
> > > HitTestResults.  Then everything downstream of hit-testing will behave as
if
> > the
> > > input device actually hit that node.
> > > 
> > > BTW, the spec says: "The control is ignored when it is not a descendant of
> the
> > > canvas element.)".  Have you enforced that somewhere already?  Do you need
> to
> > > reverify it every time it's used (in case it has moved)?
> > 
> > Done.
> 

This looks really great now, thanks!  I love how you just need to do this work
in two places now (modulo the outstanding bugs) - EventHandler.cpp changes are
nice and clean.  Thanks!

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Touch.idl (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Touch.idl:45:
[RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute DOMString?
region;
nit: add a blank line and comment that references the spec defining this
property.

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/TouchInit.idl (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TouchInit.idl:23: // TODO(zino): This
attribute should be declared as partial dictionary but
nit: remove the TODO now that we're not using partial dictionaries at all here.

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/DragEvent.cpp (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/DragEvent.cpp:49: platformTimeStamp,
syntheticEventType, String())
technically I think this is a bug - drag events are mouse events so the spec
requires they follow canvas hit testing rules also (and.  But dnd events are
pretty rarely used, I doubt anyone is really going to notice here.  Mind filing
a pri=3 bug (just in case anyone goes looking for a known issue)?  I wouldn't
suggest spending your time on fixing it at this point (maybe wait for feedback
once people start using canvas hit regions).

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/MouseEvent.idl (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/MouseEvent.idl:37:
[RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute DOMString?
region;
nit: move to the bottom in a new section with a comment above showing the spec
link (philipj has been updating all the IDL files to use this form - so you can
always see what bits come from what specs, hopefully we'll eventually have a
tool to validate the definitions match).

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/MouseEventInit.idl (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/MouseEventInit.idl:23: // TODO(zino): This
attribute should be declared as partial dictionary but
nit: again, can remove the TODO

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/WheelEvent.cpp (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/WheelEvent.cpp:79: nullptr,
platformTimeStamp, PlatformMouseEvent::RealOrIndistinguishable, String())
as discussed this is a bug, but may not be too important.  Please file a bug for
making wheel events work with canvas hit regions and reference it in a TODO
nearby somewhere.

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1093: if (m_context &&
m_context->is2d() && m_context->hitRegionsCount() > 0)
nit: move the hitRegionsCount check down into implementation of the method on
the context itself (callers shouldn't need to worry about this optimization).

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/PlatformMouseEvent.h (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/PlatformMouseEvent.h:110: // TODO(zion): This
might make more sense to put in HitTestResults or
nit: typo zion/zino  ;-)

[zino, 2016-03-11 09:35:47]

Thank you very much for you review.

I've addressed all of nits but could you confirm this once again before landing?

Thank you!

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Touch.idl (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Touch.idl:45:
[RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute DOMString?
region;
On 2016/03/10 21:36:28, Rick Byers wrote:
> nit: add a blank line and comment that references the spec defining this
> property.

Done.

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/TouchInit.idl (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TouchInit.idl:23: // TODO(zino): This
attribute should be declared as partial dictionary but
On 2016/03/10 21:36:29, Rick Byers wrote:
> nit: remove the TODO now that we're not using partial dictionaries at all
here.

Done.

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/DragEvent.cpp (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/DragEvent.cpp:49: platformTimeStamp,
syntheticEventType, String())
On 2016/03/10 21:36:29, Rick Byers wrote:
> technically I think this is a bug - drag events are mouse events so the spec
> requires they follow canvas hit testing rules also (and.  But dnd events are
> pretty rarely used, I doubt anyone is really going to notice here.  Mind
filing
> a pri=3 bug (just in case anyone goes looking for a known issue)?  I wouldn't
> suggest spending your time on fixing it at this point (maybe wait for feedback
> once people start using canvas hit regions).

Done.

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/MouseEvent.idl (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/MouseEvent.idl:37:
[RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute DOMString?
region;
On 2016/03/10 21:36:29, Rick Byers wrote:
> nit: move to the bottom in a new section with a comment above showing the spec
> link (philipj has been updating all the IDL files to use this form - so you
can
> always see what bits come from what specs, hopefully we'll eventually have a
> tool to validate the definitions match).

Done.

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/MouseEventInit.idl (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/MouseEventInit.idl:23: // TODO(zino): This
attribute should be declared as partial dictionary but
On 2016/03/10 21:36:29, Rick Byers wrote:
> nit: again, can remove the TODO

Done.

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/WheelEvent.cpp (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/WheelEvent.cpp:79: nullptr,
platformTimeStamp, PlatformMouseEvent::RealOrIndistinguishable, String())
On 2016/03/10 21:36:29, Rick Byers wrote:
> as discussed this is a bug, but may not be too important.  Please file a bug
for
> making wheel events work with canvas hit regions and reference it in a TODO
> nearby somewhere.

Done.

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1654653002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1093: if (m_context &&
m_context->is2d() && m_context->hitRegionsCount() > 0)
On 2016/03/10 21:36:29, Rick Byers wrote:
> nit: move the hitRegionsCount check down into implementation of the method on
> the context itself (callers shouldn't need to worry about this optimization).

Done.

[zino, 2016-03-12 00:47:10]

PTAL Rick@ :)

[zino, 2016-03-12 00:47:14]

PTAL Rick@ :)

[Rick Byers, 2016-03-12 01:36:20]

Yep, still LGTM

[zino, 2016-03-12 01:49:18]

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

[zino, 2016-03-12 01:49:19]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1654653002/#ps260001
(title: "addressed nits")

[commit-bot: I haz the power, 2016-03-12 01:49:56]

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

[commit-bot: I haz the power, 2016-03-12 02:39:48]

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

[commit-bot: I haz the power, 2016-03-12 02:39:50]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zino, 2016-03-12 02:45:35]

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

[commit-bot: I haz the power, 2016-03-12 02:46:14]

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

[commit-bot: I haz the power, 2016-03-12 03:55:36]

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

[commit-bot: I haz the power, 2016-03-12 03:55:38]

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

[zino, 2016-03-12 05:59:08]

https://codereview.chromium.org/1654653002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1654653002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1062: WebInputEventResult
eventResult = updatePointerTargetAndDispatchEvents(EventTypeNames::mousedown,
mev.innerNode(), m_clickCount, mev.event());
Fix trybot error:
The prepareMouseEvent() will return copy of PlatformMouseEvent (with Region).
We should use it instead of original PlatforMouseEvent.

[zino, 2016-03-12 05:59:20]

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

[zino, 2016-03-12 05:59:20]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1654653002/#ps280001
(title: "fix bot errors")

[commit-bot: I haz the power, 2016-03-12 05:59:31]

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

[commit-bot: I haz the power, 2016-03-12 08:02:18]

Description was changed from

==========
Canvas2d: Implement rerouting event by hit region's control.

According to the spec[1], should support rerouting event if there is a control
on hit region. This CL is also includes fixing two spec issues[2][3] and test.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...
[2] https://github.com/whatwg/html/issues/547
[3] https://github.com/whatwg/html/issues/548

BUG=579076,579614,582792
==========

to

==========
Canvas2d: Implement rerouting event by hit region's control.

According to the spec[1], should support rerouting event if there is a control
on hit region. This CL is also includes fixing two spec issues[2][3] and test.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...
[2] https://github.com/whatwg/html/issues/547
[3] https://github.com/whatwg/html/issues/548

BUG=579076,579614,582792
==========

[commit-bot: I haz the power, 2016-03-12 08:02:20]

Committed patchset #7 (id:280001)

[commit-bot: I haz the power, 2016-03-12 08:03:44]

Description was changed from

==========
Canvas2d: Implement rerouting event by hit region's control.

According to the spec[1], should support rerouting event if there is a control
on hit region. This CL is also includes fixing two spec issues[2][3] and test.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...
[2] https://github.com/whatwg/html/issues/547
[3] https://github.com/whatwg/html/issues/548

BUG=579076,579614,582792
==========

to

==========
Canvas2d: Implement rerouting event by hit region's control.

According to the spec[1], should support rerouting event if there is a control
on hit region. This CL is also includes fixing two spec issues[2][3] and test.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#canvas-mouseevent-rerou...
[2] https://github.com/whatwg/html/issues/547
[3] https://github.com/whatwg/html/issues/548

BUG=579076,579614,582792

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

[commit-bot: I haz the power, 2016-03-12 08:03:46]

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