Page MenuHomePhabricator

Timeless WTE2017 preview has inconsistent styling
Closed, ResolvedPublic

Description

The ones I can point out directly as being inconsistent off the cuff are the H2s and the dts, but I think some elements in other places (unordered list items) have different styling as well. Compare:

Timeless intended.png (1×1 px, 236 KB)

Timeless in 2017 WTE preview.png (714×1 px, 108 KB)

Event Timeline

Esanders triaged this task as Lowest priority.Feb 16 2018, 7:53 PM

Possibly related, there's even some inconsistent styling in VE itself - thumbs seem to have monobook styles for some reason.

Why would VE be designed to duplicate the skin CSS? Questions that keep me awake at night.

I think it's most apparent with thumbs (and most annoying), see example below. Not only is the thumb monobook-like, but it also doesn't fill the whole width of the page like it should on this page width.

image.png (578×793 px, 170 KB)

Why would VE be designed to duplicate the skin CSS? Questions that keep me awake at night.

^ That is indeed a fascinating question.

Edit: oops, I misread the task as applying to VE in general, not to new wikitext mode preview. Sorry for the confusion, but still, thumbs don't work.

Ostrzyciel raised the priority of this task from Lowest to Medium.Dec 3 2020, 5:06 PM

To be fair, that it's part of a general problem probably makes this a bit more pressing anyway. Maybe the task as a whole should be updated to reflect that?

For some reason, after clicking preview in VE, it loads core styles (maybe skinning interface but I'm not sure) inline in the head element. Because it is loaded in a <style/>element in <head/>, it overrides any styles with the same specificity and breaks any skins that styles any content element.

After some investigation I discovered that VE is made to load new modules provided by the API on preview (T147702).
Take the output of Gladius page on Wikipedia for example:

<link rel=\"stylesheet\" href=\"/w/load.php?lang=en&amp;modules=mediawiki.skinning.content.parsoid%7Cmediawiki.skinning.interface%7Csite.styles%7Cext.cite.style%7Cext.cite.styles&amp;only=styles&amp;skin=vector\"/>

mediawiki.skinning.content.parsoid and mediawiki.skinning.interface is loaded on the page even when the skin specifies to not load them. VE should check if the current skin use the said modules and not assuming that the skin is Vector.

And at this point, I would think no skins should be using these modules, as at very least mediawiki.skinning.interface has been deprecated as of... what, 1.35 at least? And the parsoid one probably shouldn't exist either at this point.

But whether it's modules or features, why should parsoid/VE even be loading any of them? Let the skin figure that out and just use the right classes so the skin's styles apply consistently to the preview.

The mediawiki.skinning.content.parsoid module is loaded here? https://gerrit.wikimedia.org/g/mediawiki/extensions/VisualEditor/+/10c84cbf34ab026310c3412e9809a2126c0e1b82/extension.json#1729

I'm not sure what is loading mediawiki.skinning.interface... but that's not done by core.

I'm not sure what is loading mediawiki.skinning.interface... but that's not done by core.

It probably comes from here,
https://github.com/wikimedia/parsoid/blob/master/src/Wt2Html/DOMPostProcessor.php#L565-L567

The mediawiki.skinning.content.parsoid module is loaded here? https://gerrit.wikimedia.org/g/mediawiki/extensions/VisualEditor/+/10c84cbf34ab026310c3412e9809a2126c0e1b82/extension.json#1729

I'm not sure what is loading mediawiki.skinning.interface... but that's not done by core.

It is loaded in line through here (T147702) from the API output here.

It probably comes from here,

https://github.com/wikimedia/parsoid/blob/master/src/Wt2Html/DOMPostProcessor.php#L565-L567

I think that can/should be safely removed. Only skins should be loading mediawiki.skinning.interface. If that module is loaded on a skin that doesn't want it, it could override default styling rules.

Since there are some eyes from people who know things, I have an idiot's question that echoes one not really answered above.

If the (PHP) skin is already loading the styles (collectively through RL) and subsequently delivering them to the user, why is it that VE needs to load any styles itself? I may not be understanding how the Javascript and CSS (systems) interact here.

@Izno it's not an idiot's question. I'd be interested in the answer too :) As far as I can see mediawiki.skinning.content.parsoid is loaded because the HTML of Parsoid is different from the traditional PHP parser, so different styles are required. The module is used to avoid loading styles on pages which don't need them. In the future, when these parsers converge it shouldn't be needed. T278560 also has a bit of context.

I am not clear on a good reason why mediawiki.skinning.interface should ever be loaded. I'm hoping that's a bug...

I am not clear on a good reason why mediawiki.skinning.interface should ever be loaded. I'm hoping that's a bug...

There's a hint at that in the commit message of https://gerrit.wikimedia.org/r/c/mediawiki/services/parsoid/+/675180

Some clients of Parsoid (Kiwix?) consume Parsoid's HTML directly and make use of that default styling. VE, however, strips the head element and only looks at the body, so that's generally ignored. That patch for T162399 has some open questions though, about doing away with including a default stylesheet in the head and instead putting the burden on all clients to parse the necessary modules from meta elements and construct the stylesheet url themselves.

https://github.com/wikimedia/mediawiki-extensions-VisualEditor/blob/master/modules/ve-mw/ui/dialogs/ve.ui.MWSaveDialog.js#L211 looks like a bug, in that, it's trying to extract any additional modules that were added by an edit (for example, using an extension that previously didn't exist on a page would require a module to render correctly after being added to the page) and just loading any module it finds instead of ignoring the above problematic ones.

Change 708358 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/extensions/VisualEditor@master] Remove skinning modules when showing preview

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

Presumably this is the same thing as T187075

Change 708358 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Remove skinning modules when showing preview

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

matmarex assigned this task to Arlolra.

Thanks for the patch!

Change 708417 had a related patch set uploaded (by Alistair3149; author: Arlolra):

[mediawiki/extensions/VisualEditor@REL1_35] Remove skinning modules when showing preview

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

Change 708418 had a related patch set uploaded (by Alistair3149; author: Arlolra):

[mediawiki/extensions/VisualEditor@REL1_36] Remove skinning modules when showing preview

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

Cherry-picked patch to latest stable and LTS

Change 708418 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@REL1_36] Remove skinning modules when showing preview

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

Change 708417 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@REL1_35] Remove skinning modules when showing preview

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