Bug 219024 - WebDriver: add support for pen pointer events
Summary: WebDriver: add support for pen pointer events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 166679
  Show dependency treegraph
 
Reported: 2020-11-17 01:35 PST by Carlos Garcia Campos
Modified: 2020-12-09 09:03 PST (History)
19 users (show)

See Also:


Attachments
WIP patch (101.74 KB, patch)
2020-11-17 01:37 PST, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (102.14 KB, patch)
2020-11-17 02:17 PST, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (102.23 KB, patch)
2020-11-17 02:29 PST, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (140.26 KB, patch)
2020-11-17 06:08 PST, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (149.01 KB, patch)
2020-11-23 03:07 PST, Carlos Garcia Campos
bburg: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (148.80 KB, patch)
2020-12-09 03:51 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-11-17 01:35:47 PST
We are not handling mouse events with pen pointer type.
Comment 1 Carlos Garcia Campos 2020-11-17 01:37:51 PST
Created attachment 414318 [details]
WIP patch

This is WIP, just to check if it builds in other ports and the result of imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_hoverable_pointers.html
Comment 2 EWS Watchlist 2020-11-17 01:38:38 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 3 Carlos Garcia Campos 2020-11-17 02:17:11 PST
Created attachment 414323 [details]
WIP patch
Comment 4 Carlos Garcia Campos 2020-11-17 02:29:29 PST
Created attachment 414324 [details]
WIP patch
Comment 5 Carlos Garcia Campos 2020-11-17 06:08:54 PST
Created attachment 414334 [details]
WIP patch
Comment 6 Carlos Garcia Campos 2020-11-17 06:48:05 PST
I don't understand the build failures on tv and watch platform. I need some help with those.
Comment 7 Tim Horton 2020-11-17 07:06:39 PST
(In reply to Carlos Garcia Campos from comment #6)
> I don't understand the build failures on tv and watch platform. I need some
> help with those.

It's a classic unified source 'using namespace' conflict. Might be an indirect one (WebCore -> WebKit -> top-level). I've applied your patch and started a build, I'll see if I can find the minimal change required to get things back on track.
Comment 8 Carlos Garcia Campos 2020-11-17 07:10:55 PST
(In reply to Tim Horton from comment #7)
> (In reply to Carlos Garcia Campos from comment #6)
> > I don't understand the build failures on tv and watch platform. I need some
> > help with those.
> 
> It's a classic unified source 'using namespace' conflict. Might be an
> indirect one (WebCore -> WebKit -> top-level). I've applied your patch and
> started a build, I'll see if I can find the minimal change required to get
> things back on track.

Thanks!
Comment 9 Tim Horton 2020-11-17 08:05:36 PST
I landed http://trac.webkit.org/changeset/269901/webkit and hit the retry button here (no idea if I waited long enough between doing those things, though... but it should be OK eventually).
Comment 10 Carlos Garcia Campos 2020-11-18 00:29:41 PST
It worked, thank you!
Comment 11 Carlos Garcia Campos 2020-11-23 03:07:12 PST
Created attachment 414798 [details]
Patch
Comment 12 Carlos Garcia Campos 2020-11-23 05:20:03 PST
Comment on attachment 414798 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        Update expectations of
> +        imported/w3c/web-platform-tests/pointerevents/pointerevent_attributes_hoverable_pointers.html. The test is now
> +        timing out, but because tesdriver doesn't correctly handle the iframe element.

See bug #219255
Comment 13 Radar WebKit Bug Importer 2020-11-26 03:18:19 PST
<rdar://problem/71747688>
Comment 14 BJ Burg 2020-12-04 08:51:00 PST
Comment on attachment 414798 [details]
Patch

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

r=me, overall the approach looks straightforward.

Unfortunately, watchOS build is still failing:

In file included from /Volumes/Data/worker/watchOS-7-Build-EWS/build/WebKitBuild/Release-watchos/DerivedSources/WebKit2/unified-sources/UnifiedSource46-mm.mm:8:
/Volumes/Data/worker/watchOS-7-Build-EWS/build/Source/WebKit/UIProcess/ios/forms/WKFocusedFormControlView.mm:134:27: error: reference to 'UIEvent' is ambiguous
- (BOOL)handleWheelEvent:(UIEvent *)event
                          ^

This looks like another UnifiedSources thing, ugh. I'll try to figure out what the right include is.

> Source/WebKit/Shared/WebMouseEvent.h:73
> +    const String& pointerType() const { return m_pointerType; }

It's a little weird to me that the pointerType is a string that we pass around. Is there a reason to not use an enum?
Comment 15 Wenson Hsieh 2020-12-04 09:22:20 PST
(In reply to Brian Burg from comment #14)
> Comment on attachment 414798 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414798&action=review
> 
> r=me, overall the approach looks straightforward.
> 
> Unfortunately, watchOS build is still failing:
> 
> In file included from
> /Volumes/Data/worker/watchOS-7-Build-EWS/build/WebKitBuild/Release-watchos/
> DerivedSources/WebKit2/unified-sources/UnifiedSource46-mm.mm:8:
> /Volumes/Data/worker/watchOS-7-Build-EWS/build/Source/WebKit/UIProcess/ios/
> forms/WKFocusedFormControlView.mm:134:27: error: reference to 'UIEvent' is
> ambiguous
> - (BOOL)handleWheelEvent:(UIEvent *)event
>                           ^
> 
> This looks like another UnifiedSources thing, ugh. I'll try to figure out
> what the right include is.

I suspect this is happening because we're trying to import `PointerEvent.h` in these UI process-side headers, which also pulls in DOM/bindings-related stuff like WindowProxy, FrameView, Node, etc.

Instead, we should probably pull the enum types and declarations we need to reference out into a separate header, and just import that instead of `PointerEvent.h`.

> 
> > Source/WebKit/Shared/WebMouseEvent.h:73
> > +    const String& pointerType() const { return m_pointerType; }
> 
> It's a little weird to me that the pointerType is a string that we pass
> around. Is there a reason to not use an enum?
Comment 16 Carlos Garcia Campos 2020-12-09 03:49:28 PST
Comment on attachment 414798 [details]
Patch

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

>>> Source/WebKit/Shared/WebMouseEvent.h:73
>>> +    const String& pointerType() const { return m_pointerType; }
>> 
>> It's a little weird to me that the pointerType is a string that we pass around. Is there a reason to not use an enum?
> 
> 

I guess it's because pointerType is defined in the spec as a DOMString. The spec says it can be mouse, pen and touch, but also other devices that must be prefixed. I guess we could use an enum internally indeed, but it's probably out fo the scope of this bug.
Comment 17 Carlos Garcia Campos 2020-12-09 03:51:46 PST
Created attachment 415738 [details]
Patch for landing
Comment 18 Carlos Garcia Campos 2020-12-09 08:54:38 PST
EWS is green this time!
Comment 19 EWS 2020-12-09 09:03:36 PST
Committed r270582: <https://trac.webkit.org/changeset/270582>

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