Bug 198124 – [iOS] Compatibility mouse events aren't prevented by calling preventDefault() on pointerdown

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Antoine Quint]

Followup work after the fix for https://bugs.webkit.org/show_bug.cgi?id=198072 which was for macOS.

[Antoine Quint]

<rdar://problem/50410863>

[Antoine Quint]

Created attachment 370415 [details]
Patch

[EWS Watchlist]

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.

[Antoine Quint]

Created attachment 370417 [details]
Patch

[EWS Watchlist]

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.

[Tim Horton]

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".

[Antoine Quint]

(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().

[Dean Jackson]

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; ?

[Antoine Quint]

(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.

[Antoine Quint]

Committed r245639: <https://trac.webkit.org/changeset/245639>

[Antoine Quint]

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.

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 372625 [details]
Patch

[Antoine Quint]

Committed r246693: <https://trac.webkit.org/changeset/246693>