Page MenuHomePhabricator

Reusing references inside and from a VE image/gallery dialog
Open, Needs TriagePublic

Description

Images or galleries have a caption attribute where Wikitext can be used that will be rendered on the page. You can define new references in there or re-use references that where defined outside of the caption. VisualEditor should support both directions.

One part of this seems to be working already:

  • I am an editor using visual editor.
  • I open the image/gallery dialog to add a new or edit an exiting image/gallery.
  • I add a reference to the caption.
  • I close the dialog and want to re-use that reference in the article.
  • The reference shows up in the re-use dialog and I can add it to the article

The other way does not though:

  • I am an editor using visual editor.
  • I directly add references to the article or there are already references that were directly added there.
  • I open the image/gallery dialog to add a new or edit an existing image/gallery.
  • I'm trying to re-use a reference that's defined in the article in the caption.
  • The reference does not show up in the re-use dialog if it's defined on the article.

There seems to be a good way to fix the second part though, by pointing the re-use dialog to the root document of the article when used from of the image dialog.

See also:

Acceptance criteria

As a visual editor,
I want to reuse existing references in the image caption dialog so that it's more like editing text anywhere else in the document.

Preconditions: Article fixtures are composed of these optional sections which will be mentioned in the scenario:

  • Existing references - an unnamed ref, a named ref and a references list are included in the document.
  • Existing caption references - document includes an image with a caption having existing named and unnamed references.

Scenario 1: An existing reference is available for reuse in the caption
Action: Add a new image and caption.
Result: The citation "re-use" menu item is enabled.

Scenario 2: List existing references (and caption refs, and newly staged caption refs) for re-use
Action: Click the re-use menu item in the image caption dialog.
Result: A menu lists all existing references from the document and from the caption, in the order they would appear if the document were saved.

Scenario 3: Reused reference has the correct number
Action: Re-use an existing reference with number > 1 in the image caption dialog.
Result: Inserted ref has the same number. Ref content appears in a popped-up preview. The preview includes a warning about reused references.

Scenario 4: New reuses are rendered in the document
Action: Add an image caption which reuses an existing citation. Exit the dialog successfully and insert into the document.
Result: The reused reference appears in the reference list with two backlinks, and both the existing and new references include a reuse warning.

Scenario 5: New caption reference is numbered correctly
Action: Add a new caption reference after an existing document reference.
Result: The new footnote marker has a number > 1, the same in the caption preview and in the document after saving the dialog.

Scenario 6: Newly-deleted reference is not reusable
Action: Delete an existing caption reference and click on the re-use menu item.
Result: The deleted reference doesn't appear in the ref reuse dialog.

Draft implementation plan

  • Media dialog uses "staging" mode to make provisional changes to the full document. (patch)
  • Reference dialog sees refs from the full document. (patch)
  • Trigger to apply image caption changes before any reference dialogs are opened. Tricky because toggling the "reuse" menu could change after every character typed. (patch)

Details

SubjectRepoBranchLines +/-
VisualEditor/VisualEditormaster+4 -30
mediawiki/extensions/Citemaster+34 -10
mediawiki/extensions/Citemaster+13 -17
mediawiki/extensions/Citemaster+41 -1
mediawiki/extensions/Citemaster+65 -0
mediawiki/extensions/Citemaster+55 -6
mediawiki/extensions/Citemaster+22 -30
VisualEditor/VisualEditormaster+6 -2
VisualEditor/VisualEditormaster+2 -15
mediawiki/extensions/Citemaster+2 -3
mediawiki/extensions/Citemaster+20 -67
mediawiki/extensions/Citemaster+26 -39
mediawiki/extensions/Citemaster+58 -0
mediawiki/extensions/Citemaster+6 -12
mediawiki/extensions/Citemaster+21 -26
mediawiki/extensions/Citemaster+4 -0
mediawiki/extensions/Citemaster+87 -0
mediawiki/extensions/Citemaster+54 -9
mediawiki/extensions/VisualEditormaster+3 -7
mediawiki/extensions/Citemaster+12 -7
mediawiki/extensions/VisualEditormaster+4 -0
mediawiki/extensions/VisualEditormaster+38 -21
Show related patches Customize query in gerrit

Event Timeline

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

[mediawiki/extensions/Cite@master] [POC] Allow reference re-use in the VE image/gallery captions

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

WMDE-Fisch moved this task from Doing to Sprint Backlog on the WMDE-TechWish-Sprint-2023-05-03 board.

After playing a bit with the current approach and figuring out a way to make it work in all cases, I'll stop working on this for now. Here's the current state:

What works

  • References from the article can be re-used in the caption when the image is already added to the article and edited.

What does not work

  • When you add an image and try to re-use a reference while adding the image there's an error. [1]
  • When the reference you re-use in the caption is not named yet there will be no auto name applied to the parent.

[1]

Uncaught TypeError: doc.internalList.getItemNode(...) is undefined
    newFromDocumentInsertion ve.dm.TransactionBuilder.js:147
    insertInternalItem ve.dm.MWReferenceModel.js:102

Change 991598 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/VisualEditor@master] [WIP] Copy parent internalList while editing media caption

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

I have a new theory about how this needs to work: the ref reuse dialog can only be correct if pending image caption changes are restaged against the full document just before opening.

Change 992096 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/VisualEditor@master] [WIP] Use staging for the media dialog

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

Change 992711 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/VisualEditor@master] [WIP] Recalculate full document after media dialog changes

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

Change 992930 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] Don't change existing listKeys

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

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

[mediawiki/extensions/Cite@master] [WIP] Notes and dubious changes

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

Change 991598 abandoned by Awight:

[mediawiki/extensions/VisualEditor@master] [WIP] Copy parent internalList while editing media caption

Reason:

Didn't seem to be necessary.

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

Change 993087 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] Stub test for ve.dm.MWReferenceModel

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

Change 993731 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [WIP] Test for duplicate ref insertion

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

If I understand correctly, these problems are based in the fact that the image captions in the dialogs are in completely separate documents from the main editor, and something goes wrong when synchronizing the references between them.

Now, this may be a bad idea, but: what if they weren't in separate documents?

VisualEditor actually almost has support for opening multiple views of the same document, and these views support surfacing different nodes from the document (not necessarily the view node). With just two weird tricks, you can make the small editor inside the dialog edit the main document: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/993810 https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/993809

Demo: https://patchdemo.wmflabs.org/wikis/22ccdf26fd/wiki/T336417

There are some obvious problems, but they're probably fixable; I haven't tried to fix them, though, so no promises.

Now, this may be a bad idea, but: what if they weren't in separate documents?

Interesting, thanks for the ideas! I've already written a small patch to use staging mode in the media dialog, which resolves one of the issues you found in your alternative patches. The citation dialog should probably work in a second level of staging, too.

I'm not sure which approach is going to be less disruptive to VE, though--maybe your team can advise on this. It seems okay in any case that refs for reuse are chosen from a list outside of the edited fragment, this is somewhat similar to how images come from Commons. Once a ref is selected for reuse, we can copy it into the fragment and then our two approaches diverge: either we stage changes to the full document, or the ref contains enough metadata to render it in isolation, and to deduplicate it later in toDomElements.

Change 993087 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] First tests for ve.dm.MWReferenceModel

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

Change 992930 abandoned by Awight:

[mediawiki/extensions/Cite@master] Don't change existing listKeys

Reason:

This was too simplistic, we can't rely on listIndex because accidental collisions look identical to intentional reuses.

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

Change 994962 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [WIP] Experiment with including body in MWReferenceNode

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

Change 997510 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [DNM] Render reference lists using the Parsoid Dom

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

Change 997511 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [POC] Reference reuse dialog pulls data from self-contained references

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

Change 997511 abandoned by Awight:

[mediawiki/extensions/Cite@master] [WIP] Reference reuse dialog pulls data from self-contained references

Reason:

squashed into I427e2e4d687b59029cf3fbdaa7b401807aa37ce3

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

Change 997909 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] New QUnit tests for ve.dm.MWReferenceModel

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

Change 997911 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [WIP] Store a copy of the reference body in the node

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

Change 997912 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] Accessors to find MWReferenceNode in the document

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

Change 997913 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [WIP] Replace easy usages of internalList

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

Change 997914 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [WIP] Switch to using body attribute for footnote content

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

Change 997915 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [WIP] Rewire change events directly to document

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

Change 997916 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [WIP] Clone own node on demand

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

Change 997917 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] Avoid creating a MWReferenceModel if not needed

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

Change 997918 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] Rewrite function to find refs without internalList

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

Change 997510 abandoned by Awight:

[mediawiki/extensions/Cite@master] [WIP] Render reference list from inline ref body

Reason:

squashed

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

Change 997919 had a related patch set uploaded (by Awight; author: Awight):

[VisualEditor/VisualEditor@master] [DNM] Suppress internalList content

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

Change 997920 had a related patch set uploaded (by Awight; author: Awight):

[VisualEditor/VisualEditor@master] [DNM] Debug node attributes

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

Note there is an existing bug where creating a new, unnamed ref inside the caption and then reusing it results in duplicated, still unnamed refs rather than assigning an automatic ":0" name.

This task is blocked on having deprecated internalList entirely.

Change 997917 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Avoid creating a MWReferenceModel if not needed

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

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

[mediawiki/extensions/Cite@master] Add tests for the MWReferenceNode accessors

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

Change 997912 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Accessors to find MWReferenceNode in the document

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

Change 1005035 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [WIP] Refs can be safely reused in a fragment

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

Change 1005767 had a related patch set uploaded (by Awight; author: Awight):

[VisualEditor/VisualEditor@master] [DNM] Exploring issue with how internalList is merged

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