Bug 198191 – [Pointer Events] Check that capturing data managed by the PointerCaptureController gets cleared upon navigation

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

[Antoine Quint]

We capture some pointer state in the PointerCaptureController, and on macOS where the pointer id is always the same, we should check that the state is correctly reset upon navigation in case the user is in the middle of an interaction. This came up in the post-commit discussion of https://bugs.webkit.org/show_bug.cgi?id=198129.

[Radar WebKit Bug Importer]

<rdar://problem/51077486>

[Antoine Quint]

Created attachment 370685 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 370685 [details]
Patch

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

> Source/WebCore/page/Page.cpp:2864
> +    m_pointerCaptureController->reset();

What about subframe navigation? I don't think that we want these to leak between documents in frames either.

[Antoine Quint]

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 370685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370685&action=review
> 
> > Source/WebCore/page/Page.cpp:2864
> > +    m_pointerCaptureController->reset();
> 
> What about subframe navigation? I don't think that we want these to leak
> between documents in frames either.

I don't think there is anything to do in this case. The problem was that if you called preventDefault() while handling a "pointerdown" event in a page and navigated to the next page without releasing the mouse button, then compatibility mouse events would be prevented in the new page until you released the mouse button.

There is one PointerCaptureController per Page, so the main document and sub-frame documents use the same one. If preventDefault() is called while handling a "pointerdown" event in a sub-frame document, even if that document is navigated, or the whole iframe removed, it sounds fine to me that the state set by the sub-frame document remains valid for the main frame or any other sub-frame.

So I think this patch is correct and all that matters is clearing the state when the Page's main frame's document changes.

[Antoine Quint]

Committed r245809: <https://trac.webkit.org/changeset/245809>

[Alexey Proskuryakov]

> There is one PointerCaptureController per Page, so the main document and
> sub-frame documents use the same one. If preventDefault() is called while
> handling a "pointerdown" event in a sub-frame document, even if that
> document is navigated, or the whole iframe removed, it sounds fine to me
> that the state set by the sub-frame document remains valid for the main
> frame or any other sub-frame.

Generally, the reasons why no event state should be preserved over page loads even for subframes are user visible behavior and security. Quickly changing documents in either main frame or subframe results in events being delivered to a wrong document, resulting in unexpected actions getting triggered by them. This is a variation of clickjacking. I don't see anything critical in this particular case, but this is the kind of behavior that we tend to see as incorrect.

I other words, I guess that what I'm saying is that it's not right to have a single PointerCaptureController per page, all event dispatch state should be per document.

[Antoine Quint]

(In reply to Alexey Proskuryakov from comment #6)
>
> I other words, I guess that what I'm saying is that it's not right to have a
> single PointerCaptureController per page, all event dispatch state should be
> per document.

I thought about having a PointerCaptureController per Document, however I'm not sure how it would work in practice. A pointer travels across the main frame's content and all of its subframes, and as it does that it produces events across all frames. The behavior of the pointer and the event it produces requires coordination across all frames, and as such it makes most sense to me to have a single one per Page.

Of course, we could have one per Document and introduce coordination between them, but it would definitely be a lot more work and the benefits are not obvious to me.

[Darin Adler]

Comment on attachment 370685 [details]
Patch

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

> Source/WebCore/page/PointerCaptureController.cpp:151
> +    m_activePointerIdsToCapturingData.set(mousePointerID, capturingData);

Should use add instead of set; set includes unnecessary code to handle the case where the key already exists in the hash table. Impossible in a freshly cleared hash table.

[Antoine Quint]

Committed r246031: <https://trac.webkit.org/changeset/246031>

[Antoine Quint]

(In reply to Darin Adler from comment #8)
> Comment on attachment 370685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370685&action=review
> 
> > Source/WebCore/page/PointerCaptureController.cpp:151
> > +    m_activePointerIdsToCapturingData.set(mousePointerID, capturingData);
> 
> Should use add instead of set; set includes unnecessary code to handle the
> case where the key already exists in the hash table. Impossible in a freshly
> cleared hash table.

Thanks Darin, this was addressed in the additional commit r246031.