Skip to content

Commit

Permalink
fix: Prevent unnecessary content changes clearing redo actions (#57028)
Browse files Browse the repository at this point in the history
* 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.

* 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.

* 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.
  • Loading branch information
dcalhoun authored and artemiomorales committed Jan 4, 2024
1 parent d4eebba commit e1e0f16
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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 );
}
Expand All @@ -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 );
Expand Down Expand Up @@ -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
) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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 '';
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -78,6 +83,26 @@ describe( '<RichText/>', () => {
} );
} );

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(
<RichText
onChange={ handleChange }
value={ defaultEmptyValue }
/>
);

// 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.
Expand Down

0 comments on commit e1e0f16

Please sign in to comment.