From 05df053997630a1bf85b809e86b30291d466a193 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Wed, 13 Dec 2023 11:48:50 -0500 Subject: [PATCH 1/3] fix: Prevent unnecessary content changes clearing redo actions In this context, `this.value` is not a string but a instance of `RichTextData`. Therefore, comparing the two values results in unexpected inequality, triggering an update of the block's `attributes.content` toggling it from a `ReactTextData` instance to a string. This toggle results in the undo manager tracking the change as a new line of editor history, clearing out any pending redo actions. The `RichTextData` type was introduced in #43204. Invoking `toString` may not be the best long-term solution to this problem. Refactoring the rich text implementation to appropriately leverage `RichTextData` and (potentially) treat `value` and `record` values different and storing them separately may be necessary. --- .../src/components/rich-text/native/index.native.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/block-editor/src/components/rich-text/native/index.native.js b/packages/block-editor/src/components/rich-text/native/index.native.js index 165316fdbde769..b7b9ebdd787b17 100644 --- a/packages/block-editor/src/components/rich-text/native/index.native.js +++ b/packages/block-editor/src/components/rich-text/native/index.native.js @@ -336,7 +336,7 @@ export class RichText extends Component { ); this.debounceCreateUndoLevel(); - const refresh = this.value !== contentWithoutRootTag; + const refresh = this.value.toString() !== contentWithoutRootTag; this.value = contentWithoutRootTag; // We don't want to refresh if our goal is just to create a record. From 5cfdf669021067223797816f2a53ba9a1fd0681b Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Wed, 13 Dec 2023 11:56:48 -0500 Subject: [PATCH 2/3] fix: Convert `RichTextData` to string before comparing values The value stored in the rich text component may be a string or a `RichTextData`. Until the value is store consistently, it may be necessary to convert each value to a string prior to equality comparisons. --- .../rich-text/native/index.native.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/block-editor/src/components/rich-text/native/index.native.js b/packages/block-editor/src/components/rich-text/native/index.native.js index b7b9ebdd787b17..116425a15b53b1 100644 --- a/packages/block-editor/src/components/rich-text/native/index.native.js +++ b/packages/block-editor/src/components/rich-text/native/index.native.js @@ -267,7 +267,7 @@ export class RichText extends Component { onCreateUndoLevel() { const { __unstableOnCreateUndoLevel: onCreateUndoLevel } = this.props; // If the content is the same, no level needs to be created. - if ( this.lastHistoryValue === this.value ) { + if ( this.lastHistoryValue.toString() === this.value.toString() ) { return; } @@ -320,7 +320,7 @@ export class RichText extends Component { unescapeSpaces( event.nativeEvent.text ) ); // On iOS, onChange can be triggered after selection changes, even though there are no content changes. - if ( contentWithoutRootTag === this.value ) { + if ( contentWithoutRootTag === this.value.toString() ) { return; } this.lastEventCount = event.nativeEvent.eventCount; @@ -567,7 +567,7 @@ export class RichText extends Component { // Check if value is up to date with latest state of native AztecView. if ( event.nativeEvent.text && - event.nativeEvent.text !== this.props.value + event.nativeEvent.text !== this.props.value.toString() ) { this.onTextUpdate( event ); } @@ -592,7 +592,7 @@ export class RichText extends Component { // this approach is not perfectly reliable. const isManual = this.lastAztecEventType !== 'input' && - this.props.value === this.value; + this.props.value.toString() === this.value.toString(); if ( hasChanged && isManual ) { const value = this.createRecord(); const activeFormats = getActiveFormats( value ); @@ -662,7 +662,7 @@ export class RichText extends Component { unescapeSpaces( event.nativeEvent.text ) ); if ( - contentWithoutRootTag === this.value && + contentWithoutRootTag === this.value.toString() && realStart === this.selectionStart && realEnd === this.selectionEnd ) { @@ -759,7 +759,7 @@ export class RichText extends Component { typeof nextProps.value !== 'undefined' && typeof this.props.value !== 'undefined' && ( ! this.comesFromAztec || ! this.firedAfterTextChanged ) && - nextProps.value !== this.props.value + nextProps.value.toString() !== this.props.value.toString() ) { // Gutenberg seems to try to mirror the caret state even on events that only change the content so, // let's force caret update if state has selection set. @@ -833,7 +833,7 @@ export class RichText extends Component { const { style, tagName } = this.props; const { currentFontSize } = this.state; - if ( this.props.value !== this.value ) { + if ( this.props.value.toString() !== this.value.toString() ) { this.value = this.props.value; } const { __unstableIsSelected: prevIsSelected } = prevProps; @@ -851,7 +851,7 @@ export class RichText extends Component { // Since this is happening when merging blocks, the selection should be at the last character position. // As a fallback the internal selectionEnd value is used. const lastCharacterPosition = - this.value?.length ?? this.selectionEnd; + this.value?.toString().length ?? this.selectionEnd; this._editor.focus(); this.props.onSelectionChange( lastCharacterPosition, @@ -893,7 +893,8 @@ export class RichText extends Component { // On android if content is empty we need to send no content or else the placeholder will not show. if ( ! this.isIOS && - ( value === '' || value === EMPTY_PARAGRAPH_TAGS ) + ( value.toString() === '' || + value.toString() === EMPTY_PARAGRAPH_TAGS ) ) { return ''; } From 3e6be64924c67f253f5a81f7cec4e51b1d8c64e5 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Thu, 14 Dec 2023 08:47:14 -0500 Subject: [PATCH 3/3] test: Verify change events with equal values do not update attributes Ensure empty string values do not cause unnecessary attribute updates when comparing string values to empty `RichTextData` values, which is the new default value. --- .../rich-text/native/test/index.native.js | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/packages/block-editor/src/components/rich-text/native/test/index.native.js b/packages/block-editor/src/components/rich-text/native/test/index.native.js index 64bfb3b183c6b9..6e7dc31fc74e79 100644 --- a/packages/block-editor/src/components/rich-text/native/test/index.native.js +++ b/packages/block-editor/src/components/rich-text/native/test/index.native.js @@ -2,13 +2,18 @@ * External dependencies */ import { Dimensions } from 'react-native'; -import { getEditorHtml, render, initializeEditor } from 'test/helpers'; +import { + fireEvent, + getEditorHtml, + render, + initializeEditor, +} from 'test/helpers'; /** * WordPress dependencies */ import { select } from '@wordpress/data'; -import { store as richTextStore } from '@wordpress/rich-text'; +import { store as richTextStore, RichTextData } from '@wordpress/rich-text'; import { coreBlocks } from '@wordpress/block-library'; import { getBlockTypes, @@ -78,6 +83,26 @@ describe( '', () => { } ); } ); + describe( 'when changes arrive from Aztec', () => { + it( 'should avoid updating attributes when values are equal', async () => { + const handleChange = jest.fn(); + const defaultEmptyValue = new RichTextData(); + const screen = render( + + ); + + // Simulate an empty string from Aztec + fireEvent( screen.getByLabelText( 'Text input. Empty' ), 'change', { + nativeEvent: { text: '' }, + } ); + + expect( handleChange ).not.toHaveBeenCalled(); + } ); + } ); + describe( 'Font Size', () => { it( 'should display rich text at the DEFAULT font size.', () => { // Arrange.