Page MenuHomePhabricator

Run OutputPageBeforeHTML hook in VisualEditor API when returning page HTML after editing a page
Closed, ResolvedPublic

Description

We should run the OutputPageBeforeHTML hook in VisualEditor API when returning page HTML after editing a page.

Two scenarios came up where that would be helpful:

  • MobileFrontend uses the OutputPageBeforeHTML hook to apply mobile-specific transformations to the page (e.g. lead paragraph, collapsible sections). Because of this, VisualEditor currently has to reload the whole page after publishing an edit, instead of using the HTML returned by its API. This makes publishing slower. (T219420)
  • DiscussionTools would like to use the OutputPageBeforeHTML hook to add the "Reply" links to comments on the page. Currently that is implemented in JavaScript, resulting in a small delay after the page loads but before the "Reply" links appear. However, that would cause those links to not appear after a discussion page is edited using VE or NWE. (T252555)

Event Timeline

matmarex created this task.Jul 27 2020, 9:39 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 27 2020, 9:39 PM
matmarex assigned this task to Esanders.Jul 27 2020, 9:40 PM

(Ed has been working on this)

Change 610025 had a related patch set uploaded (by Bartosz Dziewoński; owner: Esanders):
[mediawiki/extensions/VisualEditor@master] Edit API: Run OutputPageBeforeHTML hook on output

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

This seems a bit scary, because this hook is normally ran inside OutputPage, and OutputPage does a bajillion different things, and other extensions using this hook can depend on any of them.

But I set out to investigate and it's not as scary as it seems:


Here are all the extensions enabled on Wikimedia wikis that use this hook:

  • DoubleWiki: Used on Wikisource, e.g. https://en.wikisource.org/wiki/A_Study_in_Scarlet/Part_1/Chapter_1?match=fr. It only kicks in when the 'match' parameter is used, so it shouldn't be affected.
  • LiquidThreads: Still used on some wikis, e.g. https://hu.wikipedia.org/wiki/Szerkesztő:Matma_Rex/lqt. Looks like this patch would actually fix a bug where after editing a page containing a LQT board with VE/NWE, the board would not appear – except that you can't launch VE/NWE on them because LQT suppresses the normal edit links, and when launching via action=edit, VE will reload the whole page anyway. Overall it shouldn't be affected.
  • MobileFrontend: Only uses the hook when mobile mode is active, and mobile mode VE reloads the whole page after saving. It shouldn't be affected until we make some changes for T219420.
  • ShortUrl: Used on some non-Latin-script wikis, e.g. https://hi.wikipedia.org/wiki/सदस्य_वार्ता:Matma_Rex. The hook doesn't even do anything with the HTML, it just adds a RL module to the page, it shouldn't be affected. (As a side note, there's a different bug in its interaction with VisualEditor: after a page is edited, the link icon added by this extension next to the page title disappears.)
  • Wikibase: Their code should only do stuff on entity pages, which can't be edited with VE anyway.

Overall, it seems that nothing terrible should happen…

There are ~15 other extensions not used on Wikimedia wikis that appear to use this hook, I didn't review their code. I'm hopeful that they also don't do anything ridiculous, and either won't be affected, or this will actually fix issues like in LiquidThreads.

Change 610025 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Edit API: Run OutputPageBeforeHTML hook on output

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

I don't know what you would even test here, other than confirming that nothing is affected. But we should watch for any bug reports related to these extensions.

matmarex moved this task from To Triage to Triaged on the VisualEditor board.Jul 27 2020, 10:27 PM
ppelberg added a subscriber: ppelberg.

I don't know what you would even test here, other than confirming that nothing is affected. But we should watch for any bug reports related to these extensions.

Per a conversation with @Esanders yesterday, it sounds like the risk of the above will go up once T252555 is merged; I've indicated as much on that ticket: T252555#6345275.

ppelberg closed this task as Resolved.Jul 29 2020, 3:50 PM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptJul 29 2020, 3:50 PM