Content from child-frames is often needed in hit-tests. Currently EventHandler::hitTestResultAtPoint descends into any child-frames returned as the innerNode of a hit-test, and uses the cached localPoint value in HitTestResult to continue the hit-test where it stopped. This is problem on multiple levels for area-based hit-tests, since it only iterates into one frame and not all intersected frames, and does not handle any potential transformations to the area being hit-tested.
Created attachment 160993 [details] Patch
The patch needs a few test-cases to be complete, and I want feed-back on the following two of the new flags ChildFrameHitTest and TestChildFrameScrollBars. The first flag was necessary beause RenderLayer::hitTest always claims the hit is fully inside if it thinks it is the RootLayer. And alternative to adding that flag, could be to change the check for isRootLayer, to something that checks if it is the root layer of the top frame. The second flag was only necessary to remain consistant with the old behavior of hitTestResultAtPoint. I actually don't see any good reason not to hit-test the frame scroll-bars. The scroll-bars already have a value to indicate if they participate in hit-testing or not.
How with this patch impact performance on point based hit tests?
(In reply to comment #3) > How with this patch impact performance on point based hit tests? It shouldn't have any effect on point based tests. They already descended into child frames, it just used to happen in EventHandler::hitTestResultAtPoint, and now it is handled in RenderFrameBase::nodeAtPoint. The returned hit-test result is not completely identical, but I think this one is more correct, but nothing depends on the difference.
*** Bug 94936 has been marked as a duplicate of this bug. ***
Created attachment 161201 [details] Patch Added two tests, and made allow-child-frame content an option to nodesFromRect
Comment on attachment 161201 [details] Patch Attachment 161201 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13681210
Comment on attachment 161201 [details] Patch Attachment 161201 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13684219
Created attachment 161215 [details] Patch Updated lists of exported symbols
Comment on attachment 161215 [details] Patch Attachment 161215 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13695202
Comment on attachment 161215 [details] Patch Attachment 161215 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13695343
Created attachment 161891 [details] Patch
Comment on attachment 161891 [details] Patch Attachment 161891 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13729836 New failing tests: touchadjustment/iframe-boundary.html
Comment on attachment 161891 [details] Patch I am splitting part of the patch out into bug #95685. The patch will need a rebase once that patch lands.
Created attachment 161932 [details] Patch Rebased and slightly modified the test so it hopefully gives identical results on more platforms
Comment on attachment 161932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161932&action=review r=me > Source/WebCore/dom/Document.cpp:1387 > +PassRefPtr<NodeList> Document::nodesFromRect(int centerX, int centerY, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent) const it is getting time for us to improve this API. This bunch of books are getting out of control. We could even pass a HitTestRequest to it? > Source/WebCore/rendering/RenderLayer.cpp:3452 > + if (Frame *frame = renderer() ? renderer()->frame() : 0) > + return (frame == frame->page()->mainFrame()); > + return false; lets do if (!renderer()) return false; return blah == bleh ? a : b; > Source/WebCore/rendering/RenderLayer.h:376 > + bool isRootLayerOfMainFrame() const; should be private?
(In reply to comment #16) > (From update of attachment 161932 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161932&action=review > > r=me > > > Source/WebCore/dom/Document.cpp:1387 > > +PassRefPtr<NodeList> Document::nodesFromRect(int centerX, int centerY, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent) const > > it is getting time for us to improve this API. This bunch of books are getting out of control. We could even pass a HitTestRequest to it? bools*
(In reply to comment #16) > (From update of attachment 161932 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161932&action=review > > r=me > > > Source/WebCore/dom/Document.cpp:1387 > > +PassRefPtr<NodeList> Document::nodesFromRect(int centerX, int centerY, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent) const > > it is getting time for us to improve this API. This bunch of books are getting out of control. We could even pass a HitTestRequest to it? > Sounds like a good idea. > > Source/WebCore/rendering/RenderLayer.cpp:3452 > > + if (Frame *frame = renderer() ? renderer()->frame() : 0) > > + return (frame == frame->page()->mainFrame()); > > + return false; > > lets do > > if (!renderer()) > return false; > > return blah == bleh ? a : b; > > > Source/WebCore/rendering/RenderLayer.h:376 > > + bool isRootLayerOfMainFrame() const; > > should be private? It appears that new part in the most recent patch is going to cause a fail on fast/events/drag-outside-window.html, so I will need to rethink it. Probably going back to the extra hit-test-request flag used in the first patch.
(In reply to comment #18) > (In reply to comment #16) > > (From update of attachment 161932 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=161932&action=review > > > > r=me > > > > > Source/WebCore/dom/Document.cpp:1387 > > > +PassRefPtr<NodeList> Document::nodesFromRect(int centerX, int centerY, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent) const > > > > it is getting time for us to improve this API. This bunch of books are getting out of control. We could even pass a HitTestRequest to it? > > > Sounds like a good idea. > Btw, the same applies for EventHandler::hitTestResultAtPoint. All the bool arguments could be send as part of the HitTestRequest flag it is also getting.
Comment on attachment 161932 [details] Patch Attachment 161932 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13744206 New failing tests: fast/events/autoscroll-should-not-stop-on-keypress.html fast/events/drag-outside-window.html
Created attachment 161988 [details] Patch for landing
Comment on attachment 161988 [details] Patch for landing Clearing flags on attachment: 161988 Committed r127457: <http://trac.webkit.org/changeset/127457>
All reviewed patches have been landed. Closing bug.
This change caused bug 96593.
(In reply to comment #24) > This change caused bug 96593. That makes sense. Check if the patch uploaded to bug #96541 fixes the issue.
After getting several serious regressions on Chromium (like context menus popping at the wrong position when clicking inside frames), Antonio and myself decided to revert this change along with the follow-up fixes. The change is probably good but you are changing the behavior of a core hit-testing methods which will impact all ports. There is way too much code to cover and breakages are expected so notifying people about it would have been a good first step.
(In reply to comment #26) > After getting several serious regressions on Chromium (like context menus popping at the wrong position when clicking inside frames), Antonio and myself decided to revert this change along with the follow-up fixes. > > The change is probably good but you are changing the behavior of a core hit-testing methods which will impact all ports. There is way too much code to cover and breakages are expected so notifying people about it would have been a good first step. I understand this point. I have a handful of ideas for how to avoid the issues, but I guess I could also integrate that in a later patch. The worst part about this though, is that these issues appear in the ports instead of WebCore, making new test cases to test for the observed issues (probably) impossible. For instance they don't appear on Mac, Qt, EFL or GTK.
(In reply to comment #27) > (In reply to comment #26) > > After getting several serious regressions on Chromium (like context menus popping at the wrong position when clicking inside frames), Antonio and myself decided to revert this change along with the follow-up fixes. > > > > The change is probably good but you are changing the behavior of a core hit-testing methods which will impact all ports. There is way too much code to cover and breakages are expected so notifying people about it would have been a good first step. > > I understand this point. I have a handful of ideas for how to avoid the issues, but I guess I could also integrate that in a later patch. The worst part about this though, is that these issues appear in the ports instead of WebCore, making new test cases to test for the observed issues (probably) impossible. For instance they don't appear on Mac, Qt, EFL or GTK. A deprecation of the old behavior and opt-in into the new one would have been better as it would have avoided the situation where all the platforms get broken unless investigated & fixed. I am happy to help you get something along those lines landed.
Reopening as the fix was revert in http://trac.webkit.org/changeset/128677
Created attachment 179265 [details] Patch Rebased and refactored to work with the changes in HitTestResult
Comment on attachment 179265 [details] Patch Attachment 179265 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15323259
Created attachment 179272 [details] Patch Fix exported symbols in WK2 for Windows
Comment on attachment 179272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179272&action=review > Source/WebCore/dom/Document.h:384 > + PassRefPtr<NodeList> nodesFromRect(int centerX, int centerY, > + unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, > + bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent = false) const; I think it would be much nicer and more readable to change all these bools into a set of bitflags.
(In reply to comment #33) > (From update of attachment 179272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179272&action=review > > > Source/WebCore/dom/Document.h:384 > > + PassRefPtr<NodeList> nodesFromRect(int centerX, int centerY, > > + unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, > > + bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent = false) const; > > I think it would be much nicer and more readable to change all these bools into a set of bitflags. I suggested the same. See bug 95720.
Comment on attachment 179272 [details] Patch Attachment 179272 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15312304 New failing tests: fast/events/touch/touch-inside-iframe-scrolled.html
Comment on attachment 179272 [details] Patch Attachment 179272 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15312324 New failing tests: fast/events/touch/touch-inside-iframe-scrolled.html
(In reply to comment #33) > (From update of attachment 179272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179272&action=review > > > Source/WebCore/dom/Document.h:384 > > + PassRefPtr<NodeList> nodesFromRect(int centerX, int centerY, > > + unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, > > + bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent = false) const; > > I think it would be much nicer and more readable to change all these bools into a set of bitflags. I agree, I have patch to do that (I think it was even applied and rolled back with this one). I prefer to keep the patches separate though, because the refactoring of the bools to bitflags interferes a lot more with the ports.
Comment on attachment 179272 [details] Patch Looks generally good. One question:
Created attachment 181880 [details] Patch Rebased and fix fast/events/touch/touch-inside-iframe-scrolled.html
Comment on attachment 181880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181880&action=review > Source/WebCore/dom/Document.h:385 > + PassRefPtr<NodeList> nodesFromRect(int centerX, int centerY, > + unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, > + bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent = false) const; This is possibly the worst function signature ever. (Not suggesting that's your fault!) bools and split-coordinates as integers. Good times. :(
Comment on attachment 181880 [details] Patch Attachment 181880 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15754684
(In reply to comment #40) > (From update of attachment 181880 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181880&action=review > > > Source/WebCore/dom/Document.h:385 > > + PassRefPtr<NodeList> nodesFromRect(int centerX, int centerY, > > + unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, > > + bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent = false) const; > > This is possibly the worst function signature ever. (Not suggesting that's your fault!) bools and split-coordinates as integers. Good times. :( That is bug #95720. Hopefully I will get back to that when this one lands for good.
Created attachment 181904 [details] Patch Update the windows symbols in the new place they reside.
Comment on attachment 181904 [details] Patch Attachment 181904 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15756590
Comment on attachment 181904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181904&action=review > Source/WebCore/ChangeLog:10 > + Refactors how EventHandler::hitTestResultAtPoint handles child-frame content, > + it is now handled by the hit test itself controlled by the AllowChildFrameContent > + flag in HitTestRequest. What's the justification for this? Some more context would be good. > Source/WebCore/dom/Document.cpp:1381 > +PassRefPtr<NodeList> Document::nodesFromRect(int centerX, int centerY, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent) const Can we change these bools to bitflags? That would make things easier to read.
(In reply to comment #45) > (From update of attachment 181904 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181904&action=review > > > Source/WebCore/ChangeLog:10 > > + Refactors how EventHandler::hitTestResultAtPoint handles child-frame content, > > + it is now handled by the hit test itself controlled by the AllowChildFrameContent > > + flag in HitTestRequest. > > What's the justification for this? Some more context would be good. > I can try to improve the ChangeLog. The goal is to return elements from all frames when doing an area-based hit-test. Currently we only return the elements in the inner most iframe containing the center point of the area. This will especially improve touch adjustment in areas near frame borders. > > Source/WebCore/dom/Document.cpp:1381 > > +PassRefPtr<NodeList> Document::nodesFromRect(int centerX, int centerY, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent) const > > Can we change these bools to bitflags? That would make things easier to read. Yes, that is bug #95720.
(In reply to comment #46) > (In reply to comment #45) > > (From update of attachment 181904 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=181904&action=review > > > > > Source/WebCore/ChangeLog:10 > > > + Refactors how EventHandler::hitTestResultAtPoint handles child-frame content, > > > + it is now handled by the hit test itself controlled by the AllowChildFrameContent > > > + flag in HitTestRequest. > > > > What's the justification for this? Some more context would be good. > > > I can try to improve the ChangeLog. The goal is to return elements from all frames when doing an area-based hit-test. Currently we only return the elements in the inner most iframe containing the center point of the area. This will especially improve touch adjustment in areas near frame borders. This is important otherwise ports have to do recursive calls like http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp#L338
Comment on attachment 181904 [details] Patch Codewise, it looks relatively good to me. Simon, Julien?
Created attachment 182320 [details] Patch Update all four copies of the symbol in the win build-file, and explain the benfit of the change in the ChangeLog.
Ping? The EWS is all green and there seems to be no more objections, except the one covered by bug #95720.
Comment on attachment 182320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182320&action=review Sorry for the delay. The change looks OK with some comments. > Source/WebCore/dom/Document.cpp:1382 > -PassRefPtr<NodeList> Document::nodesFromRect(int centerX, int centerY, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, bool ignoreClipping, bool allowShadowContent) const > +PassRefPtr<NodeList> Document::nodesFromRect(int centerX, int centerY, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding, bool ignoreClipping, bool allowShadowContent, bool allowChildFrameContent) const Antonio pointed out that we should improve this API. I would rather if we did that *before* this change than after, that would avoid API churn (especially since you have to touch several exported symbols). Here using a flag would free you from future change in the API. On top of that, you are already doing a boolean <-> flag conversion below. Postponing migrating Internals' nodeFromPoint is fine, especially since that would involve touching some tests. > Source/WebCore/page/EventHandler.cpp:1024 > + // hitTestResultAtPoint always handle child frame content. A 'why' comment would be better here. > Source/WebCore/page/TouchAdjustment.cpp:228 > + Node* ancestor = node->parentOrHostNode(); > + if (ancestor) This goes on one line. > Source/WebCore/page/TouchAdjustment.cpp:231 > + if (node->isDocumentNode()) > + return static_cast<const Document*>(node)->ownerElement(); Would be nice to have a toDocument helper function. > Source/WebCore/rendering/HitTestResult.h:134 > + void setPointInInnerNodeFrame(const LayoutPoint& point) { m_pointInInnerNodeFrame = point; } Since nobody uses this function, we should probably postpone adding it no? > Source/WebCore/rendering/RenderFrameBase.cpp:111 > + if (request.allowsChildFrameContent()) { We usually prefer early returns to avoid nested code.
Created attachment 188526 [details] Patch Modified according to reviewer feedback and rebased to apply on top of the landed patch for bug #95720
Comment on attachment 188526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188526&action=review > Source/WebCore/ChangeLog:24 > + (Document): Let's remove these empty entries and have the ChangeLog entries filled too. > Source/WebCore/page/TouchAdjustment.cpp:225 > +static inline Node* parentShadowHostOrOwner(const Node* node) An explanation about why we need this new function would be nice. > Source/WebCore/rendering/RenderFrameBase.cpp:112 > + FrameView* childFrameView = static_cast<FrameView*>(widget()); Some comments: * The old code would check result.isOverWidget(), why is this not needed anymore? * The old code was checking that widget() was a FrameView before doing the cast. I would much prefer if we at least ASSERT'ed that (probably ASSERT_WITH_SECURITY_IMPLICATION).
(In reply to comment #53) > (From update of attachment 188526 [details]) > > Source/WebCore/page/TouchAdjustment.cpp:225 > > +static inline Node* parentShadowHostOrOwner(const Node* node) > > An explanation about why we need this new function would be nice. > Because we want to travel up the event-tree, so that the inner most event receiver gets the event. If this change was not made, frames would compete with their content for touch-adjustment because they are always focusable, but since an unhandled event to their content would get to the frame anyway, the frame should be ignored when their content is a candidate for touch-adjustment. This algorithm is already applied to any event handling container versus it children, the previously used method parentOrshadowHost was just stopping at frame boundaries. > > Source/WebCore/rendering/RenderFrameBase.cpp:112 > > + FrameView* childFrameView = static_cast<FrameView*>(widget()); > > Some comments: > * The old code would check result.isOverWidget(), why is this not needed anymore? The old code was trying to figure out if the hit-test had stopped at a RenderFrameBase object, so it could continue the hit-test inside that frame. Since the code has now been moved to RenderFrameBase, we can now continue immediately and do no longer need to figure out later if we were over a widget and if that widget was a frame. > * The old code was checking that widget() was a FrameView before doing the cast. I would much prefer if we at least ASSERT'ed that (probably ASSERT_WITH_SECURITY_IMPLICATION). I will do that.
Created attachment 189041 [details] Patch
(In reply to comment #55) > Created an attachment (id=189041) [details] > Patch Looks good to me.
Comment on attachment 189041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189041&action=review r=me > Source/WebCore/rendering/RenderFrameBase.cpp:112 > + FrameView* childFrameView = static_cast<FrameView*>(widget()); I don't like a static_cast without any safety checks so please add (already mentioned in the previous review): ASSERT_WITH_SECURITY_IMPLICATION(widget()->isFrameView());
Committed r143727: <http://trac.webkit.org/changeset/143727>