1294335 - [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm defined in the spec

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Comment 1

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] setPointerCapture should check if the element already captured the pointer (V1)

Comment 2

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] setPointerCapture should check if the element already captured the pointer (V2)

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8780383 [details] [diff] [review]
[Pointer Event] setPointerCapture should check if the element already captured the pointer (V2)

Review of attachment 8780383 [details] [diff] [review]:
-----------------------------------------------------------------

f- due to some flaw in this implementation.
Please see my comment for more detailed explanation, thanks!

::: layout/base/nsPresShell.cpp
@@ +6702,5 @@
>  
> +  // In the case that the pointer capture override is already set to the element
> +  // and there is a pending pointer capture, clear pending pointer capture
> +  // because the final pointer capture override is still be the element which
> +  // already captured the pointer

I think the implementation itself has told us what has been protected.
Otherwise, we should review if our implementation is not good enough to tell what has been done by itself. :-(

@@ +6707,5 @@
> +  if (pointerCaptureInfo && content && content == aContent &&
> +      pointerCaptureInfo->mPendingContent) {
> +    pointerCaptureInfo->mPendingContent = nullptr;
> +    return;
> +  }

This looks NG to me and shall be done as followed in if (pointerCaptureInfo) {...}.

Besides, we need another test case to cover the case when mPendingContent is equal to nullptr(that means mPendingContent is already promoted to mOverrideCotent) for the scenario that SetPointerCaptureContent() to the same element was invoked more than once after a pointer event is captured.

@@ +6713,5 @@
>    if (!content && (nsIDOMMouseEvent::MOZ_SOURCE_MOUSE == GetPointerType(aPointerId))) {
>      SetCapturingContent(aContent, CAPTURE_PREVENTDRAG);
>    }
>  
>    if (pointerCaptureInfo) {

Shall be better in this way:
    if (content && content == aContent) {
      if (pointerCaptureInfo->mPendingContent) {
        pointerCaptureInfo->mPendingContent = nullptr;
      }
      return;
    }

Comment 4

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] setPointerCapture should check if the element already captured the pointer (V3)

Comment 5

[Ming-Chou Shih [:stone]]

Current implementation followed the PE level1 spec and use three variables to control the behaviors of setPointerCapture/releasePointerCapture/hasPointerCapture.

In PE level2, the timing that setPointerCapture take effect was changed. A simpler algorithm is defined for processing pending pointer capture and firing gotpointercapture/lostpointercapture. I tried and found it's hard to keep current implementation with small modifications to follow PE level2 spec. So I refine it as the algorithm defined in [1].

[1] https://w3c.github.io/pointerevents/#process-pending-pointer-capture

Comment 6

[Ming-Chou Shih [:stone]]

The existed test case bug977003_inner_4.html should also be refined because in the case:
A gotpointercapture
B.setPointerCapture
A.releasePointerCapture
Then B should receive gotpointercapture.

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm deinfed in the spec (V4)

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm deinfed in the spec (V5)

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8782383 [details] [diff] [review]
[Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm deinfed in the spec (V5)

Review of attachment 8782383 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to review again after the following questions are clarified, thanks!

::: layout/base/nsPresShell.cpp
@@ +4425,5 @@
>    for (auto iter = gPointerCaptureList->Iter(); !iter.Done(); iter.Next()) {
>      nsIPresShell::PointerCaptureInfo* data = iter.UserData();
> +    if (data && data->mPendingContent &&
> +        nsContentUtils::ContentIsDescendantOf(data->mPendingContent, aChild)) {
> +      data->mPendingContent = nullptr;

m... this earns the performance but loses the consistency to reset mPendingContent in one place.
Is the list expected to be huge in real use cases?
If there is not much panic in performance, I'd prefer to keep the original one.

@@ +6716,5 @@
>    }
>    return nullptr;
>  }
>  
>  /* static */ bool

The return value is not used by the callers, please change it to void.

@@ +6728,5 @@
> +    // event listener
> +    nsIContent* pendingContent = captureInfo->mPendingContent.get();
> +    bool hasCapturingContent = false;
> +    if (captureInfo->mOverrideContent) {
> +      DispatchGotOrLostPointerCaptureEvent(false, aPointerId, aPointerType,

(/* aIsGotCapture */ false,
 aPointerId,
 aPointerType,
 aIsPrimary,
 captureInfo->mOverrideContent);

@@ +6737,1 @@
>        DispatchGotOrLostPointerCaptureEvent(true, aPointerId, aPointerType,

ditto

@@ +6742,5 @@
> +    if (nsIDOMMouseEvent::MOZ_SOURCE_MOUSE == aPointerType) {
> +      hasCapturingContent ? SetCapturingContent(captureInfo->mOverrideContent,
> +                                                CAPTURE_PREVENTDRAG) :
> +                            SetCapturingContent(nullptr, 0);
> +    }

What's the impact if we now setCapturingContent here instead of in SetCaptureContent and ReleaseCaptureContent?

Comment 10

[Ming-Chou Shih [:stone]]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #9)
> ::: layout/base/nsPresShell.cpp
> @@ +4425,5 @@
> >    for (auto iter = gPointerCaptureList->Iter(); !iter.Done(); iter.Next()) {
> >      nsIPresShell::PointerCaptureInfo* data = iter.UserData();
> > +    if (data && data->mPendingContent &&
> > +        nsContentUtils::ContentIsDescendantOf(data->mPendingContent, aChild)) {
> > +      data->mPendingContent = nullptr;
> 
> m... this earns the performance but loses the consistency to reset
> mPendingContent in one place.
> Is the list expected to be huge in real use cases?
> If there is not much panic in performance, I'd prefer to keep the original
> one.
No, it isn't. I'm ok to call the original API.

> @@ +6742,5 @@
> > +    if (nsIDOMMouseEvent::MOZ_SOURCE_MOUSE == aPointerType) {
> > +      hasCapturingContent ? SetCapturingContent(captureInfo->mOverrideContent,
> > +                                                CAPTURE_PREVENTDRAG) :
> > +                            SetCapturingContent(nullptr, 0);
> > +    }
> 
> What's the impact if we now setCapturingContent here instead of in
> SetCaptureContent and ReleaseCaptureContent?
The original idea is to keep the flag is set when we are prompting the pending pointer capture. According to the discussion about spec behavior, the pointer capture is take effect when set it. In that case I'm ok to keep it in current function.

Comment 11

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm deinfed in the spec (V6)

This version of patch includes following changes
1. use nsIPresShell::ReleasePointerCapturingContent to release pointer capture
2. SetCapturingContent in SetPointerCapturingContent / ReleasePointerCapturingContent
3. return mPendingContent in GetPointerCapturingContent
4. change the return type of CheckPointerCaptureState from bool to void
5. add comments when calling DispatchGotOrLostPointerCaptureEvent

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8783785 [details] [diff] [review]
[Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm deinfed in the spec (V6)

Review of attachment 8783785 [details] [diff] [review]:
-----------------------------------------------------------------

f=me after the following issues are addressed.

::: dom/events/test/pointerevents/pointerevent_setpointercapture_to_same_element-manual.html
@@ +33,5 @@
> +                    target1.setPointerCapture(event.pointerId);
> +                    test_setPointerCapture.step(function () {
> +                        assert_equals(target0.hasPointerCapture(event.pointerId), false, "Set capture to target0, target0.hasPointerCapture should be false");
> +                        assert_equals(target1.hasPointerCapture(event.pointerId), true, "Set capture to target1, target1.hasPointerCapture should be true");
> +                    });

In addition to this test case, we need another test to just set the target0 twice without setting to target1 to have more solid test coverage.

::: layout/base/nsPresShell.cpp
@@ +6707,5 @@
>  /* static */ void
>  nsIPresShell::ReleasePointerCapturingContent(uint32_t aPointerId)
>  {
> +  if (nsIDOMMouseEvent::MOZ_SOURCE_MOUSE == GetPointerType(aPointerId)) {
> +    SetCapturingContent(nullptr, 0);

Let's keep this unchanged but fire another bug to refine the set/restore flags of capturing content in a properly way.

Comment 13

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm deinfed in the spec (V7)

Comment 14

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm defined in the spec. (V8)

This version modified
1.Added one more test to check set the target0 twice without setting to target1.
2.keep using original arguments to call SetCapturingContent

Comment 15

[Ming-Chou Shih [:stone]]

(In reply to Ming-Chou Shih [:stone] from comment #14)
> This version modified
> 1.Added one more test to check set the target0 twice without setting to
> target1.
> 2.keep using original arguments to call SetCapturingContent
3. revert the return value of GetPointerCapturingContent. GetPointerCapturingContent should return mOverrideContent because the pending capture element may be changed again when dispatching gotpointercapture / lostpointercapure.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

There is a spec issue: https://github.com/w3c/pointerevents/issues/136

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8784205 [details] [diff] [review]
[Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm defined in the spec. (V8)

The patch needs some tweaking after the spec bug is fixed.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Hmm, or, perhaps the patch actually does the behavior we want, even though it isn't following the current spec.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8784205 [details] [diff] [review]
[Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm defined in the spec. (V8)

Yeah, ok, I think we want the spec to be change to do what this patch does.
Could you still add a test which calls releasePointerCapture right after calling setPointerCapture

Comment 20

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm defined in the spec. f=bevistseng. r=smaug. (V9)

Added one more test case and refined the patch summary

Comment 21

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm defined in the spec. f=bevistseng. r=smaug. (V10)

Renamed pointerevent_releasepointercapture_release_right_after_capture.html to pointerevent_releasepointercapture_release_right_after_capture-manual.html.

Comment 22

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6795185e7cbe2031ed20762cfe628b81588d5837

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/260826c0b2a6
[Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm defined in the spec. f=bevistseng, r=smaug

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/260826c0b2a6