Closed Bug 1294335 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: stone, Assigned: stone)

References

Details

Attachments

(1 file, 9 obsolete files)

57.58 KB, patch
stone
: review+
stone
: feedback+
Details | Diff | Splinter Review
No description provided.
Blocks: 1292437
Assignee: nobody → sshih
Summary: [Pointer Event] setPointerCapture should check the case that set to the element which already captured the pointer → [Pointer Event] setPointerCapture should check if the element already captured the pointer
Depends on: 1258804
Attachment #8780376 - Attachment is obsolete: true
Attachment #8780376 - Flags: feedback?(btseng)
Attachment #8780383 - Flags: feedback?(btseng)
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;
    }
Attachment #8780383 - Flags: feedback?(btseng) → feedback-
Attachment #8781396 - Flags: feedback?(btseng)
Summary: [Pointer Event] setPointerCapture should check if the element already captured the pointer → [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm deinfed in spec
Summary: [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm deinfed in spec → [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm deinfed in the spec
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
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.
Attachment #8782383 - Flags: feedback?(btseng)
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?
Attachment #8782383 - Flags: feedback?(btseng)
(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.
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
Attachment #8782383 - Attachment is obsolete: true
Attachment #8783785 - Flags: feedback?(btseng)
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.
Attachment #8783785 - Flags: feedback?(btseng) → feedback+
Attachment #8783867 - Flags: review?(bugs)
Summary: [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm deinfed in the spec → [Pointer Event] Refine setPointerCapture / releasePointerCapture to follow the algorithm defined in the spec
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
Attachment #8783867 - Attachment is obsolete: true
Attachment #8784205 - Flags: feedback+
(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.
Attachment #8784205 - Flags: review?(bugs)
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.
Attachment #8784205 - Flags: review?(bugs)
Hmm, or, perhaps the patch actually does the behavior we want, even though it isn't following the current spec.
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
Attachment #8784205 - Flags: review+
Added one more test case and refined the patch summary
Attachment #8784205 - Attachment is obsolete: true
Attachment #8786565 - Flags: review+
Attachment #8786565 - Flags: feedback+
Renamed pointerevent_releasepointercapture_release_right_after_capture.html to pointerevent_releasepointercapture_release_right_after_capture-manual.html.
Attachment #8786565 - Attachment is obsolete: true
Attachment #8786668 - Flags: review+
Attachment #8786668 - Flags: feedback+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/260826c0b2a6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.