Page MenuHomePhabricator

Automatically generated references appear as empty in the list of all references, with an error "this.model is null"
Closed, ResolvedPublic1 Estimated Story Points

Description

Steps to reproduce:

  1. Open a page with references for editing (or start a new page and add a list of references, which will initially be empty). When you test in enwiki, make sure that the list of references is not inserted via a template.
  2. Start inserting a new reference, use the way to generated from an URL.
  3. Create and insert the reference, note that the popup displays the contents as expected.

Expected:
The new reference is shown in the list of all references.

Actual:
There is a new reference in the list, but it is empty.
The console shows an error: TypeError: this.model is null

Making changes to the generated reference will make it eventually show up (in my tests 1 to 4 changes were needed, before it appeared).
Adding a new paragraph after the reference seems to consistently make it show up.
Using the same URL a second time will show the reference immediately after inserting.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Deskana moved this task from To Triage to TR0: Interrupt on the VisualEditor board.
Deskana subscribed.

Fortunately, from the user's perspective this is only a visual glitch, but it's still a pretty bad one.

This also happens while editing references that already exist, though much less often, as far as I can tell.

I think this is jQuery promise-async changes biting us again. MWReferencesListNode does a terribly hacky thing to generate the reference HTML and then put it in the list, by creating and then immediately destroying another node. The API call the generated node makes to build the reference HTML has a callback which assumes that there's a model available. Before the async promise change, this would have been true, as it would have been resolving immediately before the destroy could happen.

Change 365057 had a related patch set uploaded (by DLynch; owner: DLynch):
[mediawiki/extensions/VisualEditor@master] ce.MWTransclusionNode: make sure model exists before getting its document

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

That's *a* fix. A better one would be to finally fix T64682 and avoid the hack with the destroyed node in the first place. Or at least adjust MWReferencesListNode to delay the destroy calls -- sticking them behind a setTimeout would be adding a hack on top of a hack, but would probably also work.

Change 365057 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ce.MWTransclusionNode: make sure model exists before getting its document

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

@DLynch Did the above patch fix this issue? Somehow I managed to completely miss that it was merged.

@Deskana Looks like I actually missed a call to it in that patch, and it needs one more. One second.

Change 367429 had a related patch set uploaded (by DLynch; owner: DLynch):
[mediawiki/extensions/VisualEditor@master] ce.MWTransclusionNode: make sure model exists before getting its document

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

Change 367429 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ce.MWTransclusionNode: make sure model exists before getting its document

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

Okay, I need to look at this more. Get Citoid working properly on my VM again, for one thing, which I had been putting off.

Change 369948 had a related patch set uploaded (by DLynch; owner: DLynch):
[VisualEditor/VisualEditor@master] ce.GeneratedContentNode: helper for waiting for generation

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

Change 369949 had a related patch set uploaded (by DLynch; owner: DLynch):
[mediawiki/extensions/Cite@master] ce.MWReferencesListNode: wait for content before destroying temporary view node

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

Change 369948 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ce.GeneratedContentNode: helper for waiting for generation

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

Change 372842 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (72cc043b7)

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

This should be it, this time? :-)

Not quite! We also need https://gerrit.wikimedia.org/r/369949 to merge, after the core update lands.

I'm just really, really excited at the prospect of this bug being closed, evidently. ;-)

Change 372842 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (72cc043b7)

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

Change 369949 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] ce.MWReferencesListNode: wait for content before destroying temporary view node

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

Third time's the charm!