search

whatwg / whatwg.org

The WHATWG website and other static resources
https://whatwg.org/
Other
76 stars 50 forks source link

Recommend loading commit-snapshot-shortcut-key.js with defer as it installs a DOMContentLoaded listener #418

Closed jsnajdr closed 1 year ago

jsnajdr commented 1 year ago

Fixing what I believe is a little bug in the commit-snapshot-shortcut-key.js script. In html.spec.whatwg.org it's loaded with the async flag, which means it can run after the DOMContentLoaded event has fired. The listener that sets up the keyboard shortcut will never run in that case. Only a script with defer is guaranteed to run before DOMContentLoaded.

The fix uses the load event instead, which is fired only after even the async scripts are finished running.

In the spec, this is the place where all the async scripts are flushed, and it runs between the DOMContentLoaded and load events.

I found this when trying to learn what exactly the difference between async and defer is.

annevk commented 1 year ago

Hmm, perhaps we should be using defer instead? We don't really want to wait on everything else being loaded either after all.

jsnajdr commented 1 year ago

In this case, where all the code is in the DOMContentLoaded/load listener, defer wouldn't make much difference. defer script are guaranteed to be executed only after "the document has finished parsing", immediately before DOMContentLoaded. We'd be waiting for everything else to be loaded anyway.

Also, at the time when the listener runs, the a#commit-snapshot-link element must already be present in the document.

So, yes, both combinations, DOMContentLoaded + defer, and load + async, are correct (but not the DOMContentLoaded + async one!), but they do almost exactly the same thing.

annevk commented 1 year ago

That's not entirely true. We don't have to wait for things that delay the load event, such as images.

jsnajdr commented 1 year ago

We don't have to wait for things that delay the load event, such as images.

Oh, this is a good point -- I thought that the DOMContentLoaded and load events are very close together, but they aren't. We're waiting until all the resources that are neither parser-blocking nor render-blocking get loaded.

I'll rework both patches to use defer instead, and keep the DOMContentLoaded listeners.

jsnajdr commented 1 year ago

This PR has been transformed into one that merely changes the comment that describes how the script should be included. All other work is now done in https://github.com/whatwg/html/pull/9297.

annevk commented 1 year ago

For the non-HTML standards https://github.com/speced/bikeshed-boilerplate/blob/main/boilerplate/whatwg/header.include will have to be patched. Do you want to take that on as well?

jsnajdr commented 1 year ago

Do you want to take that on as well?

Yes, done in https://github.com/speced/bikeshed-boilerplate/pull/40.

annevk commented 1 year ago

Thanks for following through on everything here @jsnajdr! Each specification will need a new commit before this change becomes part of it, but hopefully that doesn't take too long overall.