Collab: Fix basic handling of nested lists

CHANGELOG.md

@@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [2.11.1] - Unreleased
+
+### Fixed
+
+- Fixed basic handling of nested lists in collab mode.
+
 ## [2.11.0] - 2022-02-18
 
 ### Added

src/components/collaborative-editing/use-yjs/algorithms/__tests__/rich-text.js

@@ -156,6 +156,36 @@ describe( 'conflict merging', () => {
 		expect( a ).toBe( '<code><em>foo</em></code>' );
 		expect( a ).toBe( b );
 	} );
+
+	it( 'should handle inline images', () => {
+		const htmlA = '<li>a<img src="#"></li>';
+		const htmlB = '<li><img src="#">b</li>';
+		const [ a, b ] = updateSimultaneously( '<li><img src="#"></li>', htmlA, htmlB );
+
+		expect( a ).toBe( '<li>a<img src="#">b</li>' );
+		expect( a ).toBe( b );
+	} );
+
+	it( 'should handle lists', () => {
+		const htmlA = '<li>#one</li><li>two</li>';
+		const htmlB = '<li>one</li><li>two#</li>';
+		const [ a, b ] = updateSimultaneously( '<li>one</li><li>two</li>', htmlA, htmlB );
+
+		expect( a ).toBe( '<li>#one</li><li>two#</li>' );
+		expect( a ).toBe( b );
+	} );
+
+	// TODO: Unsolved problem
+	// We might not be able to solve this until Yjs solves the "nested tag structure fidelity" problem
+	// that Y.XmlText has (https://github.com/yjs/yjs/issues/337)
+	it.skip( 'should handle nested lists', () => {
+		const htmlA = '<li>#outer<ul><li>inner</li></ul></li>';
+		const htmlB = '<li>outer<ul><li>inner#</li></ul></li>';
+		const [ a, b ] = updateSimultaneously( '<li>outer<ul><li>inner</li></ul></li>', htmlA, htmlB );
+
+		expect( a ).toBe( '<li>#outer<ul><li>inner#</li></ul></li>' );
+		expect( a ).toBe( b );
+	} );
 } );
 
 describe( 'multiline', () => {
@@ -174,4 +204,53 @@ describe( 'multiline', () => {
 		applyHTMLDelta( before, after, richTextMap );
 		expect( richTextMapToHTML( richTextMap ) ).toBe( after );
 	} );
+
+	it( 'should support nested lists 1', () => {
+		const before = '<li>outer<ol><li>inner<ul><li>innermost</li></ul></li><li>inner</li></ol></li><li>outer</li>';
+		const after =
+			'<li>outer<ol><li>inner<ul><li>innermost edit</li></ul></li><li>inner</li></ol></li><li>outer</li>';
+		const richTextMap = richTextMapFrom( before );
+		applyHTMLDelta( before, after, richTextMap );
+		expect( richTextMapToHTML( richTextMap ) ).toBe( after );
+	} );
+
+	it( 'should support nested lists 2', () => {
+		const before = '<li>outer<ol><li>inner</li><li>inner<ul><li>innermost</li></ul></li></ol></li>';
+		const after = '<li>outer<ol><li>inner</li><li>inner<ul><li>innermost edit</li></ul></li></ol></li>';
+		const richTextMap = richTextMapFrom( before );
+		applyHTMLDelta( before, after, richTextMap );
+		expect( richTextMapToHTML( richTextMap ) ).toBe( after );
+	} );
+
+	it( 'should support nested lists with inner formatting', () => {
+		const before = '<li>list item<ul><li><strong>nested</strong> item</li></ul></li>';
+		const after = '<li>list item<ul><li><strong>nested</strong> list item</li></ul></li>';
+		const richTextMap = richTextMapFrom( before );
+		applyHTMLDelta( before, after, richTextMap );
+		expect( richTextMapToHTML( richTextMap ) ).toBe( after );
+	} );
+
+	it( 'should support indenting a list', () => {
+		const before = '<li>outer</li>';
+		const after = '<li>outer<ul><li></li></ul></li>';
+		const richTextMap = richTextMapFrom( before );
+		applyHTMLDelta( before, after, richTextMap );
+		expect( richTextMapToHTML( richTextMap ) ).toBe( after );
+	} );
+
+	it( 'should support nested lists with empty item', () => {
+		const before = '<li>outer<ul><li></li></ul></li><li>outer</li>';
+		const after = '<li>#outer<ul><li></li></ul></li><li>outer</li>';
+		const richTextMap = richTextMapFrom( before );
+		applyHTMLDelta( before, after, richTextMap );
+		expect( richTextMapToHTML( richTextMap ) ).toBe( after );
+	} );
+
+	it( 'should support nested lists with inline image', () => {
+		const before = '<li>outer<ul><li><img src="#"></li></ul></li><li>outer</li>';
+		const after = '<li>#outer<ul><li><img src="#"></li></ul></li><li>outer</li>';
+		const richTextMap = richTextMapFrom( before );
+		applyHTMLDelta( before, after, richTextMap );
+		expect( richTextMapToHTML( richTextMap ) ).toBe( after );
+	} );
 } );

src/components/collaborative-editing/use-yjs/algorithms/rich-text.js

@@ -58,7 +58,10 @@ export function gutenFormatsToYFormats( formats ) {
  * For example, `core/bold` will be converted back to `strong`.
  */
 export function namedGutenFormatToStandardTags( format ) {
-	const { tagName, attributes = {} } = select( 'core/rich-text' ).getFormatType( format.type );
+	const formatTypeRecord = select( 'core/rich-text' ).getFormatType( format.type );
+	if ( ! formatTypeRecord ) return { [ format.type ]: true };
+
+	const { tagName, attributes = {} } = formatTypeRecord;
 	if ( ! format.attributes ) return { [ tagName ]: true };
 
 	const remappedEntries = Object.entries( format.attributes ).map( ( [ key, value ] ) => [
@@ -78,6 +81,36 @@ function getInferredMultilineTag( html ) {
 	return undefined;
 }
 
+/**
+ * Massage the Gutenberg replacements into Yjs-friendly structures.
+ *
+ * @param {array} a The `replacements` array of a Gutenberg RichText.
+ * @param {array} b The `replacements` array of another Gutenberg RichText.
+ */
+function prepareReplacementsForTransaction( a, b ) {
+	const partitionReplacementTypes = ( arr ) => {
+		let multilineWrapperReplacements = {};
+		let normalReplacements = [];
+
+		arr.forEach( ( r, index ) => {
+			if ( Array.isArray( r ) ) {
+				// If it's an array, it's a multiline wrapper tag (e.g. ul/ol) and not a normal replacement.
+				multilineWrapperReplacements[ index ] = r;
+			} else if ( r ) {
+				// Since normal replacements do not rely on an index-based mapping
+				// with the full text, let's condense the sparse array.
+				normalReplacements.push( r );
+			}
+		} );
+		return { multilineWrapperReplacements, normalReplacements };
+	};
+
+	const { normalReplacements: na } = partitionReplacementTypes( a );
+	const { multilineWrapperReplacements, normalReplacements: nb } = partitionReplacementTypes( b );
+
+	return { multilineWrapperReplacements, replacementsDiff: diff.simpleDiffArray( na, nb ) };
+}
+
 /**
  * Apply the delta between two HTML strings to a Y.XmlText.
  *
@@ -89,8 +122,10 @@ function getInferredMultilineTag( html ) {
 export function applyHTMLDelta( htmlA, htmlB, richTextMap, richTextOpts = {} ) {
 	const [ multilineTagA, multilineTagB ] = [ htmlA, htmlB ].map( getInferredMultilineTag );
 	const inferredMultilineTag = multilineTagA || multilineTagB;
+	const inferredMultilineWrapperTags = inferredMultilineTag === 'li' ? [ 'ul', 'ol' ] : [];
 	const mergedRichTextOpts = {
 		...( inferredMultilineTag ? { multilineTag: inferredMultilineTag } : {} ),
+		multilineWrapperTags: inferredMultilineWrapperTags,
 		...richTextOpts,
 	};
 
@@ -112,10 +147,10 @@ export function applyHTMLDelta( htmlA, htmlB, richTextMap, richTextOpts = {} ) {
 		{}
 	);
 
-	// Yjs can't do insertion operations on sparse arrays. Since replacements do not rely on
-	// an index-based mapping with the full text, let's condense the arrays here.
-	const toDenseArray = ( arr ) => arr.filter( ( x ) => x );
-	const replacementsDiff = diff.simpleDiffArray( toDenseArray( a.replacements ), toDenseArray( b.replacements ) );
+	const { multilineWrapperReplacements, replacementsDiff } = prepareReplacementsForTransaction(
+		a.replacements,
+		b.replacements
+	);
 
 	richTextMap.doc?.transact( () => {
 		richTextMap.get( 'xmlText' ).delete( stringDiff.index, stringDiff.remove );
@@ -137,6 +172,7 @@ export function applyHTMLDelta( htmlA, htmlB, richTextMap, richTextOpts = {} ) {
 
 		richTextMap.get( 'replacements' ).delete( replacementsDiff.index, replacementsDiff.remove );
 		richTextMap.get( 'replacements' ).insert( replacementsDiff.index, replacementsDiff.insert );
+		richTextMap.set( 'multilineWrapperReplacements', multilineWrapperReplacements );
 	} );
 }
 
@@ -149,28 +185,83 @@ export function applyHTMLDelta( htmlA, htmlB, richTextMap, richTextOpts = {} ) {
 export function richTextMapToHTML( richTextMap ) {
 	let text = richTextMap.get( 'xmlText' ).toString();
 
+	// Process multiline tags
+	const multilineTag = richTextMap.get( 'multilineTag' );
+	text = multilineTag
+		? stringAsMultiline( text, multilineTag, richTextMap.get( 'multilineWrapperReplacements' ) )
+		: text;
+
+	// Process replacements (e.g. inline images)
 	richTextMap.get( 'replacements' ).forEach( ( replacement ) => {
 		const replacementHTML = toHTMLString( {
-			value: { replacements: [ replacement ], formats: Array( 1 ), text: OBJECT_REPLACEMENT_CHARACTER },
+			value: {
+				replacements: [ replacement ],
+				formats: Array( 1 ),
+				text: OBJECT_REPLACEMENT_CHARACTER,
+			},
 		} );
 		text = text.replace( OBJECT_REPLACEMENT_CHARACTER, replacementHTML );
 	} );
 
-	const multilineTag = richTextMap.get( 'multilineTag' );
+	return text;
+}
+
+/**
+ * Get HTML replacements for each multiline wrapper tag replacement.
+ *
+ * @param {string} str
+ * @param {Record<number, {type: string}[]>} replacements
+ */
+function getMultilineWrapperTagHTMLReplacements( str, replacements ) {
+	let replacementsHTML = [];
+	let currentMultilineWrappers = [];
+	let foundLineSeparatorIndex = -1;
+
+	do {
+		foundLineSeparatorIndex = str.indexOf( __UNSTABLE_LINE_SEPARATOR, foundLineSeparatorIndex + 1 );
+		const multilineWrappers = replacements[ foundLineSeparatorIndex ] ?? [];
+		const d = diff.simpleDiffArray( currentMultilineWrappers, multilineWrappers, isEqual );
+		let html = '';
 
-	return multilineTag ? stringAsMultiline( text, multilineTag ) : text;
+		// Closing multiline wrapper tags
+		currentMultilineWrappers
+			.slice( d.index, d.index + d.remove )
+			.reverse()
+			.forEach( ( multilineWrapper ) => {
+				html += `</${ multilineWrapper.type }></li>`;
+			} );
+
+		// Opening multiline wrapper tags
+		d.insert.forEach( ( multilineWrapper ) => {
+			html += `<${ multilineWrapper.type }>`;
+		} );
+
+		replacementsHTML.push( { isOpeningTag: !! d.insert.length, html } );
+		currentMultilineWrappers = multilineWrappers;
+	} while ( foundLineSeparatorIndex !== -1 );
+
+	return replacementsHTML;
 }
 
 /**
  * Wraps each line of a multiline string with the given tags.
  *
  * @param {string} str A multiline string delimited by __UNSTABLE_LINE_SEPARATOR.
  * @param {string} multilineTag The tag name to wrap each line with.
- * @returns
+ * @param {Record<number, {type: string}[]>} replacements Multiline wrapper replacements.
+ * @returns {string} The string reconstructed with multiline considerations.
  */
-function stringAsMultiline( str, multilineTag ) {
+function stringAsMultiline( str, multilineTag, replacements ) {
+	const wrapperTagReplacements = getMultilineWrapperTagHTMLReplacements( str, replacements );
+
 	return str
 		.split( __UNSTABLE_LINE_SEPARATOR )
-		.map( ( str ) => `<${ multilineTag }>${ str }</${ multilineTag }>` )
+		.map( ( line, i ) => {
+			const { isOpeningTag, html } = wrapperTagReplacements[ i ];
+
+			return isOpeningTag
+				? `<${ multilineTag }>${ line }${ html }`
+				: `<${ multilineTag }>${ line }</${ multilineTag }>${ html }`;
+		} )
 		.join( '' );
 }

src/components/collaborative-editing/use-yjs/algorithms/yjs.js

@@ -108,6 +108,7 @@ export function updateRichText( { newBlock, attributeKey, richTexts } ) {
 				[ 'xmlText', new yjs.XmlText() ],
 				[ 'multilineTag', undefined ],
 				[ 'replacements', new yjs.Array() ],
+				[ 'multilineWrapperReplacements', new yjs.Array() ],
 			] )
 		);
 	}