Bug 146468 – The bounds on InteractionInformationAtPosition should be more precise

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Beth Dakin]

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.

[Beth Dakin]

Created attachment 255843 [details]
Patch

[Enrica Casucci]

Comment on attachment 255843 [details]
Patch

That looks great!

[Simon Fraser (smfr)]

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.

[Beth Dakin]

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

[Beth Dakin]

Thanks Simon and Enrica! http://trac.webkit.org/changeset/186132

[Darin Adler]

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.

[Beth Dakin]

(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