Page MenuHomePhabricator

mediawiki.toc module unnecessarily shipped in mobile views
Closed, ResolvedPublic8 Story Points

Description

Despite the table of contents being disabled on page views in mobile, the ResourceLoader module is still shipped. This is stored in ParserOutput but probably shouldn't be.

In desktop Minerva this also causes issues as it overrides the Minerva table of contents behaviour.

Expected:

  • The module mediawiki.toc (1.2kb) shouldn't be appearing in any page view on mobile.
  • The module mediawiki.toc (1.2kb) shouldn't be appearing in any page view on desktop Minerva.
NOTE: Announcement about new ApiParse option and removal of OutputPage::enableTOC: https://lists.wikimedia.org/pipermail/wikitech-l/2017-June/088301.html

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 275901 had a related patch set uploaded (by Jdlrobson):
Don't add mediawiki.toc to pages with toc stripped

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

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?

phuedx added a comment.Apr 8 2016, 9:05 AM

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.

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.

Jdlrobson updated the task description. (Show Details)Apr 12 2016, 8:02 PM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 19 2016, 9:59 PM
Krinkle added a comment.EditedJul 4 2016, 10:28 PM

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.

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.

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.

Krinkle moved this task from Inbox to Blocked on the Performance-Team board.
Krinkle moved this task from Blocked to Radar on the Performance-Team board.

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

Jdlrobson set the point value for this task to 8.Mar 7 2017, 7:36 PM

Preliminary estimation based on previous experiences here.

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

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.

Jdlrobson lowered the priority of this task from High to Normal.Apr 20 2017, 11:07 PM
Jdlrobson added a project: patch-welcome.

Priority is not high enough compared to other tasks. If someone can get to this before us please do.

Jdlrobson removed Jdlrobson as the assignee of this task.Apr 20 2017, 11:16 PM

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

Krinkle claimed this task.EditedApr 21 2017, 12:27 AM

Commit https://gerrit.wikimedia.org/r/349367 should resolve this issue.

Pcoombe removed a subscriber: Pcoombe.Apr 21 2017, 9:26 AM
ovasileva updated the task description. (Show Details)May 9 2017, 3:22 PM

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)

Jdlrobson updated the task description. (Show Details)

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.


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.

Jdlrobson closed this task as Resolved.May 24 2017, 1:04 PM

Thanks for checking @phuedx

Jdforrester-WMF added a subscriber: Jdforrester-WMF.

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

Krinkle updated the task description. (Show Details)Jun 12 2017, 5:14 PM