The skin option of toc being false sets injectTOC to false in ParserOutput. The documentation there says it is the responsibility of the skin to replace <mw:tocplace> if injectTOC is false.
In DiscussionTools the presence of this tag is breaking a :first-child CSS selector in our topic containers feature.
The documentation there says it is the responsibility of the skin to replace <mw:tocplace> if injectTOC is false.
I think this advice is a little misleading. Skins register a skin option "toc" either inside skin.json or their constructor, but my expectation is the parser should be removing that placeholder informed by that configuration here: https://gerrit.wikimedia.org/g/mediawiki/core/+/5a7c4c5d1ae34622b8034091acd108fa1e56b376/includes/parser/ParserOutput.php#512
I haven't thought too much about the advantages of the mw:tocplace placeholder being left in the article but I'd push back at expecting skins to have to call Parser::replaceTableOfContentsMarker explicitly.
I think the issue here is that the parser else statement doesn't call the method: https://gerrit.wikimedia.org/g/mediawiki/core/+/5a7c4c5d1ae34622b8034091acd108fa1e56b376/includes/parser/ParserOutput.php#501
@Jdlrobson it is explicitly documented as having this behavior, though. I'd assume that it's to support skins that want to put something else in place of the TOC? I don't know if it's actually used anywhere, though. But presumably we'd want to go through a deprecation process if we wanted to change it in case anyone actually is depending on the behavior, since if they are we'd be entirely breaking a skin by changing this.
(Or at least do a search through publicly available skins to check whether any exist which actually use this behavior.)
Change 809214 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):
[mediawiki/skins/Vector@master] SkinVector22: Remove the TOC marker if the TOC is moved to the sidebar
The behaviour and this documentation was added by me and that's not what I meant by that. What it's meant to say is the skin gets to decide whether the table of contents is in the article body or outside the article body which is does by an overridable skin option called toc.
I don't know if it's actually used anywhere, though. But presumably we'd want to go through a deprecation process if we wanted to change it in case anyone actually is depending on the behavior, since if they are we'd be entirely breaking a skin by changing this.
We added this in 1.39 which hasn't been released so I think it's okay for us to refine this guidance/change behaviour without going through the deprecation process.
Ah, didn't realize it was so recent. I do think that the current behavior of injectTOC is exactly what you documented it as, though -- I honestly can't see any other reading.
Okay after a bit more digging I found out that this is used in instrumentation to determine when the table of contents has been scrolled to:
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/skins/Vector/+/0ab849a4d369ce784a1555bf6f5b8f7ef6cf28cc/resources/skins.vector.es6/main.js#17
However, we definitely didn't do this with this is mind. I will need to check in with Olga about whether we still need that instrumentation before removing it.
The skin option is being passed by to the parser as injectTOC:
https://github.com/wikimedia/mediawiki/blob/master/includes/page/Article.php
I think it would be cleanest to leave the documentation as is and remove the placeholder inside Article.php based on a skin option. tocPlaceholder: true perhaps or unconditionally if it's problematic.
If the instrumentation is still important or we need to keep it a little longer are there other options we could explore to avoid the bug you are seeing in discussion tools?
My intention was for the TOC marker to eventually be a <meta> tag (this is what Parsoid emits) and for it to be an invisible marker of the author's original intent for the location of the TOC. In theory some skin could use that marker to allow the TOC to be "docked" or for it to float or for a placeholder icon to be left or...
The patch in https://gerrit.wikimedia.org/r/809214 uses the Parser::replaceTableOfContentsMarker() method to replace the marker with the empty string for Vector22. That was the original intent of how Parser::replaceTableOfContentsMarker() would be used and why it's public. If we expect the TOC marker to be removed from the output by default on all code paths, then Parser::replaceTableOfContentsMarker() isn't needed and should be private (or at least @unstable or @internal or @experimental etc).
If we want to use the location for instrumentation, etc, then replaceTableOfContentsMarker() could be used to replace the marker with a comment node <!-- toc --> which then wouldn't match :first-child in the CSS...
If we want to use the location for instrumentation, etc, then replaceTableOfContentsMarker() could be used to replace the marker with a comment node <!-- toc --> which then wouldn't match :first-child in the CSS...
For instrumentation, the element has to be visible to the user as we use the IntersectionObserver on it.
Instrumentation aside, I think it would be fine to remove this in https://github.com/wikimedia/mediawiki/blob/master/includes/page/Article.php for all skins where "toc" is defined as false in the constructor. e.g. the Vector patch at https://gerrit.wikimedia.org/r/809214 but done inside MediaWiki core (I'd rather not have skin-specific behaviour here).
@Esanders what's the urgency from the talk page side? Can this wait until web team has wrapped up the instrumentation of the table of contents, or do we need to find a path forward ASAP?
This is also useful from a gadget perspective, as it allows community members to revert to the old table of contents so I'd like to keep it if possible.
Let's try to get https://gerrit.wikimedia.org/r/c/mediawiki/core/+/733032 rolled out to resolve this.
Change 748890 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):
[mediawiki/core@master] parser: Prepare to use a <meta> tag for the internal TOC_PLACEHOLDER
Change 733032 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):
[mediawiki/core@master] parser: Use a <meta> tag for the internal TOC_PLACEHOLDER
Change 748890 merged by jenkins-bot:
[mediawiki/core@master] parser: Prepare to use a <meta> tag for the internal TOC_PLACEHOLDER
Is the goal here to use a meta tag instead of mw:tocplace but in the same place?
I ask because Vector would need to be updated (https://gerrit.wikimedia.org/g/mediawiki/skins/Vector/+/6cd7098d05e3f2284684acb0801c5366b102d4ad/resources/skins.vector.es6/main.js#18) - it needs to know the exact place the table of contents would have been for certain instrumentation that we are still using
We're also recommending editors use it to restore the old TOC:
https://www.mediawiki.org/wiki/Reading/Web/Desktop_Improvements/Features/Table_of_contents#How_can_I_get_the_old_table_of_contents?
@Esanders how important is this to the editing team to fix? Have you been able to workaround it?
Change 823520 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):
[mediawiki/skins/Vector@master] TOC: Prepare for upstream change to mw:tocplace element
Change 823520 merged by jenkins-bot:
[mediawiki/skins/Vector@master] TOC: Prepare for upstream change to mw:tocplace element
Change 824524 had a related patch set uploaded (by Esanders; author: Esanders):
[mediawiki/extensions/DiscussionTools@master] Remove mw:tocplace CSS hack
I think remaining work is with parser team so untagging. Let me know if that's not true.
Patch is in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/733032, it was on hold so that it could be merged after I returned from vacation. (I'm back!)
The patch above is likely to get merged today. Please ping web team when that's happened so we can check everything is working our side with respect to gadgets.
Change 733032 merged by jenkins-bot:
[mediawiki/core@master] parser: Use a <meta> tag for the internal TOC_PLACEHOLDER
Change 824524 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Update mw:tocplace CSS hack
Change 832530 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):
[mediawiki/core@master] Remove transitional TOC_START/TOC_END support
Change 809214 abandoned by Jdlrobson:
[mediawiki/skins/Vector@master] SkinVector22: Remove the TOC marker if the TOC is moved to the sidebar
Reason:
Change 832530 abandoned by C. Scott Ananian:
[mediawiki/core@master] Remove transitional TOC_START/TOC_END support
Reason:
Replaced by I3254d0acba31e107b50767797a2b0ad28aba59ee