Page MenuHomePhabricator

Discussion tools source mode should display navigation popups when enabled
Closed, ResolvedPublic

Description

Demonstration

  1. Enable the navigation popups gadget
  2. Load any talk page and open discussions tools (e.g. by replying to a post)
  3. In source mode, type any sequence of characters that includes an internal link, e.g. [[Brazil]]
  4. .
    1. Select the link in the editing window and mouse over the selection
    2. Mouse over the link in the preview

Expected results

  • (for both A and B) the navigation popup preview of the Brazil article opens

Actual results

  • (case A) nothing happens
  • (case B) a tootip showing the name of the linked page is displayed

Event Timeline

Thryduulf renamed this task from Discussion tools source more should display navigation popups when enabled to Discussion tools source mode should display navigation popups when enabled.Apr 8 2021, 5:14 PM

For case A, I don't think we can do much, Popups only handles that in the old wikitext editor.

For case B, we might be able to fix this by using the 'wikipage.content' hook. Popups has some code that handles it, although I haven't tested it. This would also fix some other JavaScript enhancements, e.g. collapsible content (somewhat unlikely to appear in talk page comments, but still).

[for reference: https://en.wikipedia.org/wiki/MediaWiki:Gadget-popups.js]

Change 678031 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/DiscussionTools@master] Fire the 'wikipage.content' hook on previews

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

Firing wikipage.content may however cause new issues as well. As far as I know, currently wikipage.content is always fired with the one and only content block on the page (although the documentation doesn’t state this). If it’s fired with the preview, this expectation is broken and this may cause undefined behavior regarding the rest of the page (as scripts may consider it to be removed from the DOM tree).

Change 678031 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Fire the 'wikipage.content' hook on previews

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

Firing wikipage.content may however cause new issues as well. As far as I know, currently wikipage.content is always fired with the one and only content block on the page (although the documentation doesn’t state this). If it’s fired with the preview, this expectation is broken and this may cause undefined behavior regarding the rest of the page (as scripts may consider it to be removed from the DOM tree).

It's a corner case for sure, but it is intended to be used this way. I remember that causing some issues in the past, but AFAIK the issues were fixed. (I couldn't really find an example… I only found T137935 which seems to have been a theoretical problem noticed in code review.)

We have existing places where we fire the hook on a small piece of content, and I'm not aware of them misbehaving, for example:

  • When loading a preview of a different page of a paged file (resources/src/mediawiki.page.image.pagination.js)
  • When displaying dynamic warnings on Special:Upload (resources/src/mediawiki.special.upload/upload.js)
  • When VisualEditor shows the edit notices popup (modules/ve-mw/ui/tools/ve.ui.MWPopupTool.js)

(more: https://codesearch.wmcloud.org/search/?q=mw.hook%5C(%20%27wikipage.content%27%20%5C).fire&i=nope&files=&excludeFiles=&repos=)

OK, I hope so. (Except for Kartographer, which is already broken for a long time, I was just too lazy to report it before: T280363).

I tested on https://en.wikipedia.beta.wmflabs.org/wiki/Talk:Cats and navigation popups seem to work now on reply tool previews.

(I had to update the gadget first since it wasn't working on Beta at all: https://en.wikipedia.beta.wmflabs.org/w/index.php?title=MediaWiki:Gadget-popups.js&diff=prev&oldid=487877.)

ppelberg claimed this task.

Firing wikipage.content may however cause new issues as well. As far as I know, currently wikipage.content is always fired with the one and only content block on the page (although the documentation doesn’t state this). If it’s fired with the preview, this expectation is broken and this may cause undefined behavior regarding the rest of the page (as scripts may consider it to be removed from the DOM tree).

I'm seeing this problem with the HotCat tool on enwiki (https://en.wikipedia.org/wiki/Wikipedia_talk:HotCat#Doubled_HotCat_insertions_with_Reply_Tool). HotCat is a widely used tool; breaking it would be a Bad Thing.

Thanks for the note, I'll reply there.