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 Dec 14, 2023
1 parent a5a829c commit 0576c01
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

1 comment on commit 0576c01

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flaky tests detected in 0576c01.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7210118451
📝 Reported issues:

Please sign in to comment.