Add default gadget styling to Parsoid's output
Open, NormalPublic

Arlolra created this task.Mar 24 2017, 12:09 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 24 2017, 12:09 AM

Suppose this is related to T139852

If skins is an empty list, then it means all skins. Using formatversion=2 (https://en.wikipedia.org/w/api.php?action=query&list=gadgets&gaprop=id|metadata&formatversion=2) gives real booleans for the default field.

How does this correspond to what's found here though,

<link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=en&amp;modules=ext.echo.badgeicons%7Cext.echo.styles.badge%7Cext.gadget.WatchlistBase%2CWatchlistGreenIndicators%7Cext.uls.interlanguage%7Cext.visualEditor.desktopArticleTarget.noscript%7Cext.wikimediaBadges%7Cmediawiki.legacy.commonPrint%2Cshared%7Cmediawiki.sectionAnchor%7Cmediawiki.skinning.interface%7Cskins.vector.styles&amp;only=styles&amp;skin=vector"/>

For example,

{
                "id": "featured-articles-links",
                "metadata": {
                    "settings": {
                        "rights": [],
                        "skins": [],
                        "default": true,
                        "hidden": false,
                        "shared": false,
                        "category": "appearance",
                        "legacyscripts": false
                    },
                    "module": {
                        "scripts": [
                            "MediaWiki:Gadget-featured-articles-links.js"
                        ],
                        "styles": [
                            "MediaWiki:Gadget-featured-articles-links.css"
                        ],
                        "dependencies": [],
                        "peers": [],
                        "messages": []
                    }
                }
            },

doesn't appear in ext.gadget.WatchlistBase%2CWatchlistGreenIndicators

Arlolra triaged this task as Normal priority.Mar 28 2017, 2:10 PM

The Parse API by design only exposes modules added through Parser/ParserOutput. Modules added by OutputPage, Skin, or extensions are not exposed.

I tried to fix this as part of T130632: mediawiki.toc module unnecessarily shipped in mobile views:

The main one here is df5b12264, which adds a useskin parameter to the Parse API that will pull the ParserOutput through OutputPage and Skin, ensuring that any default modules or skin-specific modules get queued as well.

For example:

https://sv.wikipedia.org/w/api.php?action=parse&page=Mir&prop=modules|jsconfigvars

"modules": [
    "ext.cite.a11y"
],
"modulescripts": [],
"modulestyles": [
    "ext.cite.styles"
],

https://sv.wikipedia.org/w/api.php?action=parse&page=Mir&prop=modules|jsconfigvars&useskin=vector

"modules": [
    "ext.cite.a11y",
    "site",
    "mediawiki.page.startup",
    "mediawiki.user",
    "mediawiki.hidpi",
    "mediawiki.page.ready",
    "mediawiki.toc",
    "mediawiki.searchSuggest",
    "mediawiki.page.watch.ajax"
],
"modulescripts": [],
"modulestyles": [
    "ext.cite.styles"
],

However, it seems gadgets are still not included. Not sure why..

Krinkle added a comment.EditedJul 22 2017, 10:33 PM

However, it seems gadgets are still not included. Not sure why..

Figured out the reason. Parse API applied OutputPage::addParserOutputMetadata() (which also runs the OutputPageParserOutput hook), and applies Skin::getDefaultModules().

However, Gadgets (like many other extensions) use the BeforePageDisplay hook to queue modules, which is only run from OutputPage::output() – which makes semantically sense, and shouldn't be run in API context probably.

On the other hand, it's unfortunate that lots of extensions have started to use BeforePageDisplay as a very generic hook to do pretty much anything (which is more reason not to run it in API context). It's not an easy fix, but I'd rather deprecate using BeforePageDisplay to add modules and instead come up with a better way. Perhaps OutputPageParserOutput makes more sense, but that hook has two problems:

  1. It's primarily intended to perform actions that relate to the ParserOutput object, not generic unconditional modules added on every page, or things specific to the current user or skin. In addition, extensions should still be able to unconditionally queue a module in a way that also applies to special pages (e.g. EventLogging).
  2. OutputPageParserOutput may run multiple times since you can add multiple parser outputs to a page (which is in fact a common thing to do on special pages).

So with that, BeforePageDisplay actually makes a lot of sense. It's run only once, relates to page rendering, and has access to the skin.

So perhaps we should consider running it in Parse API (when the "useskin" mode is enabled). Do we any usage of this hook that would cause problems in API context?

@Anomie Thoughts?

ssastry moved this task from Backlog to Read Views on the Parsoid board.Jan 12 2018, 11:38 PM

Do we any usage of this hook that would cause problems in API context?

That's the important question. There are a lot of users of that hook, someone would have to look through them all.