Bug 80847

Created attachment 131962 [details] patch v1

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

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

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 on attachment 131962 [details] patch v1 Attachment 131962 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11955390

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

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

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 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

(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...

> 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?

(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?

> > 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...

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

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 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 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

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

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.

(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 on attachment 135347 [details] (committed r113329, r=dhyatt) patch v1.2 r=me

Comment on attachment 135347 [details] (committed r113329, r=dhyatt) patch v1.2 pushing manually to watch the bots...

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.

(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 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

(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.

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

reopening for cq.

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

(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