Bug 210071 - Trackpad and Mouse scroll events on iPad only fire "pointermove" -- not "wheel"
Summary: Trackpad and Mouse scroll events on iPad only fire "pointermove" -- not "wheel"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: Other
Hardware: iPhone / iPad All
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-06 13:41 PDT by moogcharlie
Modified: 2021-01-24 01:27 PST (History)
26 users (show)

See Also:


Attachments
Patch (36.74 KB, patch)
2020-12-10 04:04 PST, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (37.38 KB, patch)
2020-12-10 04:16 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (37.62 KB, patch)
2020-12-10 04:18 PST, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (38.01 KB, patch)
2020-12-10 04:52 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (40.73 KB, patch)
2020-12-10 17:27 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (40.80 KB, patch)
2020-12-11 00:42 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (41.27 KB, patch)
2020-12-11 14:01 PST, 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 moogcharlie 2020-04-06 13:41:02 PDT
When scrolling with a trackpad/mouse on iPadOS 13.4, the browser only fires `pointermove` events. These events are not sufficient to simulate scroll interactions where a `deltaY` and deltaX are needed and `pointermove` is useless because the cursor is stationary. WebKit should fire `wheel` events!
Comment 1 Radar WebKit Bug Importer 2020-04-07 11:46:38 PDT
<rdar://problem/61403043>
Comment 2 Tim Horton 2020-04-07 12:40:58 PDT
You are not wrong!
Comment 3 Tim Horton 2020-04-07 12:41:51 PDT
If you know of specific sites that are impacted, please share. We are aware of the lack of wheel events.
Comment 4 moogcharlie 2020-04-08 08:22:50 PDT
Maybe not what you're looking for, but complex interfaces like the one linked below typically handle scrolling manually. These applications are usually behind logins which makes it difficult for me to share more examples. 

https://vscode-web-test-playground.azurewebsites.net/?enter=true
Comment 5 Sebastian 2020-05-12 09:17:33 PDT
Since moogcharlies example is no longer online, here is a docker instance of the code-server editor that demonstrates the behaviour: 

http://vultr01.p2lab.com:8080
password: b174c59894a38db016661d57

Just open the file ./project/lorem.txt and try to scroll with two fingers. 

I replicated the issue using the trackpad on the iPad Magic Keyboard on the 2020 iPad Pro with iPadOS 13.4 installed.
Comment 6 Alexandre Santos 2020-07-26 06:40:45 PDT
This is happening in more projects than I realized. Hope soon I can do more work done with iPadOS and trackpad.
Comment 7 rigel 2020-07-29 17:19:56 PDT
Can be reproduced easily here: https://ace.c9.io/build/kitchen-sink.html

I would love to code more with my iPad Pro + Magic Keyboard :)
Comment 8 Jaimyn 2020-09-06 06:28:25 PDT
Just wanted to say I’ve been trying to use code-server (in browser VS Code) and this bug is unfortunately a show stopper. It would be great to be able to use my iPad and magic keyboard to code in the browser. :)
Comment 9 KR 2020-09-26 19:32:27 PDT
Same here, iPad iOS 14 user with magic keyboard, code-server is not usable as it requires constantly touching the screen for scrolling.
Also overleaf.com, the most prominent online Latex editor, has the same issue: scrolling with the trackpad results in the whole page scrolling instead of the code editor window (free accounts available if testing needed), the PDF preview window works fine instead.
Is there a way to prioritize this? As the availability of native apps of this kind is constrained on the iPad, these web solutions are critical to a more serious use of the iPad...
Comment 10 J.R. 2020-10-02 02:13:49 PDT
This, alongside the fact that I also can‘t use a third party browser engine on iOS which would behave differently, frustrates me to a point that I consider returning my newly bought iPad Pro + Magic Keyboard, as I can‘t really do the productive work I was hoping to do on it.
Comment 11 KR 2020-10-10 19:04:22 PDT
This ticket should be updated: the issue persists with iOS 14 latest.
Comment 12 Anmol Sethi 2020-10-20 23:56:05 PDT
Could reproduce this within the iOS simulator with a debug build of master. Scrolling doesn't work on https://ace.c9.io/build/kitchen-sink.html!

As far as I can tell I can't build webkit for iPad and run it on there directly so I assume there's some closed source code related to running WebKit on iOS devices.

But since I can reproduce in the simulator, the bug/problem is very likely in the open source code so hopefully I can contribute a fix.
Comment 13 Tim Horton 2020-10-21 00:11:18 PDT
(In reply to Anmol Sethi from comment #12)
> But since I can reproduce in the simulator, the bug/problem is very likely
> in the open source code so hopefully I can contribute a fix.

While ordinarily I would welcome and encourage help implementing this missing feature, fixing this particular thing in a performant way (without e.g. synchronous IPC on every scroll event) is going to involve coordination with other parts of iOS, so would be pretty tricky to work on as an open source patch (but you are definitely welcome and encouraged to contribute to WebKit in other ways if you want!).

Also, do know that while I can't comment on the timing of fixes, I am absolutely aware of this and know that it is important.
Comment 14 Ian Ownbey 2020-11-08 13:51:08 PST
This is also broken on the web IDE for Ethereum Studio (iPad Air with magic keyboard iPad OS 14.2) https://studio.ethereum.org/5fa8653126b473001237039f?openFile=contracts/HelloWorld.sol
Comment 15 Tim Horton 2020-12-10 04:04:12 PST
Created attachment 415851 [details]
Patch
Comment 16 Tim Horton 2020-12-10 04:04:36 PST
Test is incomplete, and there are a few weird bugs, but we can start the review process.
Comment 17 Tim Horton 2020-12-10 04:16:36 PST
Created attachment 415852 [details]
Patch
Comment 18 Tim Horton 2020-12-10 04:16:51 PST
Separately we should see if there are any normal wheel event layout tests we can turn on.
Comment 19 Tim Horton 2020-12-10 04:18:15 PST
Created attachment 415853 [details]
Patch
Comment 20 Tim Horton 2020-12-10 04:52:07 PST
Created attachment 415856 [details]
Patch
Comment 21 Tim Horton 2020-12-10 05:01:02 PST
Comment on attachment 415856 [details]
Patch

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

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1649
> +        completion(false);

s/false/NO/

> Source/WebKit/UIProcess/WebPageProxy.cpp:2640
> +void WebPageProxy::dispatchWheelEventWithoutScrolling(const WebWheelEvent& event, bool isFirstEventInGesture, bool hasNonPassiveWheelHandlers, CompletionHandler<void(bool)>&& completionHandler)

Simon is going to want real types for the arguments probably

> Tools/TestWebKitAPI/Tests/ios/WKScrollViewTests.mm:124
> +        "<style>#scrollable { width: 200px; height: 200px; }</style>"

Not actually scrollable, probably shouldn't use that name.
Comment 22 Tim Horton 2020-12-10 05:03:45 PST
(In reply to Tim Horton from comment #16)
> Test is incomplete, and there are a few weird bugs, but we can start the
> review process.

Weird bugs (for follow ups):
- moogmusic + various custom test pages work, Zara and VSCode don't. Not sure why.
- Sometimes when rubber-banding with outstanding async scroll events, the scroll view buzzes between two scroll offsets. This might not be our bug.
Comment 23 Simon Fraser (smfr) 2020-12-10 11:13:36 PST
Comment on attachment 415856 [details]
Patch

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

> Source/WTF/wtf/PlatformEnableCocoa.h:639
> +#if !defined(ENABLE_WHEEL_EVENT_REGIONS) && (PLATFORM(IOS) || PLATFORM(MACCATALYST))
> +#define ENABLE_WHEEL_EVENT_REGIONS 1
> +#endif

Can't you just combine this with the:
#if !defined(ENABLE_WHEEL_EVENT_REGIONS) && PLATFORM(MAC)
#define ENABLE_WHEEL_EVENT_REGIONS 1
#endif

higher up?

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1602
> +    WebCore::IntPoint scrollLocation = WebCore::roundedIntPoint([scrollEvent locationInView:_contentView.get()]);

Is this right with insets, zooming etc etc?

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1605
> +    bool hasPassiveWheelHandlers = eventListeners.contains(WebCore::EventListenerRegionType::Wheel);

This isn't the region of passive handlers. This is the region of all handlers, so this should be hasWheelHandlers

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1606
> +    bool hasNonPassiveWheelHandlers = eventListeners.contains(WebCore::EventListenerRegionType::NonPassiveWheel);

This region is the subset with active handlers.

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1608
> +    if (!hasPassiveWheelHandlers && !hasNonPassiveWheelHandlers) {

I think you only want to early return on ! hasWheelHandlers

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1613
> +    if (scrollEvent.phase == UIScrollPhaseMayBegin) {

Test this before paying the cost of looking at regions?

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1622
> +    bool isPreventable = hasNonPassiveWheelHandlers && _currentScrollGestureWasInitiallyPrevented.valueOr(isFirstEventInGesture);

We call this Cancelable

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1625
> +    WebKit::WebWheelEvent event = {

Can we share this with other WebWheelEvent building code? It would be easy to get things wrong, e.g. approximateWallTime(). How are there no phases passed down?

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1636
> +    _page->dispatchWheelEventWithoutScrolling(event, isFirstEventInGesture, hasNonPassiveWheelHandlers, [weakSelf = WeakObjCPtr<WKWebView>(self), strongCompletion = makeBlockPtr(completion), isPreventable, isFirstEventInGesture](bool handled) {

isFirstEventInGesture should be available by inspecting the event

> Source/WebKit/UIProcess/PageClient.h:473
> +    virtual void handleAsynchronousPreventableScrollEvent(UIScrollView *, UIScrollEvent *, void (^completion)(BOOL handled)) = 0;

Having Objective-C in this header seems wrong?
Cancelable

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.h:69
> +    void handleAsynchronousPreventableScrollEvent(UIScrollView *, UIScrollEvent *, void (^completion)(BOOL handled));

Cancelable

> Source/WebKit/UIProcess/WebPageProxy.h:1838
> +    void dispatchWheelEventWithoutScrolling(const WebWheelEvent&, bool isFirstEventInGesture, bool hasNonPassiveWheelHandlers, CompletionHandler<void(bool)>&&);

Instead of the "WithoutScrolling" can you pass OptionSet<WheelEventProcessingSteps> ?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2886
> +    bool isPreventable = hasNonPassiveWheelHandlers && m_currentScrollGestureWasInitiallyPrevented.valueOr(isFirstEventInGesture);

isCancelable

> Source/WebKit/WebProcess/WebPage/WebPage.h:1374
> +    void dispatchWheelEventWithoutScrolling(const WebWheelEvent&, bool isFirstEventInGesture, bool hasNonPassiveWheelHandlers, CompletionHandler<void(bool)>&&);

Let's call them activeHandlers

> Source/WebKit/WebProcess/WebPage/WebPage.h:2187
> +    Optional<bool> m_currentScrollGestureWasInitiallyPrevented;

I think this is just an alias of EventHandler's     Optional<WheelScrollGestureState> m_wheelScrollGestureState ? Can you avoid storing state in two places?
Comment 24 Tim Horton 2020-12-10 12:41:19 PST
(In reply to Simon Fraser (smfr) from comment #23)
> Comment on attachment 415856 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415856&action=review
> 
> > Source/WTF/wtf/PlatformEnableCocoa.h:639
> > +#if !defined(ENABLE_WHEEL_EVENT_REGIONS) && (PLATFORM(IOS) || PLATFORM(MACCATALYST))
> > +#define ENABLE_WHEEL_EVENT_REGIONS 1
> > +#endif
> 
> Can't you just combine this with the:
> #if !defined(ENABLE_WHEEL_EVENT_REGIONS) && PLATFORM(MAC)
> #define ENABLE_WHEEL_EVENT_REGIONS 1
> #endif
> 
> higher up?

Sure.

> > Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1602
> > +    WebCore::IntPoint scrollLocation = WebCore::roundedIntPoint([scrollEvent locationInView:_contentView.get()]);
> 
> Is this right with insets, zooming etc etc?

It's always in content coordinates. It seems to work perfectly with insets and scrolling and zooming.

> > Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1605
> > +    bool hasPassiveWheelHandlers = eventListeners.contains(WebCore::EventListenerRegionType::Wheel);
> 
> This isn't the region of passive handlers. This is the region of all
> handlers, so this should be hasWheelHandlers

Ah!

> > Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1606
> > +    bool hasNonPassiveWheelHandlers = eventListeners.contains(WebCore::EventListenerRegionType::NonPassiveWheel);
> 
> This region is the subset with active handlers.
> 
> > Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1608
> > +    if (!hasPassiveWheelHandlers && !hasNonPassiveWheelHandlers) {
> 
> I think you only want to early return on ! hasWheelHandlers

Oh! Not a correctness problem, just unnecessary. Will change!

> > Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1613
> > +    if (scrollEvent.phase == UIScrollPhaseMayBegin) {
> 
> Test this before paying the cost of looking at regions?

Of course.

> > Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1622
> > +    bool isPreventable = hasNonPassiveWheelHandlers && _currentScrollGestureWasInitiallyPrevented.valueOr(isFirstEventInGesture);
> 
> We call this Cancelable

Sure!

> > Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1625
> > +    WebKit::WebWheelEvent event = {
> 
> Can we share this with other WebWheelEvent building code? It would be easy
> to get things wrong, e.g. approximateWallTime(). How are there no phases
> passed down?

I will look. I don't think we /need/ the phases, where this is going, but since it appears they are accurate, we can plumb them along?

> > Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1636
> > +    _page->dispatchWheelEventWithoutScrolling(event, isFirstEventInGesture, hasNonPassiveWheelHandlers, [weakSelf = WeakObjCPtr<WKWebView>(self), strongCompletion = makeBlockPtr(completion), isPreventable, isFirstEventInGesture](bool handled) {
> 
> isFirstEventInGesture should be available by inspecting the event

Only if we plumb the phases :)

> > Source/WebKit/UIProcess/PageClient.h:473
> > +    virtual void handleAsynchronousPreventableScrollEvent(UIScrollView *, UIScrollEvent *, void (^completion)(BOOL handled)) = 0;
> 
> Having Objective-C in this header seems wrong?
> Cancelable

Which ObjC! We've got other ObjC ptrs (and they're valid in C) and blocks are C.

> > Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.h:69
> > +    void handleAsynchronousPreventableScrollEvent(UIScrollView *, UIScrollEvent *, void (^completion)(BOOL handled));
> 
> Cancelable

Sure OK :D

> > Source/WebKit/UIProcess/WebPageProxy.h:1838
> > +    void dispatchWheelEventWithoutScrolling(const WebWheelEvent&, bool isFirstEventInGesture, bool hasNonPassiveWheelHandlers, CompletionHandler<void(bool)>&&);
> 
> Instead of the "WithoutScrolling" can you pass
> OptionSet<WheelEventProcessingSteps> ?

Here's the reason I didn't: after the first event in a gesture, there is a time window before the UI process knows if it was handled or not, in which we might get more events (and send them to the WP). The WP already knows whether we need to Blocking or NonBlocking, but the UI process doesn't.

So I could send empty vs. MainThreadForScrolling/ScrollingThread, and have the WP add the DOMEventDispatch ones, but can't send a fully formed WheelEventProcessingSteps (and honestly, the MainThreadForScrolling/ScrollingThread ones would be a bit weird for the UI process to deal in too, though I'd never be sending those).

Thoughts?

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:2886
> > +    bool isPreventable = hasNonPassiveWheelHandlers && m_currentScrollGestureWasInitiallyPrevented.valueOr(isFirstEventInGesture);
> 
> isCancelable
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.h:1374
> > +    void dispatchWheelEventWithoutScrolling(const WebWheelEvent&, bool isFirstEventInGesture, bool hasNonPassiveWheelHandlers, CompletionHandler<void(bool)>&&);
> 
> Let's call them activeHandlers

OK!

> > Source/WebKit/WebProcess/WebPage/WebPage.h:2187
> > +    Optional<bool> m_currentScrollGestureWasInitiallyPrevented;
> 
> I think this is just an alias of EventHandler's    
> Optional<WheelScrollGestureState> m_wheelScrollGestureState ? Can you avoid
> storing state in two places?

Will check!
Comment 25 Tim Horton 2020-12-10 17:27:55 PST
Created attachment 415951 [details]
Patch
Comment 26 Tim Horton 2020-12-11 00:42:59 PST
Created attachment 415975 [details]
Patch
Comment 27 Simon Fraser (smfr) 2020-12-11 10:12:20 PST
Comment on attachment 415975 [details]
Patch

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

> Source/WebKit/ChangeLog:29
> +        If the event is not preventable, we will synchronously reply that it was

cancelable

> Source/WebKit/Shared/ios/WebIOSEventFactory.mm:162
> +        WebKit::WebEvent::Wheel, scrollLocation, scrollLocation, delta, { } /* wheelTicks */, WebKit::WebWheelEvent::Granularity::ScrollByPixelWheelEvent, false, toWebPhase(event.phase), WebKit::WebWheelEvent::PhaseNone, true, 1, delta, { }, MonotonicTime::fromRawSeconds(event.timestamp).approximateWallTime()

I would put this one per line but  ¯\_(ツ)_/¯

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h:232
> +    Optional<bool> _currentScrollGestureWasInitiallyPrevented;

Can this be a Optional<WheelScrollGestureState>?

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1627
> +        if (!strongSelf)
> +            return;

Is it OK to not call the completion handler here?

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1629
> +        if (isCancelable) {

OK to not call strongCompletion() if not cancelable?
Comment 28 Tim Horton 2020-12-11 10:39:23 PST
Comment on attachment 415975 [details]
Patch

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

>> Source/WebKit/Shared/ios/WebIOSEventFactory.mm:162
>> +        WebKit::WebEvent::Wheel, scrollLocation, scrollLocation, delta, { } /* wheelTicks */, WebKit::WebWheelEvent::Granularity::ScrollByPixelWheelEvent, false, toWebPhase(event.phase), WebKit::WebWheelEvent::PhaseNone, true, 1, delta, { }, MonotonicTime::fromRawSeconds(event.timestamp).approximateWallTime()
> 
> I would put this one per line but  ¯\_(ツ)_/¯

Heh, I had it one per line and thought someone would not like it. I'll put it back!

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h:232
>> +    Optional<bool> _currentScrollGestureWasInitiallyPrevented;
> 
> Can this be a Optional<WheelScrollGestureState>?

Why not!

>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1627
>> +            return;
> 
> Is it OK to not call the completion handler here?

Guess we should probably still call it. Don't think it matters.

>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1629
>> +        if (isCancelable) {
> 
> OK to not call strongCompletion() if not cancelable?

If it's not cancelable, we called it synchronously below!!
Comment 29 Tim Horton 2020-12-11 14:01:36 PST
Created attachment 416045 [details]
Patch
Comment 30 EWS 2020-12-11 14:59:49 PST
Committed r270712: <https://trac.webkit.org/changeset/270712>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416045 [details].
Comment 31 Ryan Haddad 2020-12-14 12:39:03 PST
Follow up build fix in https://trac.webkit.org/changeset/270759/webkit
Comment 32 Lei Zhang 2021-01-05 02:31:46 PST
(In reply to Tim Horton from comment #29)
> Created attachment 416045 [details]
> Patch

hi, Tim. Will this patch take effect in the next iPadOS version?
Comment 33 KR 2021-01-23 22:57:45 PST
Is it confirmed that the (In reply to Tim Horton from comment #22)
> (In reply to Tim Horton from comment #16)
> > Test is incomplete, and there are a few weird bugs, but we can start the
> > review process.
> 
> Weird bugs (for follow ups):
> - moogmusic + various custom test pages work, Zara and VSCode don't. Not
> sure why.
> - Sometimes when rubber-banding with outstanding async scroll events, the
> scroll view buzzes between two scroll offsets. This might not be our bug.

Is it confirmed that the final fix solves the vscode issue?
It's not clear whether this comment still applies or not.

Can be tested at:
https://vscode-web-test-playground.azurewebsites.net/?enter=true

Thank you
Comment 34 Tim Horton 2021-01-24 01:27:11 PST
(In reply to KR from comment #33)
> Is it confirmed that the (In reply to Tim Horton from comment #22)
> > (In reply to Tim Horton from comment #16)
> > > Test is incomplete, and there are a few weird bugs, but we can start the
> > > review process.
> > 
> > Weird bugs (for follow ups):
> > - moogmusic + various custom test pages work, Zara and VSCode don't. Not
> > sure why.
> > - Sometimes when rubber-banding with outstanding async scroll events, the
> > scroll view buzzes between two scroll offsets. This might not be our bug.
> 
> Is it confirmed that the final fix solves the vscode issue?
> It's not clear whether this comment still applies or not.
> 
> Can be tested at:
> https://vscode-web-test-playground.azurewebsites.net/?enter=true
> 
> Thank you

Yeah, there was a follow up that fixed VSCode IIRC: https://trac.webkit.org/changeset/271000/webkit