From 01367637713920fb653320363dacbe9f6117a4fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Mon, 6 Mar 2023 16:43:45 +0200 Subject: [PATCH 1/5] Rich text: only consider a format active if active at every selected index --- .../src/components/rich-text/format-edit.js | 34 +---------------- packages/rich-text/src/get-active-formats.js | 38 ++++++++++++++++--- 2 files changed, 35 insertions(+), 37 deletions(-) diff --git a/packages/block-editor/src/components/rich-text/format-edit.js b/packages/block-editor/src/components/rich-text/format-edit.js index 3ab3f47d226ae..75b077ab321d4 100644 --- a/packages/block-editor/src/components/rich-text/format-edit.js +++ b/packages/block-editor/src/components/rich-text/format-edit.js @@ -1,11 +1,7 @@ /** * WordPress dependencies */ -import { - getActiveFormat, - getActiveObject, - isCollapsed, -} from '@wordpress/rich-text'; +import { getActiveFormat, getActiveObject } from '@wordpress/rich-text'; export default function FormatEdit( { formatTypes, @@ -22,37 +18,11 @@ export default function FormatEdit( { } const activeFormat = getActiveFormat( value, name ); - let isActive = activeFormat !== undefined; + const isActive = activeFormat !== undefined; const activeObject = getActiveObject( value ); const isObjectActive = activeObject !== undefined && activeObject.type === name; - // Edge case: un-collapsed link formats. - // If there is a missing link format at either end of the selection - // then we shouldn't show the Edit UI because the selection has exceeded - // the bounds of the link format. - // Also if the format objects don't match then we're dealing with two separate - // links so we should not allow the link to be modified over the top. - if ( name === 'core/link' && ! isCollapsed( value ) ) { - const formats = value.formats; - - const linkFormatAtStart = formats[ value.start ]?.find( - ( { type } ) => type === 'core/link' - ); - - const linkFormatAtEnd = formats[ value.end - 1 ]?.find( - ( { type } ) => type === 'core/link' - ); - - if ( - ! linkFormatAtStart || - ! linkFormatAtEnd || - linkFormatAtStart !== linkFormatAtEnd - ) { - isActive = false; - } - } - return ( format === formatsAtIndex[ index ] + ); + } + + return _activeFormats || EMPTY_ACTIVE_FORMATS; } From 8f9c77039a75999ac33ad6a2ad9a72fc5e576867 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Mon, 6 Mar 2023 18:20:33 +0200 Subject: [PATCH 2/5] Fix unit tests --- packages/rich-text/src/get-active-format.js | 2 +- packages/rich-text/src/get-active-formats.js | 2 +- .../src/test/__snapshots__/to-dom.js.snap | 4 +--- .../rich-text/src/test/get-active-format.js | 17 +++++++++++++++-- packages/rich-text/src/test/toggle-format.js | 8 +++++--- 5 files changed, 23 insertions(+), 10 deletions(-) diff --git a/packages/rich-text/src/get-active-format.js b/packages/rich-text/src/get-active-format.js index 783177168b60a..a259b094272f3 100644 --- a/packages/rich-text/src/get-active-format.js +++ b/packages/rich-text/src/get-active-format.js @@ -19,7 +19,7 @@ import { getActiveFormats } from './get-active-formats'; * type, or undefined. */ export function getActiveFormat( value, formatType ) { - return getActiveFormats( value )?.find( + return getActiveFormats( value ).find( ( { type } ) => type === formatType ); } diff --git a/packages/rich-text/src/get-active-formats.js b/packages/rich-text/src/get-active-formats.js index 3a1ac067af64a..a936e1e7f408f 100644 --- a/packages/rich-text/src/get-active-formats.js +++ b/packages/rich-text/src/get-active-formats.js @@ -59,7 +59,7 @@ export function getActiveFormats( value, EMPTY_ACTIVE_FORMATS = [] ) { return EMPTY_ACTIVE_FORMATS; } - // Only keep an acive format if it's active for every single index. + // Only keep an active format if it's active for every single index. _activeFormats = _activeFormats.filter( ( format, index ) => format === formatsAtIndex[ index ] ); diff --git a/packages/rich-text/src/test/__snapshots__/to-dom.js.snap b/packages/rich-text/src/test/__snapshots__/to-dom.js.snap index 756d06885ea14..a0eb7b3cb0a3c 100644 --- a/packages/rich-text/src/test/__snapshots__/to-dom.js.snap +++ b/packages/rich-text/src/test/__snapshots__/to-dom.js.snap @@ -61,9 +61,7 @@ exports[`recordToDom should create a value with image object and formatting 1`] exports[`recordToDom should create a value with image object and text after 1`] = ` - + { expect( getActiveFormat( record, 'em' ) ).toBe( undefined ); } ); - it( 'should return format at first character for uncollapsed selection', () => { + it( 'should return format when active over whole selection', () => { const record = { formats: [ [ em ], [ strong ], , ], + replacements: [ , , , ], text: 'one', start: 0, - end: 2, + end: 1, }; expect( getActiveFormat( record, 'em' ) ).toBe( em ); } ); + it( 'should return not return format when not active over whole selection', () => { + const record = { + formats: [ [ em ], [ strong ], , ], + replacements: [ , , , ], + text: 'one', + start: 0, + end: 2, + }; + + expect( getActiveFormat( record, 'em' ) ).toBe( undefined ); + } ); + it( 'should return undefined if at the boundary before', () => { const record = { formats: [ [ em ], , [ em ] ], diff --git a/packages/rich-text/src/test/toggle-format.js b/packages/rich-text/src/test/toggle-format.js index 6a71412c413fb..a3f25eff9c8ba 100644 --- a/packages/rich-text/src/test/toggle-format.js +++ b/packages/rich-text/src/test/toggle-format.js @@ -14,15 +14,15 @@ describe( 'toggleFormat', () => { const strong = { type: 'strong' }; const em = { type: 'em' }; - it( 'should remove format if it exists at start of selection', () => { + it( 'should remove format if it is active', () => { const record = { formats: [ , , , [ strong ], - [ em, strong ], - [ em ], + [ strong, em ], + [ strong, em ], [ em ], , , @@ -31,12 +31,14 @@ describe( 'toggleFormat', () => { , , ], + replacements: [ , , , , , , , , , , , , , ], text: 'one two three', start: 3, end: 6, }; const expected = { formats: [ , , , , [ em ], [ em ], [ em ], , , , , , , ], + replacements: [ , , , , , , , , , , , , , ], activeFormats: [], text: 'one two three', start: 3, From bc15d5858a25c1cc5002c986059f4e9e4cfb7aad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Mon, 6 Mar 2023 19:02:40 +0200 Subject: [PATCH 3/5] Fix for split format --- packages/rich-text/src/get-active-formats.js | 29 ++++++++++++++++---- packages/rich-text/src/test/toggle-format.js | 9 ++++-- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/packages/rich-text/src/get-active-formats.js b/packages/rich-text/src/get-active-formats.js index a936e1e7f408f..cc5a5f1f056d3 100644 --- a/packages/rich-text/src/get-active-formats.js +++ b/packages/rich-text/src/get-active-formats.js @@ -4,6 +4,7 @@ /** * Internal dependencies */ +import { isFormatEqual } from './is-format-equal'; import { slice } from './slice'; /** @@ -47,9 +48,11 @@ export function getActiveFormats( value, EMPTY_ACTIVE_FORMATS = [] ) { const selectedValue = slice( value ); - let _activeFormats = selectedValue.formats[ 0 ]; + const _activeFormats = selectedValue.formats[ 0 ]; let i = selectedValue.formats.length; + // For performance reasons, start from the end where it's much quicker to + // realise that there are no active formats. while ( i-- ) { const formatsAtIndex = selectedValue.formats[ i ]; @@ -59,10 +62,26 @@ export function getActiveFormats( value, EMPTY_ACTIVE_FORMATS = [] ) { return EMPTY_ACTIVE_FORMATS; } - // Only keep an active format if it's active for every single index. - _activeFormats = _activeFormats.filter( - ( format, index ) => format === formatsAtIndex[ index ] - ); + let ii = _activeFormats.length; + + // Loop over the active formats and remove any that are not present at + // the current index. + while ( ii-- ) { + const format = _activeFormats[ ii ]; + + if ( + ! formatsAtIndex.find( ( _format ) => + isFormatEqual( format, _format ) + ) + ) { + _activeFormats.splice( ii, 1 ); + } + } + + // If there are no active formats, we can stop. + if ( _activeFormats.length === 0 ) { + return EMPTY_ACTIVE_FORMATS; + } } return _activeFormats || EMPTY_ACTIVE_FORMATS; diff --git a/packages/rich-text/src/test/toggle-format.js b/packages/rich-text/src/test/toggle-format.js index a3f25eff9c8ba..442abbc25b307 100644 --- a/packages/rich-text/src/test/toggle-format.js +++ b/packages/rich-text/src/test/toggle-format.js @@ -20,9 +20,12 @@ describe( 'toggleFormat', () => { , , , - [ strong ], - [ strong, em ], - [ strong, em ], + // In reality, formats at a different index are never the same + // value. Only formats that create the same tag are the same + // value. + [ { type: 'strong' } ], + [ em, strong ], + [ em, strong ], [ em ], , , From 965683deee64638b912534abc4f6d6be9845d445 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Mon, 6 Mar 2023 19:05:30 +0200 Subject: [PATCH 4/5] Clone active values --- packages/rich-text/src/get-active-formats.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/rich-text/src/get-active-formats.js b/packages/rich-text/src/get-active-formats.js index cc5a5f1f056d3..97d08c23c6b00 100644 --- a/packages/rich-text/src/get-active-formats.js +++ b/packages/rich-text/src/get-active-formats.js @@ -48,7 +48,8 @@ export function getActiveFormats( value, EMPTY_ACTIVE_FORMATS = [] ) { const selectedValue = slice( value ); - const _activeFormats = selectedValue.formats[ 0 ]; + // Clone the formats so we're not mutating the live value. + const _activeFormats = [ ...selectedValue.formats[ 0 ] ]; let i = selectedValue.formats.length; // For performance reasons, start from the end where it's much quicker to From 42c81026c21925e36362d33ba5b51ee618c4d921 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Mon, 6 Mar 2023 19:26:37 +0200 Subject: [PATCH 5/5] Simplify --- packages/rich-text/src/get-active-formats.js | 9 ++++----- packages/rich-text/src/test/get-active-format.js | 2 -- packages/rich-text/src/test/toggle-format.js | 2 -- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/packages/rich-text/src/get-active-formats.js b/packages/rich-text/src/get-active-formats.js index 97d08c23c6b00..09bbef0cf2d6c 100644 --- a/packages/rich-text/src/get-active-formats.js +++ b/packages/rich-text/src/get-active-formats.js @@ -5,7 +5,6 @@ * Internal dependencies */ import { isFormatEqual } from './is-format-equal'; -import { slice } from './slice'; /** * Gets the all format objects at the start of the selection. @@ -46,16 +45,16 @@ export function getActiveFormats( value, EMPTY_ACTIVE_FORMATS = [] ) { return EMPTY_ACTIVE_FORMATS; } - const selectedValue = slice( value ); + const selectedFormats = formats.slice( start, end ); // Clone the formats so we're not mutating the live value. - const _activeFormats = [ ...selectedValue.formats[ 0 ] ]; - let i = selectedValue.formats.length; + const _activeFormats = [ ...selectedFormats[ 0 ] ]; + let i = selectedFormats.length; // For performance reasons, start from the end where it's much quicker to // realise that there are no active formats. while ( i-- ) { - const formatsAtIndex = selectedValue.formats[ i ]; + const formatsAtIndex = selectedFormats[ i ]; // If we run into any index without formats, we're sure that there's no // active formats. diff --git a/packages/rich-text/src/test/get-active-format.js b/packages/rich-text/src/test/get-active-format.js index c6e473713ad51..6ba279c7df243 100644 --- a/packages/rich-text/src/test/get-active-format.js +++ b/packages/rich-text/src/test/get-active-format.js @@ -20,7 +20,6 @@ describe( 'getActiveFormat', () => { it( 'should return format when active over whole selection', () => { const record = { formats: [ [ em ], [ strong ], , ], - replacements: [ , , , ], text: 'one', start: 0, end: 1, @@ -32,7 +31,6 @@ describe( 'getActiveFormat', () => { it( 'should return not return format when not active over whole selection', () => { const record = { formats: [ [ em ], [ strong ], , ], - replacements: [ , , , ], text: 'one', start: 0, end: 2, diff --git a/packages/rich-text/src/test/toggle-format.js b/packages/rich-text/src/test/toggle-format.js index 442abbc25b307..95f03947fa667 100644 --- a/packages/rich-text/src/test/toggle-format.js +++ b/packages/rich-text/src/test/toggle-format.js @@ -34,14 +34,12 @@ describe( 'toggleFormat', () => { , , ], - replacements: [ , , , , , , , , , , , , , ], text: 'one two three', start: 3, end: 6, }; const expected = { formats: [ , , , , [ em ], [ em ], [ em ], , , , , , , ], - replacements: [ , , , , , , , , , , , , , ], activeFormats: [], text: 'one two three', start: 3,