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

Normative: Allow Annex B scripts to start with --> #3244

Merged

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Dec 18, 2023

This PR updates the spec to match web reality (tested in Safari, Firefox, and Chrome -- XS does not implement HTML comments).

Annex B allows HTML closing comments (--> foo) after a line terminator, potentially preceded by other comments or spaces (SingleLineHTMLCloseComment). However, browsers also support them at the beginning of scripts. Some test cases:

  • eval("--> foo\nconsole.log(123)");
  • <script>--> foo
    console.log(123);
    </script>
  • <script src="./script.js"></script>
    --> foo
    console.log(123);
  • eval("/* comment */ --> foo\nconsole.log(123)");
  • (this case is already supported by the current spec)
    eval("/* com\nment */ --> foo\nconsole.log(123)");

This PR accomplishes this by adding HTMLCloseComment to InputElementHashbangOrRegExp in Annex B, where InputElementHashbangOrRegExp is the parsing goal used for the first token in the script.


EDIT There is already a PR fixing this, by @leobalter (#1493), but it was stuck on discussing how to specify this. My version has the advantage of not introducing any new spec language, but I'm happy to close this PR if we find the original one more editorially appealing.

Notes from discussing this in the past: https://github.com/tc39/notes/blob/main/meetings/2019-03/mar-26.md#normative-createdynamicfunction-early-concatenates-body

@nicolo-ribaudo nicolo-ribaudo added normative change Affects behavior required to correctly evaluate some ECMAScript source text web reality labels Dec 18, 2023
@nicolo-ribaudo
Copy link
Member Author

@gibson042 You mentioned on matrix that

All implementations that I tested reject a script starting with "-->", but JSC and SM and V8 incorrectly fail to reject eval input that does so:

but all the browsers I tested accept scripts starting with -->. Do you have a test case?

@gibson042
Copy link
Contributor

All implementations that I tested reject a script starting with "-->", but JSC and SM and V8 incorrectly fail to reject eval input that does so:

but all the browsers I tested accept scripts starting with -->. Do you have a test case?

It was tested with eshost, which might well be modifying the source in a way that is relevant. I was able to reproduce your acceptance in browsers, node, and esvu-fetched JavaScriptCore, SpiderMonkey, and V8.

@nicolo-ribaudo nicolo-ribaudo added the has consensus This has committee consensus. label Feb 6, 2024
@ljharb ljharb requested a review from a team February 6, 2024 19:16
@ljharb ljharb added the es2024 to be landed before es2024 label Feb 6, 2024
@nicolo-ribaudo nicolo-ribaudo added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Feb 6, 2024
@nicolo-ribaudo
Copy link
Member Author

I'll work on tests during this meeting so that it can be merged on time for 2024.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Feb 6, 2024

Tests: tc39/test262#4002

@nicolo-ribaudo nicolo-ribaudo removed the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Feb 6, 2024
@michaelficarra michaelficarra added ready to merge Editors believe this PR needs no further reviews, and is ready to land. has test262 tests labels Feb 12, 2024
@ljharb ljharb force-pushed the script-start-with-html-closing-comment branch from 35c0ed1 to 197f124 Compare February 14, 2024 22:03
@ljharb ljharb merged commit 197f124 into tc39:main Feb 14, 2024
7 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the script-start-with-html-closing-comment branch February 15, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
es2024 to be landed before es2024 has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text ready to merge Editors believe this PR needs no further reviews, and is ready to land. web reality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants