Page MenuHomePhabricator

Template styles get ignored when switching to Visual Editor
Closed, ResolvedPublic

Description

Steps to reproduce:

  1. Go to https://ru.wikipedia.org/wiki/Северное_море
  2. Click ‘Править код’ (Edit source)
  3. Switch to Visual Editor with a popup or a dropdown menu
  4. Compare infobox styling (done through TemplateStyles) in VE and in article

Can’t be reproduced when using visual editor from the get-go.

Event Timeline

Deskana subscribed.

Needs investigation from Editing engineers.

Perhaps the TemplateStyles engineers could help us with this? The styles are only loaded on read pages, but VE can also load from a custom edit page (using the CustomEditor hook, VisualEditorHooks.php#onCustomEditor).

Typically not, but we did implement a hack to extract RL modules from the restbase response when loading the page for preview, perhaps we need to apply that to edit pages too.

These are not RL modules, just plain style tags with the CSS inlined.
If you do not load the page HTML, where does VE get the HTML from that gets displayed in the editable area?

The requirement is more specific than just switching to VE -- it has to be switching to VE from the classic editpage, rather than from NWE, because that causes a load of VE without the underlying view of the page existing.

TemplateStyles are included in parsoid's output in the form:

<style data-mw-deduplicate="TemplateStyles:r93112722" typeof="mw:Extension/templatestyles" about="#mwt84" data-mw='{"name":"templatestyles","attrs":{"src":"Шаблон:Геокар/styles.css"},"body":null}'>
// CSS HERE
</style>

That's in the body, output immediately within the relevant template's markup. (The CSS is scoped under .mw-parser-output which applies fine to VE's output, if said CSS is used at all.)

As such, I'm fairly sure we'll wind up discarding that <style> before it ever makes it into the page's DOM. We load the parsoid HTML, make a new document from it, create VE's model by scanning that, and then render the page we display based on that.

Presumably the course here is to adjust our model creation to preserve the style tag. It seems to be contained within the <p rel="mw:Transclusion">, so it should be workable.

See ve.init.mw.ArticleTarget.prototype.loadSuccess, as that is where we pick apart the full Parsoid DOM, including header data.

I suspect the most relevant bit for that is ve.init.mw.Target.prototype.setupSurface, since that's where we actually turn the DOM into our model. (Though I grant I haven't stepped through and verified that the <style> hasn't been quietly stripped somewhere else where I didn't see it...)

We filter out the <style> in ve.ce.GeneratedContentNode.prototype.getRenderedDomElements, because it calls ve.filterMetaElements on the template's markup, which removes all the link and style tags present. (Apparently because them being in cut content was crashing Chrome, per T52043.)

I'm thinking we can do an explicit pulling-out of the style tags before that step in ve.ce.MWTransclusionNode.prototype.getRenderedDomElements so we can re-add them to the template rendering afterwards. I'll test that and see how it works.

This is all still going to leave us with the fun problem that VE's not going to display the effect of removing a template whose TemplateStyle affects other content in the document because the <style> for the original template is going to be there hidden away in the original content. (Also, possibly rendering differences because of ordering issues due to repeated style tags.)

Now, we can fiddle with the original DOM content to disable any style tags present in it... I'm pretty sure that just doing $('.mw-parser-output style[data-mw-deduplicate^="TemplateStyles:"]').prop('disabled', true) would do it safely and reversably. Worth it, you think?

This is all still going to leave us with the fun problem that VE's not going to display the effect of removing a template whose TemplateStyle affects other content in the document because the <style> for the original template is going to be there hidden away in the original content. (Also, possibly rendering differences because of ordering issues due to repeated style tags.)

Aren't the imported style rules scoped in such a way that they only apply to the target template?

Aren't the imported style rules scoped in such a way that they only apply to the target template?

No. That's something that has been talked about for a possible future, once templates can be flagged as outputting a complete DOM subtree (I think T114445 is the relevant task for that).

Even if that were to happen, the deduplication would mean it would still have to apply to every instance of the template on the whole page rather than a specific instance.

Ok. I don't think leaving old stylesheets on the page would make the experience much worse. Let's get the styles on the page, and worry about removing them as a lower-priority follow-up.

Change 454068 had a related patch set uploaded (by DLynch; owner: DLynch):
[VisualEditor/VisualEditor@master] GeneratedContentNode: make rendered element filtering overridable

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

Change 454069 had a related patch set uploaded (by DLynch; owner: DLynch):
[mediawiki/extensions/VisualEditor@master] TemplateStyles support

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

The remaining TODO after those patches: if multiple copies of a template with deduplicated styles are on the page, and the one containing the actual <style> is removed, all will lose their styling.

...also, the TODO there may not be necessary to address, because I think the Parsoid output for us just has a <style> for every template rather than using <link>s for deduplication. Not 100% confident that's always the case, though.

Change 454068 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] GeneratedContentNode: make rendered element filtering overridable

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

Change 454567 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (874470c1b)

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

Change 454567 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (874470c1b)

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

Change 454069 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] TemplateStyles support

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