Page MenuHomePhabricator

Remove Parsoid specific modules inside ResourceLoaderSkinModule
Closed, ResolvedPublic

Description

The fact The ResourceLoaderSkinModule ships both parser and parsoid specific styles is continually a source of confusion, but a necessary evil as we look to make Parsoid the default parser everywhere, to ensure they we keep compatibility with the existing parser. We need to consolidate this in the future.

Parsoid specific styles are kept inside a content-media feature and a configuration flag $wgUseNewMediaStructure is used to enable the content-media feature.

A content-thumbnails module provides some styles that relate to both parsoid and the PHP parser

A mediawiki.skinning.content.parsoid module exists, but it does not use ResourceloaderSkinModule or features. This is because several pages, for example, the editor, load this module, and need the styles regardless of whether the skin supports Parsoid or not.

Shipping both Parsoid and PHP Parser styles at the same time would unnecessary bloat page critical CSS where performance matters e.g. Minerva skins

  1. Goal
  2. The content-media and content-thumbnails modules are merged. The output of this feature should depend on which parser is being used. Presumably based on $wgUseNewMediaStructure

Descoped

  • The mediawiki.skinning.content.parsoid module is deprecated.
  • The content-parser-output feature should output CSS inside mediawiki.skinning.content.parsoid when Parsoid is enabled

My understanding is that mediawiki.skinning.content.parsoid cannot be deprecated until Parsoid is the default. Until then things like VisualEditor need it available outside the context of the skin

Open questions

  • Is there a PHP function that can be used to reliably check if PHP Parser or Parsoid code is being returned?

Event Timeline

Change 675143 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):
[mediawiki/core@master] Merge content-media and content-thumbnails modules

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

@Arlolra would this work for you, or do you imagine these 2 as being mutually exclusive?

Change 675143 abandoned by Jdlrobson:

[mediawiki/core@master] Merge content-media and content-thumbnails modules

Reason:

Waiting for further guidance on ticket from @Arlolra. Not a priority of mine right now.

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

Parsoid specific styles are kept inside a content-media feature and a configuration flag $wgUseNewMediaStructure is used to enable the content-media feature.

Not exactly, or maybe I'm misreading. The plan is to get the legacy parser to output media structured the same way Parsoid would. This is so that, when we get to enabling Parsoid, the difference in output isn't so jarring. In other words, the flag $wgUseNewMediaStructure configures the legacy parser and doesn't have anything to do with Parsoid.

The patch for that is,
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/507512

@Arlolra would this work for you, or do you imagine these 2 as being mutually exclusive?

I'm just going to copy what I wrote previously because I think that's what you're asking.

From https://gerrit.wikimedia.org/r/c/mediawiki/core/+/647333/6/includes/resourceloader/ResourceLoaderSkinModule.php#95

In theory, I think they'd eventually be mutually exclusive

But in practice, content.thumbnails.less still contains stuff like,

div.floatright,
table.floatright
which isn't in media.less and I'm not sure where that's used

Also, with $wgAllowImageTag and whatnot, there's undoubtably some other hand crafted content out there using these styles that isn't the result of the parser. So, while the generated output would only use media.less when $wgParsoidMedia is true, that other content would still rely on content.thumbnails.less and we'd want to take some time with the community getting off the old styles

Hopefully that's what you were asking.

Is there a PHP function that can be used to reliably check if PHP Parser or Parsoid code is being returned?

I can't find an explicit link but there has been some talk on the Parsoid Team about using Parsoid in core for some smaller wikitext use cases (like interface messages) as a milestone towards making Parsoid the default wikitext engine. I bring this up as a caution that maybe the above question is not the right one to be asking.

https://www.mediawiki.org/wiki/Parsing/Parser_Unification

Okay so it sounds like in future content-media might merge into content-thumbnails e.g. these rules are additional?

If that's correct, perhaps in future, we can have one feature for these, who's output depends on the feature flag?

Okay so it sounds like in future content-media might merge into content-thumbnails e.g. these rules are additional?

Yes, I believe so. To give a more concrete example of why we can't just replace content-thumbnails, see the output of the {{wide image}} from a recent task, T281652#7053582. An inline image is used inside hand-crafted (as opposed to parser generated) structure using those classes.

If that's correct, perhaps in future, we can have one feature for these, who's output depends on the feature flag?

We don't have to wait until the future if there's already a way of doing that.

Change 690769 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Merge content-media and content-thumbnails

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

^ @Arlolra for your consideration (no urgency from my side)

^ @Arlolra for your consideration (no urgency from my side)

Reviewed

Change 690769 merged by jenkins-bot:

[mediawiki/core@master] Merge content-media and content-thumbnails

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

Jdlrobson updated the task description. (Show Details)