Handle module resolution when there is no active script #4181

domenic · 2018-11-19T18:00:41Z

Closes #3295.

Thanks @jonco3 for the prompt on fixing this.

Tests: I believe this is technically not tested yet, although there are a lot of adjacent tests, e.g. https://github.com/web-platform-tests/wpt/blob/fca3dc941825b298617f8bf6c9ce7c196b4de6f5/html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-base-url-inline-module.html#L29-L32 . But those don't test the specific scenario here, because when you call the .onclick getter, there is an active script.

Instead we need a test that uses the fancy new testdriver thingies to actually click the div (instead of having JS call div.onclick() as the existing tests do).

Even that might not really work; @gsnedders do you know if the JS engine has script "on the stack" when testdriver clicks a div? (The answer we want is "no", just like when a user clicks the div.)

Ideally those tests should actually have a different document base URL than script base URL, e.g. by using an external script.

It would be really appreciated if someone else were up for writing those tests; I would be happy to review.

domfarolino

Thanks again for taking this! This looks good to me. Regarding changing the name of "default classic script fetch options" => "default script fetch options" after this PR (which was briefly discussed in the original issue), I am indifferent.

Can start looking at tests for this, but with finals coming up no guarantees on a timeline.

domfarolino · 2018-11-19T22:14:39Z

source

-   same two arguments.</p></li>
+   specifier">resolving a module specifier</span> must have been previously successful with these
+   same two arguments (either <a href="#validate-requested-module-specifiers">while creating the
+   corresponding module script</a>, or in <span>HostImportModuleDynamically</span>).</p></li>

   <li><p>Let <var>resolved module script</var> be <var>moduleMap</var>[<var>url</var>]. (This entry
   must <span data-x="map exists">exist</span> for us to have gotten to this point.)</p></li>


Not related to the changes here, and entirely optional, but I kind of like the idea of asserting that moduleMap[url] exists, instead of having this off to the side.

annevk

I initially had some nits on using less spaces in variable names, but the mixture in this algorithm was already present. It might be nice to cleanup.

One other thought I had was that perhaps an abstraction is in order, but perhaps that gets too messy with the fetch options?

gsnedders · 2018-11-20T13:40:45Z

Even that might not really work; @gsnedders do you know if the JS engine has script "on the stack" when testdriver clicks a div? (The answer we want is "no", just like when a user clicks the div.)

I don't think we guarantee that? e.g., in Blink (with run-webkit-tests) it becomes this.

domenic · 2018-11-20T16:11:50Z

@annevk yeah, the abstraction isn't worth it, I think.

@gsnedders that is what I was afraid of. I guess then I should file a WPT bug asking for test infra for clicks which behave the same way as actual user clicks?

In the meantime, should we merge this without tests, as it's probably untestable?

domenic · 2018-11-20T16:14:18Z

I guess I could write manual tests...

domfarolino · 2018-11-20T16:31:52Z

I wrote https://t3.glitch.me yesterday as a start for some Chrome debugging...with a little more work some of these might pass as manual tests, however I prefer:

Seeing what browsers need bugs
Merging this
Opening an issue on WPT to see if it might be possible to get user-simulated clicks in the infra
And if that is possible, filing an issue similar to Remaining module credential tests to write web-platform-tests/wpt#13426 for tracking
Otherwise, we port some manual tests over :(

gsnedders · 2018-11-20T17:11:19Z

@gsnedders that is what I was afraid of. I guess then I should file a WPT bug asking for test infra for clicks which behave the same way as actual user clicks?

Yeah. Do you know if there's any way to do this in Blink's LayoutTests today, FWIW?

domenic · 2018-11-20T17:14:55Z

I don't know offhand, but I'll probably poke around at least a bit and report back, before filing a request. (Including verifying that the code you linked me to doesn't do something unexpectedly good, like queuing a task.)

Closes #3295. This also fixes a related inaccurate assert in EnqueueJob, putting in guards for a null active script there as well and adding examples explaining the various scenarios.

domenic · 2018-11-20T21:12:31Z

This is ready for (and needs) re-review, as I've now incorporated some of the job-related stuff that was also discussed in #3295.

Follows whatwg/html#4181.

domenic · 2018-11-20T23:05:27Z

Manual-for-now tests at web-platform-tests/wpt#14158; discussion on how to automate at web-platform-tests/wpt#14158.

annevk · 2018-11-21T12:11:46Z

source

+     is a built-in function that does not originate from any particular <span
+     data-x="concept-script">script</span>.</p>
+
+     <p>With this step in place, the active script is propagated from the above code into the job,


Wasn't the plan to remove jobs completely and have JavaScript instead queue things to the host? That would also resolve this, no?

That is the current setup, just done via money patch. It's not a "removal" though; the things JavaScript posts are still called "jobs", which we convert to microtasks.

annevk · 2018-11-21T12:12:32Z

source

+     <p>In this case, the JavaScript function for the <span data-x="event handlers">event
+     handler</span> will be created by the <span data-x="getting the current value of the event
+     handler">get the current value of the event handler</span> algorithm as a direct result of user
+     action, with no script on the stack (i.e. no <span>active script</span>). Thus, when the


annevk

Seems fine, but please address the nit (maybe at some point we should do those programmatically). I also noticed some lack of "If ..., then ..." but we're not consistent so...

domfarolino

Makes sense & lgtm

Follows whatwg/html#4181.

…(), a=testonly Automatic update from web-platform-testsAdd tests for no active script in import() (#14157) Follows whatwg/html#4181. -- wpt-commits: 5241a3fe2c638f9fadcc040f39f3e6a71a2be2e1 wpt-pr: 14157

Closes #4267. Related to previous work in #4181.

Closes #4267. Related to previous work in #4181. Tests: web-platform-tests/wpt#15251

Closes whatwg#4267. Related to previous work in whatwg#4181. Tests: web-platform-tests/wpt#15251

…(), a=testonly Automatic update from web-platform-testsAdd tests for no active script in import() (#14157) Follows whatwg/html#4181. -- wpt-commits: 5241a3fe2c638f9fadcc040f39f3e6a71a2be2e1 wpt-pr: 14157 UltraBlame original commit: a1cc7364d13d9e2d8561042a2e204792ac344496

…(), a=testonly Automatic update from web-platform-testsAdd tests for no active script in import() (#14157) Follows whatwg/html#4181. -- wpt-commits: 5241a3fe2c638f9fadcc040f39f3e6a71a2be2e1 wpt-pr: 14157 UltraBlame original commit: 8b5324f6bd04819422f05d72d7d72219ef2b1b2b

…(), a=testonly Automatic update from web-platform-testsAdd tests for no active script in import() (#14157) Follows whatwg/html#4181. -- wpt-commits: 5241a3fe2c638f9fadcc040f39f3e6a71a2be2e1 wpt-pr: 14157 UltraBlame original commit: a1cc7364d13d9e2d8561042a2e204792ac344496

…(), a=testonly Automatic update from web-platform-testsAdd tests for no active script in import() (#14157) Follows whatwg/html#4181. -- wpt-commits: 5241a3fe2c638f9fadcc040f39f3e6a71a2be2e1 wpt-pr: 14157 UltraBlame original commit: 8b5324f6bd04819422f05d72d7d72219ef2b1b2b

…(), a=testonly Automatic update from web-platform-testsAdd tests for no active script in import() (#14157) Follows whatwg/html#4181. -- wpt-commits: 5241a3fe2c638f9fadcc040f39f3e6a71a2be2e1 wpt-pr: 14157 UltraBlame original commit: a1cc7364d13d9e2d8561042a2e204792ac344496

…(), a=testonly Automatic update from web-platform-testsAdd tests for no active script in import() (#14157) Follows whatwg/html#4181. -- wpt-commits: 5241a3fe2c638f9fadcc040f39f3e6a71a2be2e1 wpt-pr: 14157 UltraBlame original commit: 8b5324f6bd04819422f05d72d7d72219ef2b1b2b

domenic added needs tests Moving the issue forward requires someone to write tests topic: script labels Nov 19, 2018

domenic requested review from annevk and domfarolino November 19, 2018 18:00

domenic mentioned this pull request Nov 19, 2018

Do not run FinishDynamicImport steps in parallel #4182

Merged

domfarolino approved these changes Nov 19, 2018

View reviewed changes

annevk approved these changes Nov 20, 2018

View reviewed changes

domenic mentioned this pull request Nov 20, 2018

There is no active script when a user triggers an inline event handler #3295

Closed

domenic force-pushed the dynamic-import-event-handlers branch from 5355e33 to 1ac8d01 Compare November 20, 2018 20:46

Handle module resolution when there is no active script

49423b9

Closes #3295. This also fixes a related inaccurate assert in EnqueueJob, putting in guards for a null active script there as well and adding examples explaining the various scenarios.

domenic force-pushed the dynamic-import-event-handlers branch from 1ac8d01 to 49423b9 Compare November 20, 2018 21:12

domenic added a commit to web-platform-tests/wpt that referenced this pull request Nov 20, 2018

Add tests for no active script in import()

e3bf3e7

Follows whatwg/html#4181.

This was referenced Nov 20, 2018

Add tests for no active script in import() web-platform-tests/wpt#14157

Merged

Need a way to click without any JS code on the stack web-platform-tests/wpt#14158

Open

domenic removed the needs tests Moving the issue forward requires someone to write tests label Nov 20, 2018

annevk reviewed Nov 21, 2018

View reviewed changes

annevk approved these changes Nov 21, 2018

View reviewed changes

domfarolino approved these changes Nov 21, 2018

View reviewed changes

comma and "then"

531e4b7

domenic merged commit 3e48877 into master Nov 22, 2018

domenic deleted the dynamic-import-event-handlers branch November 22, 2018 00:34

foolip pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 22, 2018

Add tests for no active script in import() (#14157)

5241a3f

Follows whatwg/html#4181.

domenic added a commit that referenced this pull request Jan 17, 2019

Always set [[ScriptOrModule]] to null for event handler functions

6e7dbe9

Closes #4267. Related to previous work in #4181.

domenic mentioned this pull request Jan 17, 2019

Always set [[ScriptOrModule]] to null for event handler functions #4302

Merged

domenic added a commit that referenced this pull request Feb 8, 2019

Always set [[ScriptOrModule]] to null for event handler functions

1d6eb99

Closes #4267. Related to previous work in #4181. Tests: web-platform-tests/wpt#15251

mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019

Always set [[ScriptOrModule]] to null for event handler functions

c337172

Closes whatwg#4267. Related to previous work in whatwg#4181. Tests: web-platform-tests/wpt#15251

mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019

Always set [[ScriptOrModule]] to null for event handler functions

2adc62c

Closes whatwg#4267. Related to previous work in whatwg#4181. Tests: web-platform-tests/wpt#15251

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle module resolution when there is no active script #4181

Handle module resolution when there is no active script #4181

domenic commented Nov 19, 2018 •

edited

Loading

domfarolino left a comment

domfarolino Nov 19, 2018

annevk left a comment

gsnedders commented Nov 20, 2018 •

edited

Loading

domenic commented Nov 20, 2018

domenic commented Nov 20, 2018

domfarolino commented Nov 20, 2018

gsnedders commented Nov 20, 2018

domenic commented Nov 20, 2018

domenic commented Nov 20, 2018

domenic commented Nov 20, 2018

annevk Nov 21, 2018

domenic Nov 21, 2018

annevk Nov 21, 2018

annevk left a comment

domfarolino left a comment

Handle module resolution when there is no active script #4181

Handle module resolution when there is no active script #4181

Conversation

domenic commented Nov 19, 2018 • edited Loading

domfarolino left a comment

Choose a reason for hiding this comment

domfarolino Nov 19, 2018

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

gsnedders commented Nov 20, 2018 • edited Loading

domenic commented Nov 20, 2018

domenic commented Nov 20, 2018

domfarolino commented Nov 20, 2018

gsnedders commented Nov 20, 2018

domenic commented Nov 20, 2018

domenic commented Nov 20, 2018

domenic commented Nov 20, 2018

annevk Nov 21, 2018

Choose a reason for hiding this comment

domenic Nov 21, 2018

Choose a reason for hiding this comment

annevk Nov 21, 2018

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

domfarolino left a comment

Choose a reason for hiding this comment

domenic commented Nov 19, 2018 •

edited

Loading

gsnedders commented Nov 20, 2018 •

edited

Loading