Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix recently introduce bug affecting elements #406

Merged
merged 5 commits into from
May 31, 2024
Merged

Conversation

rhystmills
Copy link
Contributor

@rhystmills rhystmills commented May 30, 2024

Background

Developers and users have noticed some unusual behaviour in elements (in Composer) recently:

  • Attempting to switch the position of two elements of the same type did not appear to update their text fields, so the elements didn't appear to switch.
  • Certain changes to text fields in image elements appeared garbled/disordered in preview after save and close
  • Saving and closing then reopening list formats with an image element in them caused placeholders not to appear, and certain changes weren't being saved

Through testing we identified that version 9.6.1 (the most recent release of prosemirror-elements) introduced the change. The biggest and most relevant change in this release was https://github.com/guardian/prosemirror-elements/pulls?q=is%3Apr+sort%3Aupdated-desc+is%3Aclosed

That change was reverted in Composer and the issues seem to be resolved there now: https://github.com/guardian/flexible-content/pull/4803

What does this PR change

This PR is intended to solve the bugs reported above.

The bug was caused by a subtle change in our logic during the refactor in the selection PR.

Previously, we would check for a 'diff' between the previous state of the node represented by a text view, and its updated state, as follows:

if (diffStart === null) {
      return this.maybeRerenderDecorations();
    }

I had changed this check to:

    if (diffStart && diffEnd) {

However, a diff could often start at position 0 relative to the innerEditorView represented by the text field, e.g. if the entire text field had changed, as part of wider changes to an entire element, or with a change only affecting the initial character.

Changing this check to if (diffStart !== null && diffEnd) seems to have resolved the issues mentioned above when testing locally.

Tests

I've updated the existing tests to add some more clarity about when a node is passed in versus when a new selection is passed in.

I've also added some new tests that look at the internal state of the inner editor, to cover the case that occurred in a bug. Without this PR's fix, the following test fails.

should update its internal doc when a node with different content is passed in, with different content starting at the first position of the inner editor

Let me know if what's happening in these tests is unclear, I'm happy to pair.

How to test

  1. Run prosemirror-elements locally and run the tests.
  2. Release prosemirror-elements locally via yarn yalc and use in flexible-content locally with yalc add @guardian/prosemirror-elements then npm i in the composer directory.
  3. Do the aformentioned bugs occur? (They shouldn't)
  4. Creating a note by creating a text selection with your mouse, then hitting F10 - does the selection move to the end of the note so that subsequent characters are added after the note? (It should)

@rhystmills rhystmills marked this pull request as ready for review May 30, 2024 14:26
@rhystmills rhystmills requested a review from a team as a code owner May 30, 2024 14:26
@@ -197,7 +197,8 @@ export abstract class ProseMirrorFieldView extends FieldView<string> {
const diffStart = node.content.findDiffStart(state.doc.content);
const diffEnd = node.content.findDiffEnd(state.doc.content);

if (diffStart && diffEnd) {
// Check for null specifically, rather than falsiness, because a diff starting at pos 0 is a valid diff
if (diffStart !== null && diffEnd) {
Copy link
Member

Choose a reason for hiding this comment

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

Good spot! Might be nice to also check diffEnd is not null? Who knows, maybe at some point negative positions will be valid, and diffEnd could be zero but still greater than diffStart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, !== null does seem like what we really want to check for there.

Copy link
Contributor

@Georges-GNM Georges-GNM left a comment

Choose a reason for hiding this comment

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

Tested locally and all the desired behaviour seemed to be working 🥳

@rhystmills rhystmills merged commit d480886 into main May 31, 2024
3 checks passed
@rhystmills rhystmills deleted the rm/fix-element-bug branch May 31, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants