Bug 80847 – Extend HitTestResult to support not "discarding" shadow content in favor of its DOM ancestor

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antonio Gomes]

Right now, we have in HitTestResult.cpp the following code:

564 bool HitTestResult::addNodeToRectBasedTestResult(Node* node, const LayoutPoint& pointInContainer, const IntRect& rect)
565 {
...
574 
575     node = node->shadowAncestorNode();
576     mutableRectBasedTestResult().add(node);
... }

So we unconditionally add the shadow ancestor node to the results, if the original rect hit-test'ed node is in a shadow tree.

However for touch accuracy purposes, we might want to add the shadow nodes themselves into the result vector. For example, consider <video> and <audio> elements with built-in controls, when you touch either the volume or playback sliders (input type=range), or the play/pause button (input type=button), client of this class might want to handle them independently from its containing media element.

Patch coming...

[Antonio Gomes]

Created attachment 131962 [details]
patch v1

[Build Bot]

Comment on attachment 131962 [details]
patch v1

Attachment 131962 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11958309

[Early Warning System Bot]

Comment on attachment 131962 [details]
patch v1

Attachment 131962 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11954405

[Early Warning System Bot]

Comment on attachment 131962 [details]
patch v1

Attachment 131962 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11951529

[Build Bot]

Comment on attachment 131962 [details]
patch v1

Attachment 131962 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11955390

[Build Bot]

Comment on attachment 131962 [details]
patch v1

Attachment 131962 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11957372

[Antonio Gomes]

Created attachment 131981 [details]
patch v1.1 - for EWS to build

[Build Bot]

Comment on attachment 131981 [details]
patch v1.1 - for EWS to build

Attachment 131981 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11953638

[WebKit Review Bot]

Comment on attachment 131981 [details]
patch v1.1 - for EWS to build

Attachment 131981 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11954500

New failing tests:
fast/dom/nodesFromRect-shadowContent.html

[Dimitri Glazkov (Google)]

(In reply to comment #0)
> Right now, we have in HitTestResult.cpp the following code:
> 
> 564 bool HitTestResult::addNodeToRectBasedTestResult(Node* node, const LayoutPoint& pointInContainer, const IntRect& rect)
> 565 {
> ...
> 574 
> 575     node = node->shadowAncestorNode();
> 576     mutableRectBasedTestResult().add(node);
> ... }
> 
> So we unconditionally add the shadow ancestor node to the results, if the original rect hit-test'ed node is in a shadow tree.
> 
> However for touch accuracy purposes, we might want to add the shadow nodes themselves into the result vector. For example, consider <video> and <audio> elements with built-in controls, when you touch either the volume or playback sliders (input type=range), or the play/pause button (input type=button), client of this class might want to handle them independently from its containing media element.

What do you mean by "client of this class" here?

The notion that we should have some differences in how hit testing is handled depending on whether a DOM element in shadow DOM or not smells really, really bad to me.

I would really like to better understand the problem first.

> 
> Patch coming...

[Antonio Gomes]

> What do you mean by "client of this class" here?

Maybe I explained it wrong. 'Clients' is methods calling HitTestResults, for rect hittest purposes.

> The notion that we should have some differences in how hit testing is handled depending on whether a DOM element in shadow DOM or not smells really, really bad to me.

We are not hit testing differently dom and shadow content content. Shadow content is always part of the hit test system, but we do not add them to the "resulting" vector. Patch allows this flexibility.

There is no change in the hit testing system itself here at all, but instead on the set of nodes returned when a rect hittest is being performed.

Does it better explain the idea or or your still feel bad about it?

[Dimitri Glazkov (Google)]

(In reply to comment #11)
> > What do you mean by "client of this class" here?
> 
> Maybe I explained it wrong. 'Clients' is methods calling HitTestResults, for rect hittest purposes.
> 
> > The notion that we should have some differences in how hit testing is handled depending on whether a DOM element in shadow DOM or not smells really, really bad to me.
> 
> We are not hit testing differently dom and shadow content content. Shadow content is always part of the hit test system, but we do not add them to the "resulting" vector. Patch allows this flexibility.
> 
> There is no change in the hit testing system itself here at all, but instead on the set of nodes returned when a rect hittest is being performed.
> 
> Does it better explain the idea or or your still feel bad about it?

I see.  Why do we need this as an option then? Why can't it just be on by default? Does this make the shadow content leak out to a public-facing API?

[Antonio Gomes]

> > Does it better explain the idea or or your still feel bad about it?
> 
> I see.  Why do we need this as an option then? Why can't it just be on by default? Does this make the shadow content leak out to a public-facing API?

I was trying to make it optional so "users" of this extension can decide, Dimitri. It has to set a value to this parameter (since there is not default value set to it), so it is really up to the caller the granularity it needs.

I also tried to be analogy to the point-based hit hit test in this case: one can rather accept shadow content or not, depending on what it needs it for.

I have some use cases in mind that would not need any shadow content, for example...

[Antonio Gomes]

Created attachment 132132 [details]
to get win symbols from ews

[Build Bot]

Comment on attachment 132132 [details]
to get win symbols from ews

Attachment 132132 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11961007

[WebKit Review Bot]

Comment on attachment 132132 [details]
to get win symbols from ews

Attachment 132132 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11954837

New failing tests:
fast/dom/nodesFromRect-shadowContent.html

[WebKit Review Bot]

Comment on attachment 132132 [details]
to get win symbols from ews

Attachment 132132 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11956872

New failing tests:
fast/dom/nodesFromRect-shadowContent.html

[Antonio Gomes]

Created attachment 135347 [details]
(committed r113329, r=dhyatt) patch v1.2

[Rob Buis]

Comment on attachment 135347 [details]
(committed r113329, r=dhyatt) patch v1.2

View in context: https://bugs.webkit.org/attachment.cgi?id=135347&action=review

> LayoutTests/fast/dom/nodesFromRect-shadowContent-expected.txt:2
> +PASS All correct nodes found for rect

Why twice PASS?

> LayoutTests/fast/dom/nodesFromRect-shadowContent-expected.txt:3
> +This test only runs in DRT!

Would be better to only print this when DRT in not available.

[Antonio Gomes]

(In reply to comment #19)
> (From update of attachment 135347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135347&action=review
> 
> > LayoutTests/fast/dom/nodesFromRect-shadowContent-expected.txt:2
> > +PASS All correct nodes found for rect
> 
> Why twice PASS?

+      check(clickX, clickY, 10, 10, 20, 20, [e.v1, e.body]);
+
+      var shadow =['-webkit-media-controls-timeline-container', '-webkit-media-controls-play-button', '-webkit-media-controls-panel', '-webkit-media-controls'];
+      checkShadowContent(clickX, clickY, 10, 10, 20, 20, shadow);


one from 'check', one from 'checkShadowContent'.
 
> > LayoutTests/fast/dom/nodesFromRect-shadowContent-expected.txt:3
> > +This test only runs in DRT!
> 
> Would be better to only print this when DRT in not available.

Ok, can fix it before landing...

[Dave Hyatt]

Comment on attachment 135347 [details]
(committed r113329, r=dhyatt) patch v1.2

r=me

[Antonio Gomes]

Comment on attachment 135347 [details]
(committed r113329, r=dhyatt) patch v1.2

pushing manually to watch the bots...

[Antonio Gomes]

not sure if Qt port is able to run tests that make use of <video> content. CC'ing Ossy as an attempt to not cause any unneeded bot redness.

[Csaba Osztrogonác]

(In reply to comment #23)
> not sure if Qt port is able to run tests that make use of <video> content. CC'ing Ossy as an attempt to not cause any unneeded bot redness.

Good question ... All media tests are skipped now, because they were very flakey long time ago. I don't know if anybody experimented with it after skipping.

[Antonio Gomes]

Comment on attachment 135347 [details]
(committed r113329, r=dhyatt) patch v1.2

$ git svn dcommit 
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	ChangeLog
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/dom/nodesFromRect-shadowContent-expected.txt
	A	LayoutTests/fast/dom/nodesFromRect-shadowContent.html
	M	LayoutTests/fast/dom/resources/nodesFromRect.js
	M	LayoutTests/platform/chromium/test_expectations.txt
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/WebCore.exp.in
	M	Source/WebCore/dom/Document.cpp
	M	Source/WebCore/dom/Document.h
	M	Source/WebCore/page/EventHandler.cpp
	M	Source/WebCore/rendering/HitTestResult.cpp
	M	Source/WebCore/rendering/HitTestResult.h
	M	Source/WebCore/rendering/RenderImage.cpp
	M	Source/WebCore/rendering/RenderLayer.cpp
	M	Source/WebCore/testing/Internals.cpp
	M	Source/WebCore/testing/Internals.h
	M	Source/WebCore/testing/Internals.idl
	M	Source/WebKit/qt/Api/qwebpage.cpp
	M	Source/WebKit2/win/WebKit2.def
	M	Source/autotools/symbols.filter
Committed r113329

[Antonio Gomes]

(In reply to comment #24)
> (In reply to comment #23)
> > not sure if Qt port is able to run tests that make use of <video> content. CC'ing Ossy as an attempt to not cause any unneeded bot redness.
> 
> Good question ... All media tests are skipped now, because they were very flakey long time ago. I don't know if anybody experimented with it after skipping.

Lets give it a try.

[Antonio Gomes]

Created attachment 135877 [details]
(committed r113358) line left over after a rebase before landing

[Antonio Gomes]

reopening for cq.

[Antonio Gomes]

Test fails on lion: http://build.webkit.org/results/Lion%20Release%20%28Tests%29/r113330%20%287311%29/results.html

Looking...

[Antonio Gomes]

(In reply to comment #29)
> Test fails on lion: http://build.webkit.org/results/Lion%20Release%20%28Tests%29/r113330%20%287311%29/results.html
> 
> Looking...

speculative fix: http://svn.webkit.org/repository/webkit/trunk@113371