Bug 231234

Summary: Allow text selection to flip.
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Megan Gardner 2021-10-05 09:09:48 PDT
Allow text selection to flip.
Comment 1 Megan Gardner 2021-10-05 09:21:16 PDT
Created attachment 440221 [details]
Patch
Comment 2 Wenson Hsieh 2021-10-05 09:34:40 PDT
Comment on attachment 440221 [details]
Patch

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

Nice! r=mews

> Source/WTF/ChangeLog:9
> +        Add an internal flag to guard text selectin flipping while

Nit - selectin => selection

> Source/WebKit/ChangeLog:11
> +        This is currently guarded behind and off-by-default flag so that we can

Nit - "behind and" => "behind an"

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1506
> +static std::optional<SimpleRange> rangeForPointInRootViewCoordinates(Frame& frame, const IntPoint& pointInRootViewCoordinates, bool baseIsStart, bool selectionFlippingEnabled, bool &selectionFlipped)

Nit - instead of an out-reference (`bool& selectionFlipped`), I think we generally prefer to coalesce that into the return value in modern code (i.e. make this helper function either return `std::pair<SimpleRange, bool>`, or `std::pair<SimpleRange, SelectionWasFlipped>` where `SelectionWasFlipped` is a strongly typed enum)
Comment 3 Wenson Hsieh 2021-10-05 09:36:44 PDT
Comment on attachment 440221 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1506
>> +static std::optional<SimpleRange> rangeForPointInRootViewCoordinates(Frame& frame, const IntPoint& pointInRootViewCoordinates, bool baseIsStart, bool selectionFlippingEnabled, bool &selectionFlipped)
> 
> Nit - instead of an out-reference (`bool& selectionFlipped`), I think we generally prefer to coalesce that into the return value in modern code (i.e. make this helper function either return `std::pair<SimpleRange, bool>`, or `std::pair<SimpleRange, SelectionWasFlipped>` where `SelectionWasFlipped` is a strongly typed enum)

This would also allow you to do something like `std::tie(range, selectionFlipped) = rangeForPointInRootViewCoordinates(frame, point, baseIsStart, selectionFlippingEnabled());` at the call site.
Comment 4 Alexey Proskuryakov 2021-10-05 10:54:07 PDT
Comment on attachment 440221 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        In order to bring webkit more in line with UIKit, allow text seleciton to flip.

As long as we are nitpicking on "selection" spelling, here is another typo :-)
Comment 5 Megan Gardner 2021-10-05 15:01:11 PDT
Created attachment 440272 [details]
Patch
Comment 6 Wenson Hsieh 2021-10-05 18:51:35 PDT
Comment on attachment 440272 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1508
> +static std::pair<std::optional<SimpleRange>, WebKit::SelectionWasFlipped> rangeForPointInRootViewCoordinates(Frame& frame, const IntPoint& pointInRootViewCoordinates, bool baseIsStart, bool selectionFlippingEnabled)

Nit - you can omit the `WebKit::` here.
Comment 7 Megan Gardner 2021-10-05 19:10:10 PDT
Created attachment 440321 [details]
Patch
Comment 8 Megan Gardner 2021-10-06 07:26:03 PDT
Created attachment 440363 [details]
Patch for landing
Comment 9 EWS 2021-10-06 08:23:12 PDT
Committed r283619 (242571@main): <https://commits.webkit.org/242571@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440363 [details].
Comment 10 Radar WebKit Bug Importer 2021-10-06 08:24:23 PDT
<rdar://problem/83934776>