prosemirror-elements
prosemirror-elements copied to clipboard
Fix decorations bug, with inline and widget decorations supported in all text fields
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 lineWidget
decorations on either side. This makes the feedback loop for manually testingWidget
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 therepeater
, alphabetically - An element or
repeater
couldn't contain more than onenestedElement
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:
- 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.
- 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. - 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:
Example Mini Profile, with the workaround removed, working with inline and widget decorations in many contexts: