Annotations: Add end-to-end tests for annotations API #12186

atimmer · 2018-11-21T16:28:47Z

Description

After introducing prepareEditableTree we would rerender all RichText instances when typing in one RichText instance. Noted here: #11595 (comment). This was partly fixed in #12161. This PR fixes it completely and adds e2e tests to ensure that this won't happen again in the future.

How has this been tested?

Using the e2e tests and applying annotations using Yoast SEO.

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.

IreneStr · 2018-11-22T09:20:27Z

I just tested this with the Yoast annotations, and it works like a charm 😄

youknowriad · 2018-11-22T12:41:09Z

packages/annotations/src/format/annotation.js

+ *
+ * @return {Function} The onChangeEditableValue function.
+ */
+const createOnChangeEditableValue = memize( ( removeAnnotation, updateAnnotationRange, annotations ) => {


I feel the memoization is not necessary as it's an onChange handler? And withDispatch in general don't need memoization?

This does not go to withDispatch, but it goes to the onChangeEditableValue prop in the HOC for RichText. I don't think we can 'route' this through withDispatch because withDispatch only accepts functions in the returned object.

It could be that this is not necessary to prevent re-renders to TinyMCE, but I do think it's necessary to prevent re-renders to RichText.

I don't have a strong opinion against this change. I'd like us to move away from systemic memoization for performance optimization in the formats but handle these in the framework instead but I don't see this as a blocker for this PR.

youknowriad · 2018-11-22T12:42:23Z

packages/editor/src/components/rich-text/index.js

+			// If we always specify this the selection will go to this RichText
+			// regardless whether is is actually the currently selected one.
+			// So check if this RichText is the selected one.
+			if ( this.props.isSelected ) {


@iseulde Any thoughts about this particular change?

I've put some extra context about this change in the commit: 50057fc.

ellatrix · 2018-12-08T16:47:23Z

I'll try to look at this today or tomorrow.

ellatrix · 2018-12-09T17:53:09Z

I'm fine with this as a temporary solution since it won't impact the default experience, but we really need to look at moving memoization into the API and making sure it gets invalidated when needed. See #12161 (comment).

ellatrix · 2018-12-09T19:25:37Z

test/e2e/specs/annotations.test.js

+			const rerendersAfterAnnotating = await getRerenderCount();
+
+			// Typing in the second block should not trigger a re-render in the first block.
+			expect( rerendersAfterAnnotating ).toEqual( rerendersBeforeAnnotating + ANNOTATION_RERENDERS );


This can be written as toBe.

ellatrix · 2018-12-09T19:26:55Z

test/e2e/test-plugins/plugins-api/annotations-sidebar.js

+	const FORMAT_NAME = 'rerender/counter';
+
+	function countRerender( formats, text ) {
+		if ( text.startsWith( 'RerenderCounter' ) ) {


Can't we create a separate format type for these tests?

Assuming it is already separate because it's called rerender/counter. So why do we have this check?

ellatrix · 2018-12-09T19:28:38Z

test/e2e/test-plugins/plugins-api/annotations-sidebar.js

+		return formats;
+	}
+
+	domReady( () => {


Why is a format being registered when the DOM is ready? What does the DOM have to do with this?

ellatrix · 2018-12-09T19:29:31Z

test/e2e/test-plugins/plugins-api/annotations-sidebar.js

+
+	domReady( () => {
+		registerFormatType( FORMAT_NAME, {
+			name: FORMAT_NAME,


I think this attribute is not needed?

ellatrix · 2018-12-09T19:31:28Z

test/e2e/test-plugins/plugins-api/annotations-sidebar.js

+
+	function countRerender( formats, text ) {
+		if ( text.startsWith( 'RerenderCounter' ) ) {
+			window.annotationsCountingRerenders++;


It's strange to me that this is being set in a separate file. I have no idea from looking at the other file where it's coming from. Maybe better to more this registration to the test cases?

aduth · 2018-12-09T20:27:57Z

packages/annotations/src/block/index.js

@@ -15,9 +20,9 @@ const addAnnotationClassName = ( OriginalComponent ) => {
 		const annotations = select( 'core/annotations' ).__experimentalGetAnnotationsForBlock( clientId );

 		return {
-			className: annotations.map( ( annotation ) => {
+			className: uniq( annotations.map( ( annotation ) => {


For what reason do we suddenly need to consider uniqueness of annotations classnames?

I'd not consider this a blocker, in that there ought not be any harm in deduplicating classes, but it'd be better if we could not apply the logic if we're not expecting it to be necessary (or at least have some confidence in whether it is or is not necessary).

aduth · 2018-12-09T20:37:22Z

test/e2e/test-plugins/plugins-api/annotations-sidebar.js

@@ -15,6 +15,9 @@
 	var registerPlugin = wp.plugins.registerPlugin;
 	var PluginSidebar = wp.editPost.PluginSidebar;
 	var PluginSidebarMoreMenuItem = wp.editPost.PluginSidebarMoreMenuItem;
+	var applyFormat = wp.richText.applyFormat;


wp-rich-text must be defined as a dependency of this script.

aduth · 2018-12-09T20:37:30Z

test/e2e/test-plugins/plugins-api/annotations-sidebar.js

@@ -15,6 +15,9 @@
 	var registerPlugin = wp.plugins.registerPlugin;
 	var PluginSidebar = wp.editPost.PluginSidebar;
 	var PluginSidebarMoreMenuItem = wp.editPost.PluginSidebarMoreMenuItem;
+	var applyFormat = wp.richText.applyFormat;
+	var registerFormatType = wp.richText.registerFormatType;
+	var domReady = wp.domReady;


wp-dom-ready must be defined as a dependency of this script.

aduth · 2018-12-10T19:17:42Z

Separate targeted pull requests were made to extract changes into #12737 and #12741 . This branch can be rebased to address the remaining item concerning end-to-end test changes.

We should also consider (separately):

Further refactoring to className extension
- Is uniq necessary or desirable?
- Do we need to override props at all if there are no active annotations?
- Should we consider @wordpress/token-list for generating a string from an array of class names?
Unit tests covering packages/annotations/src/block/index.js

The test will check that one `RichText` doesn't rerender when typing in a different `RichText`.

gziolo · 2019-02-05T15:01:20Z

@atimmer it looks like there are still some comments that need to be addressed. This branch also got stale because of other changes applied in master. Do you plan to continue working on it? It would be great to have better test coverage for annotations as it isn't used by Gutenberg itself.

atimmer · 2019-02-06T11:14:58Z

@gziolo This is currently not a focus. The work is still valuable, but I have no short-term plans to actively pursue this to completion.

gziolo · 2019-02-06T12:43:22Z

Yes, it seems like a good addition, so if someone wants to move it forward, I assume it would be nice to help land this PR :)

gziolo · 2019-03-05T09:32:10Z

It doesn't look like it is going to be worked on in the upcoming weeks. Let's close it for now. We can always reopen, refresh and land when it's more pressing. Thanks for your initial work on this PR.

atimmer changed the title ~~Fix performance of annotations and the christmas light effect.~~ Fix performance of annotations and the React christmas light effect. Nov 21, 2018

atimmer requested review from ellatrix and youknowriad November 21, 2018 16:30

youknowriad reviewed Nov 22, 2018

View reviewed changes

atimmer added this to the WordPress 5.0.x Follow Ups milestone Nov 26, 2018

omarreiss modified the milestones: WordPress 5.0.x Follow Ups, WordPress 5.0.1 Dec 6, 2018

omarreiss added [Type] Performance Related to performance efforts [Feature] Annotations Adding annotation functionality labels Dec 6, 2018

atimmer mentioned this pull request Dec 6, 2018

Performance: Reduced responsiveness when using Annotations API #12642

Closed

mtias modified the milestones: WordPress 5.0.1, 4.7 Dec 6, 2018

atimmer force-pushed the fix/improve-annotations-performance branch 3 times, most recently from 3b2aacb to cd76538 Compare December 9, 2018 18:26

ellatrix reviewed Dec 9, 2018

View reviewed changes

aduth reviewed Dec 9, 2018

View reviewed changes

aduth requested changes Dec 9, 2018

View reviewed changes

youknowriad modified the milestones: 4.7, 4.8 Dec 9, 2018

ellatrix mentioned this pull request Dec 9, 2018

RichText: Ensure instance is selected before setting back selection #12737

Merged

4 tasks

aduth mentioned this pull request Dec 9, 2018

Annotations: Apply annotation className as string #12741

Merged

aduth mentioned this pull request Dec 10, 2018

Performance: Improve the initial loading time and the feeling fo smoothness while typing in blocks. #11782

Closed

Annotations: Add e2e tests to check for rerenders

222f942

The test will check that one `RichText` doesn't rerender when typing in a different `RichText`.

atimmer force-pushed the fix/improve-annotations-performance branch from cd76538 to 222f942 Compare December 18, 2018 09:30

youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018

aduth changed the title ~~Fix performance of annotations and the React christmas light effect.~~ Annotations: Add end-to-end tests for annotations API Jan 4, 2019

aduth added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. and removed [Type] Performance Related to performance efforts labels Jan 4, 2019

youknowriad modified the milestones: 4.9 (Gutenberg), 5.0 (Gutenberg) Jan 21, 2019

youknowriad modified the milestones: 5.0 (Gutenberg), 5.1 (Gutenberg) Feb 4, 2019

gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 5, 2019

gziolo removed this from the 5.1 (Gutenberg) milestone Feb 7, 2019

gziolo closed this Mar 5, 2019

gziolo deleted the fix/improve-annotations-performance branch March 5, 2019 09:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotations: Add end-to-end tests for annotations API #12186

Annotations: Add end-to-end tests for annotations API #12186

atimmer commented Nov 21, 2018

IreneStr commented Nov 22, 2018

youknowriad Nov 22, 2018

atimmer Nov 22, 2018

youknowriad Nov 22, 2018

youknowriad Nov 22, 2018

atimmer Nov 22, 2018

ellatrix commented Dec 8, 2018

ellatrix commented Dec 9, 2018

ellatrix Dec 9, 2018

ellatrix Dec 9, 2018

ellatrix Dec 9, 2018

ellatrix Dec 9, 2018

ellatrix Dec 9, 2018

ellatrix Dec 9, 2018

aduth Dec 9, 2018

aduth Dec 9, 2018

aduth Dec 9, 2018

aduth commented Dec 10, 2018

gziolo commented Feb 5, 2019

atimmer commented Feb 6, 2019

gziolo commented Feb 6, 2019

gziolo commented Mar 5, 2019

Annotations: Add end-to-end tests for annotations API #12186

Annotations: Add end-to-end tests for annotations API #12186

Conversation

atimmer commented Nov 21, 2018

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

IreneStr commented Nov 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Dec 8, 2018

ellatrix commented Dec 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Dec 10, 2018

gziolo commented Feb 5, 2019

atimmer commented Feb 6, 2019

gziolo commented Feb 6, 2019

gziolo commented Mar 5, 2019