Phone number data detection UI is offset for iframes and pages with topContentInset. The right coordinate math is not happening. <rdar://problem/16651235>
Created attachment 230451 [details] Patch
Comment on attachment 230451 [details] Patch Wait, missing one thing.
Created attachment 230452 [details] Patch
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.
(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.
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?
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()));
(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.
Comment on attachment 230452 [details] Patch I think we decided that we should use document overlays for this, which affects the coordinate math.
Created attachment 230526 [details] Document overlay patch
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?
Thanks Tim! http://trac.webkit.org/changeset/168053