Bug 198124 - [iOS] Compatibility mouse events aren't prevented by calling preventDefault() on pointerdown
Summary: [iOS] Compatibility mouse events aren't prevented by calling preventDefault()...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-22 09:24 PDT by Antoine Quint
Modified: 2019-06-21 13:32 PDT (History)
10 users (show)

See Also:


Attachments
Patch (47.69 KB, patch)
2019-05-22 10:23 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (48.45 KB, patch)
2019-05-22 10:34 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (7.61 KB, patch)
2019-06-21 03:29 PDT, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2019-05-22 09:24:32 PDT
Followup work after the fix for https://bugs.webkit.org/show_bug.cgi?id=198072 which was for macOS.
Comment 1 Antoine Quint 2019-05-22 09:24:43 PDT
<rdar://problem/50410863>
Comment 2 Antoine Quint 2019-05-22 10:23:06 PDT
Created attachment 370415 [details]
Patch
Comment 3 Build Bot 2019-05-22 10:25:31 PDT
Attachment 370415 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformMouseEvent.h:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/PlatformMouseEvent.h:74:  Wrong number of spaces before statement. (expected: 31)  [whitespace/indent] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Antoine Quint 2019-05-22 10:34:39 PDT
Created attachment 370417 [details]
Patch
Comment 5 Build Bot 2019-05-22 10:36:42 PDT
Attachment 370417 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformMouseEvent.h:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/PlatformMouseEvent.h:74:  Wrong number of spaces before statement. (expected: 31)  [whitespace/indent] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Tim Horton 2019-05-22 10:47:13 PDT
Comment on attachment 370417 [details]
Patch

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

> Source/WebKit/ChangeLog:376
> -        * NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp: Renamed from Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp.
> +        * NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp: Renamed from Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.pp.

This seems wrong

> Source/WebCore/page/PointerCaptureController.cpp:149
> +void PointerCaptureController::forgetPointer(PointerID pointerId)

Forget! Interesting. Remove? DidRemove? Forget is fine too.

> Source/WebCore/platform/PlatformMouseEvent.h:65
> -                           int clickCount, bool shiftKey, bool ctrlKey, bool altKey, bool metaKey, WallTime timestamp, double force, SyntheticClickType syntheticClickType)
> +                           int clickCount, bool shiftKey, bool ctrlKey, bool altKey, bool metaKey, WallTime timestamp, double force, SyntheticClickType syntheticClickType, PointerID pointerId = mousePointerID())

This is getting ridiculous

> Source/WebKit/UIProcess/ios/WKSyntheticTapGestureRecognizer.h:40
> +@property (nonatomic, weak) UIWebTouchEventsGestureRecognizer* supportingWebTouchEventsGestureRecognizer;
> +@property (nonatomic, readonly) NSNumber* lastActiveTouchIdentifier;

Stars are on the wrong side 🤷‍♂️

> Source/WebKit/UIProcess/ios/WKSyntheticTapGestureRecognizer.m:89
> +            return;

Should this just `break` out of the for() instead of returning from the function entirely? (just imagining someone coming along later and wanting to add code to touchesEnded)

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1040
> +void WebPage::forgetPointer(WebCore::PointerID pointerId)

The name is still bothering me. Maybe it's the super generic "pointer".
Comment 7 Antoine Quint 2019-05-22 11:22:50 PDT
(In reply to Tim Horton from comment #6)
> Comment on attachment 370417 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370417&action=review
> 
> > Source/WebKit/ChangeLog:376
> > -        * NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp: Renamed from Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp.
> > +        * NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp: Renamed from Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.pp.
> 
> This seems wrong

It is. Will fix in commit.

> > Source/WebCore/page/PointerCaptureController.cpp:149
> > +void PointerCaptureController::forgetPointer(PointerID pointerId)
> 
> Forget! Interesting. Remove? DidRemove? Forget is fine too.

Instead of being so directive about the action to perform, I will update the method to be something more indicative about what happened and will rename to "touchWithIdentifierWasRemoved()".

> > Source/WebKit/UIProcess/ios/WKSyntheticTapGestureRecognizer.h:40
> > +@property (nonatomic, weak) UIWebTouchEventsGestureRecognizer* supportingWebTouchEventsGestureRecognizer;
> > +@property (nonatomic, readonly) NSNumber* lastActiveTouchIdentifier;
> 
> Stars are on the wrong side 🤷‍♂️

Will fix in commit.

> > Source/WebKit/UIProcess/ios/WKSyntheticTapGestureRecognizer.m:89
> > +            return;
> 
> Should this just `break` out of the for() instead of returning from the
> function entirely? (just imagining someone coming along later and wanting to
> add code to touchesEnded)

Good point! Will fix in commit.

> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1040
> > +void WebPage::forgetPointer(WebCore::PointerID pointerId)
> 
> The name is still bothering me. Maybe it's the super generic "pointer".

Yeah, see above, this is getting renamed to WebPage::touchWithIdentifierWasRemoved().
Comment 8 Dean Jackson 2019-05-22 12:02:47 PDT
Comment on attachment 370417 [details]
Patch

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

>>> Source/WebKit/ChangeLog:376
>>> +        * NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp: Renamed from Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.pp.
>> 
>> This seems wrong
> 
> It is. Will fix in commit.

We just announced Apple's new programming language! WWDC leak!

>> Source/WebCore/platform/PlatformMouseEvent.h:65
>> +                           int clickCount, bool shiftKey, bool ctrlKey, bool altKey, bool metaKey, WallTime timestamp, double force, SyntheticClickType syntheticClickType, PointerID pointerId = mousePointerID())
> 
> This is getting ridiculous

Yeah, can we make a PlatformMouseEventInfo or Data? Which would be a subclass of PlatformEventInfo?

> Source/WebCore/platform/PointerID.h:32
> +inline PointerID mousePointerID() { return 1; }

Why is this a function rather than a static PointerID mousePointerID = 1; ?
Comment 9 Antoine Quint 2019-05-22 12:54:51 PDT
(In reply to Dean Jackson from comment #8)
> Comment on attachment 370417 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370417&action=review
> 
> > Source/WebCore/platform/PointerID.h:32
> > +inline PointerID mousePointerID() { return 1; }
> 
> Why is this a function rather than a static PointerID mousePointerID = 1; ?

Will fix in commit.
Comment 10 Antoine Quint 2019-05-22 12:58:24 PDT
Committed r245639: <https://trac.webkit.org/changeset/245639>
Comment 11 Antoine Quint 2019-05-22 22:56:57 PDT
This broke the tvOS build, see https://bugs.webkit.org/show_bug.cgi?id=198172. This was fixed in https://trac.webkit.org/changeset/245673.
Comment 12 Antoine Quint 2019-06-20 09:48:36 PDT
Reopening, this doesn't work when the synthetic tap is delayed due to double-tap-to-zoom. So the patch worked fine on iPadOS but not with iOS on an iPhone.
Comment 13 Antoine Quint 2019-06-21 03:29:46 PDT
Created attachment 372625 [details]
Patch
Comment 14 Antoine Quint 2019-06-21 13:32:02 PDT
Committed r246693: <https://trac.webkit.org/changeset/246693>