| Summary: | [iOS] Text selection in image overlays should not be limited to rectilinear quads | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||
| Component: | Platform | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bdakin, cdumez, changseok, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hi, kondapallykalyan, megan_gardner, mmaxfield, pdr, thorton, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Wenson Hsieh
2021-04-20 15:35:30 PDT
Created attachment 426609 [details]
Depends on #224820
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?? 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. Created attachment 426714 [details]
For 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]. |