Page MenuHomePhabricator

When using Parsoid rendering, DiscussionTools "empty state" appears on all talk pages
Closed, ResolvedPublic

Description

As reported here: https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#c-Vestrian24Bio-20240726114800-Start_a_discussion_notice_on_Talk_pages

When using Parsoid rendering, DiscussionTools "empty state" (the "Start a discussion about…" message, or "Start a discussion with" or "Welcome to your talk page" on user talk pages) appears on all talk pages, even those that already have comments. For example: https://en.wikipedia.org/w/index.php?title=Talk:KSKR-FM&useparsoid=1

image.png (1,920×1,080 px, 212 KB)

The code was recently changed in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/1042365. I don't immediately see how that change could cause this bug though, and I can't reproduce the problem locally.

Event Timeline

... maybe an unintended feature? Could replace the various gadgets that do similar at the bottom of the page. Just would need to update the messaging a little.

Change #1060910 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] [WIP] Clear DT data from parsing extension fragment

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

and I can't reproduce the problem locally.

Add an extension tag to the talk page that parsoid doesn't have a native implementation for and calls out to the legacy parser to parse, like templatestyles.

The patch in T371125#10052522 fixes the issue but maybe there's a better location for that code to live.

This may be silly, but… would it be better if DT was setting isNotEmptyTalkPage rather that isEmptyTalkPage?

Change #1062760 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/extensions/DiscussionTools@master] Invert logic on empty talk page

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

Change #1060910 abandoned by Arlolra:

[mediawiki/core@master] [WIP] Clear DT data from parsing extension fragment

Reason:

In favour of I5c1877f7f9eb73f88a33e001ca3c2f3d06bb90e4

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

This may be silly, but… would it be better if DT was setting isNotEmptyTalkPage rather that isEmptyTalkPage?

Doesn't seem silly. What's considered a talk page is contained in HookUtils::shouldDisplayEmptyState so the inversion is probably ok. I put up a patch in T371125#10065549

Change #1062760 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Invert logic on empty talk page

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

I've noticed this happening, but - curiously - the 'empty state' message is only displaying on some talk pages with comments. To give a few examples, the notice appears for me at the bottom of Talk:Controversial Reddit communities, Talk:Eucalyptus leprophloia, Talk:Raf Simons & Talk:Broad Recognition; but not at the bottom of Talk:Florida Studio Theatre, Talk:Attica Bank, Talk:Furlana & Talk:Femi Oluwole
AFAIK, I do not have Parsoid enabled - my preference is set to 'according to wiki configuration'. I have also reproduced these results logged-out.

The invert patch seems to have caused this to start happening in more places (e.g. T373100). I say "caused" because I can reproduce it locally, and reverting the patch fixes it there.

Reproducible case involves setting the MediaWiki:Talkpageheader message. Wikis with it set get the empty-state regardless.

Change #1064783 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/extensions/DiscussionTools@master] Revert "Invert logic on empty talk page"

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

matmarex removed a project: Patch-For-Review.

Revert merged.

Maybe this could be resolved in the same way as T372592 somehow.

It does seem to be a similar case of "when we run the parser multiple times in the same page, things get confused".

Change #1074544 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/extensions/DiscussionTools@master] Always set a value for isEmptyTalkPage when transforming html

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

I just realized that I can no longer reproduce this problem, locally or on the beta cluster. The patch should be harmless anyway, but I'm confused why it seems to no longer be necessary.

Change #1074544 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Always set a value for isEmptyTalkPage when transforming html

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

I just realized that I can no longer reproduce this problem, locally or on the beta cluster. The patch should be harmless anyway, but I'm confused why it seems to no longer be necessary.

You added a <templatestyles src="Test.css" /> on a talk page with threads and didn't see the empty state from Parsoid? I just tried it now locally with up-to-date repos and it still seemed reproducible.

ABreault-WMF triaged this task as Medium priority.
ABreault-WMF moved this task from In Progress to To Deploy on the Content-Transform-Team-WIP board.
ABreault-WMF added a subscriber: cscott.

Oh, I had missed and/or forgotten your earlier comment that pinned this on <templatestyles> and similar tags. Thanks for clarifying, I can reproduce the bug and the fix now.

Though worth noting that the thing which caused us to roll back the original refactor wasn't the templatestyles issue -- it was MediaWiki:Talkpageheader, so it's worth testing that as well.

Though worth noting that the thing which caused us to roll back the original refactor wasn't the templatestyles issue -- it was MediaWiki:Talkpageheader, so it's worth testing that as well.

The commit message in T371125#10168941 addresses that and, yes, I did test it locally.