Phone number data detection UI is offset for iframes, pages with topContentInset

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

Phone number data detection UI is offset for iframes and pages with topContentInset. The right coordinate math is not happening.

<rdar://problem/16651235>

[Beth Dakin]

Created attachment 230451 [details]
Patch

[Beth Dakin]

Comment on attachment 230451 [details]
Patch

Wait, missing one thing.

[Beth Dakin]

Created attachment 230452 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 230452 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberOverlayControllerMac.mm:77
> +        IntPoint frameOffset(frame->view()->scrollOffset().width(), frame->view()->scrollOffset().height());
> +        offset.move(frameOffset.x(), frameOffset.y());
> +        offset = frame->view()->contentsToRootView(offset);

contentsToRootView() already subtracts the scrollOffset, so I'm not sure why you have to do it manually here.

[Beth Dakin]

(In reply to comment #4)
> (From update of attachment 230452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230452&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberOverlayControllerMac.mm:77
> > +        IntPoint frameOffset(frame->view()->scrollOffset().width(), frame->view()->scrollOffset().height());
> > +        offset.move(frameOffset.x(), frameOffset.y());
> > +        offset = frame->view()->contentsToRootView(offset);
> 
> contentsToRootView() already subtracts the scrollOffset, so I'm not sure why you have to do it manually here.

Hmm, you're right. This is kind of a hack I guess. The original offset value is not actually a contents point because it already factors in the scroll position, so I was factoring it out only to get contentsToRootView() to work correctly and factor it back in.

Using Widget::convertToRootView() does the right thing for frames and subframes, but that leaves overflow areas broken. 

I'm torn about defending the hack vs. finding a better solution. Will think on this a bit more.

[Brady Eidson]

For the selection overlay controller, Tim mentioned the new PageOverlay type "PageOverlay::OverlayType::Document" that supposedly handles scrolling automatically.

Tim, do you think switching over to PageOverlay::OverlayType::Document here would just make this class of bug disappear?

[Darin Adler]

Comment on attachment 230452 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberOverlayControllerMac.mm:73
> +        Frame* frame = range->ownerDocument().frame();

The code below uses FrameView, not Frame, so I suggest putting that into a local variable. Also, I suggest using a reference, not a pointer.

> Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberOverlayControllerMac.mm:76
> +        IntPoint frameOffset(frame->view()->scrollOffset().width(), frame->view()->scrollOffset().height());
> +        offset.move(frameOffset.x(), frameOffset.y());

From a micro-code-structure point of view, this can just be:

    offset.move(view.scrollOffset());

There’s no need to convert the IntSize to an IntPoint since IntPoint::move already takes an IntSize. That’s separate from the question you are discussing with Simon about whether this is OK.

I fact, again ignoring the correctness of the coordinate math that you are discussing with Simon, I think this 6-line snippet can be written like in two lines like this:

    FrameView& view = *range->ownerDocument().view();
    rect.setLocation(view.contentsToRootView(rect.location() + view.scrollOffset()));

[Tim Horton]

(In reply to comment #6)
> For the selection overlay controller, Tim mentioned the new PageOverlay type "PageOverlay::OverlayType::Document" that supposedly handles scrolling automatically.
> 
> Tim, do you think switching over to PageOverlay::OverlayType::Document here would just make this class of bug disappear?

Not a chance. It will make the topContentInset bug disappear, but for subframes you still need to do something yourself.

[Simon Fraser (smfr)]

Comment on attachment 230452 [details]
Patch

I think we decided that we should use document overlays for this, which affects the coordinate math.

[Beth Dakin]

Created attachment 230526 [details]
Document overlay patch

[Tim Horton]

Comment on attachment 230526 [details]
Document overlay patch

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

> Source/WebKit2/WebProcess/WebPage/mac/TelephoneNumberOverlayControllerMac.mm:75
> +        rect.setLocation(mainFrameView.convertChildToSelf(viewForRange, rect.location()));

What happens with transformed frames/overflow scroll/etc.? We might need FIXMEs about those things?

[Beth Dakin]

Thanks Tim! http://trac.webkit.org/changeset/168053