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

Shadow script #1100

Merged
merged 3 commits into from
Apr 26, 2016
Merged

Shadow script #1100

merged 3 commits into from
Apr 26, 2016

Conversation

annevk
Copy link
Member

@annevk annevk commented Apr 22, 2016

This makes <script> elements work when used in shadow trees. The
beforescriptexecute and afterscriptexecute events won’t work, since
they are already disabled for modules and only makes sense together
with currentScript, which cannot be made to work for shadow scripts.
See #1013 for details.

Fixes #762.

@annevk
Copy link
Member Author

annevk commented Apr 22, 2016

This depends on #1097.

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Apr 22, 2016
@annevk
Copy link
Member Author

annevk commented Apr 22, 2016

Also, depending on how we feel about doing this right in one go, we could wait until scoped events are part of the DOM Standard. The only downside is that this would land some infrastructure that could be used for other shadow tree cleanup as well. Not sure how to decouple all that easily.

@annevk
Copy link
Member Author

annevk commented Apr 22, 2016

(Note that review can start straight away.)

@annevk annevk added the topic: shadow Relates to shadow trees (as defined in DOM) label Apr 22, 2016
into a document</span>, at the time the <span data-x="nodes are inserted">node is inserted</span>
according to the DOM, after any other <code>script</code> elements inserted at the same time that
are earlier in the <code>Document</code> in <span>tree order</span>.</li>
<li>The <code>script</code> element gets <span data-x="node is connected">connected</span>.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the rremoval of the ", at the time..." clause here problematic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since the hook it's using is only run once for each node. However, there is a problem with script execution as illustrated by http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4098.

I think technically we need to invoke the insertion steps twice. Once with "don't execute scripts" and once (once everything is actually in the tree) with "execute scripts". That flag might only need to be set to "don't" in the case of document fragments.

@smaug---- had a thought I believe how we could address this in a different way, but I forgot.

This makes <script> elements work when used in shadow trees. The
beforescriptexecute and afterscriptexecute events won’t work, since
they are already disabled for modules and only makes sense together
with currentScript, which cannot be made to work for shadow scripts.
See #1013 for details.

Fixes #762.
@@ -59121,9 +59125,10 @@ o............A....e
<dt>"<code data-x="">classic</code>"</dt>
<dd>
<ol>
<li><p>Set the <code>script</code> element's <span>node document</span>'s <code
<li><p>if the <code>script</code> element is <span>in a document</span>, then set the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uppercase I

@domenic
Copy link
Member

domenic commented Apr 25, 2016

It sounds like there's a separate issue (which I don't fully understand) illustrated by http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4098. Fix the typo, file a new issue for that, and then this LGTM.

@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Apr 25, 2016
@annevk
Copy link
Member Author

annevk commented Apr 26, 2016

If we land this now we should probably not close #762 and leave that until we have marked the event scoped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: shadow Relates to shadow trees (as defined in DOM)
Development

Successfully merging this pull request may close these issues.

2 participants