fix: Prevent unnecessary content changes clearing redo actions #57028
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcalhoun
added
[Type] Regression
|
Size Change: -373 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
dcalhoun
commented
Dec 13, 2023
There was a problem hiding this comment.
The current proposed changes include two commits. The first addressees the identified regression. The second applies similar changes to other locations as a precaution. It may be helpful to review the two commits separately.
Noting that I debugged this issue by logging values and their types while interacting with rich text inputs. Sharing it in case it proves useful for others.
Logging Diff
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 116425a15b..c30b923b13 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
@@ -104,6 +104,15 @@ const EMPTY_PARAGRAPH_TAGS = '<p></p>';
const DEFAULT_FONT_SIZE = 16;
const MIN_LINE_HEIGHT = 1;
+function logWithType( label, value, ...rest ) {
+ if ( typeof value === 'string' ) {
+ console.log( '>>> STRING:', label, value, ...rest );
+ return;
+ }
+
+ console.log( '>>> OBJECT:', label, value, ...rest );
+}
+
export class RichText extends Component {
constructor( { value, selectionStart, selectionEnd } ) {
super( ...arguments );
@@ -159,6 +168,7 @@ export class RichText extends Component {
this.lastHistoryValue = value;
// Internal values that are update synchronously, unlike props.
+ logWithType( 'constructor', value );
this.value = value;
this.selectionStart = selectionStart;
this.selectionEnd = selectionEnd;
@@ -245,6 +255,7 @@ export class RichText extends Component {
} );
this.value = this.valueToFormat( record );
+ logWithType( 'onFormatChange', this.value );
this.props.onChange( this.value );
this.setState( { activeFormats } );
this.props.onSelectionChange( start, end );
@@ -265,6 +276,8 @@ export class RichText extends Component {
}
onCreateUndoLevel() {
+ logWithType( 'onCreateUndoLevel 1', this.lastHistoryValue );
+ logWithType( 'onCreateUndoLevel 2', this.value );
const { __unstableOnCreateUndoLevel: onCreateUndoLevel } = this.props;
// If the content is the same, no level needs to be created.
if ( this.lastHistoryValue.toString() === this.value.toString() ) {
@@ -320,6 +333,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.
+ logWithType( 'onChangeFromAztec', this.value );
if ( contentWithoutRootTag === this.value.toString() ) {
return;
}
@@ -336,6 +350,7 @@ export class RichText extends Component {
);
this.debounceCreateUndoLevel();
+ logWithType( 'onTextUpdate', this.value );
const refresh = this.value.toString() !== contentWithoutRootTag;
this.value = contentWithoutRootTag;
@@ -523,6 +538,7 @@ export class RichText extends Component {
},
} );
this.value = this.valueToFormat( linkedRecord );
+ logWithType( 'onPaste', this.value );
onChange( this.value );
// Allows us to ask for this information when we get a report.
@@ -590,6 +606,7 @@ export class RichText extends Component {
// and we did not just trigger a text update
// `onChange` could be the last event and could have been triggered a long time ago so
// this approach is not perfectly reliable.
+ logWithType( 'onSelectionChange', this.value );
const isManual =
this.lastAztecEventType !== 'input' &&
this.props.value.toString() === this.value.toString();
@@ -661,6 +678,7 @@ export class RichText extends Component {
const contentWithoutRootTag = this.removeRootTagsProducedByAztec(
unescapeSpaces( event.nativeEvent.text )
);
+ logWithType( 'onSelectionChangeFromAztec', this.value );
if (
contentWithoutRootTag === this.value.toString() &&
realStart === this.selectionStart &&
@@ -833,6 +851,8 @@ export class RichText extends Component {
const { style, tagName } = this.props;
const { currentFontSize } = this.state;
+ logWithType( 'componentDidUpdate 1', this.value );
+ logWithType( 'componentDidUpdate 2', this.props.value );
if ( this.props.value.toString() !== this.value.toString() ) {
this.value = this.props.value;
}
@@ -850,6 +870,7 @@ export class RichText extends Component {
} else if ( this.shouldFocusTextInputAfterMerge( prevProps ) ) {
// 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.
+ logWithType( 'shouldFocusTextInputAfterMerge', this.value );
const lastCharacterPosition =
this.value?.toString().length ?? this.selectionEnd;
this._editor.focus();| @@ -336,7 +336,7 @@ export class RichText extends Component { | |||
| ); | |||
|
|
|||
| this.debounceCreateUndoLevel(); | |||
| const refresh = this.value !== contentWithoutRootTag; | |||
There was a problem hiding this comment.
This is the specific line resulting in incorrectly clearing pending redo actions. Comparing a RichTextData instance (this.value) against a string (contentWithoutRootTag) never resulted in equality. Making the situation more complex is the fact that iOS and Android Aztec implementations behave differently when it comes to blur and selection events, which means this nuanced scenario only occurred on iOS.
The rest of the added toString invocations separate from this line in the proposed changes are out of caution, not to address an explicitly observed issue. Transparently, it is unclear to me currently when/where RichTextData should be leverage and where Aztec's string-value-based communication might disrupt that by passing back a string. It could be that a better solution is to always leverage RichTextData values in JavaScript and convert values received from Aztec from string to RichTextData.
There was a problem hiding this comment.
To be clear, the use of toString is fairly innocuous as both String and RichTextData provide the method. So, regardless of which type is present, the method and comparison should succeed.
If nothing else, we could use the proposed changes as a stopgap solution until we fully integrate the RichTextData type.
There was a problem hiding this comment.
[...] It could be that a better solution is to always leverage
RichTextDatavalues in JavaScript and convert values received from Aztec from string toRichTextData.
I think this would be the best option for consistency. This way we'll ensure that RichTextData is the expected value of the RichText component when used anywhere.
In any case, I think there will still be some cases where we need to convert the value to a String. As an example, I noticed a couple of examples in RichText: pass value to store:
- https://github.com/WordPress/gutenberg/pull/43204/files#diff-057edfb18ec4f5bb2974a1f27e6ed3afd0f29730077c819f3b661d70f32a8cb4R10
- https://github.com/WordPress/gutenberg/pull/43204/files#diff-8ba153e0dccbf25a179a794cb63aacd09e575d5dcfa819f8aefa33086ddbbb6eR35
To be clear, the use of
toStringis fairly innocuous as bothStringandRichTextDataprovide the method. So, regardless of which type is present, the method and comparison should succeed.
For reference, this is the implementation of RichTextData.toString:
gutenberg/packages/rich-text/src/create.js
Lines 152 to 163 in 5cfdf66
dcalhoun
added
the
Mobile App - Automation
|
Flaky tests detected in 3e6be64. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7209787760
|
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.
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.
dcalhoun
force-pushed
the
fix/prevent-unnecessary-content-changes-clearing-redo-actions
branch
from
d740439
to
5cfdf66
Compare
fluiddot
approved these changes
Dec 14, 2023
There was a problem hiding this comment.
LGTM 🎊 ! Thanks @dcalhoun for addressing this issue 🏅 !
I completed both the testing instructions and the writing flow test suite.
As you mentioned in this comment, I agree it would be great to update the RichText component to ensure that its value's type is always a RichTextData type.
| @@ -336,7 +336,7 @@ export class RichText extends Component { | |||
| ); | |||
|
|
|||
| this.debounceCreateUndoLevel(); | |||
| const refresh = this.value !== contentWithoutRootTag; | |||
There was a problem hiding this comment.
[...] It could be that a better solution is to always leverage
RichTextDatavalues in JavaScript and convert values received from Aztec from string toRichTextData.
I think this would be the best option for consistency. This way we'll ensure that RichTextData is the expected value of the RichText component when used anywhere.
In any case, I think there will still be some cases where we need to convert the value to a String. As an example, I noticed a couple of examples in RichText: pass value to store:
- https://github.com/WordPress/gutenberg/pull/43204/files#diff-057edfb18ec4f5bb2974a1f27e6ed3afd0f29730077c819f3b661d70f32a8cb4R10
- https://github.com/WordPress/gutenberg/pull/43204/files#diff-8ba153e0dccbf25a179a794cb63aacd09e575d5dcfa819f8aefa33086ddbbb6eR35
To be clear, the use of
toStringis fairly innocuous as bothStringandRichTextDataprovide the method. So, regardless of which type is present, the method and comparison should succeed.
For reference, this is the implementation of RichTextData.toString:
gutenberg/packages/rich-text/src/create.js
Lines 152 to 163 in 5cfdf66
Ensure empty string values do not cause unnecessary attribute updates when comparing string values to empty `RichTextData` values, which is the new default value.
dcalhoun
deleted the
fix/prevent-unnecessary-content-changes-clearing-redo-actions
branch
This was referenced Dec 14, 2023
Merged
artemiomorales
pushed a commit
that referenced
this pull request
Jan 4, 2024
* 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.
This was referenced Jan 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related
What?
Prevent unnecessary content changes clearing redo actions.
Why?
Redo actions were lost whenever a block leveraging rich text blurred, which only
occurred on iOS due to nuance behavioral differences between Aztec
implementations.
How?
fix: Prevent unnecessary content changes clearing redo actions
In this context,
this.valueis not a string but a instance ofRichTextData. Therefore, comparing the two values results inunexpected inequality, triggering an update of the block's
attributes.contenttoggling it from aReactTextDatainstance to astring. This toggle results in the undo manager tracking the change as
a new line of editor history, clearing out any pending redo actions.
The
RichTextDatatype was introduced in #43204. InvokingtoStringmay not be the best long-term solution to this problem. Refactoring the
rich text implementation to appropriately leverage
RichTextDataand(potentially) treat
valueandrecordvalues different and storingthem separately may be necessary.
fix: Convert
RichTextDatato string before comparing valuesThe value stored in the rich text component may be a string or a
RichTextData. Until the value is store consistently, it may benecessary to convert each value to a string prior to equality
comparisons.
Testing Instructions
Prototype builds are available for testing on the sibling WordPress-iOS and WordPress-Android PRs listed above.
The foundational nature of the changes to rich text likely warrant heavily testing the writing flow. In addition to general writing flow regression testing, one specific test case is as follows:
Testing Instructions for Keyboard
N/A, no interaction changes included.
Screenshots or screencast
N/A, no visual changes included.