Bug 193917 – Implement capture for Pointer Events on iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Antoine Quint]

To further our support for the Pointer Events spec on iOS, we need to implement capture as specified at https://w3c.github.io/pointerevents/#pointer-capture: the Element APIs, the implicit capture and the gotpointercapture and lostpointercapture events.

[Radar WebKit Bug Importer]

<rdar://problem/47605689>

[Antoine Quint]

Created attachment 360413 [details]
Patch

[Antoine Quint]

Created attachment 360414 [details]
Patch

[EWS Watchlist]

Attachment 360414 [details] did not pass style-queue:


ERROR: Source/WebCore/page/PointerCaptureController.h:46:  The parameter name "implicit" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 360413 [details]
Patch

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

> Source/WebCore/dom/Element.h:504
> +#if ENABLE(POINTER_EVENTS)
> +    ExceptionOr<void> setPointerCapture(int32_t);
> +    ExceptionOr<void> releasePointerCapture(int32_t);
> +    bool hasPointerCapture(int32_t);
> +#endif

You should put:

using PointerID = int32_t;
and then use PointerID in the parameter types.

> Source/WebCore/dom/Element.idl:104
> +    [Conditional=POINTER_EVENTS, EnabledAtRuntime=PointerEvents, MayThrowException] void setPointerCapture (long pointerId);
> +    [Conditional=POINTER_EVENTS, EnabledAtRuntime=PointerEvents, MayThrowException] void releasePointerCapture (long pointerId);
> +    [Conditional=POINTER_EVENTS, EnabledAtRuntime=PointerEvents] boolean hasPointerCapture (long pointerId);

Nit: remove spaces before (

> Source/WebCore/dom/PointerEvent.h:42
> +        int32_t pointerId { 0 };

Ditto. Maybe this is where you do the using and it is included in Element.h?

> Source/WebCore/dom/PointerEvent.h:82
> +    int32_t pointerId() const { return m_pointerId; }

Ditto.

> Source/WebCore/dom/PointerEvent.h:104
> +    int32_t m_pointerId { 0 };

Ditto.

> Source/WebCore/page/PointerCaptureController.cpp:42
> +PointerCaptureController::PointerCaptureController()
> +{
> +}
> +

= default

> Source/WebCore/page/PointerCaptureController.cpp:52
> +    // 2. If the Element on which this method is invoked is not connected ([DOM4]), throw an exception with the name InvalidStateError.

Nit: Remove ([DOM4])

> Source/WebCore/page/PointerCaptureController.cpp:57
> +    // 3. If this method is invoked while the document has a locked element ([PointerLock]), throw an exception with the name InvalidStateError.

Nit: Remove ([PointerLock])

> Source/WebCore/page/PointerCaptureController.cpp:83
> +    if (implicit == ImplicitCapture::No && iterator == m_activePointerIdsToCapturingData.end())

Do the implicit check separately, first.

> Source/WebCore/page/PointerCaptureController.cpp:88
> +    if (!hasPointerCapture(capturingTarget, pointerId))
> +        return { };

Bit silly that you have to do the hash lookup again. Maybe make a private helper that takes capturingData directly....

> Source/WebCore/page/PointerCaptureController.cpp:109
> +    auto& capturingData = iterator->value;
> +    return capturingData.pendingTargetOverride == capturingTarget || capturingData.targetOverride == capturingTarget;

... and does this bit.

> Source/WebCore/page/PointerCaptureController.cpp:121
> +    // When a pointer lock is successfully applied on an element, a user agent MUST run the steps as if the releasePointerCapture()
> +    // method has been called if any element is set to be captured or pending to be captured.
> +    for (auto& capturingData : m_activePointerIdsToCapturingData.values()) {
> +        capturingData.pendingTargetOverride = nullptr;
> +        capturingData.targetOverride = nullptr;
> +    }

I don't really understand how this comment translates to this logic.

> Source/WebCore/page/PointerCaptureController.cpp:124
> +void PointerCaptureController::touchEndedOrWasCanceledForIdentifier(int32_t pointerId)

cancelled

> Source/WebCore/page/PointerCaptureController.cpp:201
> +    if (capturingData.pendingTargetOverride)
> +        capturingData.targetOverride = capturingData.pendingTargetOverride;
> +    else
> +        capturingData.targetOverride = nullptr;

capturingData.targetOverride = capturingData.pendingTargetOverride;

will work in all cases.

> Source/WebCore/page/PointerCaptureController.h:46
> +    ExceptionOr<void> releasePointerCapture(Element*, int32_t, ImplicitCapture = ImplicitCapture::No);

Did this compile?

> Source/WebCore/page/PointerCaptureController.h:58
> +        Element* pendingTargetOverride;
> +        Element* targetOverride;

Sure these shouldn't be RefPtr? What happens if the Element is removed while it is captured? Please test.

> Source/WebCore/page/PointerCaptureController.h:62
> +    HashMap<int32_t, CapturingData> m_activePointerIdsToCapturingData;

m_capturingDataForPointerId?

> LayoutTests/pointerevents/ios/pointer-events-implicit-capture-has-pointer-capture-in-pointer-down.html:17
> +    ui.beginTouches({ x: 50, y: 50 }).then(() => test.done());

await ui.beingTouches(..);
test.done();

(would need async in the parameter)

> LayoutTests/pointerevents/ios/pointer-events-implicit-capture-release-exception.html:19
> +        assert_true(target.hasPointerCapture(event.pointerId));

Why not put test.done() here?

> LayoutTests/pointerevents/ios/pointer-events-implicit-capture-release.html:19
> +        assert_false(target.hasPointerCapture(event.pointerId));

And here.

[Antoine Quint]

Committed r240634: <https://trac.webkit.org/changeset/240634>

[Lucas Forschler]

Mass move bugs into the DOM component.