Bug 146468

Summary: The bounds on InteractionInformationAtPosition should be more precise
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, enrica, esprehn+autocc, glenn, kondapallykalyan, sam, simon.fraser, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch simon.fraser: review+

Description Beth Dakin 2015-06-30 13:27:58 PDT
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.
Comment 1 Beth Dakin 2015-06-30 13:31:23 PDT
Created attachment 255843 [details]
Patch
Comment 2 Enrica Casucci 2015-06-30 13:55:53 PDT
Comment on attachment 255843 [details]
Patch

That looks great!
Comment 3 Simon Fraser (smfr) 2015-06-30 13:57:31 PDT
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.
Comment 4 Beth Dakin 2015-06-30 14:11:29 PDT
(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.
Comment 5 Beth Dakin 2015-06-30 14:41:15 PDT
Thanks Simon and Enrica! http://trac.webkit.org/changeset/186132
Comment 6 Darin Adler 2015-06-30 17:32:56 PDT
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.
Comment 7 Beth Dakin 2015-07-01 12:13:07 PDT
(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