<rdar://problem/51835327>
Created attachment 373899 [details] Patch
Created attachment 373902 [details] Patch
Comment on attachment 373902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373902&action=review > Source/WebCore/ChangeLog:9 > + The "find the node under the finger" heuristic should only find nodes that are visible to hittesting. hit-testing or hit testing. > Source/WebCore/ChangeLog:13 > + This fixes the case when "visibility: hidden" iFrame covers the target node and it might divert the synthetic click (no hijacking). Please re-write to improve readability. I think you could expand this to explain the two types of hit-testing. > Source/WebCore/page/EventHandler.cpp:1182 > + // hitTestResultAtPointIncludingSubframes is specifically used to hitTest into all frames, thus it always allows child frame content. hitTest -> hit-test > Source/WebCore/page/EventHandler.cpp:1216 > - // hitTestResultAtPoint is specifically used to hitTest into all frames, thus it always allows child frame content. > - HitTestRequest request(hitType | HitTestRequest::AllowChildFrameContent); > + HitTestRequest request(hitType); Should this mask out the AllowChildFrameContent, or assert that it's not set? > Source/WebCore/page/PointerCaptureController.cpp:395 > + target = m_page.mainFrame().eventHandler().hitTestResultAtPointIncludingSubframes(documentPoint, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::DisallowUserAgentShadowContent | HitTestRequest::AllowChildFrameContent).innerNonSharedElement(); Isn't the HitTestRequest::AllowChildFrameContent redundant here? > Source/WebCore/page/ios/FrameIOS.mm:278 > + auto adjustHitTestResultIfNeeded = [&](auto& hitTestResult) { > + // Retry hittest with the visible-to-hittest restriction on, if the initial hittest finds something inside a non-visible iFrame. > + auto* frame = hitTestResult.innerNodeFrame(); > + if (!frame || frame->isMainFrame()) > + return; > + auto* ownerElement = frame->ownerElement(); > + if (!ownerElement || !ownerElement->renderer()) > + return; > + auto& renderer = *ownerElement->renderer(); > + if (renderer.visibleToHitTesting()) > + return; > + hitTestResult = eventHandler().hitTestResultAtPoint(center); > + }; This seems wrong. You'd have to walk up the entire frame tree looking for any frame ancestor whose renderer is not renderer.visibleToHitTesting(). Also, calling hitTestResultAtPoint(), which doesn't cross frame boundaries, will only check the main frame, but maybe you found something in a nested frame; you'd have to call hitTestResultAtPoint() on that inner frame. I think you need to do the renderer.visibleToHitTesting() check inside of the initial hitTestResultAtPointIncludingSubframes() pass. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:555 > + HitTestResult result = m_page->mainFrame().eventHandler().hitTestResultAtPointIncludingSubframes(m_lastInteractionLocation, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AllowChildFrameContent); HitTestRequest::AllowChildFrameContent is redundant. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1494 > + HitTestResult hitTest = frame.eventHandler().hitTestResultAtPointIncludingSubframes(pointInDocument, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AllowChildFrameContent); HitTestRequest::AllowChildFrameContent is redundant. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2044 > + auto result = m_page->mainFrame().eventHandler().hitTestResultAtPointIncludingSubframes(point, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::DisallowUserAgentShadowContent | HitTestRequest::AllowChildFrameContent); HitTestRequest::AllowChildFrameContent is redundant. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2653 > + HitTestResult result = page.corePage()->mainFrame().eventHandler().hitTestResultAtPointIncludingSubframes(request.point, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::DisallowUserAgentShadowContent | HitTestRequest::AllowChildFrameContent); HitTestRequest::AllowChildFrameContent is redundant. > Source/WebKitLegacy/ios/WebCoreSupport/WebFrameIOS.mm:944 > + HitTestResult result = frame->eventHandler().hitTestResultAtPointIncludingSubframes(adjustedPoint, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AllowChildFrameContent); HitTestRequest::AllowChildFrameContent is redundant.
> This seems wrong. You'd have to walk up the entire frame tree looking for > any frame ancestor whose renderer is not renderer.visibleToHitTesting(). > Also, calling hitTestResultAtPoint(), which doesn't cross frame boundaries, > will only check the main frame, but maybe you found something in a nested > frame; you'd have to call hitTestResultAtPoint() on that inner frame. I think what I need to do is to look at the HitTestResult::isOverWidget flag and extract the target subframe. It's exactly what we do during mouse event dispatching (start the event with the mainframe and look inside the HitTestResult to find the target subframe).
Created attachment 374016 [details] Patch
Created attachment 374042 [details] Patch
Created attachment 374043 [details] Patch
Created attachment 374076 [details] Patch
Comment on attachment 374076 [details] Patch Clearing flags on attachment: 374076 Committed r247416: <https://trac.webkit.org/changeset/247416>
All reviewed patches have been landed. Closing bug.