hmm, appears to be a problem when having the type attribute:

It also works if there's a space between " and >:

It is caused by this rule in the JavaScript grammar:

JavaScript.tmLanguage.json:500 - (?<=(?:'|"|})>)

                {
                    "begin": "(?<=(?:'|\"|})>)",
                    "end": "(?=</)",
                    "name": "meta.jsx.children.js",
                    "patterns": [
                        {
                            "include": "#jsx-children"
                        }
                    ]
                }

As soon as HTML includes JavaScript, the JavaScript grammar does a look behind and decides to enter into a jsx children state.

To replicate my analysis please use:

c:\Alex\src\vscode-textmate>npm run inspect ..\vscode\extensions\html\syntaxes\html.json ..\vscode\extensions\javascript\syntaxes\JavaScript.tmLanguage.json test-bad.html >out-bad.txt
c:\Alex\src\vscode-textmate>npm run inspect ..\vscode\extensions\html\syntaxes\html.json ..\vscode\extensions\javascript\syntaxes\JavaScript.tmLanguage.json test-good.html >out-good.txt

Where test-bad is:

<script type="text/javascript">
var x = 3;
console.log('hello world!');
</script>

And test-good is:

<script type="text/javascript" >
var x = 3;
console.log('hello world!');
</script>

And then compare the two files. The first signficant diff points to the culprit:

I suggest the following fix to the rule in JavaScript.tmLanguage.json:500:

Change the rule (?<=(?:'|\"|})>) to (?<!javascript\">)(?<=(?:'|\"|})>)

This means to still match all text with look-behind ">, but not text with look-behind javascript">.

I am not a jsx expert, so I cannot say what's the impact of the change w.r.t. jsx

@bpasero Please discuss in the standup and let me know if I can help further.

JSX is a mix of Javascript and HTML. If there are JSX specific colors for javascript and html then those wont get applied. But writing JSX code with type text/javascript is not correct.

So, this fix LGTM.

@sandy081 you wanna take this? might need to review if our colorization tests fail with this change.

@bpasero Sure I can take it up. Will check the tests too.

After further investigation I am not in favor of doing the fix in JavaScript.tmLanguage.json. Because this file is derived from TypeScriptReact.tmLanguage. It would be good not to customize this.

I see this grammar is for js and jsx code. It makes sense only if it is used to parse the code between the <script> tags. But, I do not understand why is this grammar is being used in parsing html <script> tags also. So, I am assuming that the fix should go into the place where we parse html and delegate the script code to the js language parser.

But we can use this fix **(?<!text/javascript(?:'|\")>)**(?<=(?:'|\"|})>) as a workaround as this is safe and not much impact on jsx. Following is the case that will break for jsx

<MyComponent ... attr1="text/javascript">
     ....
</MyComponent>

I also checked all tests are passing.

@sandy081 I was thinking that this place would be here: https://github.com/Microsoft/vscode/blob/master/extensions/html/syntaxes/html.json#L180

But I am not so sure anymore how the JS language gets pulled in.

Ok it gets pulled in via TM import: https://github.com/Microsoft/vscode/blob/master/extensions/html/syntaxes/html.json#L232

@bpasero I went with the fix from @alexandrudima (made it more explicit). Pushed it as it is a regression and might make people unhappy. As this fix is a workaround and safer. See my previous comment for more details