Bug 199699 – Cannot bring up custom media controls at all on v.youku.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[alan]

<rdar://problem/51835327>

[alan]

Created attachment 373899 [details]
Patch

[alan]

Created attachment 373902 [details]
Patch

[Simon Fraser (smfr)]

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.

[alan]

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

[alan]

Created attachment 374016 [details]
Patch

[alan]

Created attachment 374042 [details]
Patch

[alan]

Created attachment 374043 [details]
Patch

[alan]

Created attachment 374076 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374076 [details]
Patch

Clearing flags on attachment: 374076

Committed r247416: <https://trac.webkit.org/changeset/247416>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.