Bug 224837 – [iOS] Text selection in image overlays should not be limited to rectilinear quads

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

rdar://76829981

[Wenson Hsieh]

Created attachment 426609 [details]
Depends on #224820

[Tim Horton]

Comment on attachment 426609 [details]
Depends on #224820

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

> Source/WebCore/rendering/RenderLineBreak.cpp:235
> +    auto behavior = HTMLElement::isImageOverlayText(element()) ? SelectionRenderingBehavior::UseIndividualQuads : SelectionRenderingBehavior::CoalesceBoundingRects;

It's a bit unfortunate that isImageOverlayText is infecting something like RenderLineBreak. Can we maybe hide it in a SelectionGeometry helper or something?

> Source/WebCore/rendering/RenderObject.cpp:740
> +    auto behavior = HTMLElement::isImageOverlayText(node()) ? SelectionRenderingBehavior::UseIndividualQuads : SelectionRenderingBehavior::CoalesceBoundingRects;

Ditto

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4808
> -static NSArray<WKTextSelectionRect *> *wkTextSelectionRects(const Vector<WebCore::SelectionGeometry>& rects)
> +static NSArray<WKTextSelectionRect *> *wkTextSelectionRects(const Vector<WebCore::SelectionGeometry>& rects, CGFloat scaleFactor)

While you're here... maybe drop the `wk` prefix?

> Source/WebKit/UIProcess/ios/WKTextSelectionRect.mm:114
> +- (WKTextSelectionRectCustomHandleInfo *)_customHandleInfo

Returning a random unrelated type here where this expects a UITextSelectionRectCustomHandleInfo is pretty sketchy, even if it's "OK". Can WKTextSelectionRectCustomHandleInfo be a subclass of WKTextSelectionRectCustomHandleInfo like WKTextSelectionRect is a subclass of UITextSelectionRect?

> Source/WebKit/UIProcess/ios/WKTextSelectionRect.mm:121
> +    return [[[WKTextSelectionRectCustomHandleInfo alloc] initWithFloatQuad:scaledQuad] autorelease];

See https://commits.webkit.org/r273062; I think this should be adoptNS().autorelease() for future-ARC-happiness

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:265
> +        convertedQuad.setP1(view.contentsToRootView(convertedQuad.p1()));

There are two other instances of this code (InspectorTimelineAgent and sendTapHighlightForNode), maybe it's time for ScrollView/FrameView to get a FloatQuad version of contentsToRootView??

[Wenson Hsieh]

Comment on attachment 426609 [details]
Depends on #224820

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

Thanks for the review!

>> Source/WebCore/rendering/RenderLineBreak.cpp:235
>> +    auto behavior = HTMLElement::isImageOverlayText(element()) ? SelectionRenderingBehavior::UseIndividualQuads : SelectionRenderingBehavior::CoalesceBoundingRects;
> 
> It's a bit unfortunate that isImageOverlayText is infecting something like RenderLineBreak. Can we maybe hide it in a SelectionGeometry helper or something?

That is true! It's a bit odd to add a helper for this on SelectionGeometry though, since `SelectionGeometry.h` is included in the UI process and ideally shouldn't know about DOM nodes/renderers. I think I'll add something like:

```
SelectionRenderingBehavior HTMLElement::selectionRenderingBehavior(const Node* node)
{
    …
}
```

…since HTMLElement is already well aware of image overlays.

>> Source/WebCore/rendering/RenderObject.cpp:740
>> +    auto behavior = HTMLElement::isImageOverlayText(node()) ? SelectionRenderingBehavior::UseIndividualQuads : SelectionRenderingBehavior::CoalesceBoundingRects;
> 
> Ditto

👍🏻

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4808
>> +static NSArray<WKTextSelectionRect *> *wkTextSelectionRects(const Vector<WebCore::SelectionGeometry>& rects, CGFloat scaleFactor)
> 
> While you're here... maybe drop the `wk` prefix?

Done!

>> Source/WebKit/UIProcess/ios/WKTextSelectionRect.mm:114
>> +- (WKTextSelectionRectCustomHandleInfo *)_customHandleInfo
> 
> Returning a random unrelated type here where this expects a UITextSelectionRectCustomHandleInfo is pretty sketchy, even if it's "OK". Can WKTextSelectionRectCustomHandleInfo be a subclass of WKTextSelectionRectCustomHandleInfo like WKTextSelectionRect is a subclass of UITextSelectionRect?

Ah, so this was a sneaky attempt to avoid introducing another compile-time guard for versions of iOS with `UITextSelectionRectCustomHandleInfo` 😅.

I'll add a compile-time guard for this and use it here.

>> Source/WebKit/UIProcess/ios/WKTextSelectionRect.mm:121
>> +    return [[[WKTextSelectionRectCustomHandleInfo alloc] initWithFloatQuad:scaledQuad] autorelease];
> 
> See https://commits.webkit.org/r273062; I think this should be adoptNS().autorelease() for future-ARC-happiness

Done!

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:265
>> +        convertedQuad.setP1(view.contentsToRootView(convertedQuad.p1()));
> 
> There are two other instances of this code (InspectorTimelineAgent and sendTapHighlightForNode), maybe it's time for ScrollView/FrameView to get a FloatQuad version of contentsToRootView??

That's a good point! I'll split this out into a separate patch, and fix all three call sites.

[Wenson Hsieh]

Created attachment 426714 [details]
For EWS

[EWS]

Committed r276388 (236863@main): <https://commits.webkit.org/236863@main>

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