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

Improve <style> and <script> processing and conformance #3024

Merged
merged 3 commits into from
Sep 14, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented Sep 8, 2017

@annevk, and ideally @zcorpan and @sideshowbarker, to review.

Not sure if we want to add more tests. If someone thinks we should, I'd appreciate help writing them :).

* De-genericizes <style> and <link rel="stylesheet"> to only deal with
  CSS. Fixes #2995.
* Makes type="" on <style> "obsolete but conforming", since it is always
  redundant.
* Makes type="(a JS MIME type)" on <script> obsolete but conforming as
  well. Previously we had a "should" requirement but had not recorded it
  in the centralized obsolete-but-conforming section that collects such
  requirements.
* Makes <style> operate on child text content. Fixes #2996.
* Replaces the conformance requirement (noted in the source as
  "temporary") prohibiting unmatched comment-like syntax inside <style>
  with a conformance requirement to be valid CSS.
* Adds pointers to #2997.
* Makes it clearer that parameters are not allowed in the content type
  value for script or style. Fixes #3022.
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I guess we also need to tweak <script>'s processing model to explicitly allow the empty string.

source Outdated
<li>
<p>If <var>element</var>'s <code data-x="attr-style-type">type</code> attribute is present, and
its value is not an <span>ASCII case-insensitive</span> match for the string "<code
data-x="">text/css</code>", return.</p>
Copy link
Member

Choose a reason for hiding this comment

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

The empty string works too in Chrome, Firefox, and Safari. (Didn't test Edge.)

source Outdated
production. Classic scripts are affected by the <code
data-x="attr-script-charset">charset</code>, <code data-x="attr-script-async">async</code>, and
<code data-x="attr-script-defer">defer</code> attributes. Authors should omit the attribute,
instead of redundantly giving a <span>JavaScript MIME type</span>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Empty string works for <script> too in all browsers.

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems like a weird detail to include in this intro section, but maybe we can work it in.

source Outdated
not be recognized as referencing any of the scripting languages listed above.</p>
<p class="note">For example, "<code data-x="">text/javascript; charset=utf-8</code>" will not be
recognized as a <span>JavaScript MIME type</span>, and scripts marked as such will not be
evaluated.</p>
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work as a general statement since this will work for module scripts if specified in Content-Type as such.

source Outdated

<li><p>The presence of a <code data-x="attr-style-type">type</code> attribute on a
<code>style</code> element if its value is an <span>ASCII case-insensitive</span> match for the
string "<code>text/css</code>".</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this just be the presence of the attribute?

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; the presence of the attribute with another value is an error, not a warning. This section is specifically about warnings. Compare to the language="" bullet above this.

@annevk
Copy link
Member

annevk commented Sep 8, 2017

I also noticed you didn't update the "valid MIME type" reference for script. Including in the Attributes index. That now needs to be "valid MIME type with no parameters" assuming that's the term we're going to use for this going forward.

I'm not sure what to do about the classification of JavaScript MIME type as a MIME type since MIME type also still includes parameters. Do we need to introduce a new "MIME type with no parameters" term that specifically means a subset of the MIME type production?

@annevk
Copy link
Member

annevk commented Sep 8, 2017

(Although that wouldn't work for the classification of JavaScript MIME type I suppose since when it's used with Content-Type parameters are "fine", so it's more complicated still.)

@domenic
Copy link
Member Author

domenic commented Sep 8, 2017

I also noticed you didn't update the "valid MIME type" reference for script. Including in the Attributes index. That now needs to be "valid MIME type with no parameters" assuming that's the term we're going to use for this going forward.

Can you point to all the locations? I thought script did not reference valid MIME type anymore as of your recent PR.

I'm not sure what to do about the classification of JavaScript MIME type as a MIME type since MIME type also still includes parameters. Do we need to introduce a new "MIME type with no parameters" term that specifically means a subset of the MIME type production?

I don't think so. I think it's fine for "MIME type" to be used, especially in conformance requirements, as a more vague term, with the processing model or specific definitions like "JavaScript MIME type" giving the exact behavior.

Can you point to a particular location where we give bad authoring advice by using the word "MIME type"? I couldn't see any, since we only reference "JavaScript MIME type" or "text/css".

@annevk
Copy link
Member

annevk commented Sep 8, 2017

Sure:

Authors must use a valid MIME type that is not a JavaScript MIME type to denote data blocks.

The requirement that data blocks must be denoted using a valid MIME type is in place to avoid potential future collisions.

And also in the Attributes index for the type attribute:

Valid MIME type; "module"

(I only separated it out to add "module", but I didn't want to fix "valid MIME type" since that would also require changes in the above-quoted bits.)

@domenic
Copy link
Member Author

domenic commented Sep 8, 2017

OK. But that seems fine. They can use one with or without parameters; both work for the purposes of avoiding collisions.

@sideshowbarker sideshowbarker self-assigned this Sep 9, 2017
@sideshowbarker
Copy link
Contributor

Assigned myself not to claim exclusive review ownership/approval-from-me-required on this but instead just so in shows up in the right place in my gitflow so that I don’t forget. Will try to review it this weekend

@annevk
Copy link
Member

annevk commented Sep 9, 2017

@domenic it doesn't seem fine for the type attribute, since it suggests text/javascript;charset=utf-8 is fine, which it's not. It's arguably fine for the valid MIME type that is not a JavaScript MIME type, but it does seem super confusing (and it's not entirely clear to me if that actually excludes text/javascript;charset=utf-8 or not).

@domenic
Copy link
Member Author

domenic commented Sep 13, 2017

Pushed most changes.

@annevk, I'm still confused what you are proposing here to replace the text you quoted. I think it's pretty unambiguous as-is. Perhaps you can help by pushing a commit with your suggestion.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Just nits. I'm happy to help fix them if you agree with them.

source Outdated
<li>
<p>If <var>element</var>'s <code data-x="attr-style-type">type</code> attribute is present, and
its value is neither the empty string nor an <span>ASCII case-insensitive</span> match for the
string "<code data-x="">text/css</code>", return.</p>
Copy link
Member

Choose a reason for hiding this comment

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

s/the string//

Copy link
Member

Choose a reason for hiding this comment

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

then return* (as per above)

@@ -57363,8 +57337,9 @@ interface <dfn>HTMLScriptElement</dfn> : <span>HTMLElement</span> {
the type of script represented:</p>

<ul>
<li><p>Omitting the attribute, or setting it to a <span>JavaScript MIME type</span>, means that
the script is a <span>classic script</span>, to be interpreted according to the JavaScript <i
<li><p>Omitting the attribute, setting it to the empty string, or setting it to an <span>ASCII
Copy link
Member

Choose a reason for hiding this comment

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

Extreme nitpicking, but with this change

Authors should omit the attribute, instead of redundantly giving a JavaScript MIME type.

should arguably also be changed, lest someone argues that <script type> is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. "instead of redundantly giving a JavaScript MIME type (or adding an empty type attribute)" perhaps?

other than JavaScript is outside the scope of this specification.</p>
but must not support other <span data-x="MIME type">MIME types</span> for JavaScript. User agents
are not required to support JavaScript. The processing model for languages other than JavaScript
is outside the scope of this specification.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Do you agree that we should have a follow-up on this to change this somehow? It seems bad if WebAssembly wouldn't result in changes to the HTML Standard due to someone thinking we don't want to consider it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this text is pre-existing and just got re-wrapped.

I suppose we could expand on this, yeah. Perhaps a call-out to WebAssembly in particular.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I didn't mean for this to block this PR on, I just noticed it in passing and wanted to do due diligence.

source Outdated
@@ -58502,8 +58477,9 @@ o............A....e
order of parameters matters is left undefined until such time as it matters; so far the only
relevant parameter was 'e4x', and it's gone for now -->
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove this while here?

source Outdated
@@ -112964,6 +112940,15 @@ if (s = prompt('What is your name?')) {
data-x="">JavaScript</code>", it has no effect), or replaced with use of the <code
data-x="attr-script-type">type</code> attribute.</p>

<p>Authors should not specify a value for the <code data-x="attr-script-type">type</code>
attribute on <code>script</code> elements that is an <span>ASCII case-insensitive</span> match for
a <span>JavaScript MIME type</span>. Instead, they should omit the attribute, which has the same
Copy link
Member

Choose a reason for hiding this comment

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

or the empty string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably "the empty string or an ASCII case..." to avoid ambiguity about it being an ACIM for the empty string.

source Outdated

<li><p>The presence of a <code data-x="attr-style-type">type</code> attribute on a
<code>style</code> element if its value is an <span>ASCII case-insensitive</span> match for the
string "<code>text/css</code>".</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

s/the string//

@annevk
Copy link
Member

annevk commented Sep 13, 2017

For the script element type attribute summary, maybe something like:

The empty string or a JavaScript MIME type; "module"; a valid MIME type that is not a JavaScript MIME type

And then at some point we define what "is not" means for MIME types.

@domenic
Copy link
Member Author

domenic commented Sep 13, 2017

Hmm do we even list discouraged values in the attribute summary? We don't list language="" at all... In which case maybe just the latter two? Or maybe that's too confusing.

@annevk annevk mentioned this pull request Sep 13, 2017
@annevk
Copy link
Member

annevk commented Sep 13, 2017

I'd be okay with that, it's a summary after all and we do indeed seem to restrict it to things that are fully conforming.

@annevk
Copy link
Member

annevk commented Sep 13, 2017

Pushed a fixup.

Copy link
Member Author

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Changes LGTM, thanks for picking up the slack here. Let me try to work on some tests for some of the edge cases we clarified here.

I will also file a bug on the checker.

<li>
<p>If <var>element</var>'s <code data-x="attr-style-type">type</code> attribute is present and
its value is neither the empty string nor an <span>ASCII case-insensitive</span> match for
"<code>text/css</code>", then return.</p>
Copy link
Member Author

Choose a reason for hiding this comment

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

Firefox also allows whitespace to surround it, interestingly. I think it's correct to disallow.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Sep 14, 2017
Follows the clarifications in whatwg/html#3024. Also renames, re-titles, and re-jiggers some existing script tests.
@domenic
Copy link
Member Author

domenic commented Sep 14, 2017

Tests ready: web-platform-tests/wpt#7344 @annevk feel free to approve and merge if you're happy with things.

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 14, 2017
Follows the clarifications in whatwg/html#3024. Also renames, re-titles, and re-jiggers some existing script tests.
@annevk annevk merged commit 9c612ac into master Sep 14, 2017
@annevk annevk deleted the script-style-tweaks branch September 14, 2017 09:42
rachelandrew pushed a commit to rachelandrew/web-platform-tests that referenced this pull request Nov 8, 2017
Follows the clarifications in whatwg/html#3024. Also renames, re-titles, and re-jiggers some existing script tests.
jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017
Follows the clarifications in whatwg/html#3024. Also renames, re-titles, and re-jiggers some existing script tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants