Bug 132888 – Clip Telephone Data Detector Overlay to Scrolling Range

• Component: Layout and Rendering
• Status: NEW

[Brent Fulgham]

The phone number datat detection UI does not get clipped by frame or overflow:scroll bounds. This is because the overlay is a "document level" view of the page, and it doesn't know about the scrolling position of various page elements.

We need to clip selection regions that live inside these <div>, <select>, and <iframe> elements so that they look right when content is scrolled or otherwise moved around.

[Brent Fulgham]

<rdar://problem/16777365>
<rdar://problem/16777238>

[Brent Fulgham]

Created attachment 231411 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 231411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231411&action=review

> Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberOverlayControllerMac.mm:121
> +{
> +    CGContextRef cgContext = graphicsContext.platformContext();
> +
> +    // If this region is enclosed in a non-mainframe scrollable region, we need to clip.
> +    ContainerNode* enclosedScrollableNode = findEnclosingScrollableContainer(range.startContainer()->parentNode());
> +    if (!enclosedScrollableNode)
> +        return;
> +
> +    Frame* frame = nullptr;
> +    if (enclosedScrollableNode->isHTMLDocument())
> +        frame = static_cast<HTMLDocument*>(enclosedScrollableNode)->frame();
> +    else if (enclosedScrollableNode->isFrameOwnerElement())
> +        frame = toHTMLFrameOwnerElement(enclosedScrollableNode)->contentFrame();
> +
> +    if (!frame && !enclosedScrollableNode->isHTMLElement())
> +        return;
> +    
> +    if (frame && frame->isMainFrame())
> +        return;
> +
> +    // Must clip:
> +    IntRect clipRect;
> +
> +    if (FrameView* view = frame ? frame->view() : nullptr)
> +        clipRect = view->windowClipRect();
> +    else if (enclosedScrollableNode->isHTMLElement())
> +        clipRect = toHTMLElement(enclosedScrollableNode)->clientRect();
> +
> +    clipRect.move(0, -topContentInset);
> +    CGContextClipToRect(cgContext, clipRect);
> +}

I think you should use RenderLayer::childrenClipRect() which we use for plug-ins and frames in various cases.

I also think this code should live at some lower level, where other things could make use of it. Makes no sense for this to be in telephone number code.

[Brent Fulgham]

Created attachment 231428 [details]
Patch

[Brent Fulgham]

Created attachment 231434 [details]
Patch

[Tim Horton]

Comment on attachment 231434 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231434&action=review

> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:242
> +void PageOverlay::clipForRange(FrameView& mainFrameView, WebCore::GraphicsContext& graphicsContext, const WebCore::Range& range, float topContentInset)

It seems really weird to me that PageOverlay knows anything about ranges. Does it need to?

> Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:213
> -        if (overlayAndLayer.key->overlayType() == PageOverlay::OverlayType::View || !frame->isMainFrame())
> +        if (!frame->isMainFrame())

I don't understand this change and it seems bad. You're making View-relative overlays not repaint on main frame scroll? That's going to destroy find-in-page, isn't it?

Your changelog says "All overlay types should be informed about scroll events, not just Views.", but you're only changing what happens if it is a View-relative overlay.

Also there's no reason why overlays need to always invalidate on scroll. Maybe we should introduce "needs invalidate on any scroll" and "needs invalidate on main frame scroll" flags?

[Brent Fulgham]

(In reply to comment #6)
> (From update of attachment 231434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231434&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:242
> > +void PageOverlay::clipForRange(FrameView& mainFrameView, WebCore::GraphicsContext& graphicsContext, const WebCore::Range& range, float topContentInset)
> 
> It seems really weird to me that PageOverlay knows anything about ranges. Does it need to?

I really only care about the renderer, and the Range's FrameView. I could revise the method to accept those parameters instead.

> > Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:213
> > -        if (overlayAndLayer.key->overlayType() == PageOverlay::OverlayType::View || !frame->isMainFrame())
> > +        if (!frame->isMainFrame())
> 
> I don't understand this change and it seems bad. You're making View-relative overlays not repaint on main frame scroll? That's going to destroy find-in-page, isn't it?

You are right! I totally misread this statement and broke it. I did not need this part of the change at all.

[Brent Fulgham]

Created attachment 231435 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 231435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231435&action=review

> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:246
> +        clipRect = roundedIntRect(renderer.enclosingLayer()->selfClipRect());
> +        clipRect.setLocation(mainFrameView.contentsToRootView(clipRect.location()));

So you're computing clipRect relative to the renderer's frame, but then using mainFrameView.contentsToRootView(). You want viewForRange. mainFrameView.contentsToRootView(). Also does this work if the iframe is scrolled?

[Ahmad Saleem]

This commit removed 'TelephoneNumberOverlayControllerMac.mm': https://github.com/WebKit/WebKit/commit/00322669352e700ccf3470a10db87c77abc2e237

I know there are changes in PageOverlay.cpp/h in this patch but do we need them now or keep this bug open?