Rich Text: Remove componentDidUpdate deep equality conditions

[aduth]

This pull request seeks to simplify the RichText component, removing two deep equality conditions performed on each component update. These were introduced in #5100 to resolve an issue described in #5081. The issue is in-fact related to forceful focusing caused by updateContent, which is slated to be separately resolved by #7620 in checking that the editor has focus when restoring the stored bookmark when setting content.

Thus, this pull request is considered blocked by #7620. There is an added end-to-end test here which fails as expected from the steps described in #5081, and are expected to pass once #7620 is merged and this pull request rebased.

Speaking more broadly, it is an issue that we cannot trust that content has changed unexpectedly without a deep equality check. We should not need to rely on these conditions. Strict equality should be sufficient.

Testing instructions:

Repeat testing instructions from #5100.

Ensure end-to-end tests pass:

npm run test-e2e

[ellatrix]

Would there be any point in keeping the check to log a warning in dev mode if values are the same but an update is triggered?

[aduth]

Would there be any point in keeping the check to log a warning in dev mode if values are the same but an update is triggered?

I love that idea!

[aduth]

Scolding added in 320c96e7108fbc6fd1e536656ee2304ae32ece49

[ellatrix]

Looks good to me after #7620. I guess this would still require a change in the quote blocks (eventually)? Looked at this as will as part of the quote nesting PR #6520, which we'll probably wait the merge for a while.

[aduth]

I usually add something like const { warn, log, error, ... } = window.console to browser dependencies to avoid the eslint comment and be explicit. Up to you though :)

Mentioned elsewhere, while I can get on board with this for things which are proper browser dependencies, I don't think console qualifies here, as it's also available in Node and Web Workers, neither of which are "browsers".

The Console object can be accessed from any global object.

https://developer.mozilla.org/en-US/docs/Web/API/Console

Could even be argued that targeting window.console makes for needless tangling dependency to the DOM.

[aduth]

Rebased to account for the since-merged #7620 blocker.

I'm finding there's an unexpected failing end-to-end test now though, in the multi-selection suite. I can sometimes reproduce in the browser, but it's mostly to do with where the click occurs (e.g. in sibling inserter space, which is technically outside of the event scope of the onPointerDown in BlockListBlock).

It also led me to find that our quote block, with it's Array#map, passes a new value to RichText on every render:

gutenberg/core-blocks/quote/index.js

Line 196 in 66581eb

value={ toRichTextValue( value ) }

(Made evidenced by our wonderful new scolding logging)

[aduth]

Rebased to keep this one fresh. It's something I'd still like to move forward on. There's two failing end-to-end tests either, neither of which I'm yet certain the cause.

1. Splitting / Merging

This one is intermittent. I'm wondering if it could perhaps be an issue of TinyMCE initialization time. I had seen this at one point earlier in authoring end-to-end tests (#6467, which included some hacks/workarounds like waitUntilNthParagraphActive)

splitting and merging blocks › Should merge into inline boundary position

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "splitting and merging blocks Should merge into inline boundary position 1".

    - Snapshot
    + Received

      "<!-- wp:paragraph -->
    - <p>Bar</p>
    + <p><strong>FBar</strong></p>
      <!-- /wp:paragraph -->"

      69 | 		await page.keyboard.type( 'Bar' );
      70 | 
    > 71 | 		expect( await getEditedPostContent() ).toMatchSnapshot();
         | 		                                       ^
      72 | 	} );
      73 | } );
      74 | 

      at Object._callee3$ (test/e2e/specs/splitting-merging.test.js:71:42)
      at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
      at asyncGeneratorStep (test/e2e/specs/splitting-merging.test.js:11:103)
      at _next (test/e2e/specs/splitting-merging.test.js:13:194)

2. Multi-block selection

Multi-block selection › Should select/unselect multiple blocks

    expect(received).toEqual(expected)

    Expected value to equal:
      StringContaining "is-multi-selected"
    Received:
      "editor-block-list__block is-selected"

      29 | 				const className = await page.$eval( selector, ( element ) => element.className );
      30 | 				if ( areMultiSelected ) {
    > 31 | 					expect( className ).toEqual( expect.stringContaining( multiSelectedCssClass ) );
         | 					                    ^
      32 | 				} else {
      33 | 					expect( className ).not.toEqual( expect.stringContaining( multiSelectedCssClass ) );
      34 | 				}

      at _callee2$ (test/e2e/specs/multi-block-selection.test.js:31:26)
      at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
      at asyncGeneratorStep (test/e2e/specs/multi-block-selection.test.js:17:103)
      at _next (test/e2e/specs/multi-block-selection.test.js:19:194)

[gziolo]

I'm not sure why I put it API freeze. It doesn't look like it touches public API. Anyways, it needs another rebase. When you do it please include in the milestone :)

[aduth]

Superseded by #7890