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

stjn created this task.Jun 18 2018, 6:09 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 18 2018, 6:09 AM
Deskana triaged this task as High priority.Jun 25 2018, 2:02 PM
Deskana added a subscriber: Deskana.

Needs investigation from Editing engineers.

Vvjjkkii renamed this task from Template styles get ignored when switching to Visual Editor to graaaaaaaa.Jul 1 2018, 1:03 AM
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from graaaaaaaa to Template styles get ignored when switching to Visual Editor.Jul 2 2018, 4:42 AM
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.
Magol added a subscriber: Magol.Jul 7 2018, 9:59 AM

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).

Tgr added a subscriber: Tgr.Aug 7 2018, 12:56 PM

The styles are part of the page content (https://ru.wikipedia.org/api/rest_v1/page/html/Северное_море). Surely VE must load that?

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.

Tgr added a comment.Aug 9 2018, 4:28 PM

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?

DLynch added a subscriber: DLynch.Aug 9 2018, 5:05 PM

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.

DLynch added a comment.Aug 9 2018, 5:21 PM

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...)

DLynch claimed this task.Aug 16 2018, 4:51 PM

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?

Anomie added a subscriber: Anomie.Aug 20 2018, 2:09 PM

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.

Deskana moved this task from Up next to Doing on the TemplateStyles board.Aug 21 2018, 6:24 PM

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

ggellerman moved this task from Doing to Done on the TemplateStyles board.Aug 31 2018, 10:10 PM
Deskana closed this task as Resolved.Sep 2 2018, 12:16 PM