[RNMobile] Fix losing undo/redo history when using non-breaking space HTML entity #57652
Conversation
The non-breaking space HTML entity is part of the RichText content, so we shouldn't remove it to preserve the original value, especially when pasting HTML. Otherwise, the removal leads to breaking the undo/redo history.
fluiddot
added
[Type] Bug
|
Size Change: +747 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
fluiddot
added
the
[Feature] Writing Flow
There was a problem hiding this comment.
I verified the test cases passed on an iPhone simulator.
I also attempted to recreate the original issue described in #12249 (comment), which appears to be the origin of unescapeSpaces. Did you happen to test that as well?
As observed in the web version of the editor, this HTML entity should be preserved as it's part of the content.
Will you elaborate on this or provide the observed example? I struggled to follow this as the rationale or validation for removing unescapeSpaces.
| @@ -83,11 +88,11 @@ describe( '<RichText/>', () => { | |||
| } ); | |||
| } ); | |||
|
|
|||
| describe( 'when changes arrive from Aztec', () => { | |||
There was a problem hiding this comment.
It is fine if you'd like to change this formatting, but, for my own education, may I ask the rationale behind dropping the "when..." format?
There was a problem hiding this comment.
I renamed it mainly to make the section more open to other test cases like the one added in the PR. The formatting change was mainly driven by using the same we have in the other describe section:
In any case, I don't have a strong opinion on the formatting. We could still use when..., but if prefer this option, it would be great to make it consistent across the file.
There was a problem hiding this comment.
Thank you for sharing. In short, I support you if you would like to change this format. Expanding the section, more specifically removing from Aztec, makes complete sense. From my perspective, that expanded group description is a logical test case grouping to avoid repetition.
My longer response, if you are interested... 😄
I am all for consistency. That said, at this time, I would argue that we do not have a consistent approach established. For this specific example, even Font Size and Value changes differ on casing. Admittedly, casing is not an important thing by any means, but I figure it is relevant nonetheless.
My original goal with my inquiry was to avoid a situation where we (contributors) swap code back and forth. Without automated enforcement via a linter, it is difficult to ensure consistency. There are examples of the "when" formatting in the code base as well. That is not to say we must adhere to that, but, again, I inquired to understand the rationale for modifying this one location.
For context, I was taught the "when" pattern as an approach to make test output read as a sentence. I.e., [subject] when [context] should [expectation]. Again, not advocating that we adhere to this format, merely sharing my rationale.
Thank you for considering my perspective. 🙇🏻
There was a problem hiding this comment.
Thanks @dcalhoun for elaborating on your response 🙇 !
I am all for consistency. That said, at this time, I would argue that we do not have a consistent approach established. For this specific example, even
Font SizeandValue changesdiffer on casing. Admittedly, casing is not an important thing by any means, but I figure it is relevant nonetheless.
Ah, good point. I'll update the description to match the casing.
My original goal with my inquiry was to avoid a situation where we (contributors) swap code back and forth. Without automated enforcement via a linter, it is difficult to ensure consistency. There are [examples of the "when" formatting](https://github.com/search?q=repo%3AWordPress%2Fgutenberg%20describe(%20%27when&type=code) in the code base as well. That is not to say we must adhere to that, but, again, I inquired to understand the rationale for modifying this one location.
For context, I was taught the "when" pattern as an approach to make test output read as a sentence. I.e.,
[subject] when [context] should [expectation]. Again, not advocating that we adhere to this format, merely sharing my rationale.
Yep, we don't have a clear guideline about the describe section. I checked the Gutenberg documentation about testing but there's no explicit reference to the describe statement, apart from the following:
Use a describe block to group test cases.
Using when, although a bit more verbose, I agree it's a format more aligned with the description in test cases. I'll update it for this file. Thanks 🙇 !
There was a problem hiding this comment.
Using
when, although a bit more verbose, I agree it's a format more aligned with the description in test cases. I'll update it for this file. Thanks 🙇 !
I applied this suggestion in 870a99a.
Oh, good catch. I found the commit where the change was introduced but overlooked that comment. Thank you very much @dcalhoun for sharing it 🙇 . I've tested the case shared here and I couldn't reproduce it in both Android and iOS platforms. The issue was spotted a long time ago, so it's likely that was fixed at some point without needing that change. That said, I'll add this case to the Testing Instructions section for reference.
Sure! Let me compile a couple of examples to elaborate on how the web version behaves when using a non-breaking space HTML entity. |
@dcalhoun I updated the PR's description to showcase how the non-breaking space is handled in the cases referenced in the Testing Instructions. On the other hand, I noticed a difference in the behavior when editing the text (see attached video captures). In the web version, the non-breaking space HTML entity is automatically transformed when the value of the
|
There was a problem hiding this comment.
We could replicate this [transforming the non-breaking space on value change] in the app version, although I'd suggest tackling it in a separate PR if needed. WDYT?
I agree. We should address this separate issue in a different PR.
I retested the changes. Let's merge! 🚀
What?
Fixes an issue related to losing undo/redo history.
Why?
Fixes #36527. Also fixes the case described in #36527 (comment).
How?
In d2b241b we introduced a function to unescape the non-breaking space HTML entity from the
RichTextvalue upon different events like content and selection change. As observed in the web version of the editor, this HTML entity should be preserved as it's part of the content. Hence, this PR simply removes theunescapeSpacesfunction.Testing Instructions
Undo/redo history is preserved
Non-breaking space HTML entity is preserved
entities are no longer included in the content.The issue that originally triggered the changes can't be reproduced
Testing Instructions for Keyboard
N/A
Screenshots or screencast
desktop-non-breaking-space.mp4
ios-non-breaking-space.mp4