The bounds on InteractionInformationAtPosition should be more precise. Specifically, the bound for links should correspond to the appropriate Range, and the bounds for an image should not include border/padding.
Created attachment 255843 [details] Patch
Comment on attachment 255843 [details] Patch That looks great!
Comment on attachment 255843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255843&action=review > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2162 > + if (element->renderer()) > info.touchCalloutEnabled = element->renderer()->style().touchCalloutEnabled(); Seems like you can just bail early from this function if element->renderer() is null. > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2171 > + RefPtr<Range> linkRange = rangeOfContents(*linkElement); > + Vector<FloatQuad> quads; > + linkRange->textQuads(quads); > + FloatRect linkBoundingBox; > + for (const auto& quad : quads) > + linkBoundingBox.unite(quad.enclosingBoundingBox()); > + info.bounds = IntRect(linkBoundingBox); This is just Range::boundingRect(), but that forces layout. > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2172 > + } else if (element->renderer()) { if (RenderElement* renderer = element->renderer())... > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2177 > + info.bounds = element->renderer()->absoluteBoundingBoxRect(true); true is the default.
(In reply to comment #3) > Comment on attachment 255843 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255843&action=review > > > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2162 > > + if (element->renderer()) > > info.touchCalloutEnabled = element->renderer()->style().touchCalloutEnabled(); > > Seems like you can just bail early from this function if element->renderer() > is null. > There is more to the function that should be executed outside of an if (element) that this line of code is wrapped in. I could break; I suppose? I feel like we don't usually use breaks to break out of conditionals, but it's not covered by the style guide so maybe it's fine? > > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2171 > > + RefPtr<Range> linkRange = rangeOfContents(*linkElement); > > + Vector<FloatQuad> quads; > > + linkRange->textQuads(quads); > > + FloatRect linkBoundingBox; > > + for (const auto& quad : quads) > > + linkBoundingBox.unite(quad.enclosingBoundingBox()); > > + info.bounds = IntRect(linkBoundingBox); > > This is just Range::boundingRect(), but that forces layout. > Layout should be up to date here, so that should;t be necessary. Unless we are sure it's smart enough to know things are up to date? > > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2172 > > + } else if (element->renderer()) { > > if (RenderElement* renderer = element->renderer())... > Fixed. > > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2177 > > + info.bounds = element->renderer()->absoluteBoundingBoxRect(true); > > true is the default. Fixed.
Thanks Simon and Enrica! http://trac.webkit.org/changeset/186132
Comment on attachment 255843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255843&action=review > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2175 > + auto& renderImage = downcast<RenderImage>(*(element->renderer())); > + info.bounds = renderImage.absoluteContentQuad().enclosingBoundingBox(); Now that you added a local variable, this is slightly nicer as a one-liner: info.bounds = downcast<RenderImage>(*renderer).absoluteContentQuad().enclosingBoundingBox(); I know you landed already, but I couldn’t resist.
(In reply to comment #6) > Comment on attachment 255843 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255843&action=review > > > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2175 > > + auto& renderImage = downcast<RenderImage>(*(element->renderer())); > > + info.bounds = renderImage.absoluteContentQuad().enclosingBoundingBox(); > > Now that you added a local variable, this is slightly nicer as a one-liner: > > info.bounds = > downcast<RenderImage>(*renderer).absoluteContentQuad(). > enclosingBoundingBox(); > > I know you landed already, but I couldn’t resist. Hehe, turned this into a one-liner with http://trac.webkit.org/changeset/186184