Implement elementsFromPoint

Implement elementsFromPoint

This patch implements elementsFromPoint from the following spec:
http://www.w3.org/TR/cssom-view/#extensions-to-the-document-interface

There are two changes in this patch:
1) HitTestResult's rect-based results have been converted into "list-
based" results. This required extracting out a mask for hit testing
with list-based results in HitTestRequest and making sure all
rect-based tests also pass HitTestRequest::ListBased when hit testing
(this is asserted in several places).

2) Document.elementsFromPoint has been implemented in a straightforward
way on top of #1. The test results match IE11 with one exception: IE11
does not ignore pointer-events none but this patch does (implied, but
not specified explicitly in the spec). Until
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27829 is fixed, this
does not work on ShadowRoots, though the patch has been implemented
in TreeScope instead of Document to make adding ShadowRoot support trivial.

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

[pdr., 2015-01-26 05:19:55]

pdr@chromium.org changed reviewers:
+ rbyers@chromium.org

[pdr., 2015-01-27 04:48:04]

On 2015/01/26 at 05:19:55, pdr wrote:
> pdr@chromium.org changed reviewers:
> + rbyers@chromium.org

[pdr., 2015-01-30 22:54:53]

On 2015/01/27 at 04:48:04, pdr wrote:
> On 2015/01/26 at 05:19:55, pdr wrote:
> > pdr@chromium.org changed reviewers:
> > + rbyers@chromium.org

Ping

[Rick Byers, 2015-02-01 23:23:07]

Sorry, didn't realize this was ready for review (saw WIP in subject,
etc.).  I'm just departing a 22 hr flight to Sydney - will look in detail
after I arrive.

Rick
On Jan 30, 2015 5:54 PM, <pdr@chromium.org> wrote:

> On 2015/01/27 at 04:48:04, pdr wrote:
>
>> On 2015/01/26 at 05:19:55, pdr wrote:
>> > pdr@chromium.org changed reviewers:
>> > + rbyers@chromium.org
>>
>
> Ping
>
> https://codereview.chromium.org/869813003/
>

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

[pdr., 2015-02-01 23:31:15]

On 2015/02/01 at 23:23:07, rbyers wrote:
> Sorry, didn't realize this was ready for review (saw WIP in subject,
> etc.).  I'm just departing a 22 hr flight to Sydney - will look in detail
> after I arrive.
> 
> Rick
> On Jan 30, 2015 5:54 PM, <pdr@chromium.org> wrote:
> 
> > On 2015/01/27 at 04:48:04, pdr wrote:
> >
> >> On 2015/01/26 at 05:19:55, pdr wrote:
> >> > pdr@chromium.org changed reviewers:
> >> > + rbyers@chromium.org
> >>
> >
> > Ping
> >
> > https://codereview.chromium.org/869813003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email to blink-reviews+unsubscribe@chromium.org.
> 

No worries. It would be great if we could get this into M42 (branch date listed
in go/chrome-release) but not required.

[pdr., 2015-02-01 23:40:11]

On 2015/02/01 at 23:31:15, pdr wrote:
> On 2015/02/01 at 23:23:07, rbyers wrote:
> > Sorry, didn't realize this was ready for review (saw WIP in subject,
> > etc.).  I'm just departing a 22 hr flight to Sydney - will look in detail
> > after I arrive.
> > 
> > Rick
> > On Jan 30, 2015 5:54 PM, <pdr@chromium.org> wrote:
> > 
> > > On 2015/01/27 at 04:48:04, pdr wrote:
> > >
> > >> On 2015/01/26 at 05:19:55, pdr wrote:
> > >> > pdr@chromium.org changed reviewers:
> > >> > + rbyers@chromium.org
> > >>
> > >
> > > Ping
> > >
> > > https://codereview.chromium.org/869813003/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
email to blink-reviews+unsubscribe@chromium.org.
> > 
> 
> No worries. It would be great if we could get this into M42 (branch date
listed in go/chrome-release) but not required.

I should have added: we are officially targeting M43.

[Rick Byers, 2015-02-04 10:34:04]

Sorry for the delay.  This seems great overall.  I'm quite shocked that it was
this easy.  I'm expecting there to be a non-trivial tail of edge cases though
where the render tree hit test code isn't quite right.  How exhaustively have
you looked for issues?  As I've mentioned before, I don't think the existing
rect-based hit-test code is particularly robust (the way we use it hides all
sorts of issues).  Then again maybe I'm biased due to the extra complexity of
hit-testing rects - the penetrating point case should be much simpler in
practice.

You could probably whip up some stress tests pretty easily.  Eg. a content
script that picks random points on any page (or perhaps just the mouse position)
to sanity-check elementsFromPoint results (eg. against getClientRects or
elemenentFromPoint, etc.).  I don't think we need to go crazy here (a couple
minor edge cases that behave a little different than IE is probably no big
deal), but it would be nice to have some confidence that usage in practice (on
complex real-world pages) is pretty good before shipping this.  WDYT?

https://codereview.chromium.org/869813003/diff/180001/LayoutTests/fast/events...
File LayoutTests/fast/events/elementsFromPoint.html (right):

https://codereview.chromium.org/869813003/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint.html:91: resultText += '#' +
element.id;
I find it hard to tell at a glance whether the test output is correct.  Perhaps
you could also add shouldBe/assert calls that verify that each element is in the
list if and only if the supplied co-ordinates are inside one of the rects
returned by getClientRects and pointer-events is not none?

I'm guessing there might be a bit of a tail of edge cases here, so it would be
nice if the tests had a system for cross-checking results.

If you wanted to, you could also cross-check the order of the returned values by
doing repeated elementFromPoint calls, temporarily setting each return value to
pointer-events: none.  I guess that's a good cross-check in itself - the list
created via that hack should be identical to the elementsFromPoint list, right?

https://codereview.chromium.org/869813003/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint.html:105: elementTestFromPoint(175,
190);
Can you add a case that hits on top of the document scrollbar since that is
called out explicitly in the spec?

Iframes are a sufficiently special-case that I think you need at least some
simple iframe tests.

Have you compared behavior against IE for some other special cases?  Eg. I'd be
interested to see the results for tables, SVG, and inlines with multiple line
boxes.

https://codereview.chromium.org/869813003/diff/180001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:299: // node should only result in a single entry.
are duplicate notes guaranteed to be adjacent, or should we really be using a
set to track all the nodes we've seen?

https://codereview.chromium.org/869813003/diff/180001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:211: HitTestResult hitTestInDocument(const
Document*, int x, int y, HitTestRequest);
I think most places pass HitTestRequest by const ref instead of making a copy
(although with the current implementation it won't make any difference).

https://codereview.chromium.org/869813003/diff/180001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:893: hitType |= HitTestRequest::ListBased;
Perhaps we should require callers to opt-in to list-based explicitly and enforce
it with ASSERTs?  It's kind of subtle that you get list behavior by providing a
width/height (I was certainly confused by that originally).  I don't think there
are many places to update (mainly EventHandler::hitTestResultForGestureEvent,
the TouchDisambiguation case you already updated, and possibly the testing path
via Internals.cpp).

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
File Source/core/rendering/HitTestRequest.h (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
Source/core/rendering/HitTestRequest.h:42: // (e.g., for elementsFromPoint or
rect-based tests).
Is it legal to ask for a list-based hit-test without supplying either an area or
the penetrating flag?

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
Source/core/rendering/HitTestRequest.h:46: Penetrating = 1 << 12,
What does it mean to have a penetrating hit test that's not list-based?

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
File Source/core/rendering/HitTestResult.cpp (left):

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
Source/core/rendering/HitTestResult.cpp:410: ASSERT(isRectBasedTest() &&
other.isRectBasedTest());
why don't you want to assert both are list-based hit-tests?  "Appending" doesn't
really make sense otherwise, right?

Related: I kind of hate that we have m_innerNode etc. for list-based tests - I
think it's value isn't really meaningful (just one of the set in the list). 
I've seen several bugs where we accidental use the value for something and in
edge cases do the wrong thing.

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:1885: ASSERT(!result.isRectBasedTest() ||
request.listBased());
Asserts like this can probably move into HitTestResult rather than be sprinkled
at the different use sites, eh?

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:2072: // If it a list-based test, we can
safely append the temporary result since it might had hit
nit: s/it/it is/

[pdr., 2015-02-17 04:10:14]

Thanks for the review! PTAL

https://codereview.chromium.org/869813003/diff/180001/LayoutTests/fast/events...
File LayoutTests/fast/events/elementsFromPoint.html (right):

https://codereview.chromium.org/869813003/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint.html:91: resultText += '#' +
element.id;
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> I find it hard to tell at a glance whether the test output is correct. 
Perhaps you could also add shouldBe/assert calls that verify that each element
is in the list if and only if the supplied co-ordinates are inside one of the
rects returned by getClientRects and pointer-events is not none?

Done.

> 
> I'm guessing there might be a bit of a tail of edge cases here, so it would be
nice if the tests had a system for cross-checking results.
> 
> If you wanted to, you could also cross-check the order of the returned values
by doing repeated elementFromPoint calls, temporarily setting each return value
to pointer-events: none.  I guess that's a good cross-check in itself - the list
created via that hack should be identical to the elementsFromPoint list, right?

I ran all of our layouttests with elementFromPoint(x, y) stubbed out to return
elementsFromPoint(x, y)[0] and did find one bug: http://crbug.com/459070.
Because this affects all rect-based hit testing, I'd like to fix it in a
followup.

https://codereview.chromium.org/869813003/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint.html:105: elementTestFromPoint(175,
190);
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> Can you add a case that hits on top of the document scrollbar since that is
called out explicitly in the spec?

Done in new elementsFromPoint-invalid-cases.html. This also caught a bug where I
failed to return the HTML element when the empty document was clicked.

> 
> Iframes are a sufficiently special-case that I think you need at least some
simple iframe tests.

Done in new elementsFromPoint-iframes.html.

> 
> Have you compared behavior against IE for some other special cases?  Eg. I'd
be interested to see the results for tables, SVG, and inlines with multiple line
boxes.

I'm not /too/ worried because this patch is largely built on the back of our
stress-tested elementFromPoint (singular) and rect-based touch hit testing code.

https://codereview.chromium.org/869813003/diff/180001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:299: // node should only result in a single entry.
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> are duplicate notes guaranteed to be adjacent, or should we really be using a
set to track all the nodes we've seen?

This is a good question. Because result.listBasedTestResult() above is a
ListHashSet, we'll only have unique nodes in the hit test list. Because some of
the nodes are pseudo or text, we can end up with duplicate entries. For example:

listBasedTestResult: [NodeA, NodeAText, NodeB, NodeAPseudo]

When iterating through this list we end up converting the text and pseudo nodes
to their parent, so effectively the list becomes:

[NodeA, NodeA, NodeB, NodeA]

To match the spec we really do want to have NodeA in the list before and after
NodeB, but we don't want to have NodeA listed twice in a row. The above
algorithm prunes the adjacent NodeA's so we get:

[NodeA, NodeB, NodeA]

This matches IE as well.

https://codereview.chromium.org/869813003/diff/180001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:211: HitTestResult hitTestInDocument(const
Document*, int x, int y, HitTestRequest);
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> I think most places pass HitTestRequest by const ref instead of making a copy
(although with the current implementation it won't make any difference).

Done

https://codereview.chromium.org/869813003/diff/180001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:893: hitType |= HitTestRequest::ListBased;
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> Perhaps we should require callers to opt-in to list-based explicitly and
enforce it with ASSERTs?  It's kind of subtle that you get list behavior by
providing a width/height (I was certainly confused by that originally).  I don't
think there are many places to update (mainly
EventHandler::hitTestResultForGestureEvent, the TouchDisambiguation case you
already updated, and possibly the testing path via Internals.cpp).

Added back! I took this out thinking it would be easier to review, but that was
dumb of me.

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
File Source/core/rendering/HitTestRequest.h (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
Source/core/rendering/HitTestRequest.h:42: // (e.g., for elementsFromPoint or
rect-based tests).
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> Is it legal to ask for a list-based hit-test without supplying either an area
or the penetrating flag?

It's not, I've updated the comment to be a bit more clear here.

I've also added an assert in HitTestRequest's constructor to enforce this.

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
Source/core/rendering/HitTestRequest.h:46: Penetrating = 1 << 12,
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> What does it mean to have a penetrating hit test that's not list-based?

It doesn't make sense. I've renamed this PenetratingList to make this clearer.

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
File Source/core/rendering/HitTestResult.cpp (left):

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
Source/core/rendering/HitTestResult.cpp:410: ASSERT(isRectBasedTest() &&
other.isRectBasedTest());
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> why don't you want to assert both are list-based hit-tests?  "Appending"
doesn't really make sense otherwise, right?

I've added a const HitTestRequest& parameter to append and put an assert that
we're a list in append() instead of a bunch of error-prone asserts like I had
before.

> 
> Related: I kind of hate that we have m_innerNode etc. for list-based tests - I
think it's value isn't really meaningful (just one of the set in the list). 
I've seen several bugs where we accidental use the value for something and in
edge cases do the wrong thing.

I agree :/ I filed crbug.com/451914 for one of these.

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:1885: ASSERT(!result.isRectBasedTest() ||
request.listBased());
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> Asserts like this can probably move into HitTestResult rather than be
sprinkled at the different use sites, eh?

Done.

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:1885: ASSERT(!result.isRectBasedTest() ||
request.listBased());
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> Asserts like this can probably move into HitTestResult rather than be
sprinkled at the different use sites, eh?

Done.

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:2072: // If it a list-based test, we can
safely append the temporary result since it might had hit
On 2015/02/04 at 10:34:03, Rick Byers wrote:
> nit: s/it/it is/

Done.

[pdr., 2015-02-18 22:47:21]

The fix for crbug.com/451914 just landed so I've updated the patch now that we
can pass all the tests.

[pdr., 2015-02-20 06:44:53]

Pre-vacation ping?

[Rick Byers, 2015-02-20 17:00:45]

On 2015/02/20 06:44:53, pdr wrote:
> Pre-vacation ping?

Yep, I started yesterday but unfortunately got interrupted - sorry.  Should be
done soon.

[Rick Byers, 2015-02-20 20:02:24]

Looks great, thanks.  I've got a couple minor outstanding questions / concerns,
but I'm OK landing the patch either way, and since I'm on vacation starting
tomorrow (and failed to complete this review yesterday as I planned) I'll LGTM
this now.  If you want eyes from someone else when I'm away, you can ask bokan@.
 But if there's only minor tweaks feel free to just land (either way I'll look
it over when I'm back and consider follow-up actions).

Thanks for pushing on this!

https://codereview.chromium.org/869813003/diff/180001/LayoutTests/fast/events...
File LayoutTests/fast/events/elementsFromPoint.html (right):

https://codereview.chromium.org/869813003/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint.html:91: resultText += '#' +
element.id;
On 2015/02/17 04:10:13, pdr wrote:
> On 2015/02/04 at 10:34:03, Rick Byers wrote:
> > I find it hard to tell at a glance whether the test output is correct. 
> Perhaps you could also add shouldBe/assert calls that verify that each element
> is in the list if and only if the supplied co-ordinates are inside one of the
> rects returned by getClientRects and pointer-events is not none?
> 
> Done.

Looks like you now verify that you get the expected element, but not what other
elements you're getting or that you aren't getting any incorrect elements
returned.  In addition to checking for the presence of the expected element in
the returned list, I was thinking you could look at each element in the returned
list and verify that it has a client rect surrounding the point you hit.  That
would be a much more rigorous test of the API's behavior.  It's your call
though, it's OK with me as-is if you prefer, but I wasn't trying to suggest you
should weaken your tests expectations...

> I ran all of our layouttests with elementFromPoint(x, y) stubbed out to return
> elementsFromPoint(x, y)[0] and did find one bug: http://crbug.com/459070.
> Because this affects all rect-based hit testing, I'd like to fix it in a
> followup.

Nice.  Sounds good.

https://codereview.chromium.org/869813003/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint.html:105: elementTestFromPoint(175,
190);
> > 
> > Have you compared behavior against IE for some other special cases?  Eg. I'd
> be interested to see the results for tables, SVG, and inlines with multiple
line
> boxes.
> 
> I'm not /too/ worried because this patch is largely built on the back of our
> stress-tested elementFromPoint (singular) and rect-based touch hit testing
code.

Heh heh, that's exactly why I AM worried :-).  I know we paper over all sorts of
issues with the rect-based touch hit testing code because we always use the
resulting point in a new point-based hit-test.  But regardless it's not like
you're likely to break a bunch of existing websites with this new API, so I'm
probably being overly paranoid.

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
File Source/core/rendering/HitTestRequest.h (right):

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
Source/core/rendering/HitTestRequest.h:42: // (e.g., for elementsFromPoint or
rect-based tests).
On 2015/02/17 04:10:13, pdr wrote:
> On 2015/02/04 at 10:34:03, Rick Byers wrote:
> > Is it legal to ask for a list-based hit-test without supplying either an
area
> or the penetrating flag?
> 
> It's not, I've updated the comment to be a bit more clear here.
> 
> I've also added an assert in HitTestRequest's constructor to enforce this.

That enforces the penetrating part, but does anything enforce that you can't ask
for simple list-based with an area of 0?  Either we should enforce that, or just
make it work and return a single-element list.  The latter would avoid adding
new complexity to a few of your callsites  (the hit type could return to being
non-conditional - always list based, area may or may not be zero).

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
File Source/core/rendering/HitTestResult.cpp (left):

https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
Source/core/rendering/HitTestResult.cpp:410: ASSERT(isRectBasedTest() &&
other.isRectBasedTest());
On 2015/02/17 04:10:14, pdr wrote:
> On 2015/02/04 at 10:34:03, Rick Byers wrote:
> > why don't you want to assert both are list-based hit-tests?  "Appending"
> doesn't really make sense otherwise, right?
> 
> I've added a const HitTestRequest& parameter to append and put an assert that
> we're a list in append() instead of a bunch of error-prone asserts like I had
> before.

Seems reasonable, thanks.

I was naively thinking we could tell from the result whether it was list-based
(without having the request), but of course an empty result looks the same
whether it's list-based or not.  I don't see any good reason why the result
shouldn't keep a copy of the request (it has the location, which is logically as
much a part of the "request" as the HitTestRequest is).  If we did that, I think
it would clean some of this up (since we used to be able to tell just from the
result if it was expected to produce a list or not).  I can do that in a
follow-up CL though if you think it's a reasonable idea but don't want to mix it
in here.

> > 
> > Related: I kind of hate that we have m_innerNode etc. for list-based tests -
I
> think it's value isn't really meaningful (just one of the set in the list). 
> I've seen several bugs where we accidental use the value for something and in
> edge cases do the wrong thing.
> 
> I agree :/ I filed crbug.com/451914 for one of these.

https://codereview.chromium.org/869813003/diff/260001/LayoutTests/fast/events...
File LayoutTests/fast/events/elementsFromPoint-iframes.html (right):

https://codereview.chromium.org/869813003/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint-iframes.html:22: overflow-y: scroll;
great test case - I've also seen hit-testing problems specific to subframe
scrollbars...

https://codereview.chromium.org/869813003/diff/260001/LayoutTests/fast/events...
File LayoutTests/fast/events/elementsFromPoint-invalid-cases-expected.txt
(right):

https://codereview.chromium.org/869813003/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint-invalid-cases-expected.txt:5: PASS
document.elementsFromPoint(786, 1).length is 0
is this number really stable?  It depends on the width of scrollbars, which I
thought could be different on different versions of MacOS (well scrollbars are
definitely different, not sure if the width ever is).  You could enable mock
scrollbars with the internals API to reduce the risk if platform-specific
failures (not sure if that affects the validity of the test though - probably
shouldn't).

https://codereview.chromium.org/869813003/diff/260001/LayoutTests/fast/events...
File LayoutTests/fast/events/elementsFromPoint-invalid-cases.html (right):

https://codereview.chromium.org/869813003/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint-invalid-cases.html:32: <div
id="beyondTopLeft"></div>
did you intend to use this div somewhere?  Seems like an interesting test
case...

https://codereview.chromium.org/869813003/diff/260001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/869813003/diff/260001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:2620: hitType |= HitTestRequest::ListBased;
This function it intended to be used only with a non-empty radius (seems weird
to add logic that results in working on an empty list when radius is 0). 
Perhaps just bail out early on zero radius?  We could assert non-zero except I
believe clusterFuzz will fuzz calls to window.internals APIs (at least I know it
fuzzes calls to eventSender).

[pdr., 2015-02-22 22:56:07]

New patchsets have been uploaded after l-g-t-m from rbyers@chromium.org

[pdr., 2015-02-22 23:28:14]

https://codereview.chromium.org/869813003/diff/260001/LayoutTests/fast/events...
File LayoutTests/fast/events/elementsFromPoint-invalid-cases-expected.txt
(right):

https://codereview.chromium.org/869813003/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint-invalid-cases-expected.txt:5: PASS
document.elementsFromPoint(786, 1).length is 0
On 2015/02/20 at 20:02:24, Rick Byers (Out until 3-3) wrote:
> is this number really stable?  It depends on the width of scrollbars, which I
thought could be different on different versions of MacOS (well scrollbars are
definitely different, not sure if the width ever is).  You could enable mock
scrollbars with the internals API to reduce the risk if platform-specific
failures (not sure if that affects the validity of the test though - probably
shouldn't).

Done

https://codereview.chromium.org/869813003/diff/260001/LayoutTests/fast/events...
File LayoutTests/fast/events/elementsFromPoint-invalid-cases.html (right):

https://codereview.chromium.org/869813003/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/elementsFromPoint-invalid-cases.html:32: <div
id="beyondTopLeft"></div>
On 2015/02/20 at 20:02:24, Rick Byers (Out until 3-3) wrote:
> did you intend to use this div somewhere?  Seems like an interesting test
case...

It's used in the last testcase where we try to hittest (-10, -10) and verify
nothing is returned.

https://codereview.chromium.org/869813003/diff/260001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/869813003/diff/260001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:2620: hitType |= HitTestRequest::ListBased;
On 2015/02/20 at 20:02:24, Rick Byers (Out until 3-3) wrote:
> This function it intended to be used only with a non-empty radius (seems weird
to add logic that results in working on an empty list when radius is 0). 
Perhaps just bail out early on zero radius?  We could assert non-zero except I
believe clusterFuzz will fuzz calls to window.internals APIs (at least I know it
fuzzes calls to eventSender).

Done

[pdr., 2015-02-22 23:31:55]

On 2015/02/20 at 20:02:24, rbyers wrote:
>
https://codereview.chromium.org/869813003/diff/180001/Source/core/rendering/H...
> Source/core/rendering/HitTestResult.cpp:410: ASSERT(isRectBasedTest() &&
other.isRectBasedTest());
> On 2015/02/17 04:10:14, pdr wrote:
> > On 2015/02/04 at 10:34:03, Rick Byers wrote:
> > > why don't you want to assert both are list-based hit-tests?  "Appending"
> > doesn't really make sense otherwise, right?
> > 
> > I've added a const HitTestRequest& parameter to append and put an assert
that
> > we're a list in append() instead of a bunch of error-prone asserts like I
had
> > before.
> 
> Seems reasonable, thanks.
> 
> I was naively thinking we could tell from the result whether it was list-based
(without having the request), but of course an empty result looks the same
whether it's list-based or not.  I don't see any good reason why the result
shouldn't keep a copy of the request (it has the location, which is logically as
much a part of the "request" as the HitTestRequest is).  If we did that, I think
it would clean some of this up (since we used to be able to tell just from the
result if it was expected to produce a list or not).  I can do that in a
follow-up CL though if you think it's a reasonable idea but don't want to mix it
in here.

This CL is getting a little big so I filed crbug.com/460804 to do this as a
followup.

[pdr., 2015-02-23 19:31:09]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-02-23 19:31:57]

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

[commit-bot: I haz the power, 2015-02-23 20:09:39]

Committed patchset #16 (id:300001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190686