Change 280487 had a related patch set uploaded (by Jdlrobson):
Disable toc in ParserCacheSaveComplete hook

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

Change 275901 abandoned by Jdlrobson:
Don't add mediawiki.toc to pages with toc stripped

Reason:
See https://gerrit.wikimedia.org/r/280487

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

@phuedx let's pair on this tomorrow. I investigated again and this patch seems to do the job for me so I'm keen to understand why it's not working for you. It also seems impossible to do this on the skin level...

OutputPageParserOutput hook runs way before the skin even gets to touch the code so ParserCacheSaveComplete still seems the right place to do this for me.

Change 280487 abandoned by Jdlrobson:
Disable toc in ParserCacheSaveComplete hook

Reason:
Let's talk about this issue on the bug. I feel like we are going round in circles.

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

Let's discuss this here - @Krinkle and @phuedx.
I think we will need a new hook to make this happen.
The problem is essentially this:

1. onOutputPageParserOutput hook runs at the end of addParserOutputMetadata, it is the only hook we can reliably use to manipulate the ParserOutput object.
2. addParserOutputMetadata runs inside addParserOutput before $parserOutput->setTOCEnabled is called - this allows us to disable the table of contents in the output HTML...
3. ... however $this->addModules( $parserOutput->getModules() ); runs before the OutputPageParserOutput hook runs inside addParserOutputMetadata. This means that the table of contents module is irreversibly passed to the OutputPage.

To avoid this we have several options:

1. Add a hook so that table of contents HTML and JavaScript can be disabled at the start of addParserOutputMetadata
2. Do not automatically add the module to the page at this level inside addParserOutputMetadata. Allow that to happen further down the stack - e.g. in the Skin.

Which is a better solution do we think?

I think we could go a step further here: it should be the responsibility of the skin to decide what to do with the TOC, including which RL modules, if any, to add to the output if there's a TOC present on the page. However, the parser currently adds the module directly to the output.

If the parser were to flag that there was a TOC present on the page, e.g. via ParserOutput#setTOCEnabled, then the skin can make a decision as to how to respond. To maintain backwards compatibility, all skins would add the mediawiki.toc module to the output by default.

In T130632#2189449, @phuedx wrote:

I think we could go a step further here: it should be the responsibility of the skin to decide what to do with the TOC, including which RL modules, if any, to add to the output if there's a TOC present on the page. However, the parser currently adds the module directly to the output.

If the parser were to flag that there was a TOC present on the page, e.g. via ParserOutput#setTOCEnabled, then the skin can make a decision as to how to respond. To maintain backwards compatibility, all skins would add the mediawiki.toc module to the output by default.

Problematically the ParserOutput is used by API requests to the parse API to work out which modules to add.
We could potentially move the call to OutputPage->addModules from the parser into the skin, but I'm not sure whether this will break other things.

In T130632#2189360, @Jdlrobson wrote:

Let's discuss this here - @Krinkle and @phuedx.
I think we will need a new hook to make this happen.

A hook will probably increase complexity. I don't think that's worth it in this case, and will make things harder to reason about overall. Especially because it makes behaviour unpredictable from the caller's perspective, and is likely to introduce bugs over the course of future maintenance.

In T130632#2189449, @phuedx wrote:

I think we could go a step further here: it should be the responsibility of the skin to decide what to do with the TOC, including which RL modules, if any, to add to the output if there's a TOC present on the page.

Sounds good. This is a clean and appropriate solution.

In T130632#2195327, @Jdlrobson wrote:

Problematically the ParserOutput is used by API requests to the parse API to work out which modules to add.
We could potentially move the call to OutputPage->addModules from the parser into the skin, but I'm not sure whether this will break other things.

A good use case is Live Preview. It should know what to do when getting the parser output and metadata from the Parse API.

Either we need to move responsibility to the caller (e.g. the code that calls the Parse API should know about skin-specific stuff, this requires adding skinScripts to Live Preview JS). Or we need a 'skin' parameter to the Parse API, that would invoke the relevant callback from the Skin class and allow it to manipulate the response html and meta-data in some way.

The latter seems ugly, but it does sound like the right separation. I'm curious how mobile handles this right now since MobileFrontend also changes the output and which modules are loaded, but does so in an output-way only. Meaning, if mobile JS calls the Parse API, it cannot blindly insert that into the page without additional knowledge. Right?

@Jdlrobson - should this still be high priority?

We're still loading code that we shouldn't be so yes. It would be worth estimating/discussing this task.

This also shows up in the Lighthouse report for mobile pages.

MobileFrontend has a custom table of contents for tablets. And while the TOC is successfully stripped from the HTML on the server-side, it fails to also unload the mediawiki.toc module, which means mediawiki.toc.css is still loaded on every page.

Here's the full report.

Thanks for the bump and helpful/awesome information @Krinkle!

Preliminary estimation based on previous experiences here.

Would it be easier to use skinStyles and skinScripts for this to allow skins to empty these?

In T130632#3084858, @Jdlrobson wrote:

Would it be easier to use skinStyles and skinScripts for this to allow skins to empty these?

The table of contents HTML does not vary by skin and core dropped support for skin-varying HTML of most Linker methods in favour of standardising these across skins and using only CSS to customise them. MobileFrontend does not universally alter table of contents generation with its own (because it can't, it only does so on page views where possible). There may very well be places that output core/Linker's version of TOC html through some extension.

I donk think it would be appropiate for MobileFrontend to re-use the mediawiki.toc module for its styles for its table of contents, since they're not meant to be compatible. Something that loads mediawiki.toc is expecting styles applicable to core's TOC HTML.

Right now MediaWiki core only supports the parser deciding to enable or disable the TOC. And along with it, whether or not mediawiki.toc is loaded.

I don't know why, but OutputPage and ParserOutput have a setTOCEnabled method. And this is used by ParserOutput::getText() to retroactively strip out the table of contents HTML. However, the meta data containing the module queue is already unconditionally absorbed by OutputPage. This is why the module is loaded on mobile even though the is TOC was disabled.

I'm proposing that the Parser doesn't enqueue this module. Instead, have it be loaded by either Skin/SkinTemplate or OutputPage. That way, it is enqueued after any call to setTOCEnabled() will have happened.

Many similar module loading happens this way already, see Skin::getDefaultModules() and OutputPage::output() for examples. Both have a section that has addModule calls in various conditionals based on the current page and user.

Moving this to Triaged but Future as it's been in Sprint +1 for close to two months.

Change 349365 had a related patch set uploaded (by Krinkle):
[mediawiki/core@master] Deprecate OutputPage methods meant for changing ParserOutput

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

Change 349367 had a related patch set uploaded (by Krinkle):
[mediawiki/core@master] OutputPage: Move mediawiki.toc load from Parser to OutputPage

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

From grooming, @Jdlrobson Do we want to bring this to the sprint to help review or create followup tasks to update MobileFrontend from Krinkle's recommendation?

Change 353592 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] mediawiki.action.edit.preview: Enable 'useskin' mode for action=parse API

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

Change 354682 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] OutputPage: Move hardcoded default modules to Skin::getDefaultModules

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

Change 353573 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] Move loading of mediawiki.toc from OutputPage to Skin

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

Change 353592 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.action.edit.preview: Enable 'useskin' mode for action=parse API

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

Change 354682 merged by jenkins-bot:
[mediawiki/core@master] OutputPage: Move hardcoded default modules to Skin::getDefaultModules

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

Change 349367 merged by jenkins-bot:
[mediawiki/core@master] ApiParse: Add Skin::getDefaultModules() in useskin mode

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

Change 353573 merged by jenkins-bot:
[mediawiki/core@master] Move loading of mediawiki.toc from Parser to Skin

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

Change 355273 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MobileFrontend@master] Use ParserOutput::setTOCEnabled instead of OutputPage::enableTOC

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

Just noticed the mobile site is sending fatal exceptions. Can someone please confirm that https://gerrit.wikimedia.org/r/353573 didn't hit the train?

Change 355273 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Use ParserOutput::setTOCEnabled instead of OutputPage::enableTOC

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

Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Chattertocks

mw.loader.getState('mediawiki.toc')
"registered"

https://en.wikipedia.beta.wmflabs.org/wiki/Chattertocks

mw.loader.getState('mediawiki.toc')
"ready"

Provided the train didn't get cut to include https://gerrit.wikimedia.org/r/355273 but not https://gerrit.wikimedia.org/r/355273
I think we are good to resolve this now (thanks @Krinkle)

In T130632#3287942, @Jdlrobson wrote:

Just noticed the mobile site is sending fatal exceptions. Can someone please confirm that https://gerrit.wikimedia.org/r/353573 didn't hit the train?

You can confirm this via the Gerrit UI by navigating to the change and clicking "Included in" in the top right-hand corner. You can see that rMWc7e00974c7eb: Move loading of mediawiki.toc from Parser to Skin wasn't included in the -wmf.2 branch:

You can also confirm it via the CLI by running git fetch origin; git log origin/wmf/1.30.0-wmf.2.

In T130632#3287960, @Jdlrobson wrote:

Provided the train didn't get cut to include https://gerrit.wikimedia.org/r/355273 but not https://gerrit.wikimedia.org/r/355273

Neither change is on the -wmf.2 branch.

Mass-moving all items tagged for MediaWiki 1.30.0-wmf.3, as that was never released; instead, we're using -wmf.4.