prosemirror-elements icon indicating copy to clipboard operation
prosemirror-elements copied to clipboard

Fix decorations bug, with inline and widget decorations supported in all text fields

Open rhystmills opened this issue 6 months ago • 0 comments

What does this change?

Since the introduction of the nestedElement field, we have noticed various problems related to decorations. This PR:

  • Aims to solve the decorations problems once and for all. The core fix is in 'ProseMirrorFieldView.ts'. I've removed some now unnecessary code related to the previous attempted fix from 'field.ts'.

  • I've added some tests, building on tests from @jonathonherbert in https://github.com/guardian/prosemirror-elements/pull/403. The new element specs and forms ('NestedElement' and 'RepeaterElement') and helpers in 'editor.ts' and 'test.ts' are all related to testing.

  • I've added a plugin for testing Widget decorations in the demo - any time 'widget' appears in text, it will be marked up with two red line Widget decorations on either side. This makes the feedback loop for manually testing Widget decorations a bit faster. It's also used in some of the new Cypress integration tests.

(Note - I came across an issue with Typescript being unable to have union types with 25 or more items in them, affecting compilation when we had more than 24 elements in the demo elements list. This already could do with a trim - using a handful of elements that demonstrate library features, rather than replicating everything we have in Composer. I've removed the witness element for now, but we should do a general tidy up. If we do need more than 24, we'll need a different approach for handling the types).

Background

The most obvious errors occurred when adding elements to nestedElement fields within repeater fields - usually out of bounds errors, crashing the editor and forcing a page reload and preventing the saving of content.

Inline decorations (e.g. our Typerighter matches) have generally worked better in these contexts than Widget decorations (like our prosemirror-noting start/end decorations and placeholder decorations).

There have been a few attempts to fix this problem (e.g. https://github.com/guardian/prosemirror-elements/pull/332), but we hadn't quite got to the bottom of it - so have introduced workarounds in certain places and delayed affected features like a main media field in the Timelines element. The most recent fix seemed promising, but it seems to break decorations for repeater nodes containing standard flat text fields, and we found a few cases it didn't fix:

  • nestedElement fields would only work if they were the first field in the repeater, alphabetically
  • An element or repeater couldn't contain more than one nestedElement field
  • Repeaters containing nestedElement fields, within another repeater

I'm quite confident that this PR will fix decorations in these contexts.

I think the problems with decorations were not specifically caused by repeaters or nested elements, but our existing decoration code was not really capable of handling all decorations properly in those contexts. The addition of those new field types exposed more complicated structures for us to apply decorations to, which exposed some of the problems with our existing approach.

We've not really had to handle Widget decorations within elements containing content until the introduction of the nestedElement field, at which point external placeholders and notes were being added within the context of a prosemirror-elements field - both of which use Widget decorations rather than inline decorations. Adding to this, most fields in most elements have been 'flat text' (containing text without wrapping p blocks, sometimes with br tags for line breaks) - presenting quite a simple structure to apply decorations to.

I also suspect we are passed DecorationGroup more often than before the introduction of those fields.

How does this PR fix our problem

I've fixed the problems with decorations by rewriting applyDecorationsFromOuterEditor in 'ProseMirrorFieldView'.

Handling DecorationGroup and DecorationSet

We sometimes receive a DecorationGroup from the outer editor, rather than a DecorationSet.

The type and class for DecorationGroup are not made available by prosemirror-view, so I've had to add a type declaration for that class. It would be good to make a change upstream so we can import that from the source.

The code now explicitly handles these two options. DecorationGroup contains a collection of DecorationSet in its members property, so we can handle them fairly similarly.

Mapping decorations from the outer editor (the document) to the inner editor (the field)

This happens in getMappedDecorationsFromSet and I've added quite detailed comments to explain what I do there.

In plain text:

  1. I find the relevant decorations in the context of the outer editor (the document containing the prosemirror-elements plugin) within the range of the field in that editor.
  2. I create a new set of decorations, again in the context of the original document. I've done these two steps because mapping the decorations may fail (often silently, without an error) if any of the decorations will fall outside of the valid range during the mapping operation. If so, it gives us a DecorationSet where Widget and Node decorations won't be handled properly, though inline decorations might work.
  3. I 'map' the decoration set with an offset so that they match the positions in the inner editor (which starts at 0 from the inner editor's perspective).

DecorationSet is a custom data structure containing a collection of Decoration. The key thing is - when you create one, it needs to be given a document to which the decorations can be logically applied. Inline decorations seem to be quite forgiving - the editor will try to apply them as long as they have relevant positions, but Widget and Node decorations need to map up properly with nodes in the document.

How to test

With the demo application running (yarn start), run the integration tests - (test:integration). There are some new ones that reflect inline and widget decorations in different contexts.

You may also want to run the demo app and test decorations manually. Insert different element types, and type "deco" and "widget" to test inline and widget decorations in different contexts. Do they work everywhere? (They should.)

You may also want to test the plugin in Composer using yalc. If you're feeling especially diligent, try renaming the Mini Profiles' elements miniBio back to bio - and removing the use of its transformer (reverting the additions in this PR). This used to fail due to the decorations bug.

Images

Example in the context of the demo app, with inline and widget decorations in many contexts:

image

Example Mini Profile, with the workaround removed, working with inline and widget decorations in many contexts:

image

rhystmills avatar Aug 14 '24 15:08 rhystmills