Bug 231234 – Allow text selection to flip.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Allow text selection to flip.

[Megan Gardner]

Created attachment 440221 [details]
Patch

[Wenson Hsieh]

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)

[Wenson Hsieh]

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.

[Alexey Proskuryakov]

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 :-)

[Megan Gardner]

Created attachment 440272 [details]
Patch

[Wenson Hsieh]

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.

[Megan Gardner]

Created attachment 440321 [details]
Patch

[Megan Gardner]

Created attachment 440363 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/83934776>