1413102 - Ensure Shadow DOM boundaries are dealt properly in event handling

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement, P2)

Description

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

.

Comment 1

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

Attachment: shadow_dom_event_dispatch.diff

Finally.

https://dom.spec.whatwg.org/#concept-event-dispatch needs to be read before reviewing this.

I tried to add quite a few comments, especially to nsIContent::GetEventTargetParent in FragmentOrElement.cpp.

Note, in our implementation GetEventTargetParent has double meaning. It both tells whether some target can
handle the event and gets its parent too. That is why the implementation doesn't quite map to the spec.
Same applies to event.mTarget handling, which is already retargeted in GetEventTargetParent, but not in the spec when it is about to add something to the path. That is why we need mTargetInKnownToBeHandledScope.

mRelatedTargetRetargetedInCurrentScope is an optimization to try to not figure out the right relatedTarget all the time, like what the spec does.

The stuff in Event.cpp is just removal of v0 Shadow DOM code.

I changed mOriginalTargetIsInAnon to deal with non-Shadom DOM only.

The change to event-composed-path-with-related-target.html is needed, since the test is buggy.
Webkit has the same behavior what the patch gives, and Blink will be fixed too.

My previous try run looked good
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/f9244ea50d20effb5854401e1815ca9164b54f11
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9244ea50d20effb5854401e1815ca9164b54f11
remote: recorded changegroup in replication log in 0.072s

Comment 2

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

Attachment: shadow_dom_event_dispatch_2.diff

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/5280f8f41355209642f875dea623540969d695c6
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=5280f8f41355209642f875dea623540969d695c6
remote: recorded changegroup in replication log in 0.079s

Comment 3

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

Comment on attachment 8944929 [details] [diff] [review]
shadow_dom_event_dispatch_2.diff

See comment 1 for some explanations.

This is rather tricky, and so is the relevant spec
https://dom.spec.whatwg.org/#dispatching-events
The new code in nsIContent::GetEventTargetParent refers to the spec algorithm.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for taking long time to review it. I'm still reviewing it. Let give me one more day.

Comment 5

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

no problem. The patch is difficult to review.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8944929 [details] [diff] [review]
shadow_dom_event_dispatch_2.diff

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

Honestly, I've not understood this well yet... (The spec is rally difficult to understand... That's just wording an implementation!)

I think that you should insert spec's list number and explanation to existing code too. And add explanation to each variable which appear in the spec as comment.

Temporarily canceling the review request. Either is okay that you'll request review to me again or to the other person.

::: dom/base/FragmentOrElement.cpp
@@ +921,5 @@
>  
> +already_AddRefed<nsINode>
> +FindChromeAccessOnlySubtreeOwner(EventTarget* aTarget)
> +{
> +  nsCOMPtr<nsINode> node = do_QueryInterface(aTarget);

Although not scope of this bug, cannot we avoid QI from EventTarget to nsINode/nsIContent/Element with using virtual method of something?

@@ +1081,5 @@
> +    // We don't support Shadow DOM in native anonymous content yet.
> +    aVisitor.mRelatedTargetRetargetedInCurrentScope = true;
> +    if (aVisitor.mEvent->mOriginalRelatedTarget) {
> +      // https://dom.spec.whatwg.org/#concept-event-dispatch
> +      // Step 3

Is it good thing to write list number of WHATWG spec? If the spec is modified, the number may be changed. How about to write the declaration too? I.e.,

// Step 3: Let relatedTarget be the result of retargeting event’s relatedTarget
// against target if event’s relatedTarget is non-null, and null otherwise.

@@ +1093,5 @@
> +        originalTargetAsNode =
> +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalTarget);
> +        initialTarget = originalTargetAsNode == this;
> +      }
> +      if (initialTarget) {

Hmm, I still don't understand what true of initialTaget means in the spec...

@@ +1110,5 @@
> +            // Step 4. If target is relatedTarget and target is not event's
> +            // relatedTarget, then return true.
> +            aVisitor.mCanHandle = false;
> +            aVisitor.SetParentTarget(nullptr, false);
> +            aVisitor.mEventTargetAtParent = nullptr;

If there is a method to set these members as this with proper name, I guess this is easier to understand what here does.

@@ +1120,5 @@
> +            return NS_OK;
> +          }
> +
> +          // Part of step 5. Retargeting target has happened already higher
> +          // up in this this method.

nit: "in this method"?

@@ +1125,5 @@
> +          aVisitor.mRelatedTargetOverride = retargetedRelatedTarget;
> +        }
> +      } else {
> +        nsCOMPtr<nsINode> relatedTargetAsNode =
> +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalRelatedTarget);

This is exactly same as what you do in above if block. However, your code restricts the scope of relatedTargetAsNode. So, I cannot say moving this should be moved before the |if| block. Up to you.

@@ +1133,5 @@
> +          nsCOMPtr<nsINode> targetInKnownToBeHandledScope =
> +            FindChromeAccessOnlySubtreeOwner(aVisitor.mTargetInKnownToBeHandledScope);
> +          if (nsContentUtils::ContentIsShadowIncludingDescendantOf(
> +                this, targetInKnownToBeHandledScope->SubtreeRoot())) {
> +            // Step 11.4

So, please append the explanation of the spec for 11.4 here because the number may be changed in the future.

@@ +1136,5 @@
> +                this, targetInKnownToBeHandledScope->SubtreeRoot())) {
> +            // Step 11.4
> +            aVisitor.mRelatedTargetOverride = retargetedRelatedTarget;
> +          } else if (this == retargetedRelatedTarget) {
> +            // Step 11.5

So, please append the explanation of the spec for 11.5 here because the number may be changed in the future.

@@ +1147,5 @@
> +            // point to a node in Shadow DOM.
> +            aVisitor.mEvent->mTarget = aVisitor.mTargetInKnownToBeHandledScope;
> +            return NS_OK;
> +          } else {
> +            // Step 11.6

So, please append the explanation of the spec for 11.6 here because the number may be changed in the future.

::: dom/base/nsContentUtils.cpp
@@ +2567,5 @@
> +    return true;
> +  }
> +
> +  do {
> +    if (aPossibleDescendant == aPossibleAncestor)

Please wrap the following line with {}.

@@ +2644,5 @@
> +nsContentUtils::Retarget(nsINode* aTargetA, nsINode* aTargetB)
> +{
> +  while (true) {
> +    // If A's root is not a shadow root...
> +    nsINode* root = aTargetA->SubtreeRoot();

aTargetA is modified when it reaches end of this while-loop. Then, it's set by ShadowRoot::FromNode(rood)->GetHost(). I don't know if this may return nullptr, but its prefix is "Get".

If it never returns nullptr, please insert MOZ_ASSERT or something for making clearer that it never becomes nullptr.  Otherwise, please handle error cases.

::: dom/base/nsContentUtils.h
@@ +362,5 @@
>    static bool
>    ContentIsFlattenedTreeDescendantOfForStyle(const nsINode* aPossibleDescendant,
>                                               const nsINode* aPossibleAncestor);
>  
> +  

nit: remove these unnecessary whitespaces.

::: dom/events/EventDispatcher.cpp
@@ +365,5 @@
>    void PostHandleEvent(EventChainPostVisitor& aVisitor);
>  
>  private:
>    nsCOMPtr<EventTarget>             mTarget;
> +  nsCOMPtr<EventTarget>             mOverrideRelatedTarget;

nit: I don't like redundant whitesapces at declaring member variables since if adding member variable whose type is too long, this format will be easily broken.

@@ +928,5 @@
>    aEvent->mFlags.mIsBeingDispatched = false;
>    aEvent->mFlags.mDispatchedAtLeastOnce = true;
>  
> +  // https://dom.spec.whatwg.org/#concept-event-dispatch
> +  // Step 18

So, I'd like you to add explanation of the spec explicitly because the spec's number may be changed in the future.

::: dom/events/EventDispatcher.h
@@ +149,5 @@
>      mWantsPreHandleEvent = false;
>      mRootOfClosedTree = false;
>      mParentIsSlotInClosedTree = false;
>      mParentIsChromeHandler = false;
> +    // Note, we don't clear mRelatedTargetRetargetedInCurrentScope explicitly.

Adding its reason here helps other developers to understand.

@@ +259,5 @@
> +  /**
> +   * If the related target of the event needs to be retargeted, set this
> +   * to a new EventTarget.
> +   */
> +  dom::EventTarget* mRelatedTargetOverride;

Could you add explanation that this is what in the spec? I'm still confused whether this is |relatedTarget| or |targetOverride|.

::: widget/BasicEvents.h
@@ +593,5 @@
>    nsCOMPtr<dom::EventTarget> mOriginalTarget;
>  
>    /// The possible related target
>    nsCOMPtr<dom::EventTarget> mRelatedTarget;
> +  nsCOMPtr<dom::EventTarget> mOriginalRelatedTarget;

FYI: Bug 1433008 added move constructor. You need to add this to its initializing list.

Comment 7

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

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> Comment on attachment 8944929 [details] [diff] [review]
> shadow_dom_event_dispatch_2.diff
> 
> Review of attachment 8944929 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Honestly, I've not understood this well yet... (The spec is rally difficult
> to understand... That's just wording an implementation!)
Well, spec's should be very precise. Issue here with the spec is that it has very slow algorithm for this all.

> I think that you should insert spec's list number and explanation to
> existing code too.
well, I've tried to add as much as possible, but I'll add more.

> And add explanation to each variable which appear in the
> spec as comment.
well, can't really, since some variables are optimization, because the spec algorithm as such can't be implemented since it would be too slow (checking relatedTarget retargeting all the time)


> > +already_AddRefed<nsINode>
> > +FindChromeAccessOnlySubtreeOwner(EventTarget* aTarget)
> > +{
> > +  nsCOMPtr<nsINode> node = do_QueryInterface(aTarget);
> 
> Although not scope of this bug, cannot we avoid QI from EventTarget to
> nsINode/nsIContent/Element with using virtual method of something?
Yeah, I'm planning to add AsFoo() methods to EventTarget, but not about this bug.

> 
> @@ +1081,5 @@
> > +    // We don't support Shadow DOM in native anonymous content yet.
> > +    aVisitor.mRelatedTargetRetargetedInCurrentScope = true;
> > +    if (aVisitor.mEvent->mOriginalRelatedTarget) {
> > +      // https://dom.spec.whatwg.org/#concept-event-dispatch
> > +      // Step 3
> 
> Is it good thing to write list number of WHATWG spec? If the spec is
> modified, the number may be changed. How about to write the declaration too?
> I.e.,
> 
> // Step 3: Let relatedTarget be the result of retargeting event’s
> relatedTarget
> // against target if event’s relatedTarget is non-null, and null otherwise.
> 
Sounds good

> @@ +1093,5 @@
> > +        originalTargetAsNode =
> > +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalTarget);
> > +        initialTarget = originalTargetAsNode == this;
> > +      }
> > +      if (initialTarget) {
> 
> Hmm, I still don't understand what true of initialTaget means in the spec...
In our case the original target can be in native anonymous content, but that isn't something in DOM spec.
DOM knows only about the first target in light or shadow DOM. I'll add a comment.

> @@ +1110,5 @@
> > +            // Step 4. If target is relatedTarget and target is not event's
> > +            // relatedTarget, then return true.
> > +            aVisitor.mCanHandle = false;
> > +            aVisitor.SetParentTarget(nullptr, false);
> > +            aVisitor.mEventTargetAtParent = nullptr;
> 
> If there is a method to set these members as this with proper name, I guess
> this is easier to understand what here does.
Well, not technically about this bug, since similar stuff is done elsewhere too. But sure, I could add a helper.
The name could be...  IgnoreCurrentTarget()



> @@ +1125,5 @@
> > +          aVisitor.mRelatedTargetOverride = retargetedRelatedTarget;
> > +        }
> > +      } else {
> > +        nsCOMPtr<nsINode> relatedTargetAsNode =
> > +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalRelatedTarget);
> 
> This is exactly same as what you do in above if block. However, your code
> restricts the scope of relatedTargetAsNode. So, I cannot say moving this
> should be moved before the |if| block. Up to you.
Yeah, I'd prefer not to, in order to follow the spec language more and hopefully keeping the code easier
to maintain.


> @@ +2644,5 @@
> > +nsContentUtils::Retarget(nsINode* aTargetA, nsINode* aTargetB)
> > +{
> > +  while (true) {
> > +    // If A's root is not a shadow root...
> > +    nsINode* root = aTargetA->SubtreeRoot();
> 
> aTargetA is modified when it reaches end of this while-loop. Then, it's set
> by ShadowRoot::FromNode(rood)->GetHost(). I don't know if this may return
> nullptr, but its prefix is "Get".
Technically it can return after cycle collection.

> ::: dom/events/EventDispatcher.cpp
> @@ +365,5 @@
> >    void PostHandleEvent(EventChainPostVisitor& aVisitor);
> >  
> >  private:
> >    nsCOMPtr<EventTarget>             mTarget;
> > +  nsCOMPtr<EventTarget>             mOverrideRelatedTarget;
> 
> nit: I don't like redundant whitesapces at declaring member variables since
> if adding member variable whose type is too long, this format will be easily
> broken.
Sure, but I'm just using the existing coding style


> @@ +259,5 @@
> > +  /**
> > +   * If the related target of the event needs to be retargeted, set this
> > +   * to a new EventTarget.
> > +   */
> > +  dom::EventTarget* mRelatedTargetOverride;
> 
> Could you add explanation that this is what in the spec? I'm still confused
> whether this is |relatedTarget| or |targetOverride|.
This is the relatedTarget which the event should use when handling the current target.
In normal case it is the retargeted relatedTarget.

Comment 8

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

Attachment: shadow_dom_event_dispatch_3.diff

remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/52aa3b0cd2fd80f867f99f7e5217c9b2a88aa802
remote:   https://hg.mozilla.org/try/rev/609668e0ec26ccec5994198bdb7ea285cc4c6f55
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=609668e0ec26ccec5994198bdb7ea285cc4c6f55
remote: recorded changegroup in replication log in 0.071s

Comment 9

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

Attachment: shadow_dom_event_dispatch_4.diff

I tried to improve comments and renamed some variables to improve readability.
I'll we on vacation next week, but if you had time to look at this, great.
If not, I'll try to find someone else to look at this (just not quite sure who).

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/1a4909f8542a28e77ecb02db27a8b6dc99526bd5
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a4909f8542a28e77ecb02db27a8b6dc99526bd5
remote: recorded changegroup in replication log in 0.077s

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8947782 [details] [diff] [review]
shadow_dom_event_dispatch_4.diff

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

Perhaps, I still do not fully understand well. So, I don't find any bugs/issues from your patch. However, at least, I don't see any problem comparing with the spec. So, giving r+. If you think my review isn't enough. Please ask additional review to somebody. However, according to the testcase changes, this must be fine.

::: dom/base/FragmentOrElement.cpp
@@ +1085,5 @@
> +      //
> +      // This is a bit complicated because the event might be from native
> +      // anonymous content, but we need to deal with non-native anonymous
> +      // content there.
> +      bool initialTarget = this == aVisitor.mEvent->mOriginalTarget;

So, if initialTarget is true here, we're just reach original target.

@@ +1091,5 @@
> +      // Use of mOriginalTargetIsInAnon is an optimization here.
> +      if (!initialTarget && aVisitor.mOriginalTargetIsInAnon) {
> +        originalTargetAsNode =
> +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalTarget);
> +        initialTarget = originalTargetAsNode == this;

But here, if initialTarget is true, we exited from native anonymous tree.

@@ +1095,5 @@
> +        initialTarget = originalTargetAsNode == this;
> +      }
> +      if (initialTarget) {
> +        nsCOMPtr<nsINode> relatedTargetAsNode =
> +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalRelatedTarget);

Therefore, you find related target of same scope here?

@@ +1096,5 @@
> +      }
> +      if (initialTarget) {
> +        nsCOMPtr<nsINode> relatedTargetAsNode =
> +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalRelatedTarget);
> +        if (!originalTargetAsNode) {

If we were not in native anonymous tree, we event's original target is in current scope, so, just use it?

@@ +1111,5 @@
> +            // "If target is relatedTarget and target is not event's
> +            //  relatedTarget, then return true."
> +            aVisitor.mCanHandle = false;
> +            aVisitor.SetParentTarget(nullptr, false);
> +            aVisitor.mEventTargetAtParent = nullptr;

So, why didn't you wrap these lines with IgnoreCurrentTarget() as you replied?

@@ +1129,5 @@
> +        }
> +      } else {
> +        nsCOMPtr<nsINode> relatedTargetAsNode =
> +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalRelatedTarget);
> +        if (relatedTargetAsNode) {

We're in native anonymous content subtree and relatedTargetAsNode is its host? But looks like this is 11.1?

@@ +1131,5 @@
> +        nsCOMPtr<nsINode> relatedTargetAsNode =
> +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalRelatedTarget);
> +        if (relatedTargetAsNode) {
> +          nsINode* retargetedRelatedTarget =
> +            nsContentUtils::Retarget(relatedTargetAsNode, this);

This must be 11.3 "Let relatedTarget be the result of retargeting event’s relatedTarget against parent if event’s relatedTarget is non-null, and null otherwise.". If so, add comment here and above points where I mentioned.

@@ +1148,5 @@
> +            // "Otherwise, if parent and relatedTarget are identical, then set
> +            //  parent to null."
> +            aVisitor.mCanHandle = false;
> +            aVisitor.SetParentTarget(nullptr, false);
> +            aVisitor.mEventTargetAtParent = nullptr;

IgnoreCurrentTarget()?

::: dom/base/nsContentUtils.h
@@ +362,5 @@
>    static bool
>    ContentIsFlattenedTreeDescendantOfForStyle(const nsINode* aPossibleDescendant,
>                                               const nsINode* aPossibleAncestor);
>  
> +  

nit: remove those unnecessary whites spaces.

Comment 11

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

I'll fix the nits after my vacation.

Comment 12

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

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10)
> > +        initialTarget = originalTargetAsNode == this;
> > +      }
> > +      if (initialTarget) {
> > +        nsCOMPtr<nsINode> relatedTargetAsNode =
> > +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalRelatedTarget);
> 
> Therefore, you find related target of same scope here?
same non native anonymous scope

> 
> @@ +1096,5 @@
> > +      }
> > +      if (initialTarget) {
> > +        nsCOMPtr<nsINode> relatedTargetAsNode =
> > +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalRelatedTarget);
> > +        if (!originalTargetAsNode) {
> 
> If we were not in native anonymous tree, we event's original target is in
> current scope, so, just use it?
But related target can be in NAC. Perhaps I misunderstand the question.


> 
> So, why didn't you wrap these lines with IgnoreCurrentTarget() as you
> replied?
oh, I was going to comment, that since we have similar stuff elsewhere too, I'd prefer to do it separately.
But I guess I can do it now too for this code.


> > +        }
> > +      } else {
> > +        nsCOMPtr<nsINode> relatedTargetAsNode =
> > +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalRelatedTarget);
> > +        if (relatedTargetAsNode) {
> 
> We're in native anonymous content subtree and relatedTargetAsNode is its
> host? But looks like this is 11.1?
11.1 is about slots. not about this stuff.


> 
> @@ +1131,5 @@
> > +        nsCOMPtr<nsINode> relatedTargetAsNode =
> > +          FindChromeAccessOnlySubtreeOwner(aVisitor.mEvent->mOriginalRelatedTarget);
> > +        if (relatedTargetAsNode) {
> > +          nsINode* retargetedRelatedTarget =
> > +            nsContentUtils::Retarget(relatedTargetAsNode, this);
> 
> This must be 11.3 "Let relatedTarget be the result of retargeting event’s
> relatedTarget against parent if event’s relatedTarget is non-null, and null
> otherwise.". If so, add comment here and above points where I mentioned.
Ah yes, I can add a comment.

Comment 13

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

Attachment: shadow_dom_event_dispatch_5.diff

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2320b7fd9266
Ensure Shadow DOM boundaries are dealt properly in event handling, r=masayuki

Comment 15

[Dorel Luca [:dluca]]

Backed out changeset 2320b7fd9266 (bug 1413102) for Mochitest failure on dom/tests/mochitest/pointerlock/test_pointerlock-api.html

Log of the fail:
https://treeherder.mozilla.org/logviewer.html#?job_id=162167379&repo=mozilla-inbound&lineNumber=50595

Backed out changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56c8882ff0b26e1a5025033f8c1ec61eb843afce

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2320b7fd9266924ccd4ca51c6f0f43183482d9b3

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15d9502b0ff0
Ensure Shadow DOM boundaries are dealt properly in event handling, r=masayuki

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/15d9502b0ff0