Page MenuHomePhabricator

ContentTranslation must provide an element #mw-content-text
Closed, ResolvedPublic

Description

In mediawiki.page.ready a hook is fired wikipage.content with #mw-content-text as the only argument.
In ContentTranslation this element doesn't exist https://ca.wikipedia.org/wiki/Special:ContentTranslation?title=Special:ContentTranslation&campaign=contributionsmenu&to=ca&page=Ash+%28artist%29&from=en&targettitle=Ash+%28artist%29#suggestions
As a result this breaks any extensions or gadgets relying on this hook.

https://gerrit.wikimedia.org/g/mediawiki/core/+/2d0cf09cdfb8f35e45e8f3ec461428f868ff3180/resources/src/mediawiki.page.startup.js#39

The element should be retained in the output page.

If it's not meant to be in the output page ContentTranslation should create its own skin which doesn't load mediawiki.page.ready

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 618240 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Do not trigger wikipage.content hook when #mw-content-text is not present

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

@santosh @Nikerabbit - looking at ContentTranslation I have to say I think this might work better as a dedicated skin. I was also surprised to see it executes the entirety of Vector despite having no need for most of it. Was this an intentional decision or was this an attempt to workaround some of the limitations of the system?

Krinkle removed a project: Performance-Team.
Krinkle subscribed.

Overall sounds good to me. I do note however that the element missing in CX might in itself also be a bug, which, if fixed, means it will fire again there, but with a proper element rather than an empty collection.

Jdlrobson renamed this task from mediawiki.page.ready fires wikipage.content event even when #mw-content-text does not exist to ContentTranslation must provide an element #mw-content-text.Aug 11 2020, 10:30 PM
Jdlrobson triaged this task as High priority.
Jdlrobson updated the task description. (Show Details)

After talking to @Krinkle our conclusion here is that ContentTranslation is serving incompatible HTML by removing this element.

@Nikerabbit when you have the time could you please provide some input here on whether the removal of this element was intentional and whether adding this element back would be problematic?

Change 618240 abandoned by Ammarpad:
[mediawiki/core@master] mediawiki.page.ready: Only fire wikipage.content if #mw-content-text exists

Reason:

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

Ammarpad subscribed.

I see that mw.hook( 'wikipage.content' ).fire( $( '#mw-content-text' ) ); for Use this hook if you need to enhance or modify parts of a wiki page.. But for CX, the UI has nothing common with any Special page or content page by product design. We reimplemented the personal toolbar too. There is no side bar. Since CX was launched in 2014, we have seen some reports that says it broke some gadgets and we talked to the authors and fixed(most of them were using document.getElementById and assuming the existance of sidebar). That means, so far we were in the assumption that there is no such contracts to have an element with id mw-content-text.

To clarify, the removal of page chrome and taking up the whole space for Content translation pages were intentional product design decision from 2014 to have maximum horizontal space to show articles in two languages side by side.

Even if we add a dummy element with #mw-content-text, the gadgets or any customization code will be useless. But it may preventing js errors.# mw-content-text is under body > div#content > div#bodyContent. Should we have this hierarchy as well as dummy? If some gadget render something to this node, won't it break CX page layout? I think the fundamental problem is about the contract or assumption about page structure. Either CX should assume that such contract exists and have dummy nodes just to prevent js errors or mediawiki.page.ready should checks before using such nodes. Am I right?

I was also surprised to see it executes the entirety of Vector despite having no need for most of it.

The SpecialPage implementation for CX(code) is a skin override. @Nikerabbit was primary author for this, but if we see that certain things can be optimized, suggestions are welcome. From the skin, presonal tools information is extracted and used for rendering our custom top bar. But this implementation mostly cared about avoiding page elements that are not relevant for CX and keeping everything else to avoid any issues with skin system.

(Niklas is in vacation)

It seems like content translation is a single page application requiring access to ResourceLoader modules, not a special page with access to the mediawiki UI e.g. personal tools, sidebar. Is that a fair assessment? If so, it seems you have more in common with the portal static homepage [1] than a special page.

The current approach is coming with a lot of overhead - no caching of HTML and a lot of additional JavaScript and CSS being unnecessarily loaded (performance problems) that's useless including as we've discovered with this bug, loading and executing various incompatible gadgets. The page is loading 80kb of render blocking CSS right now and only needs 540bytes! Much of this includes OOUI styles and Echo.

With the current approach there are some minimal implied expectations one of which is a #mw-content-text element. Here are some short term solutions to that:

  • leaving an empty element in the HTML with that ID and using CSS to hide it as a short term solution.
  • disabling all gadgets on the page using safe mode using OutputPage::disallowUserJs and review also extension usages of the hook 'wikipage.content' to make sure they are not running on ContentTranslation. [2]
  • If @Krinkle can be convinced, a core change to not run this hook on special pages per your argument. [3]

On the longer-term there are 2 potential options:

  1. with some refactors to $out->headElement( $skin ) and/or in the skin itself we should be able to remove the need for this element and disable a lot of the code that runs on this page by making use of a dedicated skin with minimal ResourceLoader code which would mimic the code you have in [4].
  2. use a static HTML page

Some of the benefits would potentially be:

  • Nicer URLs for browsing (no special page)
  • Small amount of critical path CSS hopefully leading to more users and better conversion.
  • Less potential for breakage - right now styles for the new Version of Vector's have potential to override content translation as well as possible gadget breakage. I suspect T260842 might relate to one of these kind of issues.

[1] https://www.wikipedia.org/
[2] https://codesearch.wmcloud.org/extensions/?q=%27wikipage.content%27&i=nope&files=&repos=
[3] https://gerrit.wikimedia.org/g/mediawiki/core/+/2cc3d4f0ba759ef6fc36d3e3ea71df5c0588dd2f/resources/src/mediawiki.page.ready/ready.js#71
[4] https://github.com/wikimedia/mediawiki-extensions-ContentTranslation/blob/0e2a36e7c6798930214d613c22a3dab387fe8ef5/specials/ContentTranslationSpecialPage.php#L101

Nikerabbit claimed this task.

I believe that this is no longer an issue, since we now use a custom skin. Please re-open and elaborate if you disagree.

It remains an issue as long as we emit wikipage.content events on CX with an unusable object that leads code to produce errors.

Change 766853 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.page.ready: Skip 'wikipage.content' when content is absent

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

Change 766853 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.page.ready: Skip 'wikipage.content' when content is absent

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

Note ContentTranslation does have the mw-content-text ID now (since it calls Skin::wrapHTML) but I think it's always best to be defensive with these triggers since skins could be overriding the default or might be removing/altering it with JS.