If people like the straw proposal and say so before I finish with the TemplateStyles work, I'll implement it. If not, I suppose someone else will do something here eventually.

Seems like a good plan. One nitpick:

The tokens will be general strip markers, with content being <link rel="stylesheet" href="/w/load.php?..." data-mw-embed-module="..."/>. That way even if they somehow don't get replaced the browser should still load the styles, just in a less-efficient way.
This will replace the above <link> tags with <style data-mw-embeded-module="...">...</style>.

What's the point of that, given that we agreed that duplicated inline styles are strongly preferable to external styles? Why not just set the markers to <style...</style> in the first place?

On T160563, @Anomie wrote:

[..] it became clear that for performance it would be desirable if parser tags and parser functions that cause stylesheets to be added to the page (i.e. by calling addModuleStyles()) would embed those styles in the body content in <style> tags rather than adding them to the stylesheets loaded in the page head, with deduplication if the same styles were to be included multiple times.

This could be used for the following components and extensions. [..]

I support the purpose of this task for TemplateStyles and maybe 1 or 2 other extensions, however I don't agree that it is "undesirable" to call addModuleStyles() from a parser function. I would be rather concerned if all the aforementioned extensions would embed style tags instead. It would almost certainly regress performance and provide no benefit, except for giving a false impression that content-slicing services would have an easier job removing redundant styles when stripping parts of the page. "False impression", because styles loaded on a page inevitably relate to the content on that page; This won't change that. Perhaps we should add invisible meta data in the Parser output and Parsoid HTML (stripped when post-processed for viewing) that indicates which modules it added, so that when all instances of <meta property="mw:modulestyles" content="foobar"/> are gone, the service knows to update the modulestyles list. (Or something like that). But.. that's for another task.

At least CategoryTree, Cite, TimedMediaHandler, TemplateData, and SyntaxHighlight seem like they should not embed their stylesheets. They are not user-authored, don't register an uncontrolled number of modules, and should not be subject to page caching. Rather, their application would benefit from external reference and shared caching across pages. And their cache is expected to roll over quickly after a deployment, and not be part of 24-hour, 7-day, or 30-day static HTML caching that may vary from page to page. This trade-off is acceptable for template styles because it provides users with a simple model where template transclusion and template styles always go hand-in-hand when they are updated after an edit. In addition, it also relieves any duty of keeping backwards-compatibility with previous versions of a template, just like how inline styles are maintained now. And updates wil still propagate through the job queue to other pages as before.

We also want to keep the door open for a future where we embed critical styles with the rest being deferred (T127328 and related tasks). Some of these (such as SyntaxHighlight) don't affect layout and can be deferred, whereas others cannot.

In T160563#3108819, @Tgr wrote:

What's the point of that, given that we agreed that duplicated inline styles are strongly preferable to external styles? Why not just set the markers to <style...</style> in the first place?

To avoid loading and processing the embedded RL module every time a "token" is requested, and then keeping the whole thing around in the StripState and concatenating it into the final HTML only for it to be removed soon after. But if people think that overhead doesn't matter, I don't care too much.

In T160563#3108924, @Krinkle wrote:

I support the purpose of this task for TemplateStyles and maybe 1 or 2 other extensions, however I don't agree that it is "undesirable" to call addModuleStyles() from a parser function. I would be rather concerned if all the aforementioned extensions would embed style tags instead.

In T155813#3087239, @Gilles stated "Styles in the head should be kept to a vital minimum (think site header, logo, chrome, space placeholders to avoid things jumping around as the page loads)." Anything that's included via a parser tag would seem to not be part of that vital minimum, it can be delayed until just before the associated content is reached in the body section.

But if you and Gilles want to argue about that, feel free. I don't care except that I keep getting mixed messages on this topic. I would much prefer clear guidelines on when exactly styles should be embedded in the head versus linked in the head versus embedded in the body versus loaded with async JS versus some new way to async-link the stylesheet without requiring JS (so e.g. SyntaxHighlight doesn't break for people with no JS), rather than getting told different absolutes in different contexts or by different people pushing different agendas.

Perhaps we should add invisible meta data in the Parser output and Parsoid HTML (stripped when post-processed for viewing) that indicates which modules it added, so that when all instances of <meta property="mw:modulestyles" content="foobar"/> are gone, the service knows to update the modulestyles list. (Or something like that). But.. that's for another task.

That other task is T156414: VisualEditor does not handle wikitext that adds ResourceLoader modules correctly, I believe.

This trade-off is acceptable for template styles because it provides users with a simple model where template transclusion and template styles always go hand-in-hand when they are updated after an edit. In addition, it also relieves any duty of keeping backwards-compatibility with previous versions of a template, just like how inline styles are maintained now. And updates wil still propagate through the job queue to other pages as before.

None of that applies to the model that was actually proposed for TemplateStyles-using-RL, specifically that the RL modules would have referred to the stylesheet by revision ID rather than title so not-yet-updated pages would have still loaded the old version of the stylesheet. But it doesn't matter since TemplateStyles-using-RL was not opposed on those grounds.

We also want to keep the door open for a future where we embed critical styles with the rest being deferred (T127328 and related tasks). Some of these (such as SyntaxHighlight) don't affect layout and can be deferred, whereas others cannot.

To some extent we do some of that on an ad hoc basis by having one module (loaded with addModuleStyles) for the critical styles and another (loaded with addModule) for the rest. It doesn't actually embed them, but it reduces the size of the linked stylesheet and defers the rest to being loaded with async JS. Except that doesn't work if we want the styles loaded even if the browser doesn't have JS.

OTOH, there's a potential debate to be had there over whether the flash of unstyled (but properly laid out) code before SyntaxHighlight's stylesheet loads is an issue. Again, I don't care too much but definition of when exactly FOUC does and doesn't matter would be helpful.

The strawman seems reasonable to me.

As for Parsoid, yes, we'll have to implement deduplication in Parsoid. It will be done as a DOM pass right before serialization (which is the equivalent of ParserBeforeTidy hook -- tangentially, we've discussed plans of redoing the parser hooks so they don't reference parser pipeline events and the same hook can be implemented in alternate parsers). The data-mw-embedded-module unique id can help with deduplication. So, whether you emit <link> tags or <style> tags (as @Tgr suggests), please add these unique attributes to help with this depuplication since Parsoid doesn't know about nested templates which could also emit their own template-styles. I suppose there will be a separate style tag for every nested template that emits its own template styles vs. all the styles getting consolidated into one blob per top-level transclusion (which would make deduping more difficult).

I also think @Krinkle has a valid observation about embedding vs. header styles for extensions like Cite, SyntaxHighlight, etc.

In T160563#3109934, @ssastry wrote:

The data-mw-embedded-module unique id can help with deduplication. So, whether you emit <link> tags or <style> tags (as @Tgr suggests), please add these unique attributes to help with this depuplication

Parsoid is the reason I included that on the <style> tags, I knew you'd find it useful.

I suppose there will be a separate style tag for every nested template that emits its own template styles vs. all the styles getting consolidated into one blob per top-level transclusion (which would make deduping more difficult).

Yes, that's the plan. Note this isn't just TemplateStyles anymore, BTW.

I agree with @Krinkle's position. You took my recommendation that was about TemplateStyles out of context. We never said styles embedding should be a solution for everything. User-generated styles and extension styles are different. I expect extension styles to be able to modify anything on the page, including the chrome, while template styles should only affect the template contents. This is the main reason why the technical compromise for TemplateStyles can't be generalized to both situations. Their scope is fundamentally different. I can't speak for each extension listed because I don't know them all, but you're trying to solve something that isn't broken. Extensions should already only include with addModuleStyles things that *have to* be in the head to avoid FOUC, otherwise they should use addModule. There's no need to change that and the benefits of cross-page caching for using RL is high for extensions, that tend to apply (and need) their styles to whole namespaces or even all pages. Whereas TemplateStyles will create a much more scattered usage pattern of unknown distribution and a multiplication of small snippets that can't be grouped easily in a consistent fashion, whereas extension styles are easily rolled up together for great caching.

There might be some low-hanging fruit in extensions that use addModuleStyles more than they should. I'd start with that technical debt before considering a change in extension styles delivery.

In T160563#3113722, @Gilles wrote:

I expect extension styles to be able to modify anything on the page, including the chrome, while template styles should only affect the template contents.

Do you expect styles that an extension adds in response to certain wikitext in the page to be modifying chrome?

Extensions should already only include with addModuleStyles things that *have to* be in the head to avoid FOUC, otherwise they should use addModule.

You're missing a third possibility: an extension that wants to provide styles even for browsers without JavaScript does not seem to have any option besides addModuleStyles. addModule seems to require JS to function.

Good point, so you're talking about taking those styles statements that are in the head solely because of our no-JS support, and wouldn't need to be in the head if it wasn't for that, and bringing them inline inside the page?

That's one thing I'm talking about.

The extensions I flagged are (unless I made a mistake) all extensions that are calling addModuleStyles in their parser tag hook or parser function handlers, which I presumed is for styling the output of their tags or functions. If that's actually not the case for some extension, then the extension should probably be stricken from the list. Something like Cite on enwiki could possibly also be struck since, even though the styles it outputs are only for styling the output of its tags, it's the primary mechanism for adding citations and citations of some sort are by local policy supposed to be used on all articles.

Styles that are in the head only because they need to support people with JS disabled could potentially also be solved by coming up with some mechanism for asynchronous loading of external CSS without requiring JS, but that seems more likely to create more instances of FOUC even if they're not layout-affecting FOUC (e.g. SyntaxHighlight's output initially rendering without color, then suddenly getting color once the stylesheet loads).

In T160563#3114346, @Anomie wrote:

In T160563#3113722, @Gilles wrote:

Extensions should already only include with addModuleStyles things that *have to* be in the head to avoid FOUC, otherwise they should use addModule.

You're missing a third possibility: an extension that wants to provide styles even for browsers without JavaScript does not seem to have any option besides addModuleStyles. addModule seems to require JS to function.

For the moment, which way an extension goes for this use case is a judgement call. Either way is fine and merely an optimization. Further in the roadmap for the stylesheet queue is that the non-critical styles (which don't logically require JS, but also don't need to be synchronously loaded in the head) will be deferred using JavaScript (which also enables perfect defragmentation and local caching) - with a noscript fallback as regular link-rel-stylesheet. (T127328)

That essentially solves this use case. The extension would load it using addModules(), and the module in question would be entirely or partially marked as non-critical in its registration.

Either way, I stand by my point that extension stylesheets and user-authored stylesheets are for the most part very different use cases. The only thing they have in common is that their styles may have their effective scope limited to a piece of content (mandatory for template styles; optional for extensions).

Extensions should continue to use the head stylesheet for critical styles. In the future, non-critical styles will load in a batch request for deferred styles. However, the user-generated styles will be inlined regardless of them being critical or not because of the other use cases and requirements for template styles.

Change 347441 had a related patch set uploaded (by Anomie):
[mediawiki/core@master] Parser: Allow embedding ResourceLoader style modules inline, with deduplication

https://gerrit.wikimedia.org/r/347441

Change 347443 had a related patch set uploaded (by Anomie):
[mediawiki/extensions/TemplateSandbox@master] Set the ParserOptions::setCurrentRevisionCallback() $isSafe flag

https://gerrit.wikimedia.org/r/347443

Change 347445 had a related patch set uploaded (by Anomie):
[mediawiki/extensions/MobileFrontend@master] ApiMobileView: Handle embedded stylesheets

https://gerrit.wikimedia.org/r/347445

In T160563#3169556, @gerritbot wrote:

Change 347441 had a related patch set uploaded (by Anomie):
[mediawiki/core@master] Parser: Allow embedding ResourceLoader style modules inline, with deduplication

https://gerrit.wikimedia.org/r/347441

In T168333#3363475, @gerritbot wrote:

Change 347444 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/extensions/TemplateStyles@master] Deduplicate embedded style rules

https://gerrit.wikimedia.org/r/347444

Looking briefly over these patches, I'm still somewhat uncomfortable with the Parser needing to know so much about ResourceLoader. (Not to mention potential implications for Parsoid, but leaving that outside the picture for now.)

I'd like to evaluate possibilities for simplifying this somewhat, hopefully allowing us to get to a point where we can de-duplicate the styles without the Parser needing to know about ResourceLoader specifically, and without the solution being specific to the embedding of invariant styles-only ResourceLoader modules.

• Strip markers should make this easier, which I see the proposed patches are already making use of.
• If I recall correctly, we're avoiding using the expanded <styles> elements as the strip marker value because, while that would solve the above and provide de-duplication, it would do so at the cost of needing to re-build the stylesheet repeatedly during parsing. Outcome is fine, but cost is not. Agreed.
  • Perhaps we can reduce or eliminate this cost? Actually, since this RFC started, a fair amount of refactoring has taken place in ResourceLoader. Specifically 5ddd7f91c767 and various subsequent patches. That refactor introduced an in-object cached getModuleContent method. It would be good to confirm where we stand now, and how much overhead is left (if any).
  • Alternatively, given strip markers, strip marker Closure callbacks, and the Parser's state management, I'm wonder if could use that state management to naturally de-duplicate as part of strip-marker expansion. Even something as simple as ParserOutput/setExtensionData (or some reliable/contextual object) could perhaps work to track which ones have already been expanded. This would essentially eliminate the need for the in-between stage (strip markers to style tags, instead of strip markers, to link tags, to style tags).

The "alternative" idea also has the benefit / side-effect of it not being mandatory for the TemplateStyles modules to be registered, nor be instantiable by name, nor be loadable from load.php. It would essentially allow a minimalist approach of just using the WikiModule interface to fetch and generate the stylesheets directly.

I don't think registering the modules or exposing them at load.php is a bad thing. I'm just trying to reduce the number of public interfaces introduced that don't directly relate to TemplateStyles' use cases - especially given the long-term commitments and maintenance overhead they introduce (E.g. Gadgets and user scripts would be able to arbitrary load template styles - interesting, but not discussed or proposed at this point.)

In T160563#3483015, @Krinkle wrote:

• Alternatively, given strip markers, strip marker Closure callbacks, and the Parser's state management, I'm wonder if could use that state management to naturally de-duplicate as part of strip-marker expansion. Even something as simple as ParserOutput/setExtensionData (or some reliable/contextual object) could perhaps work to track which ones have already been expanded. This would essentially eliminate the need for the in-between stage (strip markers to style tags, instead of strip markers, to link tags, to style tags).

The problem with that plan is that strip markers can be expanded without the result actually ending up in the output page. We need to avoid situations where the first instance gets expanded but thrown away, then later instances are output but empty thanks to that missing first instance.

Not to mention potential implications for Parsoid, but leaving that outside the picture for now.

Parsoid would presumably call into the PHP parser via the API for extension parser functions and tags, like it currently does. It would end up with no deduplication unless it does it itself though.

I'd like to evaluate possibilities for simplifying this somewhat, hopefully allowing us to get to a point where we can de-duplicate the styles without the Parser needing to know about ResourceLoader specifically, and without the solution being specific to the embedding of invariant styles-only ResourceLoader modules.

After giving this some thought, if RL modules are indeed cheap to load multiple times then we have some other options available.

• We could output the <style> tags immediately (as TemplateStyles does now) and deduplicate them later.
  • The deduplication could potentially happen in a post-output transform (what you're adding in If2fb32fc), making it easier for something like MobileFrontend to avoid the core deduplication if necessary (see Ie4b788af).
• We could avoid the Parser dealing with RL at all, instead having the extension pass the CSS text.
  • TemplateStyles wouldn't need to register RL modules at all then.
  • Other extensions might use your "just using the WikiModule interface to fetch and generate the stylesheets directly" idea.

Change 347441 abandoned by Anomie:
Parser: Allow embedding ResourceLoader style modules inline, with deduplication

Reason:
Going to do things a different way

https://gerrit.wikimedia.org/r/347441

Change 347443 abandoned by Anomie:
Set the ParserOptions::setCurrentRevisionCallback() $isSafe flag

Reason:
Per abandonment of Ibc3fc372

https://gerrit.wikimedia.org/r/347443

Change 347445 abandoned by Anomie:
ApiMobileView: Handle embedded stylesheets

Reason:
Per abandonment of Ibc3fc372, going to do this differently

https://gerrit.wikimedia.org/r/347445

Change 393283 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/extensions/MobileFrontend@master] ApiMobileView: Disable 'deduplicateStyles' ParserOutput::getText() transformation

https://gerrit.wikimedia.org/r/393283

Change 393284 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] ParserOutput: Add 'deduplicateStyles' post-cache transformation

https://gerrit.wikimedia.org/r/393284

Change 393283 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] ApiMobileView: Disable 'deduplicateStyles' ParserOutput::getText() transformation

https://gerrit.wikimedia.org/r/393283

Change 393284 merged by jenkins-bot:
[mediawiki/core@master] ParserOutput: Add 'deduplicateStyles' post-cache transformation

https://gerrit.wikimedia.org/r/393284

The merged patches cover deduplication for the PHP parser (except for the mobileviews API which opted out and needs to do its own, per-section deduplication).