Page MenuHomePhabricator

VisualEditor hangs after saving page in custom skin due to missing subtitle element
Closed, ResolvedPublicBUG REPORT

Description

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

  1. Install a custom skin that does not include a mw-content-subtitle element.
  2. Open any page in VisualEditor.
  3. Edit the page and save changes.

What happens?
After saving, VisualEditor “hangs” and the edit dialog is not properly cleaned up. The browser console did not show any errors

What should have happened instead?
VisualEditor should successfully close the editing dialog and update the page, even if the custom skin does not include a subtitle element. The javascript logic shouldn't stop working.

Software version:
MediaWiki 1.43, VisualEditor latest for this version, custom skin applied

Other information:
I'm sure the issue is inside

VisualEditor/modules/ve-mw/init/targets/ve.init.mw.ArticleTarget.js

Specifically, the following code inside the function replacePageContent:

mw.util.clearSubtitle();
mw.util.addSubtitle(contentSub);

The util function throws an Error when mw-content-subtitle does not exist in the DOM, which stops further cleanup of the VE dialog.

Notes: I could not find anywhere in MediaWiki or VisualEditor documentation that mw-content-subtitle is a required element for custom skins.

Suggested fix:

  • In ve.init.mw.ArticleTarget.js, wrap the addSubtitle call in a try/catch or check for the existence of the element before attempting to update the subtitle.
  • This might also be considered for MediaWiki core if mw.util.addSubtitle is expected to work without crashing the Javascript
  • clarify in the documentation that custom skins must include mw-content-subtitle.

Event Timeline

i'm not sure which tags are correct for this task. so far i've only applied VisualEditor.

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

[mediawiki/extensions/VisualEditor@master] mw.ArticleTarget: don't crash if the skin doesn't have a subtitle

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

DLynch subscribed.

I'm just going to catch that error -- we don't really depend on this existing, we're just passing on whatever the API has given us, so if the skin doesn't support subtitles we might as well just throw it away and continue.

DiscussionTools does the same.

I think we should fix the upstream method to return false instead of throwing - I think that's a bit extreme.

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

[mediawiki/core@master] mediawiki.util: Don't throw in addSubtitle if the skin lacks a subtitle

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

Change #1239531 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.util: Don't throw in addSubtitle if the skin lacks a subtitle

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

Change #1239399 abandoned by DLynch:

[mediawiki/extensions/VisualEditor@master] mw.ArticleTarget: don't crash if the skin doesn't have a subtitle

Reason:

I1d52b8504d45b88dba59206ff2347ef44c84a2de instead

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

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

[mediawiki/core@REL1_43] mediawiki.util: Don't throw in addSubtitle if the skin lacks a subtitle

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

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

[mediawiki/core@REL1_44] mediawiki.util: Don't throw in addSubtitle if the skin lacks a subtitle

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

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

[mediawiki/core@REL1_45] mediawiki.util: Don't throw in addSubtitle if the skin lacks a subtitle

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

Change #1240013 merged by jenkins-bot:

[mediawiki/core@REL1_43] mediawiki.util: Don't throw in addSubtitle if the skin lacks a subtitle

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

Change #1240015 merged by jenkins-bot:

[mediawiki/core@REL1_45] mediawiki.util: Don't throw in addSubtitle if the skin lacks a subtitle

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

Change #1240014 merged by jenkins-bot:

[mediawiki/core@REL1_44] mediawiki.util: Don't throw in addSubtitle if the skin lacks a subtitle

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