Bug 95204 – Allow child-frame content in hit-tests.

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 160993 [details]
Patch

[Allan Sandfeld Jensen]

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.

[Kevin Ellis]

How with this patch impact performance on point based hit tests?

[Allan Sandfeld Jensen]

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

[Allan Sandfeld Jensen]

*** Bug 94936 has been marked as a duplicate of this bug. ***

[Allan Sandfeld Jensen]

Created attachment 161201 [details]
Patch

Added two tests, and made allow-child-frame content an option to nodesFromRect

[Build Bot]

Comment on attachment 161201 [details]
Patch

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

[Build Bot]

Comment on attachment 161201 [details]
Patch

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

[Allan Sandfeld Jensen]

Created attachment 161215 [details]
Patch

Updated lists of exported symbols

[Build Bot]

Comment on attachment 161215 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 161215 [details]
Patch

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

[Allan Sandfeld Jensen]

Created attachment 161891 [details]
Patch

[WebKit Review Bot]

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

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 161932 [details]
Patch

Rebased and slightly modified the test so it hopefully gives identical results on more platforms

[Antonio Gomes]

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?

[Antonio Gomes]

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

[Allan Sandfeld Jensen]

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

[Allan Sandfeld Jensen]

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

[WebKit Review Bot]

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

[Allan Sandfeld Jensen]

Created attachment 161988 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 161988 [details]
Patch for landing

Clearing flags on attachment: 161988

Committed r127457: <http://trac.webkit.org/changeset/127457>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

This change caused bug 96593.

[Allan Sandfeld Jensen]

(In reply to comment #24)
> This change caused bug 96593.

That makes sense. Check if the patch uploaded to bug #96541 fixes the issue.

[Julien Chaffraix]

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.

[Allan Sandfeld Jensen]

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

[Julien Chaffraix]

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

[Julien Chaffraix]

Reopening as the fix was revert in http://trac.webkit.org/changeset/128677

[Allan Sandfeld Jensen]

Created attachment 179265 [details]
Patch

Rebased and refactored to work with the changes in HitTestResult

[Build Bot]

Comment on attachment 179265 [details]
Patch

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

[Allan Sandfeld Jensen]

Created attachment 179272 [details]
Patch

Fix exported symbols in WK2 for Windows

[Simon Fraser (smfr)]

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.

[Antonio Gomes]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Allan Sandfeld Jensen]

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

[Antonio Gomes]

Comment on attachment 179272 [details]
Patch

Looks generally good. One question:

[Allan Sandfeld Jensen]

Created attachment 181880 [details]
Patch

Rebased and fix fast/events/touch/touch-inside-iframe-scrolled.html

[Eric Seidel (no email)]

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

[Build Bot]

Comment on attachment 181880 [details]
Patch

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

[Allan Sandfeld Jensen]

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

[Allan Sandfeld Jensen]

Created attachment 181904 [details]
Patch

Update the windows symbols in the new place they reside.

[Build Bot]

Comment on attachment 181904 [details]
Patch

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

[Simon Fraser (smfr)]

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.

[Allan Sandfeld Jensen]

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

[Antonio Gomes]

(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

[Antonio Gomes]

Comment on attachment 181904 [details]
Patch

Codewise, it looks relatively good to me. Simon, Julien?

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Ping? The EWS is all green and there seems to be no more objections, except the one covered by bug #95720.

[Julien Chaffraix]

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.

[Allan Sandfeld Jensen]

Created attachment 188526 [details]
Patch

Modified according to reviewer feedback and rebased to apply on top of the landed patch for bug #95720

[Julien Chaffraix]

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

[Allan Sandfeld Jensen]

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

[Allan Sandfeld Jensen]

Created attachment 189041 [details]
Patch

[Antonio Gomes]

(In reply to comment #55)
> Created an attachment (id=189041) [details]
> Patch

Looks good to me.

[Julien Chaffraix]

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

[Allan Sandfeld Jensen]

Committed r143727: <http://trac.webkit.org/changeset/143727>