Page MenuHomePhabricator

Visual diff fails with "jQuery.Deferred exception: listContents is undefined"
Closed, ResolvedPublic

Description

https://de.wikipedia.org/w/index.php?title=Venus_von_Savignano&diff=prev&oldid=179042971&diffmode=visual&visualdiff=1 just shows a loading bar, and the error "jQuery.Deferred exception: listContents is undefined" in the console.

Details

Related Gerrit Patches:
mediawiki/extensions/VisualEditor : masterUpdate VE core submodule to master (e0f344c72)
VisualEditor/VisualEditor : masterMake mode for getting document data without metadata

Event Timeline

Schnark created this task.Jul 11 2018, 7:29 AM
Restricted Application added a project: VisualEditor. · View Herald TranscriptJul 11 2018, 7:29 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Let's see... this is ve.dm.VisualDiff.prototype.flattenList failing when it finds a CommentNode that's a child of a ListNode.

* list item
<!--* list item-->
* list item

Editing any of the nodes in that list will produce a visual diff which errors out.

Simple attempt at fixing it by doing:

		if ( !listContents ) {
			// Situation like a CommentNode, which doesn't have children,
			// mixed into a List.
			flatList.metadata.push( {
				listNode: listNode,
				listItem: listItem,
				depth: depth
			} );
			flatList.nodes.push( listItem );
			continue;
		}

...resulted in a visual diff which didn't report any errors but also never passed the loading bar stage. So, something more subtle goes wrong after that.

@Tchanders: does this bring up anything really obvious which would be wrong, so I don't have to debug more? :D

Having a comment node as a child of a list node seems like a violation of
ve.dm.ListNode.static.childNodeTypes = [ 'listItem' ];
so ideally we would just fix the placement of that comment node (which might be more in @dchan 's ball park)

Make the converter give us a CommentMetaItem instead? We try to do that for things like comments mixed into tables between rows already, I think.

It already is giving us a CommentMetaItem... Sounds like it's related to T189543

so ideally we would just fix the placement of that comment node (which might be more in @dchan 's ball park)

For most MetaItems, their location in the doc isn't meaningful, so we could diff them separately (see also T200006). That would solve the current problem, since the CommentMetaItem would be ignored.

AlienMetaItems and CommentMetaItems are the only meta items where placement is meaningful, but it looks like we still haven't figured out where they should be (T189826 and T189543)...

Change 446850 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[VisualEditor/VisualEditor@master] Make mode for getting document data without metadata

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

dchan added a comment.Jul 27 2018, 6:19 PM

I feel the following code isn't that sensible now that meta items live in the main linear data:

ve.dm.CommentNode.static.toDataElement = function ( domElements, converter ) {
      var textarea, text;
      ...
      return {
              // Disallows comment nodes between table rows and such
              type: converter.isValidChildNodeType( 'comment' ) && text !== '' ? 'comment' : 'commentMeta',
              attributes: {
                      text: text
              }
      };
};

Before 21a5d55b85dd098ee0a5f9fdf3edb67841d13939 , this code would have made the awkwardly-placed comment disappear out of the main document tree, but not any more.

Probably we should extend ve.dm.Converter.static.moveInlineMetaItems to move awkwardly-placed block meta items too.

Change 446850 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Make mode for getting document data without metadata

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

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

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

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

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

Deskana closed this task as Resolved.Aug 10 2018, 10:35 AM
Deskana triaged this task as Medium priority.