Mousemove event should be fired when the mouse stays in the document but there is no element.

Mousemove event should be fired when the mouse stays in the document but there is no element.

BUG=405826
R=jchaffraix@chromium.org
TEST=LayoutTests/fast/events/mouse-events-within-no-element.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182622

[Mike West, 2014-08-21 15:14:34]

I don't know the hit testing code well, jchaffraix@ is probably a better
reviewer.

[Miyoung Shin(g), 2014-08-25 12:52:00]

Julien, Could you take a look ?

[Miyoung Shin(g), 2014-08-26 00:41:18]

Patchset #2 (id:20001) has been deleted

[Miyoung Shin(g), 2014-08-28 08:39:01]

jchaffraix@, PTAL

[Julien - ping for review, 2014-09-08 20:07:41]

Sorry for the delay, I was on vacations when you pinged me the first time and it
took me a while to see the other ping.

https://codereview.chromium.org/494913003/diff/1/LayoutTests/fast/events/reso...
File
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html
(right):

https://codereview.chromium.org/494913003/diff/1/LayoutTests/fast/events/reso...
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html:29:
<script>
This test is super difficult to analyze for several reasons:
- you're dumping only the target but we don't know which frame they belong to
(main frame, iframe?).
- you're using ALERT, which I would recommend to avoid, prefer a console
element. Alternatively console.log but I prefer some DOM dumping.
- mostly, you don't describe *what* you're testing (description) and what a
failing / passing output would look like (expectations).

https://codereview.chromium.org/494913003/diff/1/LayoutTests/fast/events/reso...
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html:38:
x=x+200;
You did a good job at following Blink's style below so these 2 lines should also
follow it for consistency.

[Miyoung Shin(g), 2014-09-11 23:53:29]

https://codereview.chromium.org/494913003/diff/1/LayoutTests/fast/events/reso...
File
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html
(right):

https://codereview.chromium.org/494913003/diff/1/LayoutTests/fast/events/reso...
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html:29:
<script>
On 2014/09/08 20:07:41, Julien Chaffraix - PST wrote:
> This test is super difficult to analyze for several reasons:
> - you're dumping only the target but we don't know which frame they belong to
> (main frame, iframe?).
I'm changing the test case but there is a flaky issue and I'm analyzing it.
it's taking longer than I thought. 
Anyway, I will change it to dumpChildFramesAsText to be easy to know.

> - you're using ALERT, which I would recommend to avoid, prefer a console
> element. Alternatively console.log but I prefer some DOM dumping.
I will change ALERT to shouldBexxxxx 

> - mostly, you don't describe *what* you're testing (description) and what a
> failing / passing output would look like (expectations).
I will add the description in the test case.

https://codereview.chromium.org/494913003/diff/1/LayoutTests/fast/events/reso...
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html:38:
x=x+200;
On 2014/09/08 20:07:41, Julien Chaffraix - PST wrote:
> You did a good job at following Blink's style below so these 2 lines should
also
> follow it for consistency.

I will fix it.

[Miyoung Shin(g), 2014-09-15 14:22:08]

jchaffraix@, I re-uploaded the patch to apply you comment. PTAL

[Julien - ping for review, 2014-09-19 16:56:41]

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/mouse-events-within-no-element.html (right):

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/mouse-events-within-no-element.html:1: <iframe
id="sourceFrame" style="width: 400px; height: 300px;"
src="resources/mouse-events-within-no-element-iframe.html"></iframe>
All tests should have a DOCTYPE unless they are explicitly testing quirks mode.

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/mouse-events-within-no-element.html:6:
description("Tests the mouse event when the mouse event is fired in the document
but there is no body element");
Let's rephrase this as we always have a <body> element per the HTML parsing
specification:

This test checks that mouse events are fired in the document, even outside of
the body element.

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
File
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html
(right):

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html:67:
if (events[i] == eventType) {
I don't think this branch is needed or even a good idea. If we don't match the
expected type, it seems like we should fail the test.

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html:89:
verifyEventLog('mouseleave', '#document');// x, y - 100
x, y + 200?

https://codereview.chromium.org/494913003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/494913003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2414: if (!request.isChildFrameHitTest()
&& ((request.active() || request.release()) || (request.move() &&
hitTestArea.contains(hitPoint.x(), hitPoint.y()))) && isRootLayer()) {
Why do we special case mouse move here? Wouldn't mouse clicks or mouse drags
also work?

[Miyoung Shin(g), 2014-09-20 03:59:43]

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/mouse-events-within-no-element.html (right):

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/mouse-events-within-no-element.html:1: <iframe
id="sourceFrame" style="width: 400px; height: 300px;"
src="resources/mouse-events-within-no-element-iframe.html"></iframe>
On 2014/09/19 16:56:41, Julien Chaffraix - PST wrote:
> All tests should have a DOCTYPE unless they are explicitly testing quirks
mode.

OK. I will add

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/mouse-events-within-no-element.html:6:
description("Tests the mouse event when the mouse event is fired in the document
but there is no body element");
On 2014/09/19 16:56:41, Julien Chaffraix - PST wrote:
> Let's rephrase this as we always have a <body> element per the HTML parsing
> specification:
> 
> This test checks that mouse events are fired in the document, even outside of
> the body element.

OK. I will change.

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
File
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html
(right):

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html:67:
if (events[i] == eventType) {
On 2014/09/19 16:56:41, Julien Chaffraix - PST wrote:
> I don't think this branch is needed or even a good idea. If we don't match the
> expected type, it seems like we should fail the test.

OK. I will change to always check expected eventType and target

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html:89:
verifyEventLog('mouseleave', '#document');// x, y - 100
On 2014/09/19 16:56:41, Julien Chaffraix - PST wrote:
> x, y + 200?

"x, y + 200" is the coordinates out of iframe.
I intended to move the mouse position out of iframe

https://codereview.chromium.org/494913003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/494913003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2414: if (!request.isChildFrameHitTest()
&& ((request.active() || request.release()) || (request.move() &&
hitTestArea.contains(hitPoint.x(), hitPoint.y()))) && isRootLayer()) {
On 2014/09/19 16:56:41, Julien Chaffraix - PST wrote:
> Why do we special case mouse move here? Wouldn't mouse clicks or mouse drags
> also work?

Mouse click is working well because there is already the condition of
request.active() here.
When I investigated this problem at first, I was wondering why mouse click is
working, then I found this condition.
That's the reason why I changed here.

In case of mouse drag, when I tested it, currently it doesn't work, but if I
apply my patch, it works well.

[Miyoung Shin(g), 2014-09-20 04:39:18]

FYI,
Here is the case.
|                                          |
|                .....                     |
|                                          |
|     HTML & Body's Rendering box          |
|__________________________________________|
|button|     x[1]
|______|     ↑
             |___ there isn't any element here.

[Miyoung Shin(g), 2014-09-20 06:09:39]

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
File
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html
(right):

https://codereview.chromium.org/494913003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/resources/mouse-events-within-no-element-iframe.html:89:
verifyEventLog('mouseleave', '#document');// x, y - 100
On 2014/09/20 03:59:43, Miyoung Shin(g) wrote:
> On 2014/09/19 16:56:41, Julien Chaffraix - PST wrote:
> > x, y + 200?
> 
> "x, y + 200" is the coordinates out of iframe.
> I intended to move the mouse position out of iframe

Ah, I got it. this event is fired by "x, y + 200". I will change

[Julien - ping for review, 2014-09-23 14:15:05]

lgtm

https://codereview.chromium.org/494913003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/494913003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2403: // In addtion, the mouse is possible
to stay in document but there is no element.
I don't think we say "the mouse is possible to stay" but "it is possible for the
mouse to stay in the document".

[Miyoung Shin(g), 2014-09-24 01:34:15]

https://codereview.chromium.org/494913003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/494913003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2403: // In addtion, the mouse is possible
to stay in document but there is no element.
On 2014/09/23 14:15:05, Julien Chaffraix - PST wrote:
> I don't think we say "the mouse is possible to stay" but "it is possible for
the
> mouse to stay in the document".

Thank you to disabuse.

[Miyoung Shin(g), 2014-09-24 14:52:54]

The CQ bit was checked by myid.o.shin@gmail.com

[Julien - ping for review, 2014-09-24 14:53:00]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2014-09-24 14:53:01]

lgtm

[commit-bot: I haz the power, 2014-09-24 14:53:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/494913003/80001

[commit-bot: I haz the power, 2014-09-24 18:00:28]

Committed patchset #4 (id:80001) as 182622