RTC: fix #77532, rich text offset mismatch with diff API#77658
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @danluu! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
c4128a8 to
cfcfe75
Compare
peterwilsoncc
left a comment
There was a problem hiding this comment.
Are you able to add docblocks for the items that are missing them.
|
@peterwilsoncc I will add the docs and handle WPCS issues. |
| currentVal, | ||
| newVal, | ||
| resolveRichTextCursorPosition( cursorPosition, cursorScope, newVal ) | ||
| ); |
There was a problem hiding this comment.
we’ve added a cursorScope parameter to mergeYMapValues() in this patch, but transformed the existing cursorPosition for mergeRichTextUpdate().
is there a reason to avoid including the cursor scope here as well? do calls to the mergeRichTextUpdate() often not carry cursor-scope relevance?
There was a problem hiding this comment.
Following up on this and one of your comments elsewhere, I tried adding types to this in 610e02e. That's pushed to this PR, although I'm not sure if that should be in this PR or in an independent PR (or if the change there really makes sense).
…ckages. The `WPBlockSelection` type is more complicated than it may seem at face value. This change expands the docblock information on the type and links together the currently-existing duplicates in other packages, which are duplicated to avoid creating circular dependencies. This is prepartory work for #77658, but stands on its own as a clarifying update.
…ckages. The `WPBlockSelection` type is more complicated than it may seem at face value. This change expands the docblock information on the type and links together the currently-existing duplicates in other packages, which are duplicated to avoid creating circular dependencies. This is prepartory work for #77658, but stands on its own as a clarifying update.
…ckages. The `WPBlockSelection` type is more complicated than it may seem at face value. This change expands the docblock information on the type and links together the currently-existing duplicates in other packages, which are duplicated to avoid creating circular dependencies. This is prepartory work for #77658, but stands on its own as a clarifying update.
610e02e to
f136c42
Compare
…ckages. The `WPBlockSelection` type is more complicated than it may seem at face value. This change expands the docblock information on the type and links together the currently-existing duplicates in other packages, which are duplicated to avoid creating circular dependencies. This is prepartory work for #77658, but stands on its own as a clarifying update.
…ckages. The `WPBlockSelection` type is more complicated than it may seem at face value. This change expands the docblock information on the type and links together the currently-existing duplicates in other packages, which are duplicated to avoid creating circular dependencies. This is prepartory work for #77658, but stands on its own as a clarifying update.
…ckages. (#77862) The `WPBlockSelection` type is more complicated than it may seem at face value. This change expands the docblock information on the type and links together the currently-existing duplicates in other packages, which are duplicated to avoid creating circular dependencies. This is prepartory work for #77658, but stands on its own as a clarifying update.
What?
Closes #77532.
See also #77862.
This is AI generated code from codex (5.4 and 5.5). Codex's description of what the fix does is:
[end AI generated text]
Note that the isDeltaVerificationMatch function isn't necessary for this fix. It's part of a proposed defense-in-depth solution that aims to prevent analogous bugs. Also note that the e2e repro with Playwright is not checked in here. My reasoning for this is:
My proposal here is to defer putting in the playwright tests for these issues until we have quite a few and then have an integration or end-to-end test that does things that would cover many bugs.
Testing Instructions
The repro instructions from #77532 also work here, except that the top-level playwright repro is not checked in for reasons noted above. That test can be run via the branch linked to in #77532, if desired.
Unit Repros
Run the lower-level RTC write-path repros:
npm run test:unit -- packages/core-data/src/utils/test/rtc-rich-text-offset-space.test.js
Run the higher-level useEntityBlockEditor().onInput() repro:
npm run test:unit -- packages/core-data/src/test/rtc-rich-text-offset-space.test.js
Use of AI Tools
As noted above, the original issue and this PR are AI generated. @dmsnell and @alecgeatches looked at the original issue and think that it's a real bug (thanks again for taking a look, all, and @alecgeatches also looked at the proposed fix.