Bug 80847 - Extend HitTestResult to support not "discarding" shadow content in favor of its DOM ancestor
Summary: Extend HitTestResult to support not "discarding" shadow content in favor of i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on: 80886
Blocks: 73144
  Show dependency treegraph
 
Reported: 2012-03-12 08:50 PDT by Antonio Gomes
Modified: 2012-04-05 14:30 PDT (History)
7 users (show)

See Also:


Attachments
patch v1 (25.78 KB, patch)
2012-03-14 17:10 PDT, Antonio Gomes
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch v1.1 - for EWS to build (27.45 KB, patch)
2012-03-14 20:35 PDT, Antonio Gomes
buildbot: commit-queue-
Details | Formatted Diff | Diff
to get win symbols from ews (28.01 KB, patch)
2012-03-15 15:13 PDT, Antonio Gomes
buildbot: commit-queue-
Details | Formatted Diff | Diff
(committed r113329, r=dhyatt) patch v1.2 (29.09 KB, patch)
2012-04-03 09:56 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
(committed r113358) line left over after a rebase before landing (1.73 KB, patch)
2012-04-05 12:29 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2012-03-12 08:50:28 PDT
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...
Comment 1 Antonio Gomes 2012-03-14 17:10:58 PDT
Created attachment 131962 [details]
patch v1
Comment 2 Build Bot 2012-03-14 17:41:57 PDT
Comment on attachment 131962 [details]
patch v1

Attachment 131962 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11958309
Comment 3 Early Warning System Bot 2012-03-14 17:52:33 PDT
Comment on attachment 131962 [details]
patch v1

Attachment 131962 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11954405
Comment 4 Early Warning System Bot 2012-03-14 17:55:32 PDT
Comment on attachment 131962 [details]
patch v1

Attachment 131962 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11951529
Comment 5 Build Bot 2012-03-14 18:34:28 PDT
Comment on attachment 131962 [details]
patch v1

Attachment 131962 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11955390
Comment 6 Build Bot 2012-03-14 18:37:15 PDT
Comment on attachment 131962 [details]
patch v1

Attachment 131962 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11957372
Comment 7 Antonio Gomes 2012-03-14 20:35:46 PDT
Created attachment 131981 [details]
patch v1.1 - for EWS to build
Comment 8 Build Bot 2012-03-14 21:08:46 PDT
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
Comment 9 WebKit Review Bot 2012-03-14 22:56:52 PDT
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
Comment 10 Dimitri Glazkov (Google) 2012-03-15 10:20:01 PDT
(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...
Comment 11 Antonio Gomes 2012-03-15 10:25:31 PDT
> 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?
Comment 12 Dimitri Glazkov (Google) 2012-03-15 12:12:19 PDT
(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?
Comment 13 Antonio Gomes 2012-03-15 13:01:16 PDT
> > 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...
Comment 14 Antonio Gomes 2012-03-15 15:13:39 PDT
Created attachment 132132 [details]
to get win symbols from ews
Comment 15 Build Bot 2012-03-15 15:44:08 PDT
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
Comment 16 WebKit Review Bot 2012-03-15 16:32:10 PDT
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
Comment 17 WebKit Review Bot 2012-03-15 17:29:13 PDT
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
Comment 18 Antonio Gomes 2012-04-03 09:56:47 PDT
Created attachment 135347 [details]
(committed r113329, r=dhyatt) patch v1.2
Comment 19 Rob Buis 2012-04-03 12:46:26 PDT
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.
Comment 20 Antonio Gomes 2012-04-03 13:16:25 PDT
(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...
Comment 21 Dave Hyatt 2012-04-04 12:22:00 PDT
Comment on attachment 135347 [details]
(committed r113329, r=dhyatt) patch v1.2

r=me
Comment 22 Antonio Gomes 2012-04-04 12:23:17 PDT
Comment on attachment 135347 [details]
(committed r113329, r=dhyatt) patch v1.2

pushing manually to watch the bots...
Comment 23 Antonio Gomes 2012-04-05 07:35:33 PDT
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.
Comment 24 Csaba Osztrogonác 2012-04-05 07:42:03 PDT
(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.
Comment 25 Antonio Gomes 2012-04-05 09:51:46 PDT
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
Comment 26 Antonio Gomes 2012-04-05 09:53:22 PDT
(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.
Comment 27 Antonio Gomes 2012-04-05 12:29:03 PDT
Created attachment 135877 [details]
(committed r113358) line left over after a rebase before landing
Comment 28 Antonio Gomes 2012-04-05 12:29:36 PDT
reopening for cq.
Comment 29 Antonio Gomes 2012-04-05 12:30:04 PDT
Test fails on lion: http://build.webkit.org/results/Lion%20Release%20%28Tests%29/r113330%20%287311%29/results.html

Looking...