Bug 225788 - Add a way to create `"wheel"` events from gesture/touch events
Summary: Add a way to create `"wheel"` events from gesture/touch events
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: Devin Rousso
URL:
Keywords: InRadar
: 145214 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-05-13 16:45 PDT by Devin Rousso
Modified: 2021-09-21 10:18 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.49 KB, patch)
2021-05-13 16:54 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (35.25 KB, patch)
2021-05-18 20:16 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (35.28 KB, patch)
2021-05-18 22:45 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (34.18 KB, patch)
2021-05-19 15:29 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (40.30 KB, patch)
2021-05-19 15:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-05-13 16:45:23 PDT
.
Comment 1 Devin Rousso 2021-05-13 16:45:35 PDT
<rdar://problem/76714308>
Comment 2 Devin Rousso 2021-05-13 16:54:27 PDT
Created attachment 428577 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-05-13 19:43:23 PDT
Comment on attachment 428577 [details]
Patch

This needs some tests.
Comment 4 Devin Rousso 2021-05-18 20:16:52 PDT
Created attachment 429027 [details]
Patch
Comment 5 Devin Rousso 2021-05-18 22:45:18 PDT
Created attachment 429032 [details]
Patch
Comment 6 Simon Fraser (smfr) 2021-05-19 10:21:57 PDT
Comment on attachment 429032 [details]
Patch

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

> Source/WebCore/platform/PlatformWheelEvent.cpp:49
> +    platformWheelEvent.m_timestamp = platformGestureEvent.timestamp();
> +    platformWheelEvent.m_modifiers = platformGestureEvent.modifiers() | PlatformEvent::Modifier::ControlKey;

Can we give PlatformWheelEvent a PlatformEvent copy ctor that initializes the shared members? Otherwise someone will add something in future and not know to fix this code path.

> Source/WebCore/platform/PlatformWheelEvent.cpp:61
> +        platformWheelEvent.m_phase = deltaY ? PlatformWheelEventPhase::Began : PlatformWheelEventPhase::MayBegin;

This only makes sense if the first event has deltaY=0 and no subsequent event does (MayBegin should only ever be followed by Began or Canceled).

> Source/WebCore/platform/PlatformWheelEvent.cpp:62
> +        platformWheelEvent.m_momentumPhase = deltaY ? PlatformWheelEventPhase::Began : PlatformWheelEventPhase::None;

It doesn't make sense to have an event where both m_phase and m_momentumPhase are PlatformWheelEventPhase::Began. m_phase and m_momentumPhase are really mutually exclusive. There are no momentum phases in this gesture, I think.

> Tools/ChangeLog:42
> +        Make sure to pass all feature flags when generating JS files from IDL files.

Oh finally! But what about DumpRenderTree?

> Tools/WebKitTestRunner/DerivedSources.make:43
> +FRAMEWORK_FLAGS := $(shell echo $(BUILT_PRODUCTS_DIR) $(FRAMEWORK_SEARCH_PATHS) $(SYSTEM_FRAMEWORK_SEARCH_PATHS) | $(PERL) -e 'print "-F " . join(" -F ", split(" ", <>));')
> +HEADER_FLAGS := $(shell echo $(BUILT_PRODUCTS_DIR) $(HEADER_SEARCH_PATHS) $(SYSTEM_HEADER_SEARCH_PATHS) | $(PERL) -e 'print "-I" . join(" -I", split(" ", <>));')
> +FEATURE_AND_PLATFORM_DEFINES := $(shell $(CC) -std=gnu++1z -x c++ -E -P -dM $(SDK_FLAGS) $(TARGET_TRIPLE_FLAGS) $(FRAMEWORK_FLAGS) $(HEADER_FLAGS) -include "wtf/Platform.h" /dev/null | $(PERL) -ne "print if s/\#define ((HAVE_|USE_|ENABLE_|WTF_PLATFORM_)\w+) 1/\1/")

Someone else should review this.

> Tools/WebKitTestRunner/EventSenderProxy.h:94
> +    void scaleGestureStart(double scale);
> +    void scaleGestureChange(double scale);
> +    void scaleGestureEnd(double scale);

Don't these have an origin too?

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:133
> +    CGEventSetIntegerValueField(cgEvent.get(), kCGEventGestureHIDType, 8 /* kIOHIDEventTypeZoom */);

Should we put kIOHIDEventTypeZoom in an SPI header?

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:910
> +    auto event = adoptNS([[EventSenderSyntheticEvent alloc] initMagnifyEventAtLocation:NSMakePoint(m_position.x, m_position.y)

Maybe pull [m_testController->mainWebView()->platformView() into a local variable.
Comment 7 Devin Rousso 2021-05-19 13:21:23 PDT
Comment on attachment 429032 [details]
Patch

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

>> Source/WebCore/platform/PlatformWheelEvent.cpp:62
>> +        platformWheelEvent.m_momentumPhase = deltaY ? PlatformWheelEventPhase::Began : PlatformWheelEventPhase::None;
> 
> It doesn't make sense to have an event where both m_phase and m_momentumPhase are PlatformWheelEventPhase::Began. m_phase and m_momentumPhase are really mutually exclusive. There are no momentum phases in this gesture, I think.

Ah good catch.  I misread `momentumPhaseForEvent` as using `[event phase]` not `[event momentumPhase]`.  I'll double check this.

>> Tools/ChangeLog:42
>> +        Make sure to pass all feature flags when generating JS files from IDL files.
> 
> Oh finally! But what about DumpRenderTree?

I didn't see any IDL files in `DumpRenderTree`.  I can add it though.

>> Tools/WebKitTestRunner/DerivedSources.make:43
>> +FEATURE_AND_PLATFORM_DEFINES := $(shell $(CC) -std=gnu++1z -x c++ -E -P -dM $(SDK_FLAGS) $(TARGET_TRIPLE_FLAGS) $(FRAMEWORK_FLAGS) $(HEADER_FLAGS) -include "wtf/Platform.h" /dev/null | $(PERL) -ne "print if s/\#define ((HAVE_|USE_|ENABLE_|WTF_PLATFORM_)\w+) 1/\1/")
> 
> Someone else should review this.

FYI this is a copypasta from `WebCore/DerivedSources.make`.

>> Tools/WebKitTestRunner/EventSenderProxy.h:94
>> +    void scaleGestureEnd(double scale);
> 
> Don't these have an origin too?

The most recent mouse location is the origin.  I was matching how force events are simulated in that it requires a `moveMouseTo` beforehand.

>> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:133
>> +    CGEventSetIntegerValueField(cgEvent.get(), kCGEventGestureHIDType, 8 /* kIOHIDEventTypeZoom */);
> 
> Should we put kIOHIDEventTypeZoom in an SPI header?

It's actually already in `Source/WebCore/PAL/pal/spi/cocoa/IOKitSPI.h`.  I'll just include it in this file.
Comment 8 Devin Rousso 2021-05-19 15:29:19 PDT
Created attachment 429101 [details]
Patch
Comment 9 Devin Rousso 2021-05-19 15:32:11 PDT
Created attachment 429102 [details]
Patch

oops forgot test
Comment 10 Devin Rousso 2021-05-19 19:04:54 PDT
Comment on attachment 429102 [details]
Patch

test failures are probably unrelated as this patch doesn't actually have an functional changes (other than to how the test runner is compiled)
Comment 11 EWS 2021-05-19 19:26:16 PDT
Committed r277772 (237933@main): <https://commits.webkit.org/237933@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429102 [details].
Comment 12 Tim Horton 2021-08-04 02:44:00 PDT
*** Bug 145214 has been marked as a duplicate of this bug. ***
Comment 13 Simon Fraser (smfr) 2021-09-21 10:18:22 PDT
*** Bug 230545 has been marked as a duplicate of this bug. ***