Bug 218638

Created attachment 413368 [details] Patch

Created attachment 413372 [details] Patch

Created attachment 413373 [details] Patch

Created attachment 413374 [details] Patch

Created attachment 413429 [details] Patch

Comment on attachment 413429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413429&action=review > Source/WebCore/dom/EventDispatcher.cpp:91 > + shouldClearTargetsAfterDispatch = isInShadowTree(eventContext.target()) || isInShadowTree(eventContext.relatedTarget()); Is it important to do this shadow tree check before dispatching?

Comment on attachment 413429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413429&action=review >> Source/WebCore/dom/EventDispatcher.cpp:91 >> + shouldClearTargetsAfterDispatch = isInShadowTree(eventContext.target()) || isInShadowTree(eventContext.relatedTarget()); > > Is it important to do this shadow tree check before dispatching? Yes, I believe it is to match the specification, especially because the event handlers could remove the node from the shadow tree.

Comment on attachment 413429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413429&action=review >>> Source/WebCore/dom/EventDispatcher.cpp:91 >>> + shouldClearTargetsAfterDispatch = isInShadowTree(eventContext.target()) || isInShadowTree(eventContext.relatedTarget()); >> >> Is it important to do this shadow tree check before dispatching? > > Yes, I believe it is to match the specification, especially because the event handlers could remove the node from the shadow tree. OK. Do we have test cases for the converse where event handles move a node into a shadow tree to check that it does not get cleared in that case?

(In reply to Darin Adler from comment #8) > Comment on attachment 413429 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413429&action=review > > >>> Source/WebCore/dom/EventDispatcher.cpp:91 > >>> + shouldClearTargetsAfterDispatch = isInShadowTree(eventContext.target()) || isInShadowTree(eventContext.relatedTarget()); > >> > >> Is it important to do this shadow tree check before dispatching? > > > > Yes, I believe it is to match the specification, especially because the event handlers could remove the node from the shadow tree. > > OK. Do we have test cases for the converse where event handles move a node > into a shadow tree to check that it does not get cleared in that case? I will check. If not, I will add one.

Created attachment 413452 [details] Patch

(In reply to Chris Dumez from comment #9) > (In reply to Darin Adler from comment #8) > > Comment on attachment 413429 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=413429&action=review > > > > >>> Source/WebCore/dom/EventDispatcher.cpp:91 > > >>> + shouldClearTargetsAfterDispatch = isInShadowTree(eventContext.target()) || isInShadowTree(eventContext.relatedTarget()); > > >> > > >> Is it important to do this shadow tree check before dispatching? > > > > > > Yes, I believe it is to match the specification, especially because the event handlers could remove the node from the shadow tree. > > > > OK. Do we have test cases for the converse where event handles move a node > > into a shadow tree to check that it does not get cleared in that case? > > I will check. If not, I will add one. As promised, I have added test coverage for this particular case. I have also verified that our behavior is consistent with Blink and Gecko here.

(In reply to Chris Dumez from comment #11) > As promised, I have added test coverage for this particular case. I have > also verified that our behavior is consistent with Blink and Gecko here. Which test covers nodes that are moved *into* a shadow tree by an event handler?

(In reply to Darin Adler from comment #12) > (In reply to Chris Dumez from comment #11) > > As promised, I have added test coverage for this particular case. I have > > also verified that our behavior is consistent with Blink and Gecko here. > > Which test covers nodes that are moved *into* a shadow tree by an event > handler? Oh, I indeed this not cover this case. I will add one more test then.

(In reply to Chris Dumez from comment #13) > (In reply to Darin Adler from comment #12) > > (In reply to Chris Dumez from comment #11) > > > As promised, I have added test coverage for this particular case. I have > > > also verified that our behavior is consistent with Blink and Gecko here. > > > > Which test covers nodes that are moved *into* a shadow tree by an event > > handler? > > Oh, I indeed this not cover this case. I will add one more test then. I added this test. One thing to note that is this new test passes in Safari and Firefox but fails in Chrome. Chrome DOES clear the event targets if the node is initially outside the shadow tree but gets moved to a shadow tree by an event listener. I am not inclined to match Chrome though since we have the specification and Firefox on our side.

Created attachment 413466 [details] Patch

I presume consistency is our goal so that we get interoperability. If so, then we should make sure WPT tests cover this so Chrome knows they have it wrong. Outside of consistency, I’m not clear on how important these semantics are, and what motivated either rule. Seems arbitrary that we clear these pointers based on them being in shadow tree in *some* cases but not in others: does not create an invariant.

(In reply to Darin Adler from comment #16) > I presume consistency is our goal so that we get interoperability. If so, > then we should make sure WPT tests cover this so Chrome knows they have it > wrong. > > Outside of consistency, I’m not clear on how important these semantics are, > and what motivated either rule. Seems arbitrary that we clear these pointers > based on them being in shadow tree in *some* cases but not in others: does > not create an invariant. I agree, here is the WPT PR: https://github.com/web-platform-tests/wpt/pull/26440

Committed r269546: <https://trac.webkit.org/changeset/269546> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413466 [details].

<rdar://problem/71135539>

Does this fix https://bugs.webkit.org/show_bug.cgi?id=206374 and https://bugs.webkit.org/show_bug.cgi?id=184079 ?

*** Bug 206374 has been marked as a duplicate of this bug. ***

*** Bug 184079 has been marked as a duplicate of this bug. ***