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.

I looked at https://sv.wikipedia.org/wiki/Mir?veaction=editsource in the 2017WTE preview (Parsoid) and the saved page. While the two aren't absolutely identical (e.g., the white space in the infobox is slightly different), I didn't see anything that actually looked broken. Is that expected, or has this problem somehow gotten better since this task was created ~16 months ago?

cscott added a subscriber: cscott.Sep 13 2018, 5:36 PM

Strawman: what if we (a) deprecate using BeforePageDisplay to add modules, but (b) add a new hook (AddParserModules?) which is run at basically the same time, but which is also run during the Parse API when useskin is enabled. That allows us to incrementally migrate users of BeforePageDisplay as a module-hook, without risking breakage by running BeforePageDisplay in the ParseAPI where existing callers may not expect.

cscott claimed this task.Sep 13 2018, 5:37 PM

Change 460406 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/core@master] WIP: hard-deprecate adding modules from BeforePageDisplay hook

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

The other thing the Gadget extension does in the BeforePageDisplay hook is add warnings about using legacy APIs as raw HTML concatenated to the end of the display page. This is the sort of thing we probably *don't* want to expose/allow in the Parse API?

https://github.com/wikimedia/mediawiki-extensions-Gadgets/blob/master/includes/GadgetHooks.php#L222

So we'd split the hook in two; the ExtraPageModules hook would iterate through the gadgets and add the styles and modules, and then the BeforePageDisplay hook would just add the warning text at the bottom.

Since adding modules should be idempotent, extensions which want to migrate to the new hook structure could invoke the ExtraPageModules hook function explicitly at the top of BeforePageDisplay, and we can be sure that that is 'safe' (ie, doesn't emit deprecation warnings, the redundant modules are ignored).

Change 460422 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/extensions/Gadgets@master] WIP: Use new ExtraPageModules hook

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

Reedy edited projects, added Parsoid-Read-Views; removed Parsoid.Sep 17 2018, 7:25 PM
cscott added a comment.EditedSep 18 2018, 1:28 PM

@Krinkle, @Legoktm does the above plan seem reasonable? there's a WIP patch in gerrit, but I don't want to develop it further before getting signoff on the theory.

EDIT: nevermind, I see discussion has started on https://gerrit.wikimedia.org/r/460406

Other users of the BeforePageDisplay hook, via codesearch:

  • AdvancedMeta, for example
    • Uses: $out->addJsConfigVars(), $out->getTitle()
    • Don't move to new hook: $out->addMeta(), $out->setIndexPolicy(), $out->setHTMLTitle()
  • Wikibase, for example
    • Uses: $out->addJsConfigVars(), $out->getProperty( 'wikibase_item' ), $skin, $out->addModules()
  • CentralAuth, for example
    • Uses: $out->addJsConfigVars(). $out->addModules()
    • Don't move: $out->addHeadItem(), $out->addHTML(), $out->getRequest()->getSessionData()
  • ContentTranslation, for example
    • Uses: $title, $user, $out->getRequest()->getCookie(), $out->addModules(), Action::getActionName( $out->getContext() )
  • Echo, for example
    • Uses: $user, $skin->getSkinName(), $out->addModules(), $out->addModuleStyles()
  • EventLogging, for example
    • Uses: $out->addModules(), $user, $out->getRevisionId(), $out->addSubtitle() (don't move this one?)
  • MobileFrontend, for example
    • Don't move? $out->addLink(), $out->addVaryHeader(), etc
  • MultiLanguageManager, for example
    • usual stuff, jsvars, modules, modulestyles, $title
  • ORES, for example
    • Uses: $out->getProperty( 'oresData' ), Helpers::isHighlightEnabled( $out ) ($user/$title, aka $context)
  • PageImages, for example
    • Don't move: $out->addMeta()

... ran out of energy to review codesearch results at this point.

There's also a BeforePageDisplayMobile hook...