Page MenuHomePhabricator

The entire reference list is deleted when attempting to replace citation with a reused reference
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue
  • In VisualEditor, when I click on an item in the reference list I get a dialog with a button at the bottom left corner "replace citation"
  • I click the button "replace citation"
  • The entire reference list is replaced by the three dots and the "replace citation" dialog is shown.
  • I click on the "re-use" tab and select a reference from the list
  • the entire list is replaced by the reference I just reused.
  • When I switch to Wikitext all contents that were originally defined in the reference list get replaced with that one reference I selected.
What happens?

The contents of the reference list are replaced by the selected reference. As an editor I lose the entire reference list (I can obviously get it back with Ctrl+z).

What should have happened instead?
  • I was not expecting to see the three dots appear in place of the reference list.
  • I was not expecting to find the standard citoid dialog with the reuse tab, when I am editing a reference from the reference list.
    • I am questioning why this dialog appears in the first place? Why is it that I get to reuse a reference while I am in the reference list? Is there a specific use case in mind?
  • Even if the standard citoid dialog should be served to the user, then the reference list should not be replaced.
Screencast

Implemented workaround

We disable the convert button when editing refs from the reflist.

Next steps

T413760: Allow MWCitationDialog and MWReferenceDialog to work on refs that are not selected

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Not a regression per-se; I did some quick checking out of old revisions and it looks like this has been doing this ever since we launched the edit-from-reflist feature in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cite/+/903311. Clicking the "replace" button is presumably just uncommon enough that we never noticed.

Curiously, the behavior differs depending on the new citation type you choose while replacing – when using the "Automatic" or "Re-use" tab, it deletes the reference list (replacing it with the new citation), while when using the "Manual" tab, it inserts the new citation after the reference list. Both are incorrect of course, I'm just making a note for whoever decides to look into the problem.

Change #1204577 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Citoid@master] Fix "replace" behavior deleting unrelated nodes

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

Change #1205141 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Citoid@master] Change CitoidAction to accept arguments as object

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

Change #1205142 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] Update CitoidAction call to use new signature

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

Change #1205143 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Cite@master] Update CitoidAction call to use new signature

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

Change #1205159 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Citoid@master] Forward ref node to citoid action when editing ref list

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

Change #1204577 merged by jenkins-bot:

[mediawiki/extensions/Citoid@master] Fix "replace" behavior deleting unrelated nodes

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

Curiously, the behavior differs depending on the new citation type you choose while replacing – when using the "Automatic" or "Re-use" tab, it deletes the reference list (replacing it with the new citation), while when using the "Manual" tab, it inserts the new citation after the reference list.

After Thiemo's first patch that I just merged, all tabs' workflows insert a new citation after the reference list, instead of replacing the reference list.

After Thiemo's first patch that I just merged, all tabs' workflows insert a new citation after the reference list, instead of replacing the reference list.

It's a first step and definitely an improvement! We will want to explore further if we can make the behavior of the replace citation button as intended - replace the currently selected reference and its uses in the article. If we are unable to do so or run into complex edge cases, we can look into the usage of the button and whether it's even needed in the reference list. The solution might just be to hid/remove that button when users interact with references in the reference list.

https://gerrit.wikimedia.org/g/mediawiki/extensions/Citoid/+/81fcd35020e5f2f69a400990e1f6fd3db9aedfad/modules/ve/ve.ui.CitoidInspector.js#549

this.replaceRefNode = data.replace && this.getSelectedNode();

this.getSelectedNode() is not set when the action is being called from the reflist and therefore the this.replaceRefNode is set to false, although it should be true.

Change #1205141 merged by jenkins-bot:

[mediawiki/extensions/Citoid@master] Change CitoidAction to accept arguments as object

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

Change #1205143 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Update CitoidAction call to use new signature

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

We talked about the problem that making this work might be pretty hard and are for now simply deleting the button. Since it is currently broken anyway, there is no functionality loss. Also the same action can still be done when the dialog is being opened from the article.

Change #1205142 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update CitoidAction call to use new signature

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

Change #1219863 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Cite@master] Disable replace feature when ReferenceList is selected

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

Change #1219902 had a related patch set uploaded (by WMDE-Fisch; author: WMDE-Fisch):

[mediawiki/extensions/Cite@master] VE: Refactor MWReferenceDialog setup

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

Change #1219903 had a related patch set uploaded (by WMDE-Fisch; author: WMDE-Fisch):

[mediawiki/extensions/Cite@master] VE: Introduce edit mode flag to MWReferenceDialog

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

Change #1219863 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Disable replace feature when ReferenceList is selected

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

Change #1219863 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Disable replace feature when ReferenceList is selected

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

This is a temporary solution that at least avoids that users run into weird situations. I sketched a way forward that includes a bit more work to solve the root cause of the problem ( and also started working into that direction a bit ). But we probably wont have the time to solve it completely T413760: Allow MWCitationDialog and MWReferenceDialog to work on refs that are not selected

Change #1223672 had a related patch set uploaded (by WMDE-Fisch; author: WMDE-Fisch):

[mediawiki/extensions/Cite@master] VE: Introduce MWEditReferenceNodeCommand and wire it for basic editing

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

Change #1223680 had a related patch set uploaded (by WMDE-Fisch; author: WMDE-Fisch):

[mediawiki/extensions/Cite@master] Disable CitationDialog replace feature when ReferenceList is selected

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

WMDE-Fisch subscribed.

Change #1223680 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Disable CitationDialog replace feature when ReferenceList is selected

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

We are currently disabling the replace citation button for the cases where it was broken, which would solve the described issue. (Although I would even ask, why not remove it completely...)

The experiment where we try to make the button actually do what it should do (all other patches in review here), does currently not work for me (the list disappears during editing, appears again and instead of the citation being replaced, a new ref shows up after the reflist).

Since there is a new ticket for that experiment (T413760), I would suggest removing the patches from this ticket here and close it after demo time. We can then prio the other ticket again in our normal workflows.

Change #1219902 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] VE: Refactor MWReferenceDialog setup

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

We are currently disabling the replace citation button for the cases where it was broken, which would solve the described issue. (Although I would even ask, why not remove it completely...)

The experiment where we try to make the button actually do what it should do (all other patches in review here), does currently not work for me (the list disappears during editing, appears again and instead of the citation being replaced, a new ref shows up after the reflist).

Since there is a new ticket for that experiment (T413760), I would suggest removing the patches from this ticket here and close it after demo time. We can then prio the other ticket again in our normal workflows.

Done :-)

Tobi_WMDE_SW claimed this task.

Change #1205159 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/extensions/Citoid@master] Forward ref node to citoid action when editing ref list

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

Change #1243832 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Citoid@master] CitoidInspector should never destroy reference lists

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

Change #1243832 merged by jenkins-bot:

[mediawiki/extensions/Citoid@master] CitoidInspector should never destroy reference lists

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