Page MenuHomePhabricator

Question: Should it be possible to load mediawiki.diff.styles on non-diff pages?
Closed, ResolvedPublicBUG REPORT

Description

Description of problem

In T350515 we had an UBN because the mediawiki.diff.styles were loaded on a page which wasn't a diff page (by FlaggedRevisions in this case). While that bug is easily fixed (and will be patched), it would be helpful to have some clearer guidance around which core ResourceModules modules are considered libraries (reusable) and which are not.

To take further examples:

  • the name mediawiki.special implies that will only ever be loaded on special pages
  • The name mediawiki.action.styles implies that it will only ever be loaded on ?action pages

For these there is no mention in the ResourceModule definition or any comment blocks inside the file so I am not sure what we would do if we encountered a patch trying to load either on these on diff pages in code review for example.

This may be a separate problem, but in the case of the diff page, there is no way to distinguish a diff page from an article page in CSS as there is no class on the body tag that distinguishes it ( action-view class is present on both diff and article pages) which makes diff-specific styling troublesome.

Timeline

There is no specific timeline for this, however having some clarity would likely lead to less problems of this nature on the long run.

Questions to answer

  • Should these files be documented explaining where they are expected to run? The module mediawiki.action.view.filepage for example has a comment saying "File description page"
  • Should the frontend stable policy have guidance around use of CSS. For example I wonder if @stable to use would be helpful for stylesheets that we expect to be reused.
  • Should we have a convention for module names for distinguishing reusable code and page specific code?
  • What is the recommended way for a skin to style a diff page differently from an article? (E.g. Should there be separate modules that it can apply a skinStyle to, or should there be class names on the BODY tag for styling of the diff page?

Event Timeline

The name mediawiki.diff.styles implies that it’s used to style diffs. Diffs appear not only on pages whose URLs contain diff=: they can also appear while editing (Show changes button), on special pages (e.g. MediaWiki-extensions-Translate's Special:PageTranslation may show diffs of changed translation units), or on article views like in case of FlaggedRevs (or VisualEditor edits, which are also technically article views). Diffs are all over the place, the module should make no assumptions as to what appears outside of the diff area.

I filed T252335: Decide how and where to describe ResourceLoader modules about the documentation aspect a while ago.

I think in general our most of our CSS/LESS files are terribly underdocumented (it's much easier to understand undocumented code than undocumented CSS code since for the latter you also need to figure out what UIs it applies to).

"Reusable vs. page-specific" doesn't seem like a useful distinction to me for CSS code. For JS, you can have library-style code which doesn't do anything until you invoke it, and code that has side effects when you load it; but CSS always has side effects when you load it.

Wrt. styling diff pages, keep in mind that diff pages can include the article itself, and various non-diff pages can include the diff (edit conflicts for example) so I think it's better to think about it in terms of styling diff blocks and styling content (article) blocks then styling pages.

MediaWiki-Platform-Team maintains ResourceLoader itself but doesn't generally own the resources that are loaded with it, so this question is outside the team's scope. (If you're specifically asking for a new ResourceLoader feature, such as a new mechanism for documenting modules, let's discuss in a dedicated task.) For documentation standards for frontend code, maybe the Front-end-Standards-Group (does it still exist?) or Design-System-Team might have an opinion. Diffs are unowned, so for the specific question in the task title, I think whoever is working on it currently is best placed to make a decision.

@Tgr I've made a suggestion on T252335. While it wouldn't answer the question here - it would allow us a way to encapsulate the answer to this question if we decide mediawiki.diff.styles is a reusable module.

It seems like we talked about this previously in 2012: T38294#418881 and the arguments there seem pretty sound, so I think it's fine to answer this question as "yes it should be possible". Before resolving I would like some way to document this decision so we don't run into it again. @Tgr do you think T252335 would be in scope for the platform team to unblock this?

I'd recommend resolving first and documenting whenever, I don't think it makes sense to delay fixing a bug just because there is no natural place to document the fix. I'll defer to @Krinkle on T252335.

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

[mediawiki/core@master] Diffs: Document usage of this module

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

As a general rule for all modules of all kinds, JS and styles alike, they expected to be safe to load anywhere at any time. Loading of modules is an optimisation, not a functional contract. I'm not aware of any module where this isn't the case today. Documenting this, implies it is a noteworth aspect of a small number of modules, which simply isn't the case, and has no precedent or enforcement in our ecosystem, to my knowledge.

Modules can and do arrive lazily through various means. And, importantly, stylesheets don't stop applying when the page content is dynamically changed. Pages can change quite a lot during the life of a browser tab.

I understand that T350515 is easily framed as an edge case where one could choose to blame the person adding new uses of a module via server-side loading. However, the same problem is already trivially exposed in the previous version of the software. For example: When ajax previewing during an edit, or when lazy-loading a a diff with ajax preview, or rendering a diff inside a modal dialog on any given article or special page, or FlaggedRevs adding UI elements in various places, or gadgets like RTRC adding diffs on special pages. Likewise in the other direction, when you start on a normal" diff, at a normal "diff" URL, you can transition from any page view or diff to an edit with VisualEditor, extensions and gadgets may introduce parts of UI or page content above, below, or in a modal on top of a diff page. In short: Anything can be added to a diff. And a diff can be added to anything. The same holds up for virtually any other UI feature in the software.

A button on Special:RecentChanges would be styled by .btn-recentchanges, .special-RecentChanges .btn, or .my-widget .btn (in the latter case, you're generalising and saying the widget has its own name and can appear elsewhere, but still scoped to it that way). Definitely not as generically as button { .. } in a CSS file and hope it only applies to one kind of URL.

That doesn't mean we have to invest in scenarios that we believe are unlikely or impossible, or that we can't take shortcuts or make mistakes. If it works on your machine, it's probably good enough to ship to production. But, when we then learn that we made assumptions that don't hold up, or find that our shortcut is incompatible with something, it's time to simply add the missing class to the selector and carry on.

It seems that what happened here is Minerva was modified to insert some extra styles into the mediawiki.diff.styles module that took a shortcut and used it to style unrelated UI elements elsewhere on a typical diff page. In addition, it did so without any parent selector or context that limited it to diff elements or diff pages. Can happen to anyone, no big deal, and is easily fixed.

In a nut shell: Styles can appear anywhere, even without someone adding them explicilty in a new server-side context. This is a given in our platform. Styles must not assume a load context other than through the CSS selector. That's the idea of declarative styling. Everything else is a load-time optimisation.

Similar deciplines were imposed by popular 2010-era frameworks that perform client-side rendering and e.g. split, tree-shake, and lazy load particular routes. All code there is written with the expectation that it can be bundled all on one page, you might SPA or Pjax navigate in some case. All the system promises is that stuff is there when you need it, but it most definitely is also there if you needed it in the past, or are likely to need it in the future. These choices are made to improve performance by preloading or bundle. We sometimes combine code for features that are likely to be used later in the same browsing session, even if not on the same URL, so that they arrive at once with better compression, and then later can render instantly from the browser cache without a network roundtrip.

addition, it did so without any parent selector or context that limited it to diff elements or diff pages. Can happen to anyone, no big deal, and is easily fixed.

Only there is no suitable parent selector. Unlike recent changes there is no page-diff class on the HTML element which means we still have an open bug with no clear way to fix it (except for Minerva to add its own class to the HTML element)

Diff pages (that is, non-special pages corresponding to action=view&diff=... URLs) are handled by Article::showDiffPage(). It should be trivial to add a body class there if you want to differentiate them from other kinds of article pages and/or from other pages that show diffs.

@Krinkle made arguments against this in T38294#418881 but that was back in 2012 so a long time ago: "It is not a different page action or layout, it therefor doesn't make sense to style the entire page differently just because it contains a diff component."

I do think given the mobile use case, it would be helpful to have this class to avoid this happening again. The specific remaining issue in Minerva (which is also blocking the removal of SpecialMobileDiff) is this one: T350637

Change 975344 merged by jenkins-bot:

[mediawiki/core@master] Diffs: Document usage of this module

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

Jdlrobson claimed this task.

Okay seems like we have a clear answer. Let's continue the conversation about body tag class in T350637. Thanks all!