FlaggedRevisions incorrectly displays table of contents on Vector 2022 for flagged pages
Closed, ResolvedPublicBUG REPORT
Actions

Assigned To

Authored By

	Jdlrobson
	Sep 2 2022, 4:19 PM

Description

Steps to replicate the issue (include links if applicable):

Go to https://bn.wikipedia.org/wiki/%E0%A6%9A%E0%A6%BE%E0%A6%9F%E0%A6%97%E0%A6%BE%E0%A6%81%E0%A6%87%E0%A6%AF%E0%A6%BC%E0%A6%BE_%E0%A6%AD%E0%A6%BE%E0%A6%B7%E0%A6%BE in an incognito window
Note an unexpected and unstyled TOC in the article (see screenshot below)
We are fairly confident this is because of FlaggedRevisions

What happens?:

What should have happened instead?:

The skin preference for no table of contents in article should be honored - the table of contents should be on the right

Other information (browser name/version, screenshots, etc.):

Also happens on other FlaggedRevs wikis e.g. https://pl.wikipedia.org/wiki/Religia_S%C5%82owian?useskin=vector-2022

all help pages are flagged on plwiki, (and display incorrectly) e.g. https://pl.wikipedia.org/wiki/Pomoc:Jak_napisa%C4%87_doskona%C5%82y_artyku%C5%82?useskin=vector-2022
this article hasn't been flagged yet(it's brand new), (displays correctly)
flagged articles example, (displays incorrectly)
pages in the WP namespace aren't flagged, (displays correctly)

QA Results - Prod

AC	Status	Details
1	✅	T316947#8230962

Details

Subject	Repo	Branch	Lines +/-
Respect skin's TOC option	mediawiki/extensions/FlaggedRevs	master	+10 -1
ParserOptions: default with skin options when constructing	mediawiki/core	master	+5 -0
Respect skin's TOC option	mediawiki/extensions/FlaggedRevs	wmf/1.39.0-wmf.28	+10 -1
Respect skin's TOC option	mediawiki/extensions/FlaggedRevs	wmf/1.39.0-wmf.27	+10 -1

Customize query in gerrit

Related Objects

Mentioned In: T317333: addParserOutput should consult skin and call setSections when setting options relating to table of contents
Mentioned Here: T317333: addParserOutput should consult skin and call setSections when setting options relating to table of contents
T305123: Showing Table of contents sometimes requires reload (Vector 22, wmf.5)
T307256: ToC is not shown during Preview when using the standard WikiEditor
T311529: New Vector TOC is not shown when viewing a diff with preview
T185664: Code stewardship review: FlaggedRevs
T277883: Drop all low-use and unused features of FlaggedRevs to make it more maintainable
T307961: Suggested edits: followup changes to refactor

Event Timeline

Jdlrobson created this task.Sep 2 2022, 4:19 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 2 2022, 4:19 PM

Jdlrobson renamed this task from FlaggedRevisions interferes with table of contents handling for some pages to FlaggedRevisions interferes with table of contents handling for pages that are pending review.Sep 2 2022, 4:19 PM

My guess is something is interfering with the parser options set in https://gerrit.wikimedia.org/g/mediawiki/core/+/160ec9bfb1799538863e97c53417b49af1727f81/includes/page/Article.php#485

Change 829229 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] ParserOptions: default with skin options when constructing

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

gerritbot added a project: Patch-For-Review.Sep 2 2022, 4:34 PM

@cscott looks like FlaggedRevisions calls Article:::makeParserOptions https://github.com/wikimedia/mediawiki-extensions-FlaggedRevs/blob/1b7ff4e5efa7210288bff454f4dfda08806539ef/frontend/FlaggablePageView.php#L599
This is a guess, as I don't understand FlaggedRevisions at all, but since that calls ParserOptions::__construct, is it possible we need to update the constructor to set skin options here (see patch https://gerrit.wikimedia.org/r/829229)?

@Ladsgroup I think you know a little about this extension.. perhaps you can help us troubleshoot what might be happening?

Jdlrobson renamed this task from FlaggedRevisions interferes with table of contents handling for pages that are pending review to FlaggedRevisions parses page incorrectly for flagged pages.Sep 2 2022, 4:37 PM

Jdlrobson updated the task description. (Show Details)

Jdlrobson added a project: Web-Team-Backlog.

Jdlrobson added a subscriber: Ladsgroup.

This is causing significant visual regressions in production across a number of wikis. Marking as UBN.

@Jdlrobson: Scott added a setSuppressTOC flag to ParserOptions as part of T307961. So, FlaggedRevs should probably set that flag based on the user skin pref.

So maybe $parserOptions->setSuppressTOC(...) after this line you linked above.

There are several instances where a ParserOptions object is created in this extension and I don't know a whole lot about this extension, but it might be necessary to at least inspect that. Meanwhile, I left you a comment on your gerrit patch.

So maybe $parserOptions->setSuppressTOC(...) after this line you linked above.

This would probably suppress the table of contents from the article, but presumably it wouldn't populate the sections data needed for the table of contents in the sidebar?

Change 829229 abandoned by Jdlrobson:

[mediawiki/core@master] ParserOptions: default with skin options when constructing

Reason:

I think I might be barking up the wrong tree here :(

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

Maintenance_bot removed a project: Patch-For-Review.Sep 2 2022, 8:29 PM

In T316947#8209201, @Jdlrobson wrote:

So maybe $parserOptions->setSuppressTOC(...) after this line you linked above.

This would probably suppress the table of contents from the article, but presumably it wouldn't populate the sections data needed for the table of contents in the sidebar?

That is correct, but rereading the description, I see that is not what you want. So, never mind on the suppressTOC then.

RhinosF1 subscribed.Sep 3 2022, 3:25 PM

ovasileva edited projects, added Web-Team-Backlog (Kanbanana-2022-23-Q1); removed Web-Team-Backlog.Sep 5 2022, 3:28 PM

Jdlrobson added a subscriber: aaron.Sep 7 2022, 7:09 PM

I discovered that if you use the "&oldid=.." parameter, the problem goes away.

For example open these links in anonymous mode: plwiki page with oldid paramater vs. same plwiki revision without the oldid.

I also cannot repro this locally on my local wiki with flagged revs enabled which makes this more annoying to debug.

FlaggedRevs replaces and reimplements large chunks of the Article class in order to display non-latest revision (using the ArticleViewHeader hook). The code that disables 'injectTOC' and uses setSections() instead probably isn't even running.

Hi @aaron - I wanted to ping here as you're listed as the maintainer of the FlaggedRevisions Extension here. Would you be able to help provide additional context on this issue, and/or otherwise point us to folks who might be able to help? We've separately scheduled a pairing session to discuss this issue together in person, but any amount of insight you can provide as to the behavior we're seeing would be appreciated!

In T316947#8218635, @ssastry wrote:

I also cannot repro this locally on my local wiki with flagged revs enabled which makes this more annoying to debug.

Pro tip (gained from lots of pain): FR gives a lot of freedom in configuration which makes the extension practically a fully different extension on a different wiki. Try making sure plwiki's config listed in https://github.com/wikimedia/operations-mediawiki-config/blob/master/wmf-config/flaggedrevs.php are applied there.

HTH

I leave T185664: Code stewardship review: FlaggedRevs and T277883: Drop all low-use and unused features of FlaggedRevs to make it more maintainable here. I try to help (as volunteer) but I'm not sure I'll be very useful. Let me try.

In T316947#8218715, @matmarex wrote:

FlaggedRevs replaces and reimplements large chunks of the Article class in order to display non-latest revision (using the ArticleViewHeader hook). The code that disables 'injectTOC' and uses setSections() instead probably isn't even running.

In general, everyone's life would be much easier if the new TOC stuff just used the existing interfaces, instead of inventing new ones: check the skin to decide where to show the TOC, and do it in the addParserOutput() method. This issue is exactly the same problem as T311529, T307256, T305123, and so on. I recommend filing a follow-up task about this.

But while it's the way it is, it should be easy enough to add the special stuff to FlaggedRevs too, like we did in other places in those tasks. It probably needs to be done in these two places: https://codesearch.wmcloud.org/search/?q=addParserOutput&i=nope&files=&excludeFiles=&repos=Extension:FlaggedRevs

My guess is that something ( a constructor / static helper function ) in core is not calling Parser::setSections where it should be, but I've been struggling to pin down the exact code path FlaggedRevisions uses.

In T316947#8218635, @ssastry wrote:

I also cannot repro this locally on my local wiki with flagged revs enabled which makes this more annoying to debug.

I managed to repro this, but looks like @matmarex has a clue already.

Change 830696 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/FlaggedRevs@master] Respect skin's TOC option

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

gerritbot added a project: Patch-For-Review.Sep 7 2022, 8:26 PM

Change 830601 had a related patch set uploaded (by Jdlrobson; author: Amir Sarabadani):

[mediawiki/extensions/FlaggedRevs@wmf/1.39.0-wmf.27] Respect skin's TOC option

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

Change 830602 had a related patch set uploaded (by Jdlrobson; author: Amir Sarabadani):

[mediawiki/extensions/FlaggedRevs@wmf/1.39.0-wmf.28] Respect skin's TOC option

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

Jdlrobson moved this task from Needs Analysis to Doing on the Web-Team-Backlog (Kanbanana-2022-23-Q1) board.Sep 7 2022, 8:44 PM

Change 830696 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Respect skin's TOC option

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

matmarex renamed this task from FlaggedRevisions parses page incorrectly for flagged pages to FlaggedRevisions incorrectly displays table of contents on Vector 2022 for flagged pages.Sep 7 2022, 8:59 PM

matmarex removed matmarex as the assignee of this task.

ReleaseTaggerBot added a project: MW-1.40-notes (1.40.0-wmf.1; 2022-09-12).Sep 7 2022, 9:00 PM

Change 830601 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@wmf/1.39.0-wmf.27] Respect skin's TOC option

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

Mentioned in SAL (#wikimedia-operations) [2022-09-07T21:07:21Z] <samtar@deploy1002> Started scap: Backport for [[gerrit:830601|Respect skin's TOC option (T316947)]]

Mentioned in SAL (#wikimedia-operations) [2022-09-07T21:07:45Z] <samtar@deploy1002> samtar and jdlrobson: Backport for [[gerrit:830601|Respect skin's TOC option (T316947)]] synced to the testservers: mwdebug1002.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2001.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2022-09-07T21:14:27Z] <samtar@deploy1002> Finished scap: Backport for [[gerrit:830601|Respect skin's TOC option (T316947)]] (duration: 07m 06s)

Change 830602 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@wmf/1.39.0-wmf.28] Respect skin's TOC option

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

Mentioned in SAL (#wikimedia-operations) [2022-09-07T21:18:55Z] <samtar@deploy1002> Started scap: Backport for [[gerrit:830602|Respect skin's TOC option (T316947)]]

Mentioned in SAL (#wikimedia-operations) [2022-09-07T21:19:18Z] <samtar@deploy1002> samtar and jdlrobson: Backport for [[gerrit:830602|Respect skin's TOC option (T316947)]] synced to the testservers: mwdebug1001.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug1002.eqiad.wmnet, mwdebug2001.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2022-09-07T21:26:57Z] <samtar@deploy1002> Finished scap: Backport for [[gerrit:830602|Respect skin's TOC option (T316947)]] (duration: 08m 02s)

Maintenance_bot removed a project: Patch-For-Review.Sep 7 2022, 9:30 PM

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.28; 2022-09-05).Sep 7 2022, 10:00 PM

Backported so after QAing in production we can call this done.

Created follow up https://phabricator.wikimedia.org/T317333

Test Result - Prod

Status: ✅ PASS (tentative)
Environment: bnwiki
OS: macOS Monterey
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

Go to https://bn.wikipedia.org/wiki/%E0%A6%9A%E0%A6%BE%E0%A6%9F%E0%A6%97%E0%A6%BE%E0%A6%81%E0%A6%87%E0%A6%AF%E0%A6%BC%E0%A6%BE_%E0%A6%AD%E0%A6%BE%E0%A6%B7%E0%A6%BE in an incognito window

✅ AC1: The skin preference for no table of contents in article should be honored - the table of contents should be on the right
@ovasileva The screenshot in the description shows the TOC on the left, but the text says on the right. I suspect the text is wrong. But just in case, I'm flagging it for you. If it's the former, as seen below, it's a pass.

Screen Shot 2022-09-12 at 6.33.52 PM.png (761×1 px, 286 KB)

Edtadros updated the task description. (Show Details)Sep 13 2022, 1:38 AM

Looks good, also tested on: https://pl.wikipedia.org/wiki/Pomoc:Jak_napisa%C4%87_doskona%C5%82y_artyku%C5%82?useskin=vector-2022. Resolving. Thank you all!

	F35518485: Screen Shot 2022-09-12 at 6.33.52 PM.png
	Sep 13 2022, 1:37 AM

FlaggedRevisions incorrectly displays table of contents on Vector 2022 for flagged pagesClosed, ResolvedPublicBUG REPORTActions