From c821aaec4690c4c4e6224d49dc9910f7265af1bb Mon Sep 17 00:00:00 2001 From: Lena Morita Date: Fri, 25 Feb 2022 17:42:43 +0900 Subject: [PATCH 1/4] Check for valid registered format This doesn't actually fix anything, but adds a fallback layer of safety when unexpected things happen. --- .../collaborative-editing/use-yjs/algorithms/rich-text.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/components/collaborative-editing/use-yjs/algorithms/rich-text.js b/src/components/collaborative-editing/use-yjs/algorithms/rich-text.js index 4fd1dd41f..8f62f1e2e 100644 --- a/src/components/collaborative-editing/use-yjs/algorithms/rich-text.js +++ b/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 ] ) => [ From 293659ac6e780bd2d0874ef1325e2c4d72318ab6 Mon Sep 17 00:00:00 2001 From: Lena Morita Date: Fri, 25 Feb 2022 17:44:06 +0900 Subject: [PATCH 2/4] Add tests --- .../use-yjs/algorithms/__tests__/rich-text.js | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/src/components/collaborative-editing/use-yjs/algorithms/__tests__/rich-text.js b/src/components/collaborative-editing/use-yjs/algorithms/__tests__/rich-text.js index bac4071fc..527fcbdba 100644 --- a/src/components/collaborative-editing/use-yjs/algorithms/__tests__/rich-text.js +++ b/src/components/collaborative-editing/use-yjs/algorithms/__tests__/rich-text.js @@ -156,6 +156,36 @@ describe( 'conflict merging', () => { expect( a ).toBe( 'foo' ); expect( a ).toBe( b ); } ); + + it( 'should handle inline images', () => { + const htmlA = '
  • a
    • '; + const htmlB = '
  • b
    • '; + const [ a, b ] = updateSimultaneously( '
    • ', htmlA, htmlB ); + + expect( a ).toBe( '
  • ab
    • ' ); + expect( a ).toBe( b ); + } ); + + it( 'should handle lists', () => { + const htmlA = '
  • #one
  • two
    • '; + const htmlB = '
  • one
  • two#
    • '; + const [ a, b ] = updateSimultaneously( '
  • one
  • two
    • ', htmlA, htmlB ); + + expect( a ).toBe( '
  • #one
  • two#
    • ' ); + expect( a ).toBe( b ); + } ); + + // TODO: Unsolved problem + // We won't 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 = '
  • #outer
    • inner
    • '; + const htmlB = '
  • outer
    • inner#
    • '; + const [ a, b ] = updateSimultaneously( '
  • outer
    • inner
    • ', htmlA, htmlB ); + + expect( a ).toBe( '
  • #outer
    • inner#
    • ' ); + 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 = '
  • outer
    1. inner
      • innermost
    2. inner
  • outer
    • '; + const after = + '
  • outer
    1. inner
      • innermost edit
    2. inner
  • outer
    • '; + const richTextMap = richTextMapFrom( before ); + applyHTMLDelta( before, after, richTextMap ); + expect( richTextMapToHTML( richTextMap ) ).toBe( after ); + } ); + + it( 'should support nested lists 2', () => { + const before = '
  • outer
    1. inner
    2. inner
      • innermost
    • '; + const after = '
  • outer
    1. inner
    2. inner
      • innermost edit
    • '; + const richTextMap = richTextMapFrom( before ); + applyHTMLDelta( before, after, richTextMap ); + expect( richTextMapToHTML( richTextMap ) ).toBe( after ); + } ); + + it( 'should support nested lists with inner formatting', () => { + const before = '
  • list item
    • nested item
    • '; + const after = '
  • list item
    • nested list item
    • '; + const richTextMap = richTextMapFrom( before ); + applyHTMLDelta( before, after, richTextMap ); + expect( richTextMapToHTML( richTextMap ) ).toBe( after ); + } ); + + it( 'should support indenting a list', () => { + const before = '
  • outer
    • '; + const after = '
  • outer
    • '; + const richTextMap = richTextMapFrom( before ); + applyHTMLDelta( before, after, richTextMap ); + expect( richTextMapToHTML( richTextMap ) ).toBe( after ); + } ); + + it( 'should support nested lists with empty item', () => { + const before = '
  • outer
  • outer
    • '; + const after = '
  • #outer
  • outer
    • '; + const richTextMap = richTextMapFrom( before ); + applyHTMLDelta( before, after, richTextMap ); + expect( richTextMapToHTML( richTextMap ) ).toBe( after ); + } ); + + it( 'should support nested lists with inline image', () => { + const before = '
  • outer
  • outer
    • '; + const after = '
  • #outer
  • outer
    • '; + const richTextMap = richTextMapFrom( before ); + applyHTMLDelta( before, after, richTextMap ); + expect( richTextMapToHTML( richTextMap ) ).toBe( after ); + } ); } ); From 79173670f2bab7328ef64a6198da545e651628da Mon Sep 17 00:00:00 2001 From: Lena Morita Date: Fri, 25 Feb 2022 20:31:14 +0900 Subject: [PATCH 3/4] Handle nested lists --- .../use-yjs/algorithms/__tests__/rich-text.js | 2 +- .../use-yjs/algorithms/rich-text.js | 108 ++++++++++++++++-- .../use-yjs/algorithms/yjs.js | 1 + 3 files changed, 100 insertions(+), 11 deletions(-) diff --git a/src/components/collaborative-editing/use-yjs/algorithms/__tests__/rich-text.js b/src/components/collaborative-editing/use-yjs/algorithms/__tests__/rich-text.js index 527fcbdba..e8fe0651d 100644 --- a/src/components/collaborative-editing/use-yjs/algorithms/__tests__/rich-text.js +++ b/src/components/collaborative-editing/use-yjs/algorithms/__tests__/rich-text.js @@ -176,7 +176,7 @@ describe( 'conflict merging', () => { } ); // TODO: Unsolved problem - // We won't be able to solve this until Yjs solves the "nested tag structure fidelity" 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 = '
  • #outer
    • inner
    • '; diff --git a/src/components/collaborative-editing/use-yjs/algorithms/rich-text.js b/src/components/collaborative-editing/use-yjs/algorithms/rich-text.js index 8f62f1e2e..1c38af5b5 100644 --- a/src/components/collaborative-editing/use-yjs/algorithms/rich-text.js +++ b/src/components/collaborative-editing/use-yjs/algorithms/rich-text.js @@ -81,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. * @@ -92,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, }; @@ -115,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 ); @@ -140,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 ); } ); } @@ -152,16 +185,62 @@ 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} 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 += ``; + } ); + + // 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; } /** @@ -169,11 +248,20 @@ export function richTextMapToHTML( richTextMap ) { * * @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} 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 }` ) + .map( ( line, i ) => { + const { isOpeningTag, html } = wrapperTagReplacements[ i ]; + + return isOpeningTag + ? `<${ multilineTag }>${ line }${ html }` + : `<${ multilineTag }>${ line }${ html }`; + } ) .join( '' ); } diff --git a/src/components/collaborative-editing/use-yjs/algorithms/yjs.js b/src/components/collaborative-editing/use-yjs/algorithms/yjs.js index 1bc40cf4a..7d91aed59 100644 --- a/src/components/collaborative-editing/use-yjs/algorithms/yjs.js +++ b/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() ], ] ) ); } From 3e072e03e21248b0a17d4d184776c8bf30886f69 Mon Sep 17 00:00:00 2001 From: Lena Morita Date: Fri, 25 Feb 2022 20:05:26 +0900 Subject: [PATCH 4/4] Update changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20c283bbd..e49e918f3 100644 --- a/CHANGELOG.md +++ b/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