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 annotations text tracking #11861

Merged
merged 8 commits into from
Nov 19, 2018
Merged

Conversation

atimmer
Copy link
Member

@atimmer atimmer commented Nov 14, 2018

Description

This makes it possible to have an editor-only format that stays in
position when typing inside RichText.

Apply this to annotations, because it is necessary there.

Before this PR the annotations API worked via the functional prepareEditableTree. This works technically, but fails functionally. The rendering inside the TinyMCE instance is kept in sync with the annotations state. So when an annotation is from position 50 to position 100 the annotation will stay in those positions, this makes no sense from a user perspective. Instead what you want is that the annotation behaves like other formatting. It flows with edits of the text.

This morning I tried a first iteration by watching for edits to the RichText value and updating the annotation state accordingly, moving positions based on what has changed in the content. This was very unreliable and probably hard to make performant. While with this PR this tracking comes for free, because the DOM does it for us.

I'm thinking that I can iterate on this by having the annotations state be updated based on the formats the annotations module sees when running valueToFormat. Because that is the actual way that the annotation has moved in the text.

Note: With a fully functional paradigm I would update the annotations as edits from the user come in via events, but that's not feasible in the current RichText paradigm

How has this been tested?

With the Yoast SEO 9.2-RC2 plugin and with the annotations-tester plugin.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@ellatrix
Copy link
Member

What are you trying to achieve? I think this is already possible?

@atimmer
Copy link
Member Author

atimmer commented Nov 14, 2018

@iseulde I've updated the description with more context.

@ellatrix
Copy link
Member

With this example, the format stays in its place? Do you need a way to update the positions?

wp.richText.registerFormatType( 'plugin/annotation', {
	title: 'invisible',
	tagName: 'mark',
	className: 'annotation',
	__experimentalGetPropsForEditableTreePreparation( select ) {
		return {
			isEnabled: select( 'core/edit-post' ).getActiveGeneralSidebarName() === 'edit-post/block',
		};
	},
	__experimentalCreatePrepareEditableTree( props ) {
		return ( formats, text ) => {
			if ( ! props.isEnabled ) {
				return formats;
			}

			const search = 'Gutenberg';
			const index = text.indexOf( search );

			if ( index === -1 ) {
				return formats;
			}

			const start = index;
			const end = index + search.length;

			const newValue = wp.richText.applyFormat( { text, formats }, { type: 'plugin/annotation' }, start, end );

			return newValue.formats;
		};
	},
} );

return record.formats;
value = applyAnnotations( value, annotations );

return value;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need the ability to replace text?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I do, it's was just easier in rich-text to pass the whole value. I can easily change it to only allow changing the formats like prepareEditableTree.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I should have commented this. We don't want to allow prepareEditableTree to change anything other than the formats. See #11595 (comment)

@ellatrix
Copy link
Member

I would love a more elaborate description of what's happening in this PR.

This makes it possible to have an editor-only format that stays in position when typing inside RichText.

The above test code proves that this is already possible.

@@ -801,29 +804,87 @@ export class RichText extends Component {
return false;
}

// Allow primitives and arrays:
if ( ! isPlainObject( this.props[ name ] ) ) {
return this.props[ name ] !== prevProps[ name ];
Copy link
Member

Choose a reason for hiding this comment

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

Why are we comparing non format_ props?

Copy link
Member Author

@atimmer atimmer Nov 14, 2018

Choose a reason for hiding this comment

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

At this point in the code, it is guaranteed that name starts with format_.

Copy link
Member

Choose a reason for hiding this comment

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

I think this needs more commenting. I don't understand what's going on.

@atimmer
Copy link
Member Author

atimmer commented Nov 14, 2018

I would love a more elaborate description of what's happening in this PR.

This makes it possible to have an editor-only format that stays in position when typing inside RichText.

The above test code proves that this is already possible.

Pardon my inability to communicate this in text.

What I want is:

Before

Raw text:

Piece of text.

Annotated (classes left out for brevity):

Piece of <mark>text</mark>

Annotation state:

{
 // ..
    range: {
        start: 9,
        end: 13,
    }
}

After

Raw text:

Piece of texxt

Annotated:

Piece of <mark>texxt</mark>

Annotation state:

{
 // ..
    range: {
        start: 9,
        end: 14,
    }
}

Given the fact that the annotation state only has positions to work with. I cannot use the method in the __experimentalCreatePrepareEditableTree, because I cannot search for a known string.

Note: I do understand that for marking specific strings the technique provided in your invisible example would work. An annotation doesn't mark a specific string though, it references a selection of text, that can grow or shrink or move.

return {
annotations: select( 'core/annotations' ).__experimentalGetAnnotationsForRichText( blockClientId, richTextIdentifier ),
};
return select( 'core/annotations' ).__experimentalGetAnnotationsForRichText( blockClientId, richTextIdentifier );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this wrong? the method is called getProps I expected it to return an object here?

Copy link
Member Author

@atimmer atimmer Nov 14, 2018

Choose a reason for hiding this comment

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

The thing I tried to solve here was not re-rendering unless necessary. select( 'core/annotations' ).__experimentalGetAnnotationsForRichText( blockClientId, richTextIdentifier ) is cached based on the annotations for a block, so it will only re-render if a different annotation is added.

With:

return {
	annotations: select( 'core/annotations' ).__experimentalGetAnnotationsForRichText( blockClientId, richTextIdentifier ),
};

it would create a new object every time the function was called, so it would trigger a re-render. And I didn't want to introduce another memoized location, when a simpler solution was available.

Copy link
Contributor

Choose a reason for hiding this comment

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

My hope/thoughts, was that withSelect already checks the props (using shallow equality) and we can probably use the same mechanism to do the same.

@youknowriad
Copy link
Contributor

youknowriad commented Nov 14, 2018

So if I understand properly:

__experimentalCreatePrepareEditableTree

was solving the Formats => RichText (link)

but we need an onChange to solve the opposite direction since the RichText can edit the formats, and this is what __experimentalCreateValueToFormat is about right?


I think if we assume that the RichText component is capable of editing the format ranges. (which it is obviously doing since we have "boundaries" support), it's legitimate to expect the format to receive the changes (if it wants to do something with it).

Still not sure about the implementation details, I need to think a little bit more but I agree with the issue we're trying to solve being legitimate.

@atimmer
Copy link
Member Author

atimmer commented Nov 14, 2018

but we need an onChange somewhere and this is what __experimentalCreateValueToFormat is about right?

Yes, that's currently not in this PR. But my thinking is that I can update the annotation state based on the value in __experimentalCreateValueToFormat.

I think if we assume that the RichText component is capable of editing the format ranges. (which it is obviously doing since we have "boundaries" support), it's legitimate to expect the format to receive the changes (if it wants to do something with it).

That would work too.

Still not sure about the implementation details, I need to think a little bit more but I agree with the issue we're trying to solve being legitimate.

I wanted to at least put something up that works. I'm open to better solutions.

@ellatrix
Copy link
Member

So what you currently have is: annotations state => value formats.
What you want as well is: value formats => annotations state.

So if we call same function with the value formats whenever they change, you can derive new annotation state from that? This probably requires you to pass some id to the format so you can identify it later on.

I'm not sure how this PR does that. The lack of test doesn't make it clearer.

@youknowriad
Copy link
Contributor

What about this API?

const format = {
   prepareEditableValue: select => (value, context) => { return formats; }
    onChangeEditableValue: dispatch => (formats, context) => { // do something }
}

I feel this is simple enough to be understood properly it assumes we select props when rendering and we dispatch things onChange.

The downside here is that we can't "optimize" prepareEditableValue in the Framework, it's left to the format author. Concretly, it means the format author would need to memoize the prepareEditableValue inner function it self to avoid generating new formats if the props (computed using the select argument) didn't change.

Thoughts?

@youknowriad
Copy link
Contributor

Alternatively, if we were to keep the separation between "receiving the props" and "providing the formats based on the props and the value" to be able to bundle the performance optimization into the framework, I'll do the same for the opposite direction:

const format = {
   getPropsForEditableTreePreparation => (select, context) => props,
   prepareEditableValue: (value, props, context) => { return formats; },
   
    getPropsForEditableTreeChangeHandler => (dispatch, context) => props,
    onChangeEditableValue: (formats, props, context) => { // do something }
}

@atimmer atimmer force-pushed the fix/annotations-text-tracking branch from 6b8344f to ea44b15 Compare November 15, 2018 12:54
@atimmer
Copy link
Member Author

atimmer commented Nov 15, 2018

So what you currently have is: annotations state => value formats.
What you want as well is: value formats => annotations state.

Yes.

So if we call same function with the value formats whenever they change, you can derive new annotation state from that? This probably requires you to pass some id to the format so you can identify it later on.

Yes, I've implemented this in the latest commit: ea44b15.

I'm not sure how this PR does that. The lack of test doesn't make it clearer.

It didn't yet update the annotations state because I hadn't gotten to that yet. In the last commit I've implemented that part.

@atimmer
Copy link
Member Author

atimmer commented Nov 15, 2018

The downside here is that we can't "optimize" prepareEditableValue in the Framework, it's left to the format author. Concretly, it means the format author would need to memoize the prepareEditableValue inner function it self to avoid generating new formats if the props (computed using the select argument) didn't change.

Thoughts?

I think we cannot leave this hole open. If all RichText instances re-render on any change the performance is instantly completely broken.

const format = {
   getPropsForEditableTreePreparation => (select, context) => props,
   prepareEditableValue: (value, props, context) => { return formats; },
   
    getPropsForEditableTreeChangeHandler => (dispatch, context) => props,
    onChangeEditableValue: (formats, props, context) => { // do something }
}

What is the benefit of the getPropsForEditableTreeChangeHandler handler? Is there a huge downside if implementation call wp.dispatch( ... ) inside onChangeEditableValue?

@youknowriad The version I have now works completely, I will try to implement your proposed second solution to see if that works. I don't know yet how I am going to work around/with https://github.com/WordPress/gutenberg/blob/master/packages/rich-text/src/create.js#L60, but I'll try to find a solution.

@youknowriad
Copy link
Contributor

What is the benefit of the getPropsForEditableTreeChangeHandler handler? Is there a huge downside if implementation call wp.dispatch( ... ) inside onChangeEditableValue?

I think we discussed this in a previous #core-js but I agree that it's not obvious :). Ideally we should never use wp.dispatch or wp.select (the globals) in Core Packages. We have an issue to move away from these singletons.

The bug that can happen here is that you'd rely on the wrong registry. For example if you have a Gutenberg editor inside another Gutenberg editor (which can happen for reusable blocks), you won't be calling the right registry if you do wp.dispatch since the registry will be overriden using a RegistryProvider.

@atimmer
Copy link
Member Author

atimmer commented Nov 16, 2018

@youknowriad I've implemented your suggestions in 44939f9. The downside of this approach compared to the formatToValue/valueToFormat approach is that this approach doesn't work in a few cases. For example, annotations are not preserved when pasting text inside a RichText. While that use-case did work in the solution I build in 02b3943 with ea44b15.

I think maybe that approach is simpler as well, because it's very clear what is happening:

Outside world         ------>         RichText state            ---->         TinyMCE.
                         ^
                    formatToValue

Outside world         <------         RichText state            <----         TinyMCE.
                         ^
                    valueToFormat

While the refactored approach works like this:

Outside world         ------>         RichText state            ---->         TinyMCE.
                                                                  ^
                                                          prepareEditableTree

Outside world         <------         RichText state            <----         TinyMCE.
                         ^                            ^
                removeEditorOnlyFormats      onChangeEditableValue???

My takeaway is: The second diagram shows we are trying to build a functional reactive program, but because our internals are not that (TinyMCE) it's awkward. The first diagram shows an embrace of the fact that TinyMCE needs a two way binding. Given we are moving towards making RichText fully functional reactive, the second one is a better API. But that does mean we need to solve the broken behavior (pasting text destroys annotations).

@youknowriad
Copy link
Contributor

In my mind it's slightly different (there's two outside words) and TinyMCE is not really important no matter the approach we choose.

Also, pasting remove the annotaitons => This doesn't seem like a bug for me. When you paste something, you are overwriting the whole value?

Here's how I see it.

untitled diagram 1

@ellatrix
Copy link
Member

I think onChangeEditableValue: (formats, props, context) => {} is closer to what we want. With prepareEditableValue you're deriving new formats from annotation state. With onChangeEditableValue you'd derive new annotation state from the formats (likely in a different position). So you need to loop through all formats and set new start and end positions. We could provide helper functions if this would make it easier, e.g. getFormatPositions( value ) = [ { format, start, end }, ... ]. I'll have a look on Monday to implement it if needed.

@youknowriad
Copy link
Contributor

Unit tests are failing. Probably docs needs to be regenerated or something.

I'd love if we can update the annotations e2e test to check updates, say like we type in an annotation and we check that it's being updated properly in the state or something.

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

I don't see any tests at all. This would make it hard for anyone changing RichText in the future to know if they broke the API.

const positions = {};

formats.forEach( ( characterFormats, i ) => {
characterFormats = characterFormats || [];
Copy link
Member

Choose a reason for hiding this comment

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

This should always be an array, so the casting seems unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be undefined right, for when no formatting is present on that character?

Copy link
Member

Choose a reason for hiding this comment

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

It's a sparse array, there's no indices set as undefined, they're just not there. forEach will only loop over any indices that are set, which is only arrays.

Copy link
Member

Choose a reason for hiding this comment

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

Try:

[ , [] ].forEach( ( { forEach } ) => {} );
[ undefined, [] ].forEach( ( { forEach } ) => {} );


formats.forEach( ( characterFormats, i ) => {
characterFormats = characterFormats || [];
characterFormats = characterFormats.filter( ( format ) => format.type === FORMAT_NAME );
Copy link
Member

Choose a reason for hiding this comment

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

Could these two loops be merged?

@atimmer
Copy link
Member Author

atimmer commented Nov 19, 2018

Also, pasting remove the annotaitons => This doesn't seem like a bug for me. When you paste something, you are overwriting the whole value?

When pasting, I think the annotations should stay in place if the text they apply to is still present. If I cut and paste a sentence from the middle of the paragraph to the beginning of the paragraph the annotations should probably stay in place.

const positions = retrieveAnnotationPositions( formats );
const { removeAnnotation, updateAnnotationRange, annotations } = props;

updateAnnotationsWithPositions( annotations, positions, { removeAnnotation, updateAnnotationRange } );
Copy link
Member

Choose a reason for hiding this comment

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

Why can retrieveAnnotationPositions and updateAnnotationsWithPositions not be done in one go, something like updateAnnotationsFromFormats?

Copy link
Member Author

Choose a reason for hiding this comment

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

I felt like these two functions were good to make the code more understandable, they can be rolled into one if that's better.

* @param {Array} formats The formats of the latest rich-text value.
* @param {string} text The text of the latest rich-text value.
*/
onChangeEditableValue( { formats, text } ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not build in the rich-text package instead of the component, just like __experimentalCreatePrepareEditableTree? The RichText component should not be aware of any editor-only formats. When a value is created with create, the formats should already be gone. I could update this PR, but I'm not confident without any tests to verify the behaviour.

Copy link
Member Author

@atimmer atimmer Nov 19, 2018

Choose a reason for hiding this comment

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

I don't know if I understand the rich-text package enough to be able to rewrite this code myself. Should I try to take a stab at writing those tests so you can verify your code?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good!

@@ -57,10 +57,6 @@ function toFormat( { type, attributes } ) {
return attributes ? { type, attributes } : { type };
}

if ( formatType.__experimentalCreatePrepareEditableTree ) {
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/WordPress/gutenberg/pull/11861/files#r234544043

In other words, these lines shouldn't be removed.

@ellatrix
Copy link
Member

I think this is in a good place, it should only get some tests and move formats => positions to create.

@atimmer
Copy link
Member Author

atimmer commented Nov 19, 2018

I've split out the annotations e2e into their own file so we can expand on that fairly easily. I've also added tests for the text tracking. I tested 3 scenario's:

  1. Typing before an annotation
  2. Typing after an annotation
  3. Typing inside an annotation.

@iseulde That should be enough to verify to verify the refactor. Do you miss any scenarios?

This makes it possible to have an editor-only format that stays in
position when typing inside `RichText`.

Apply this to annotations, because it is necessary there.
When the user types before or in an annotation we want the annotation
to be adjusted accordingly. We can determine the location of the mark
elements in the `RichText` based on the value passed to
`__experimentalCreateValueToFormat` so we can update the annotations
based on the values expressed there.
Instead of having APIs for `formatToValue` and `valueToFormat`, create
an `__experimentalCreateOnChangeEditableValue` function that is called
when the editable value changes. The annotations API can use this to
update its state.
@atimmer atimmer force-pushed the fix/annotations-text-tracking branch from b369505 to 492ce7a Compare November 19, 2018 14:59
@atimmer atimmer added this to the 4.5 milestone Nov 19, 2018
return state.all;
return flatMap( state, ( annotations ) => {
return annotations;
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

This is equivalent to flatMap( state, identity ), which can be reduced to flatten( state ), unless I misread.

@@ -1,3 +1,3 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Using Plugins API Sidebar Should open plugins sidebar using More Menu item and render content 1`] = `"<div class=\\"components-panel__header edit-post-sidebar-header__small\\"><span class=\\"edit-post-sidebar-header__title\\">(no title)</span><button type=\\"button\\" aria-label=\\"Close plugin\\" class=\\"components-button components-icon-button\\"><svg aria-hidden=\\"true\\" role=\\"img\\" focusable=\\"false\\" class=\\"dashicon dashicons-no-alt\\" xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"20\\" height=\\"20\\" viewBox=\\"0 0 20 20\\"><path d=\\"M14.95 6.46L11.41 10l3.54 3.54-1.41 1.41L10 11.42l-3.53 3.53-1.42-1.42L8.58 10 5.05 6.47l1.42-1.42L10 8.58l3.54-3.53z\\"></path></svg></button></div><div class=\\"components-panel__header edit-post-sidebar-header\\"><strong>Sidebar title plugin</strong><button type=\\"button\\" aria-label=\\"Unpin from toolbar\\" aria-expanded=\\"true\\" class=\\"components-button components-icon-button is-toggled\\"><svg aria-hidden=\\"true\\" role=\\"img\\" focusable=\\"false\\" class=\\"dashicon dashicons-star-filled\\" xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"20\\" height=\\"20\\" viewBox=\\"0 0 20 20\\"><path d=\\"M10 1l3 6 6 .75-4.12 4.62L16 19l-6-3-6 3 1.13-6.63L1 7.75 7 7z\\"></path></svg></button><button type=\\"button\\" aria-label=\\"Close plugin\\" class=\\"components-button components-icon-button\\"><svg aria-hidden=\\"true\\" role=\\"img\\" focusable=\\"false\\" class=\\"dashicon dashicons-no-alt\\" xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"20\\" height=\\"20\\" viewBox=\\"0 0 20 20\\"><path d=\\"M14.95 6.46L11.41 10l3.54 3.54-1.41 1.41L10 11.42l-3.53 3.53-1.42-1.42L8.58 10 5.05 6.47l1.42-1.42L10 8.58l3.54-3.53z\\"></path></svg></button></div><div class=\\"components-panel\\"><div class=\\"components-panel__body is-opened\\"><div class=\\"components-panel__row\\"><label for=\\"title-plain-text\\">Title:</label><textarea class=\\"editor-plain-text\\" id=\\"title-plain-text\\" placeholder=\\"(no title)\\" rows=\\"1\\" style=\\"overflow: hidden; overflow-wrap: break-word; resize: none; height: 18px;\\"></textarea></div><div class=\\"components-panel__row\\"><button type=\\"button\\" class=\\"components-button is-button is-primary\\">Reset</button></div><button type=\\"button\\" class=\\"components-button is-button is-primary\\">Add annotation</button></div></div>"`;
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the difference here is that it was moved to a separate test. there's no annotations button anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is the reason.

@atimmer atimmer changed the title Allow filtering formatToValue and valueToFormat Fix annotations text tracking Nov 19, 2018
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I'm approving this as it seems in a decent shape to include in 4.5 but let's make sure we follow up on the comments.

@youknowriad youknowriad merged commit 221afa1 into master Nov 19, 2018
@youknowriad youknowriad deleted the fix/annotations-text-tracking branch November 19, 2018 18:14
@omarreiss omarreiss added the [Feature] Annotations Adding annotation functionality label Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Annotations Adding annotation functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants