Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOM: reset target/relatedTarget #9919

Merged
merged 6 commits into from Mar 28, 2018

Conversation

Projects
None yet
5 participants
@annevk
Copy link
Member

annevk commented Mar 8, 2018

Helps with issue whatwg/dom#562 and PR whatwg/dom#585.

@wpt-pr-bot wpt-pr-bot added the dom label Mar 8, 2018

@wpt-pr-bot wpt-pr-bot requested review from ayg, jdm and zqzhang Mar 8, 2018

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 8, 2018

I just realized I might also have to modify some tests added by @TakayoshiKochi in #7481. At least skimming through it seems they assume relatedTarget is not (always) reset.

@annevk annevk requested a review from TakayoshiKochi Mar 8, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 8, 2018

Build PASSED

Started: 2018-03-15 15:25:25
Finished: 2018-03-15 15:32:11

View more information about this build on:

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 12, 2018

Reviewing shadow-dom/event-post-dispatch.html it seems that expects some kind of difference in exposure depending on whether composed was set. I don't think we ever discussed that.

@TakayoshiKochi

This comment has been minimized.

Copy link
Member

TakayoshiKochi commented Mar 13, 2018

Re #9919 (comment)

So for when composed flag is set to false, do you think more changes needed for the spec, regarding computing relatedTargets?

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 13, 2018

@TakayoshiKochi per the specification the "composed flag" only affects https://dom.spec.whatwg.org/#interface-shadowroot (its "get the parent" algorithm). The tests Google wrote seem to assume that it also affects other things, but I'm not sure why it would.

@TakayoshiKochi

This comment has been minimized.

Copy link
Member

TakayoshiKochi commented Mar 13, 2018

Ah, I understand. (maybe off-topic for this PR, discussion can be elsewhere)

At dispatching events step 18, after dispatching the event to all targets, when composed flag is false, the event stops at the shadow root, and matches the condition of "target's root is a shadow root". Therefore both target and relatedTarget have to be nullified.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 13, 2018

Yeah, note that part of the thing being tested here is that it's important it was in a shadow root prior to dispatch. If an event listener moved it outside a shadow tree, we still want it nullified.

@TakayoshiKochi

This comment has been minimized.

Copy link
Member

TakayoshiKochi commented Mar 13, 2018

So in your spec change clearTargetsPostDispatch seems intended to capture the conditions that target and relatedTarget should be nullified, but you still need another condition after dispatch, then?

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 13, 2018

@TakayoshiKochi you mean in case someone moved the target inside a shadow tree? I don't think we should consider that case, but I wonder what @smaug---- thinks. We've been solely focused on pre-dispatch conditions for all kinds of parts of eventing.

@smaug----

This comment has been minimized.

Copy link
Contributor

smaug---- commented Mar 13, 2018

yeah, I think it was discussed in the other issue where relatedTarget handling was fixed on spec level that moving nodes into ShadowDOM isn't really an issue, since if you do that, you already have cross ShadowDOM access and such, and are somewhat explicitly breaking the encapsulation.

@TakayoshiKochi

This comment has been minimized.

Copy link
Member

TakayoshiKochi commented Mar 13, 2018

I see, then makes sense the latter half of the async_test cases in this test.
Yeah, I was wondering if we should take care of cases where an event listener pulled a node out of shadow tree to document, we should retain the target/relatedTarget or not (answer: no, still should be nullified per pre-dispatch condition).

@@ -0,0 +1,50 @@
const host = document.createElement("div"),

This comment has been minimized.

Copy link
@TakayoshiKochi

TakayoshiKochi Mar 13, 2018

Member

I was not familiar with this style of writing a test. Looks nice to avoid boilerplate in the HTML.
Can we add metadata, especially corresponding spec URL as a comment?

This comment has been minimized.

Copy link
@annevk

annevk Mar 13, 2018

Author Member

You mean link to https://dom.spec.whatwg.org/#concept-event-dispatch? Feels a little self-explanatory, but I could do.

This comment has been minimized.

Copy link
@TakayoshiKochi

TakayoshiKochi Mar 13, 2018

Member

Yeah, I think it would be nice.

@TakayoshiKochi TakayoshiKochi requested a review from hayatoito Mar 14, 2018

@TakayoshiKochi

This comment has been minimized.

Copy link
Member

TakayoshiKochi commented Mar 14, 2018

Adding @hayatoito as he's looking the spec change more closely.

annevk added some commits Mar 15, 2018

}
}, "Reset if relatedTarget pointed to a shadow tree");

async_test(t => {

This comment has been minimized.

Copy link
@TakayoshiKochi

TakayoshiKochi Mar 19, 2018

Member

Can this be a sync test()?
Because this seems to be testing a synchronous event dispatch and does not need to be async_test(),
and checking the side-effect of the event handler directly e.g. on line 35.

Although it is still okay to use async_test() if you prefer as is, as it covers synchronous testing.
I'm a bit feeling redundant that at the t.done() is called at the end of an async_test.

This comment has been minimized.

Copy link
@annevk

annevk Mar 19, 2018

Author Member

I don't think you can use t.step stuff in a sync test.

This comment has been minimized.

Copy link
@TakayoshiKochi

TakayoshiKochi Mar 20, 2018

Member

Yes, but as you do on line 80 (seen = true in the event handler to negate the value) then immediately after dispatching the event, check the value on line 83, you should be able to do the same here.
In fact, you expect the side effect of the event handler at line 35, so you don't need to wait for the step_func() to finish on line 39.

This comment has been minimized.

Copy link
@annevk

annevk Mar 20, 2018

Author Member

I'm not waiting for it? Callbacks need to be wrapped in steps and you can only have steps in asynchronous tests, I believe. Is that not true?

This comment has been minimized.

Copy link
@TakayoshiKochi

TakayoshiKochi Mar 20, 2018

Member

You are right, so to be clear, I'm not against async_test - t.step_func - t.done construct used here.

What I suggested was alternatively this test could be written as a sync test, as the callback (event handler) here is expected to be called in synchronous manner, and you can expect its side effect on the next line after dispatchEvent, and technically I thought it would be possible.

Probably it's a matter of taste, either isn't much better than the other.

This comment has been minimized.

Copy link
@annevk

annevk Mar 20, 2018

Author Member

That does not work, since if the callback ends up throwing in that case it'd end up being catched by the framework and not the test.

This comment has been minimized.

Copy link
@TakayoshiKochi

TakayoshiKochi Mar 20, 2018

Member

Ah, okay, I understand. Then LGTM!

@TakayoshiKochi
Copy link
Member

TakayoshiKochi left a comment

Maybe you want to add further tests?
The current set of tests looks good to me.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 20, 2018

Yeah, I'm a little unsure how to proceed. I think the tests Google wrote I pointed out earlier need to be updated and there probably need to be more tests as well.

I guess I can try updating the Google tests as part of this PR and add new tests, if any, in separate PRs.

Does that seem reasonable?

@TakayoshiKochi

This comment has been minimized.

Copy link
Member

TakayoshiKochi commented Mar 20, 2018

I think you can do either way, I prefer keeping a PR small and I don't mind small additions later, but if you prefer the spec change and the tests to have 1:1 relationship, it's also fine.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 27, 2018

I had a look through these and these are all still correct per the latest changes to whatwg/dom#585.

@annevk annevk merged commit 2f758ac into master Mar 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annevk annevk deleted the annevk/reset-event-targets branch Mar 28, 2018

annevk added a commit to whatwg/dom that referenced this pull request Mar 28, 2018

Shadow: fix event dispatch
This makes a number of large changes to event dispatch:

* The algorithm can no longer return early with true. The observable
  aspect is that the event object is "uninitialized" and false might 
  be returned if the event object was already canceled.
* It accounts for Touch Events in shadow trees by adding the 
  "touch target list" which is somewhat similar to relatedTarget.
* Retargeting for event targets is now solely controlled by the event 
  dispatch algorithm. The hook for other specifications is removed.
* Retargeting now properly accounts for non-node objects.
* Dispatch in general accounts for non-node objects too now.
* If targets need to be unset to prevent exposure of a shadow tree
  this is now calculated prior to invoking listeners.

Initializing relatedTarget and "touch target list" is up to the event
classes that need it. #614 will add "event constructing steps" that
enable this.

Tests: web-platform-tests/wpt#9919
(probably still needs work given today's changes)

This closes #402, fixes #561, fixes #562, fixes #580, and fixes #602.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.