Bug 236289 – WKHoverPlatter should scale up the platter content to make it easier to see

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

WKHoverPlatter should scale up the platter content to make it easier to see

[Tim Horton]

Created attachment 451222 [details]
Patch

[Tim Horton]

Created attachment 451223 [details]
Patch

[Wenson Hsieh]

Comment on attachment 451223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451223&action=review

r=me with tvOS/watchOS builds fixed!

> Source/WebKit/UIProcess/WebPageProxy.cpp:11159
> +void WebPageProxy::interactableRegionsInRect(WebCore::FloatRect rect, CompletionHandler<void(Vector<WebCore::FloatRect>)>&& completionHandler)

Maybe give this a name that better describes the coordinate space? Given the usage, it looks like it's in root view space.

(Also, you can remove the WebCore:: here)

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3107
> +    if (WKHoverPlatterDomain.rootSettings.enabled)
> +        return [_hoverPlatter adjustedPointForPoint:location];

Nit - maybe it would it be cleaner if we folded these `WKHoverPlatterDomain.rootSettings.enabled` checks into -adjustedPointForPoint:?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3117
> +    if (WKHoverPlatterDomain.rootSettings.enabled)
> +        return [_hoverPlatter adjustedPointForPoint:location];

(Ditto)

> Source/WebKit/UIProcess/ios/WKHoverPlatter.mm:122
> +    if (!boundingRect.contains(point) && point != WebCore::FloatPoint { -1 , -1 })

Nit - extra space before the ,

(It's also not super obvious where the (-1, -1) is coming from..)

> Source/WebKit/UIProcess/ios/WKHoverPlatter.mm:300
> +    delta.scale(1.f / WKHoverPlatterDomain.rootSettings.platterScale);

`WKHoverPlatterDomain.rootSettings.platterScale` is always guaranteed to be nonzero, right?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6268
> +void WebPage::interactableRegionsInRect(WebCore::FloatRect rect, CompletionHandler<void(Vector<WebCore::FloatRect>)>&& completionHandler)

Nit - I think you can omit the WebCore:: here

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6275
> +    auto result = HitTestResult { WebCore::LayoutRect(rect) };

(here too)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6289
> +    HitTestResult::NodeSet linkNodes;

Nit - maybe `interactableNodes` or `hitTestedNodes`? (linkNodes makes it sounds a bit like it's only about links)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6301
> +        rects.append(renderer->absoluteBoundingBoxRect());

(Just to double check — we don't need to convert to root view coordinates here because all the hit-tested nodes are in the main frame?)

[Tim Horton]

(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 451223 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451223&action=review
> 
> r=me with tvOS/watchOS builds fixed!
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:11159
> > +void WebPageProxy::interactableRegionsInRect(WebCore::FloatRect rect, CompletionHandler<void(Vector<WebCore::FloatRect>)>&& completionHandler)
> 
> Maybe give this a name that better describes the coordinate space? Given the
> usage, it looks like it's in root view space.

Totally!

> (Also, you can remove the WebCore:: here)
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3107
> > +    if (WKHoverPlatterDomain.rootSettings.enabled)
> > +        return [_hoverPlatter adjustedPointForPoint:location];
> 
> Nit - maybe it would it be cleaner if we folded these
> `WKHoverPlatterDomain.rootSettings.enabled` checks into
> -adjustedPointForPoint:?

It would, but _hoverPlatter is nil in that case, so it would break all interaction.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3117
> > +    if (WKHoverPlatterDomain.rootSettings.enabled)
> > +        return [_hoverPlatter adjustedPointForPoint:location];
> 
> (Ditto)
> 
> > Source/WebKit/UIProcess/ios/WKHoverPlatter.mm:122
> > +    if (!boundingRect.contains(point) && point != WebCore::FloatPoint { -1 , -1 })
> 
> Nit - extra space before the ,
> 
> (It's also not super obvious where the (-1, -1) is coming from..)

We don't have it named anywhere that I know, but it's the view-level "mouseout" position hardcoded in a few other places. Still, it is a good point... let me see what I can do.

> > Source/WebKit/UIProcess/ios/WKHoverPlatter.mm:300
> > +    delta.scale(1.f / WKHoverPlatterDomain.rootSettings.platterScale);
> 
> `WKHoverPlatterDomain.rootSettings.platterScale` is always guaranteed to be
> nonzero, right?

I will change the minimum in WKHoverPlatterParameters to 1 (but, regardless, the divide-by-zeroes would be on you if you set the debug setting to 0 :P)

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6268
> > +void WebPage::interactableRegionsInRect(WebCore::FloatRect rect, CompletionHandler<void(Vector<WebCore::FloatRect>)>&& completionHandler)
> 
> Nit - I think you can omit the WebCore:: here
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6275
> > +    auto result = HitTestResult { WebCore::LayoutRect(rect) };
> 
> (here too)
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6289
> > +    HitTestResult::NodeSet linkNodes;
> 
> Nit - maybe `interactableNodes` or `hitTestedNodes`? (linkNodes makes it
> sounds a bit like it's only about links)

Right! I renamed the method from being about "link"s but missed this one.

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6301
> > +        rects.append(renderer->absoluteBoundingBoxRect());
> 
> (Just to double check — we don't need to convert to root view coordinates
> here because all the hit-tested nodes are in the main frame?)

Will double check the coordinate space, very possible this is wrong.

[Tim Horton]

Created attachment 451340 [details]
Patch

[Tim Horton]

Created attachment 451342 [details]
Patch

[EWS]

Committed r289463 (247006@main): <https://commits.webkit.org/247006@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451342 [details].

[Radar WebKit Bug Importer]

<rdar://problem/88678328>