-
Notifications
You must be signed in to change notification settings - Fork 3k
Declarative CSS Modules #11687
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
base: main
Are you sure you want to change the base?
Declarative CSS Modules #11687
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces declarative CSS modules support to HTML, allowing CSS to be imported as modules through <style>
elements with a specifier
attribute and <template>
elements with a shadowrootadoptedstylesheets
attribute.
- Adds a
specifier
attribute to<style>
elements that creates module import maps for CSS content - Introduces a
shadowrootadoptedstylesheets
attribute for<template>
elements to declaratively adopt CSS modules - Implements algorithms for creating declarative CSS module scripts and stylesheet adoption
Comments suppressed due to low confidence (5)
source:1
- Missing attribute name in IDL definition. Should be
[SameObject, PutForwards=value, Reflect] readonly attribute DOMString <dfn attribute for="HTMLStyleElement" data-x="dom-style-specifier">specifier</dfn>;
to match the pattern used for other attributes in this interface.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Grammar error: 'is defines' should be 'defines' - remove the word 'is'.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Grammar error: 'appended with the of the' should be 'appended with the' - remove the duplicate 'the of'.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Logic error: The algorithm references
<var>specifier</var>
but this variable is not defined in the algorithm. It should reference the value of theshadowrootadoptedstylesheets
attribute instead.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Incorrect data-x reference: Should be
data-x=\"attr-style-specifier\"
notdata-x=\"attr-style-blocking\"
for the specifier attribute row.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (9)
source:1
- The IDL attribute should be named to match the content attribute. The content attribute is
specifier
but the IDL should use camelCase convention:[SameObject, PutForwards=value, Reflect] readonly attribute DOMString specifier;
should have a data-x attribute defining the DOM property name.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- The
shadowrootadoptedstylesheets
attribute is listed twice in the content attributes section for the template element. This duplication should be removed.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- The
shadowrootadoptedstylesheets
attribute is listed twice in the content attributes section for the template element. This duplication should be removed.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- The algorithm step is missing proper HTML structure. It should end with
</li>
and the nested<ol>
should be properly closed with</ol>
.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- The variable
moduleScript
is referenced but never defined in this algorithm. This should likely be the current module script or settings object context.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- These variable assignments are missing closing
</p>
tags. Each should end with</p></li>
.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Missing spaces after commas in the parameter list. Should be
<var>fetchClient</var>, <var>destination</var>, <var>options</var>, <var>settingsObject</var>, <var>referrer</var>
for consistency.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Incorrect data-x reference in the table. Line 148289 should reference
data-x=\"element-template\"
or similar, notdata-x=\"attr-template-shadowrootclonable\"
.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Incorrect data-x reference in the table. Line 148363 should reference
data-x=\"element-style\"
instead ofdata-x=\"attr-style-blocking\"
.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review in progress, sharing feedback so far.
https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2740 | ||
https://github.com/w3c/csswg-drafts/issues/3096 --> | ||
|
||
<li><p>If <var>element</var>'s <code data-x="attr-style-type">type</code> attribute is present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<li><p>If <var>element</var>'s <code data-x="attr-style-type">type</code> attribute is present | |
<li><p>If <var>element</var>'s <code data-x="attr-style-type">type</code> attribute is not present |
https://github.com/w3c/csswg-drafts/issues/3096 --> | ||
|
||
<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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and its value is not an <span>ASCII case-insensitive</span> match for | |
or its value is not an <span>ASCII case-insensitive</span> match for |
element's <span>child text content</span>, then return. <ref>CSP</ref></p></li> | ||
|
||
<li><p>Let <var>styleDataURI</var> be the string "<code data-x="">data:text/css,</code>" | ||
appended with the <span>child text content</span> of the <code>style</code> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to do the urlencoding here rather than further down, to avoid the data:text/css
part of the string getting encoded into something like data%3Atext%2Fcss
.
<var>styleDataURI</var>.</li> | ||
</ol> | ||
</li> | ||
<li>Perform <span>parse an import map string</span> with <var>input</var> as the resulting JSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates an import map but doesn't actually apply it. We still need to do some stuff to apply it to the document.
See https://html.spec.whatwg.org/#create-an-import-map-parse-result and https://html.spec.whatwg.org/#script-processing-model:create-an-import-map-parse-result, and https://html.spec.whatwg.org/#register-an-import-map
|
||
<ol> | ||
<li>A single key of "<code data-x="">imports</code>".</li> | ||
<li>A single JSON value containing a <span>module specifier map</span> with a key consisting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little unclear that this value is meant to go with the key "imports"
. Might be clearer to combine these steps: "A single key of "imports"
whose value is a module specifier map with a key consisting of...".
</ol> | ||
</li> | ||
<li>Perform <span>parse an import map string</span> with <var>input</var> as the resulting JSON | ||
string from the prior step and <var>baseURL</var> as the <span>document base URL</span>.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of saying "the resulting JSON string" we probably need to reference an actual algorithm, i.e. the algo underlying JSON.stringify
.
Actually, it could be overall clearer/easier to just construct the JSON string directly in the previous step rather than creating a JSON object, then converting it to a string.
<dfn data-x="concept-documentorshadowroot-extensions-mixin" | ||
data-x-href="https://www.w3.org/TR/cssom-1/#extensions-to-the-document-or-shadow-root-interface">Extensions to the <code>DocumentOrShadowRoot</code> Interface Mixin</dfn>: | ||
<ul class="brief"> | ||
<li><dfn data-x="concept-documentorshadowroot-extensions-mixin-stylesheets" data-x-href="https://www.w3.org/TR/cssom-1/#dom-documentorshadowroot-stylesheets">styleSheets</dfn></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
styleSheets
isn't used here so I don't think you have to list that one.
<hr> | ||
|
||
<div algorithm> | ||
<p>The <dfn>stylesheet adopting steps</dfn> for <code>template</code> elements are:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the entry point to this algorithm still needs to be added somewhere.
<var>adoptedStyleSheets</var>. If any error occurs, then <span>continue</span>.</p></li> | ||
</ol> | ||
</li> | ||
<li><p><span data-x="set append">Append</span> <span>adoptedStyleSheets</span> to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be https://infra.spec.whatwg.org/#list-extend
?
(See WHATWG Working Mode: Changes for more details.)
/acknowledgements.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/scripting.html ( diff )
/semantics.html ( diff )