Bug 236289 - WKHoverPlatter should scale up the platter content to make it easier to see
Summary: WKHoverPlatter should scale up the platter content to make it easier to see
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-08 01:51 PST by Tim Horton
Modified: 2022-02-09 01:32 PST (History)
3 users (show)

See Also:


Attachments
Patch (52.46 KB, patch)
2022-02-08 01:52 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (52.44 KB, patch)
2022-02-08 02:01 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (52.59 KB, patch)
2022-02-08 23:03 PST, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (52.77 KB, patch)
2022-02-08 23:26 PST, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2022-02-08 01:51:55 PST
WKHoverPlatter should scale up the platter content to make it easier to see
Comment 1 Tim Horton 2022-02-08 01:52:59 PST
Created attachment 451222 [details]
Patch
Comment 2 Tim Horton 2022-02-08 02:01:16 PST
Created attachment 451223 [details]
Patch
Comment 3 Wenson Hsieh 2022-02-08 16:40:10 PST
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?)
Comment 4 Tim Horton 2022-02-08 20:19:23 PST
(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.
Comment 5 Tim Horton 2022-02-08 23:03:02 PST
Created attachment 451340 [details]
Patch
Comment 6 Tim Horton 2022-02-08 23:26:11 PST
Created attachment 451342 [details]
Patch
Comment 7 EWS 2022-02-09 01:31:57 PST
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].
Comment 8 Radar WebKit Bug Importer 2022-02-09 01:32:18 PST
<rdar://problem/88678328>