146468 2015-06-30 13:27:58 -0700 The bounds on InteractionInformationAtPosition should be more precise 2015-07-01 12:13:07 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 bdakin webkit-unassigned bdakin commit-queue enrica esprehn+autocc glenn kondapallykalyan sam simon.fraser thorton oldest_to_newest 1105771 0 bdakin 2015-06-30 13:27:58 -0700 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. 1105773 1 255843 bdakin 2015-06-30 13:31:23 -0700 Created attachment 255843 Patch 1105791 2 255843 enrica 2015-06-30 13:55:53 -0700 Comment on attachment 255843 Patch That looks great! 1105793 3 255843 simon.fraser 2015-06-30 13:57:31 -0700 Comment on attachment 255843 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. 1105804 4 bdakin 2015-06-30 14:11:29 -0700 (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. 1105820 5 bdakin 2015-06-30 14:41:15 -0700 Thanks Simon and Enrica! http://trac.webkit.org/changeset/186132 1105908 6 255843 darin 2015-06-30 17:32:56 -0700 Comment on attachment 255843 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. 1106147 7 bdakin 2015-07-01 12:13:07 -0700 (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 255843 2015-06-30 13:31:23 -0700 2015-06-30 13:57:57 -0700 Patch for-review.txt text/plain 4023 bdakin SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4NjEyOCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE1LTA2LTMwICBCZXRoIERh a2luICA8YmRha2luQGFwcGxlLmNvbT4KKworICAgICAgICBUaGUgYm91bmRzIG9uIEludGVyYWN0 aW9uSW5mb3JtYXRpb25BdFBvc2l0aW9uIHNob3VsZCBiZSBtb3JlIHByZWNpc2UKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0NjQ2OAorICAgICAgICAt YW5kIGNvcnJlc3BvbmRpbmctCisgICAgICAgIHJkYXI6Ly9wcm9ibGVtLzIwNzM5ODM0CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRXhwb3J0IGFic29s dXRlQ29udGVudFF1YWQoKS4KKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyQm94Lmg6CisKIDIw MTUtMDYtMzAgIFphbGFuIEJ1anRhcyAgPHphbGFuQGFwcGxlLmNvbT4KIAogICAgICAgICBBZGRy ZXNzaW5nIHBvc3QtcmV2aWV3IGNvbW1lbnRzIGluIHIxODU5MTYKSW5kZXg6IFNvdXJjZS9XZWJD b3JlL3JlbmRlcmluZy9SZW5kZXJCb3guaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9y ZW5kZXJpbmcvUmVuZGVyQm94LmgJKHJldmlzaW9uIDE4NTk2MikKKysrIFNvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9SZW5kZXJCb3guaAkod29ya2luZyBjb3B5KQpAQCAtMTY3LDcgKzE2Nyw3IEBA IHB1YmxpYzoKICAgICAvLyBUaGUgY29udGVudCBib3ggaW4gYWJzb2x1dGUgY29vcmRzLiBJZ25v cmVzIHRyYW5zZm9ybXMuCiAgICAgSW50UmVjdCBhYnNvbHV0ZUNvbnRlbnRCb3goKSBjb25zdDsK ICAgICAvLyBUaGUgY29udGVudCBib3ggY29udmVydGVkIHRvIGFic29sdXRlIGNvb3JkcyAodGFr aW5nIHRyYW5zZm9ybXMgaW50byBhY2NvdW50KS4KLSAgICBGbG9hdFF1YWQgYWJzb2x1dGVDb250 ZW50UXVhZCgpIGNvbnN0OworICAgIFdFQkNPUkVfRVhQT1JUIEZsb2F0UXVhZCBhYnNvbHV0ZUNv bnRlbnRRdWFkKCkgY29uc3Q7CiAKICAgICAvLyBUaGlzIHJldHVybnMgdGhlIGNvbnRlbnQgYXJl YSBvZiB0aGUgYm94IChleGNsdWRpbmcgcGFkZGluZyBhbmQgYm9yZGVyKS4gVGhlIG9ubHkgZGlm ZmVyZW5jZSB3aXRoIGNvbnRlbnRCb3hSZWN0IGlzIHRoYXQgY29tcHV0ZWRDU1NDb250ZW50Qm94 UmVjdAogICAgIC8vIGRvZXMgaW5jbHVkZSB0aGUgaW50cmluc2ljIHBhZGRpbmcgaW4gdGhlIGNv bnRlbnQgYm94IGFzIHRoaXMgaXMgd2hhdCBzb21lIGNhbGxlcnMgZXhwZWN0IChsaWtlIGdldENv bXB1dGVkU3R5bGUpLgpJbmRleDogU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwkocmV2aXNpb24gMTg2MTI4KQorKysgU291cmNl L1dlYktpdDIvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTUt MDYtMzAgIEJldGggRGFraW4gIDxiZGFraW5AYXBwbGUuY29tPgorCisgICAgICAgIFRoZSBib3Vu ZHMgb24gSW50ZXJhY3Rpb25JbmZvcm1hdGlvbkF0UG9zaXRpb24gc2hvdWxkIGJlIG1vcmUgcHJl Y2lzZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ2 NDY4CisgICAgICAgIC1hbmQgY29ycmVzcG9uZGluZy0KKyAgICAgICAgcmRhcjovL3Byb2JsZW0v MjA3Mzk4MzQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBGb3IgbGlua3MsIGdldCB0aGUgVGV4dFF1YWRzIGZyb20gdGhlIFJhbmdlLCBhbmQgZm9yIGlt YWdlcywgbG9vayBhdCB0aGUgCisgICAgICAgIGFic29sdXRlQ29udGVudFF1YWQoKS4gQWxsIG90 aGVyIGl0ZW1zIGNhbiBjb250aW51ZSB0byB1c2UgYWJzb2x1dGVCb3VuZGluZ0JveC4KKyAgICAg ICAgKiBXZWJQcm9jZXNzL1dlYlBhZ2UvaW9zL1dlYlBhZ2VJT1MubW06CisgICAgICAgIChXZWJL aXQ6OldlYlBhZ2U6OmdldFBvc2l0aW9uSW5mb3JtYXRpb24pOgorCiAyMDE1LTA2LTMwICBBbmRl cnMgQ2FybHNzb24gIDxhbmRlcnNjYUBhcHBsZS5jb20+CiAKICAgICAgICAgRGlzYWJsZSBTcGlu dHJhY2VyIHdoZW4gZ2V0dGluZyB0aGUgbGlzdCBvZiBwbHVnLWlucwpJbmRleDogU291cmNlL1dl YktpdDIvV2ViUHJvY2Vzcy9XZWJQYWdlL2lvcy9XZWJQYWdlSU9TLm1tCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9pb3MvV2ViUGFnZUlPUy5tbQkocmV2 aXNpb24gMTg2MDE4KQorKysgU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQYWdlL2lvcy9X ZWJQYWdlSU9TLm1tCSh3b3JraW5nIGNvcHkpCkBAIC0yMTU4LDkgKzIxNTgsMjMgQEAgdm9pZCBX ZWJQYWdlOjpnZXRQb3NpdGlvbkluZm9ybWF0aW9uKGNvbgogICAgICAgICAgICAgaW5mby50aXRs ZSA9IGVsZW1lbnQtPmZhc3RHZXRBdHRyaWJ1dGUoSFRNTE5hbWVzOjp0aXRsZUF0dHIpLnN0cmlu ZygpOwogICAgICAgICAgICAgaWYgKGxpbmtFbGVtZW50ICYmIGluZm8udGl0bGUuaXNFbXB0eSgp KQogICAgICAgICAgICAgICAgIGluZm8udGl0bGUgPSBlbGVtZW50LT5pbm5lclRleHQoKTsKLSAg ICAgICAgICAgIGlmIChlbGVtZW50LT5yZW5kZXJlcigpKSB7Ci0gICAgICAgICAgICAgICAgaW5m by5ib3VuZHMgPSBlbGVtZW50LT5yZW5kZXJlcigpLT5hYnNvbHV0ZUJvdW5kaW5nQm94UmVjdCh0 cnVlKTsKKyAgICAgICAgICAgIGlmIChlbGVtZW50LT5yZW5kZXJlcigpKQogICAgICAgICAgICAg ICAgIGluZm8udG91Y2hDYWxsb3V0RW5hYmxlZCA9IGVsZW1lbnQtPnJlbmRlcmVyKCktPnN0eWxl KCkudG91Y2hDYWxsb3V0RW5hYmxlZCgpOworCisgICAgICAgICAgICBpZiAoZWxlbWVudCA9PSBs aW5rRWxlbWVudCkgeworICAgICAgICAgICAgICAgIFJlZlB0cjxSYW5nZT4gbGlua1JhbmdlID0g cmFuZ2VPZkNvbnRlbnRzKCpsaW5rRWxlbWVudCk7CisgICAgICAgICAgICAgICAgVmVjdG9yPEZs b2F0UXVhZD4gcXVhZHM7CisgICAgICAgICAgICAgICAgbGlua1JhbmdlLT50ZXh0UXVhZHMocXVh ZHMpOworICAgICAgICAgICAgICAgIEZsb2F0UmVjdCBsaW5rQm91bmRpbmdCb3g7CisgICAgICAg ICAgICAgICAgZm9yIChjb25zdCBhdXRvJiBxdWFkIDogcXVhZHMpCisgICAgICAgICAgICAgICAg ICAgIGxpbmtCb3VuZGluZ0JveC51bml0ZShxdWFkLmVuY2xvc2luZ0JvdW5kaW5nQm94KCkpOwor ICAgICAgICAgICAgICAgIGluZm8uYm91bmRzID0gSW50UmVjdChsaW5rQm91bmRpbmdCb3gpOwor ICAgICAgICAgICAgfSBlbHNlIGlmIChlbGVtZW50LT5yZW5kZXJlcigpKSB7CisgICAgICAgICAg ICAgICAgaWYgKGVsZW1lbnQtPnJlbmRlcmVyKCktPmlzUmVuZGVySW1hZ2UoKSkgeworICAgICAg ICAgICAgICAgICAgICBhdXRvJiByZW5kZXJJbWFnZSA9IGRvd25jYXN0PFJlbmRlckltYWdlPigq KGVsZW1lbnQtPnJlbmRlcmVyKCkpKTsKKyAgICAgICAgICAgICAgICAgICAgaW5mby5ib3VuZHMg PSByZW5kZXJJbWFnZS5hYnNvbHV0ZUNvbnRlbnRRdWFkKCkuZW5jbG9zaW5nQm91bmRpbmdCb3go KTsKKyAgICAgICAgICAgICAgICB9IGVsc2UKKyAgICAgICAgICAgICAgICAgICAgaW5mby5ib3Vu ZHMgPSBlbGVtZW50LT5yZW5kZXJlcigpLT5hYnNvbHV0ZUJvdW5kaW5nQm94UmVjdCh0cnVlKTsK ICAgICAgICAgICAgIH0KICAgICAgICAgfQogICAgIH0K