Fix hit testing to be aware of inert inlines.

Fix hit testing to be aware of inert inlines.

This patch does the following. 
- Makes InlineBox::visibleToHitTestRequest use
RenderObject::visibleToHitTestRequest, so that inertness is checked.
Otherwise, hit testing succeeds on an InlineFlowBox inside an inert
renderer testing its line boxes in RenderBlock::hitTestContents, for
example, a TEXT node inside a DIV.
- Makes RenderObject::isInert find the first element node ancestor and
check its inertness. Otherwise, an anonymous renderer or a renderer for
a non-element node like RenderText is always considered non-inert and
hit testing succeeds on it.

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

[falken, 2013-05-31 12:58:44]

Julien or Morita-san, could you review this patch please?

It's a continuation of
https://src.chromium.org/viewvc/blink?view=rev&revision=151223

[Hajime Morrita, 2013-06-03 03:28:35]

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Inli...
File Source/core/rendering/InlineBox.h (right):

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Inli...
Source/core/rendering/InlineBox.h:280: bool visibleToHitTestRequest(const
HitTestRequest& request) const { return renderer()->style()->visibility() ==
VISIBLE && (request.ignorePointerEventsNone() ||
renderer()->style()->pointerEvents() != PE_NONE) && !renderer()->isInert(); }
Can we factored out this somehow to getrid of duplicate between InlineBox and
RenderObject?

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Rend...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Rend...
Source/core/rendering/RenderObject.cpp:2959: bool RenderObject::isInert() const
Having these kind of loops here smells wrong, especially
- hit testing is tree traversal and can be hot operation
- in most of the case the result is false.

Can we have some fast paths here?
For example, it seems that this is always false if document dosn't have any
active dialog.

[falken, 2013-06-03 03:47:55]

Thanks.

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Inli...
File Source/core/rendering/InlineBox.h (right):

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Inli...
Source/core/rendering/InlineBox.h:280: bool visibleToHitTestRequest(const
HitTestRequest& request) const { return renderer()->style()->visibility() ==
VISIBLE && (request.ignorePointerEventsNone() ||
renderer()->style()->pointerEvents() != PE_NONE) && !renderer()->isInert(); }
On 2013/06/03 03:28:35, morrita1 wrote:
> Can we factored out this somehow to getrid of duplicate between InlineBox and
> RenderObject?

Yeah it should be possible.

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Rend...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Rend...
Source/core/rendering/RenderObject.cpp:2959: bool RenderObject::isInert() const
On 2013/06/03 03:28:35, morrita1 wrote:
> Having these kind of loops here smells wrong, especially
> - hit testing is tree traversal and can be hot operation
> - in most of the case the result is false.
> 
> Can we have some fast paths here?
> For example, it seems that this is always false if document dosn't have any
> active dialog.

Right, I had thought about a fast path like that but I figured it's at most a
couple hops to the nearest ancestor element node, and someday isInert should
support the 'inert' attribute. Well, I'll try experimenting with a fast path and
see if I can measure an improvement.

[falken, 2013-06-04 04:33:26]

Morita-san, PTAL.

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Inli...
File Source/core/rendering/InlineBox.h (right):

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Inli...
Source/core/rendering/InlineBox.h:280: bool visibleToHitTestRequest(const
HitTestRequest& request) const { return renderer()->style()->visibility() ==
VISIBLE && (request.ignorePointerEventsNone() ||
renderer()->style()->pointerEvents() != PE_NONE) && !renderer()->isInert(); }
On 2013/06/03 03:47:55, falken wrote:
> On 2013/06/03 03:28:35, morrita1 wrote:
> > Can we factored out this somehow to getrid of duplicate between InlineBox
and
> > RenderObject?
> 
> Yeah it should be possible.
> 

Done.

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Rend...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/16240007/diff/2001/Source/core/rendering/Rend...
Source/core/rendering/RenderObject.cpp:2959: bool RenderObject::isInert() const
On 2013/06/03 03:47:55, falken wrote:
> On 2013/06/03 03:28:35, morrita1 wrote:
> > Having these kind of loops here smells wrong, especially
> > - hit testing is tree traversal and can be hot operation
> > - in most of the case the result is false.
> > 
> > Can we have some fast paths here?
> > For example, it seems that this is always false if document dosn't have any
> > active dialog.
> 
> Right, I had thought about a fast path like that but I figured it's at most a
> couple hops to the nearest ancestor element node, and someday isInert should
> support the 'inert' attribute. Well, I'll try experimenting with a fast path
and
> see if I can measure an improvement.

Through some quick benchmarking, I don't think isInert meaningfully
impacts hit testing performance. visibleToHitTesting does take some
hit but end-to-end it doesn't seem to matter. Specifically, on my
machine 10,000 iterations of visibleToHitTesting increases from ~10ms
(if isInert is not called) to about 20-30ms, but no impact is seen on
my benchmark of mouse movement at
https://codereview.chromium.org/16347002/
(not really in a state to be committed to the tree).

[Hajime Morrita, 2013-06-04 04:42:40]

lgtm

[commit-bot: I haz the power, 2013-06-04 05:30:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/16240007/10001

[commit-bot: I haz the power, 2013-06-04 06:50:47]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-06-04 07:19:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/16240007/10001

[eseidel, 2013-06-04 07:24:55]

I find the concept of "inert" elements confusing.  I guess these are
non-interactive wrt editing?

[falken, 2013-06-04 07:30:54]

On 2013/06/04 07:24:55, eseidel wrote:
> I find the concept of "inert" elements confusing.  I guess these are
> non-interactive wrt editing?

These actually should be unresponsive to any user interaction. Inert nodes is a
new concept added for modal <dialog>:
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#inert

Maybe I should add a comment above Element::isInert.

[eseidel, 2013-06-04 07:43:01]

Hmmm... This is an inherited property of the DOM element.  Feels a bit like rare
data.  I'm not sure who should be responsible for keeping this up to date.

We almost need a non-style resolve phase which keeps track of these kind of
bools on renderrs.  Otherwise you end up doing a whole bunch of tree walking
during hit testing, no?

[eseidel, 2013-06-04 07:44:45]

https://codereview.chromium.org/16240007/diff/10001/Source/core/rendering/Inl...
File Source/core/rendering/InlineBox.h (right):

https://codereview.chromium.org/16240007/diff/10001/Source/core/rendering/Inl...
Source/core/rendering/InlineBox.h:280: bool visibleToHitTestRequest(const
HitTestRequest& request) const { return
renderer()->visibleToHitTestRequest(request); }
Does this introduce an ancestor tree walk into hit-testing?  Wouldn't that make
hit-testing then O(N) based on the depth of the tree?  I'm extrapolating from an
imagined naive impelmentation.  That may not actually be a concern for your
patch at alll!

[falken, 2013-06-04 07:57:37]

On 2013/06/04 07:44:45, eseidel wrote:
>
https://codereview.chromium.org/16240007/diff/10001/Source/core/rendering/Inl...
> File Source/core/rendering/InlineBox.h (right):
> 
>
https://codereview.chromium.org/16240007/diff/10001/Source/core/rendering/Inl...
> Source/core/rendering/InlineBox.h:280: bool visibleToHitTestRequest(const
> HitTestRequest& request) const { return
> renderer()->visibleToHitTestRequest(request); }
> Does this introduce an ancestor tree walk into hit-testing?  Wouldn't that
make
> hit-testing then O(N) based on the depth of the tree?  I'm extrapolating from
an
> imagined naive impelmentation.  That may not actually be a concern for your
> patch at alll!

Yes, it walks up the ancestors. However, if no modal dialog is open,
Element::isInert
will just return false. So most of the time, it just walks to the nearest
element node ancestor.
(More details in comment #4 above)

But yes it might mean hit testing is slow when a modal dialog is open.

Hmm.. my plan was to just implement naively and see if there's a performance
impact. To
maintain a flag in rare data, I imagine the tricky part would be to keep all
nodes properly updated whenever a modal dialog opens/closes or a node moves in
the DOM
tree. Do you think the naive O(n) approach is a dead-end?

[commit-bot: I haz the power, 2013-06-04 08:33:27]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[eseidel, 2013-06-04 08:50:48]

How does it short-circuit out in the no-open-dialog case?

[eseidel, 2013-06-04 08:51:06]

I think the naive approach is fine, so long as the non-dialog case is fast.

[falken, 2013-06-05 01:51:51]

On 2013/06/04 08:51:06, eseidel wrote:
> I think the naive approach is fine, so long as the non-dialog case is fast.

Yes it should be fast. Element::isInert() short-circuits when
document()->activeModalDialog() returns NULL, which is O(1).
But I'll have to revisit this later for implementing the 'inert' attribute

[commit-bot: I haz the power, 2013-06-05 01:56:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/16240007/10001

[commit-bot: I haz the power, 2013-06-05 02:20:45]

Change committed as 151797