Page MenuHomePhabricator

CSS is duplicated on each template usage
Closed, ResolvedPublic

Description

Another blocker in case the developing team haven’t noticed this − I thought would’ve not been repeated in Wikimedia extension (CSS extension [1] already has it and we all kind of learn on others’ mistakes). Templates such as navbox etc. are repeated for a dozen times sometimes [2], so it is deeply unacceptable to have same declarations in the code of the page.

[1]: https://www.mediawiki.org/wiki/Extension:CSS
[2]: https://ru.wikipedia.org/wiki/Загалло,_Марио

Details

Related Gerrit Patches:
mediawiki/extensions/TemplateStyles : masterDeduplicate embedded style rules
mediawiki/extensions/TemplateStyles : masterDeduplicate embedded style rules

Related Objects

Event Timeline

stjn created this task.Jun 19 2017, 6:36 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 19 2017, 6:36 PM
stjn updated the task description. (Show Details)Jun 19 2017, 6:38 PM
stjn updated the task description. (Show Details)
Anomie added a subscriber: Anomie.

It was decided in T155813: Decide on storage and delivery method for TemplateStyles CSS that deduplication would not be a blocker. Providing the necessary infrastructure is already filed as T160563: Provide infrastructure for embedding styles in the content HTML with deduplication and patches are already filed as Gerrit 347441, Gerrit 347442, and Gerrit 347444 (and Gerrit 347445).

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

@Tgr @Anomie is your sense about this patch we can do it with relatively low risk ahead of a deployment to technical type wikis (e.g., wikitech)? Or would it make more sense to deploy what we have and then apply this patch later?

CC @Fjalapeno

Tgr added a comment.Jul 13 2017, 3:48 PM

The patch is blocked on review of required ResourceLoader changes (which is one of those areas with fuzzy ownership; the Performance team maybe?). I don't think it's risky either way.

@Gilles what do you think about the ResourceLoader changes?

Anomie added a subscriber: Krinkle.Jul 16 2017, 2:00 AM

@Tgr @Anomie is your sense about this patch we can do it with relatively low risk ahead of a deployment to technical type wikis (e.g., wikitech)? Or would it make more sense to deploy what we have and then apply this patch later?

Consensus in T155813 was that we'd deploy TemplateStyles as it is now and deploy deduplication at a later date whenever it's ready. For example,

We don't want anything blocked on a performance impact/phenomenon we haven't studied yet. But we don't want it to be completely ignored either, because we know it's wasteful. Which is why we recommend starting with 3A and aiming for 3E where practical.

Let's not have that discussion all over again, please.

The patch is blocked on review of required ResourceLoader changes (which is one of those areas with fuzzy ownership; the Performance team maybe?).

"Performance Team" is what https://www.mediawiki.org/wiki/Developers/Maintainers says, although in practice it might really be "@Krinkle".

Hello, any progress here?

Thanks for asking. Aligning resources for code review is on the project manager's task list.

Thanks for answer!

Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.

Change 347444 abandoned by Anomie:
Deduplicate embedded style rules

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

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

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

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

Anomie moved this task from Up next to Doing on the TemplateStyles board.Jan 29 2018, 4:15 AM

Change 393285 merged by jenkins-bot:
[mediawiki/extensions/TemplateStyles@master] Deduplicate embedded style rules

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

Tgr added a comment.Feb 11 2018, 7:35 AM

Fixed for desktop views. Parsoid (for apps) and the mobileview API (for mobile web) still need deduplication, not sure if we want to track that in this task.

Anomie closed this task as Resolved.Feb 11 2018, 2:39 PM
Anomie claimed this task.

Nothing more is going to need to be done in TemplateStyles for Parsoid or mobileview. They'll need to do the equivalent of T160563: Provide infrastructure for embedding styles in the content HTML with deduplication / rMW9b2b375fcef1: ParserOutput: Add 'deduplicateStyles' post-cache transformation, for which separate tasks should be filed if necessary.

Deskana moved this task from Doing to Done on the TemplateStyles board.Mar 6 2018, 6:44 PM
Od1n added a subscriber: Od1n.Jul 22 2018, 9:32 AM

For the deduplicated styles, do we really need the <link> tags? Couldn't we completely remove the tag?

Only if we know that no client anywhere will ever need to reduplicate the styles.

stjn added a comment.Jul 23 2018, 3:02 PM

It doesn’t do anything bad to keep them, the only thing that might be interested in removing them is probably mobile version (since every byte seems to count there).

It doesn’t do anything bad to keep them, the only thing that might be interested in removing them is probably mobile version (since every byte seems to count there).

I don't see any particular reason to do this specifically in MobileFrontend. More generally, this is a matter of distinguishing between canonical HTML and presentation mode.

When editing in VisualEditor, or when rendering partials of pages on mobile web or mobile apps, it is important that the client can keep the stylesheet around, even if it removed the section it was a part of, as long as there is another section that had a reference to it. Likewise, when a section if viewed without any references to that template, it can be omitted.

However, for page views as output by a skin (Vector, Minerva, MobileFrontend), they are indeed not needed. It seems like this falls in the same category as a lot of Parsoid attributes not needed that we strip on rendering. Except this is the first (obvious) instance of such concept for the PHP parser output.

Od1n removed a subscriber: Od1n.Jul 23 2018, 3:49 PM
ssastry added a subscriber: ssastry.EditedAug 15 2018, 5:05 PM

I am starting to look at this from the Parsoid angle. Does the core parser de-duping rely on serialized text order to determine which one is the first occurrence ... i.e. depth-first pre-order traversal of the tree?

Alternatively, if a dom tree T with child subtrees S1 and S2 both have the same template style used in S1 and S2, ideally, both of those will be hoisted out and a single reference inserted in the root of T with link references in the original locations of S1 and S2. This ensures that styles for S1 and S2 are loaded before S1 and S2 are rendered.

Is one better / preferred compared to the other?

I am starting to look at this from the Parsoid angle. Does the core parser de-duping rely on serialized text order to determine which one is the first occurrence ... i.e. depth-first pre-order traversal of the tree?

I am going to go with this one for now.

Does the core parser de-duping rely on serialized text order to determine which one is the first occurrence [...] ?

Yes, it keeps the first instance of the <style> in the serialized HTML text.

Change 452964 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/services/parsoid@master] WIP: Dedupe template style occurrences

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

Tgr added a comment.Aug 20 2018, 12:13 PM

Is one better / preferred compared to the other?

Depth-first pre-order probably, as that loads the style as late as possible (while still avoiding flash of unstyled content) so it does not delay first paint if the elements are way down below the initial viewport. OTOH with hoisting you could easily get style tags just before the main content block if different paragraphs or sections of the article use the same template.