Bug 220454 – Double tap to select does not work if the page clears selections on tap, like grammarly.com does

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

[Megan Gardner]

Double tap to select does not work if the page clears selections on tap, like Grammerly.com does.

[Megan Gardner]

Created attachment 417239 [details]
Patch

[Megan Gardner]

Created attachment 417313 [details]
Patch

[Wenson Hsieh]

Comment on attachment 417313 [details]
Patch

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

The general approach (deferring the selection change when performing a synthetic click) seems reasonable. Should we also invoke the selection change completion handler in the case where we don't end up completing the synthetic click?

> Source/WebKit/ChangeLog:15
> +        Reviewed by NOBODY (OOPS!).

Nit - reviewed by line should move up before the description.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1830
> +    WTF::Function<void()> _selectionChangedBlock;

This should probably be something like `m_selectionChangedHandler` or `m_selectionChangedFunction`, since it's not an Objective-C block (and it's also a member of a C++ class).

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2137
> +

Nit - extra newline here.

> LayoutTests/fast/events/touch/ios/double-tap-on-editable-content-for-selection-with-clear-on-touch.html:52
> +            var editableElement =document.getElementById('editable');

Nit - space after the =

[Megan Gardner]

Created attachment 417319 [details]
Patch

[Megan Gardner]

Created attachment 417393 [details]
Patch

[Megan Gardner]

Created attachment 417394 [details]
Patch

[Wenson Hsieh]

Comment on attachment 417394 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Double tap to select content did not work on grammerly.com on iPad because this is a desktop website run on a touch-based device,

Nit - grammerly.com => grammarly.com

> Source/WebKit/ChangeLog:14
> +        these two events. This makes for a more expected change of events, and does not let grammerly.com clear a valid selection based off of synthetic 

Ditto.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:856
> +    if (auto selectionChangedHandler = std::exchange(m_selectionChangedHandler, nil))

Nit - nil => { }.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1172
> +    if (auto selectionChangedHandler = std::exchange(m_selectionChangedHandler, nil))

Nit - nil => { }.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1191
> +    if (auto selectionChangedHandler = std::exchange(m_selectionChangedHandler, nil))

Nit - nil => { }.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2141
> +

We should assert that `m_selectionChangedHandler` is not set here, and/or invoke the previous handler if it is set.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2142
> +    m_selectionChangedHandler = [point, granularity, isInteractingWithFocusedElement, completionHandler = WTFMove(completionHandler), webPage = makeWeakPtr(*this), this]()mutable {

Nit - space before the `mutable`.

[Megan Gardner]

Created attachment 417420 [details]
Patch for landing

[Megan Gardner]

rdar://67757411

[EWS]

Committed r271386: <https://trac.webkit.org/changeset/271386>

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