Bug 214424 - iPad cursor is sometimes slow to change to I-beam (e.g. on reddit.com)
Summary: iPad cursor is sometimes slow to change to I-beam (e.g. on reddit.com)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-16 14:25 PDT by Tim Horton
Modified: 2020-07-16 16:33 PDT (History)
6 users (show)

See Also:


Attachments
Patch (14.29 KB, patch)
2020-07-16 14:26 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (16.28 KB, patch)
2020-07-16 15:38 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2020-07-16 14:25:43 PDT
iPad cursor is sometimes slow to change to I-beam (e.g. on reddit.com)
Comment 1 Tim Horton 2020-07-16 14:26:02 PDT
Created attachment 404482 [details]
Patch
Comment 2 Tim Horton 2020-07-16 14:26:04 PDT
<rdar://problem/59503572>
Comment 3 Tim Horton 2020-07-16 14:28:31 PDT
One thing to note that I forgot to include in the changelog: it's OK that we don't request nodeAtPositionHasDoubleClickHandler be filled in for cursor updates, because trackpad touches will not be allowed to trigger the only gesture recognizer that reads from this bit, AND, the code that does read from it uses a isValid check, which will correctly return NO if the position information was requested without that data.
Comment 4 Wenson Hsieh 2020-07-16 14:39:15 PDT
Comment on attachment 404482 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8675
> +            auto deferredRequest = WTFMove(_deferredCursorInteractionRequest);

Nit - I _think_ we prefer std::exchange for cases like this, where the intention is to reset the thing being moved.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2950
> +        info.nodeAtPositionHasDoubleClickHandler = m_page->mainFrame().nodeRespondingToDoubleClickEvent(request.point, adjustedPoint);

I wonder if `nodeAtPositionHasDoubleClickHandler` should be an Optional<bool> (or maybe a Yes/No/🤷🏻‍♂️ enum class).
Comment 5 Tim Horton 2020-07-16 15:10:00 PDT
(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 404482 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404482&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8675
> > +            auto deferredRequest = WTFMove(_deferredCursorInteractionRequest);
> 
> Nit - I _think_ we prefer std::exchange for cases like this, where the
> intention is to reset the thing being moved.

Right! I never folded that adjustment into my brain.

> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2950
> > +        info.nodeAtPositionHasDoubleClickHandler = m_page->mainFrame().nodeRespondingToDoubleClickEvent(request.point, adjustedPoint);
> 
> I wonder if `nodeAtPositionHasDoubleClickHandler` should be an
> Optional<bool> (or maybe a Yes/No/🤷🏻‍♂️ enum class).

I *almost* Optional<>'d it, but oddly that is not how the other optional things work (instead, they depend on people doing isValid checks). But maybe it's best to just do it, to avoid confusion later.
Comment 6 Tim Horton 2020-07-16 15:38:08 PDT
Created attachment 404489 [details]
Patch
Comment 7 EWS 2020-07-16 16:33:26 PDT
Committed r264484: <https://trac.webkit.org/changeset/264484>

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