Block Library: Avoid setting Embed className when empty, non-responsive

[aduth]

This pull request seeks to resolve an issue where embed blocks apply changes immediately upon page load, prompting a user when attempting to navigate away. This only affects themes which do not opt in to support for responsive embeds.

Implementation notes:

The root cause is that the embed block assigns a different, new value for the className attribute. There is no default value for the className attribute, which is reasonable in that we don't want to be assigning an empty class="" in markup. However, the internal logic of the embed block in producing class names via getClassNames will wrongly produce a different value ("") when no changes are necessary (if the class name had been undefined).

Testing instructions:

These instructions can be repeated on the master branch serving as "Steps to Reproduce" the original issue.

1. (Prerequisite) Activate a theme which does not add theme support for responsive embeds (e.g. "Astra").
2. Navigate to Posts > Add New
3. Click the writing prompt
4. Paste an embeddable URL
   • Example: https://twitter.com/CincinnatiZoo/status/1050770683536568322
5. Save the post
6. Reload the page
7. Verify that (a) the "Save Draft" button is not present but instead "Saved" is displayed and (b) reloading the page does not prompt about unsaved changes

Ensure unit tests pass:

npm run test-unit packages/block-library/src/embed/test/util.js

Ensure end-to-end tests pass:

npm run test-e2e packages/e2e-tests/specs/embedding.test.js

[notnownikki]

I'm having trouble reproducing this in the way described, running WP 5.0.3

If I revert the changes to util.js, the unit test fails as expected, but the e2e test still passes. With Astra active, I never see the "Save Draft" prompt when reloading a saved post either.

However, I'm seeing that when Astra is active, any post I load is immediately has the update button active and you can update the post, even when it has no embed blocks, but this doesn't happen in twentynineteen. Strangely though, isEditedPostDirty returns false no matter what theme is active.

Astra:

Twentynineteen:

[aduth]

However, I'm seeing that when Astra is active

Oops, I think I meant to recommend a different theme since I had found that Astra has a few meta boxes which make the saving work differently. I think I was going to recommend "Storefront" instead, or really any super-barebones theme which isn't enhanced for Gutenberg or which includes their own legacy meta boxes in the editor.

I'll have to check on the end-to-end tests though, as there's something clearly problematic there if they still pass on master.

[rodrigoi]

@aduth I'm getting a deprecation warning and a validation error when trying to run the e2e tests. We may need a rebase to update the tests configuration.

● Deprecation Warning:

  Option "setupTestFrameworkScriptFile" was replaced by configuration "setupFilesAfterEnv", which supports multiple paths.

  Please update your configuration.

  Configuration Documentation:
  https://jestjs.io/docs/configuration.html

● Validation Error:

  Options: setupTestFrameworkScriptFile and setupFilesAfterEnv cannot be used together.
  Please change your configuration to only use setupFilesAfterEnv.

  Configuration Documentation:
  https://jestjs.io/docs/configuration.html

Also, the packages/e2e-tests/specs/embedding.test.js suite fails for me in master with Astra.

[aduth]

@rodrigoi Thanks for the follow-up. I expect some of the warnings you see could be resolved by a fresh npm install in the branch, since those messages are a result of a Jest version upgrade which occurred more recent than this branch (#13922).

I'm not too sure about the failures you're seeing on master. Presumably it must be something in your environment, if they're passing on Travis? Could be similar to what's being considered with macOS incompatibilities improved in #14243 .

That said, this branch still needs a refresh from me. If I recall correctly, the implementation itself is pretty complete, but per #13706 (comment) I needed to check if the E2E test was durable enough to actually capture the issue (it was noted that it passes on master as well, which is not expected).