Page MenuHomePhabricator

VE overwrites "lead section edit links" gadget when in SET mode
Closed, ResolvedPublicBUG REPORT

Description

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

What happens?:
Edittop vanished.

What should have happened instead?:
Edittop still there.

The visual editor can do the same thing if it's the only editor. The problem doesn't seem to trigger when the editing mode is set to "show me both editor tabs" and both editors are available.

Edit: for the sake of completeness (as the task description/title was updated): I originally discovered this issue while testing my own script, Factotum. I figured any script/gadget that resides in #firstHeading would be affected and testing edittop proved me right. As edittop is better known I used that for the example. Kudos for the quick patch btw.

Edit 2: the updated reproduction steps could (sort of) fail: if you haven't enabled the 2017 wikitext editor and your last editor was the 2010 source editor, you'd only have an "edit source" (not "edit") link which doesn't load VE and thus wouldn't trigger this issue.

Event Timeline

This is caused by this code: https://github.com/wikimedia/mediawiki-extensions-VisualEditor/blob/fd71fbe6050ebc6d13c3eb797ca93f9e797a0450/modules/ve-mw/init/targets/ve.init.mw.ArticleTarget.js#L332-L336

It's intended to update the heading when switching from old wikitext to visual mode (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/265815), but the condition incorrectly also matches when using the "remember my last editor" mode.

This probably needs to be moved somewhere else.

Esanders renamed this task from VE breaks gadgets in #firstHeading to VE overwrites "lead section edit links" gadget when in SET mode.Sep 28 2022, 1:56 PM
Esanders updated the task description. (Show Details)
Esanders added a subscriber: Esanders.

Can we just check wgAction instead of the query string param?

Change 836245 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] ArticleTarget: Only overwrite title when not loading from view page

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

Esanders moved this task from To Triage to Triaged on the VisualEditor board.
Esanders moved this task from Incoming to Code Review on the Editing-team (Kanban Board) board.

VE/NWE will also overwrite the page title after saving an edit (to account for changes in {{DISPLAYTITLE:}}). I think this is the correct behavior on our side, and gadgets that need to rebuild their interface inside of the page title should use mw.hook( 'postEdit' ).add( function () { … } ) to check if their interface has been wiped out and add it back if needed. (Note that you need to check, because this hook is also fired in other scenarios – unfortunately there isn't a more specific hook for the page title being updated.)

I think this is the correct behavior on our side

No chance you could just update .mw-page-title-namespace,.mw-page-title-separator,.mw-page-title-main?

Change 836245 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] ArticleTarget: Only overwrite title when not loading from view page

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

I think this is the correct behavior on our side

No chance you could just update .mw-page-title-namespace,.mw-page-title-separator,.mw-page-title-main?

Not really, because if {{DISPLAYTITLE:}} is used, it replaces those extra elements.

Not really, because if {{DISPLAYTITLE:}} is used, it replaces those extra elements.

So you could ditch .mw-page-title-namespace,.mw-page-title-separator, and replace .mw-page-title-main with a <span class="mw-page-title-displaytitle">new title</span>?

Yeah, that seems like a good idea, if mw-page-title-displaytitle existed. Note that the other mw-page-title-... wrappers were only added recently (T306440 / change 821353), and there were some other ideas for DISPLAYTITLE handling proposed in that task, so you might need to argue the benefits of yours more thoroughly (and it's out of scope for this task in my opinion, I probably should not even have mentioned the thing about saving). But it seems like a good idea to me.