RichText: pass value to store #43204
Merged
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
|
Size Change: +972 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
try/pass-rich-text-value-to-store
branch
from
db12b70
to
54e3708
Compare
ellatrix
commented
Aug 13, 2022
ellatrix
commented
Aug 13, 2022
ellatrix
commented
Aug 13, 2022
ellatrix
force-pushed
the
try/pass-rich-text-value-to-store
branch
from
1be952f
to
62cdd52
Compare
ellatrix
requested review from
fluiddot,
ajitbohra,
ntwb and
nerrad
as code owners
ellatrix
force-pushed
the
try/pass-rich-text-value-to-store
branch
from
76135e8
to
edf956e
Compare
kevin940726
reviewed
May 3, 2023
|
Flaky tests detected in bce8a2d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7112599031
|
ellatrix
force-pushed
the
try/pass-rich-text-value-to-store
branch
from
27389f8
to
fc88c6e
Compare
ellatrix
commented
May 4, 2023
dmsnell
force-pushed
the
try/pass-rich-text-value-to-store
branch
2 times, most recently
from
540caa8
to
310d80f
Compare
ellatrix
force-pushed
the
try/pass-rich-text-value-to-store
branch
from
310d80f
to
1aab304
Compare
ellatrix
commented
Aug 11, 2023
ellatrix
force-pushed
the
try/pass-rich-text-value-to-store
branch
from
bff3746
to
1c09d7c
Compare
dmsnell
reviewed
Sep 15, 2023
dmsnell
reviewed
Sep 15, 2023
ellatrix
force-pushed
the
try/pass-rich-text-value-to-store
branch
from
15b4e94
to
14edd7a
Compare
ellatrix
force-pushed
the
try/pass-rich-text-value-to-store
branch
from
99600d3
to
bce8a2d
Compare
dmsnell
approved these changes
Dec 7, 2023
There was a problem hiding this comment.
This PR doesn't need a second birthday party. This is good timing since a release just went out, and I think every major item has been addressed, so it's good time to get this out the door and in testing.
Maybe we can remain paced at further changes so we have an extended period of time to catch any unforeseen bugs before it becomes harder to revert should we need.
I'm looking forward to this and even more to some later follow-up work with things like .toReactComponent() that can further bypass printing/parsing steps.
Well done, @ellatrix - this took a lot of persistence, a lot of preparation work in other PRs, and a lot of experimentation. I hope it goes well.
| @@ -6,5 +6,6 @@ | |||
| * @return {string} The value with anchor tags removed. | |||
| */ | |||
| export default function removeAnchorTag( value ) { | |||
| return value.replace( /<\/?a[^>]*>/g, '' ); | |||
| // To do: Refactor this to use rich text's removeFormat instead. | |||
There was a problem hiding this comment.
or at least a parsed version of the HTML! in another PR. This breaks as-is in trivial cases (e.g. <a title="love > hate">) but the breakage isn't introduced in this PR so we don't have to fix it here either
|
|
||
| if ( /[\n\t\r\f]/.test( newNodeValue ) ) { | ||
| newNodeValue = newNodeValue.replace( /[\n\t\r\f]+/g, ' ' ); | ||
| } |
There was a problem hiding this comment.
I don't think there's any reason to run this test before the replace, because the test has to scan the same content that the replace is going to do. if there are none of these characters then the test will still scan the entire string. so there are no cases where the test is going to save us work that the replace itself would be doing. we might as well always run the replace.
There was a problem hiding this comment.
I thought it might be slightly faster. Either way, most of the time there shouldn't be any of these characters present.
|
Thanks @dmsnell! Let's indeed merge now that it's still early. I'll be available to deal with any unforeseen consequences and do everything we can to avoid a revert. :) |
dcalhoun
added a commit
that referenced
this pull request
Dec 13, 2023
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.
dcalhoun
added a commit
that referenced
this pull request
Dec 14, 2023
* 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 PR seems to break the TOC block. Adding a TOC block and a heading block to the editor throws the below error.
|
Curious to see thoughts on this. While not a block, the native mobile rich text feature experienced similar issues that we addressed in #57028, but we are discovering more breakages that we need to address. What is the likelihood that third-party blocks break because of this change as well? Do we consider this change of |
|
I commented on this in #57093. We could add all the string methods for back compat if needed. However, it seems quite rare to be touching another block's attributes. Third party blocks by themselves can breaks because the value is opt in, it only happens when you use attributes from other blocks. I'm happy to add string methods though, it's not a big deal. |
11 tasks
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.
|
I discovered that this PR causes problems for blocks with |
This was referenced Mar 21, 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.
What?
Currently the RichText value is passed to the store as HTML, but it can be useful to have access to the rich text value.
Solution: pass an instance of RichTextValue, which inherits from String, but has additional properties.
Currently the RichText value is passed to the store as HTML. This PR creates a new
rich-texttype andrich-textmatcher so that rich text data can be stored directly in block attributes and the block editor store.Why?
Storing rich text data in the block attributes and the block editor store has many advantages.
blockspackages parses the blocks, it actually has access to the DOM tree. What we currently do is serialise that content back to HTML (withinnerHTML) and then therich-texthas to parse it again. Similarly on typing, the internal value needs to be constantly serialised before it can send the change to the store.START_OF_SELECTED_AREAto keep track of the selection when splitting and merging, because we can now simply set a property to the object.How?
With a new
rich-texttype and source added to the block attribute schema.Testing Instructions
Screenshots or screencast