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 prosemirror-noting bugs by passing storedMarks to inner editor, and passing up paste meta #409

Merged
merged 17 commits into from
Jun 10, 2024

Conversation

rhystmills
Copy link
Contributor

@rhystmills rhystmills commented Jun 7, 2024

What does this change?

We have encountered a few bugs related to the interaction of nested fields with notes from @guardian/prosemirror-noting - specifically:

  1. Typing in the first or final character of a note doesn't extend it
  2. Pasting into a note doesn't apply the note mark to the pasted characters
  3. Moving over the note boundary via key presses skips a character

This PR fixes these issues by making some changes to the ProsemirrorFieldView:

  1. Passes in storedMarks defined by the outer editor to the inner editor, applying them on the next transaction in the inner editor. The note extension functionality is managed by storedMarks, rather than by setting an inclusive attribute on the mark, as we see with something like strong or em marks. Until now, we haven't passed stored marks from the outer editor to our fields.
  2. Swaps the order in which we update the inner and outer editor in dispatchTransaction - we now update the inner editor first, then update the outer editor.

The existing order of operations here was leading to some issues with my storedMarks work.

When a user moved their cursor in a note within a text field: the inner editor dispatched a transaction, the outer editor (which has the copy of the prosemirror-noting plugin, and therefore 'knows' about the note) would then introduce a stored mark representing the note mark, which was passed into the plugin. The inner state change would then finish, with no knowledge of the intermediate transaction from the outer editor that had occurred, effectively ignoring the introduced stored mark.

Now, the inner transaction is dispatched, then the outer editor transaction is dispatched, including the stored mark, and it can be applied on the next transaction (e.g. a typed character).

This change also fixed the 'moving over a note boundary' issue.

  1. Passes 'paste' meta to the outer editor. prosemirror-noting uses this to identify transactions that are pastes within a note, and apply the note mark to them.

Noting without these changes:
Kapture 2024-06-07 at 12 34 05

Noting with the change:
Kapture 2024-06-07 at 12 37 45

How to test

Run the tests, I've added some.
You may want to test in Composer via yalc to test the noting behaviour specifically.

@rhystmills rhystmills requested a review from a team as a code owner June 7, 2024 10:16
…n tests

It's not clear why this is happening, and the behaviour of the editor in normal use appears to be correct (e.g. click 'bold' in field, focus field, begin typing works as expected)
Copy link
Contributor

@jonathonherbert jonathonherbert left a comment

Choose a reason for hiding this comment

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

This looks great. The tests are really clear.

There are a few non-blocking comments. Lots of value here so keen to let you 🚢 at a cadence which makes sense for stakeholders!

Also non-blocking: ideally each fix would belong in its own PR, which would making managing reverting easier if there were problems downstream for those that don't know the code so well.

});

const getEditorWithTextField = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to DRY up this repetition! 👍

src/plugin/fieldViews/__tests__/TextFieldView.spec.ts Outdated Show resolved Hide resolved
src/plugin/plugin.ts Outdated Show resolved Hide resolved
src/plugin/fieldViews/ProseMirrorFieldView.ts Outdated Show resolved Hide resolved
src/plugin/plugin.ts Show resolved Hide resolved
rhystmills and others added 5 commits June 10, 2024 12:53
Co-authored-by: Jonathon Herbert <jonathon.herbert@guardian.co.uk>
Co-authored-by: Jonathon Herbert <jonathon.herbert@guardian.co.uk>
@rhystmills
Copy link
Contributor Author

Also non-blocking: ideally each fix would belong in its own PR, which would making managing reverting easier if there were problems downstream for those that don't know the code so well.

I will bear the separation of fixes in mind next time - would be keen to get this one out though as it's affecting users of the formats in production at the moment!

@rhystmills rhystmills merged commit 679d53c into main Jun 10, 2024
3 checks passed
@rhystmills rhystmills deleted the rm/noting-extension-bug branch June 10, 2024 16: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

2 participants