Editor: Support className in save for custom className difference

[aduth]

Fixes #11352

This pull request seeks to resolve an issue where if a block's save implementation considers the className attribute in determining its own output result, the behavior of the custom class name "parsed difference" logic may mistakenly consider additional classes to be custom classes, resulting in issues such as the one described in #11352.

Implementation notes:

While all tests pass and therefore this can be considered an effective fix, I'm not too fond of the current implementation of suspending the filter while the default save content is generated. I'll want to see if there's a more elegant way to go about this (suggestions welcome).

Testing instructions:

Repeat steps to reproduce from #11352, ensuring there are no duplicate color classes.

Ensure unit tests pass:

npm run test-unit packages/editor/src/hooks/test/custom-class-name.js

[aduth]

The failing test cases appear legitimate.

[noisysocks]

The failing test cases appear legitimate.

I see a ✅ from Travis CI? 🙂

[aduth]

The failing test cases appear legitimate.

I see a ✅ from Travis CI? 🙂

I honestly... don't have the slightest clue what failures I had seen at the time, nor what could have happened between November and now to have made a difference.

I'd want to at least give it a rebase and refresh myself on what it is I was trying to implement here before trusting to move forward here.

[gziolo]

@aduth, this PR needs to be rebased since changed files were moved to the new block-editor package.

[mmtr]

@aduth I just rebased this for you.

I followed the reproduction steps from #11352 and I can confirm I don't see the duplicated classes.

I also ran the unit tests (note that the test is now on the block-editor package, so I needed to execute npm run test-unit packages/block-editor/src/hooks/test/custom-class-name.js) and they passed.

[mmtr]

I found a weird behavior trying to confirm this is not breaking the changes introduced by #8232:

• Create a quote block
• Edit the HTML of the quote block and add another class in the <blockquote>:
  <blockquote class="wp-block-quote new”><p>fdsfsdfsdd</p></blockquote>
• Update the post and confirm you can see the new class on the published post.
• Edit again as HTML the block and remove the new class.
• Update the post.
• ⚠️The new class is still on the published post.

This seems to be unrelated to the changes of this PR (replicated on master) so I created a new issue: #14359

[codebykat]

The code is a little beyond me, but after rebasing this locally, I was able to verify that it fixes the issue. It does have failing tests again though if you run the whole suite.

  Test Suites: 2 failed, 325 passed, 327 total
  Tests:       15 failed, 2941 passed, 2956 total

[codebykat]

Needs tests fixed

[mmtr]

The code is a little beyond me, but after rebasing this locally, I was able to verify that it fixes the issue. It does have failing tests again though.

  Test Suites: 2 failed, 325 passed, 327 total
  Tests:       15 failed, 2941 passed, 2956 total

Good one! I didn't run the full test suite. Seems we're getting the same results on Travis CI: https://travis-ci.com/WordPress/gutenberg/jobs/183597748

[youknowriad]

What's blocking this PR? is this still needed?

[aduth]

@youknowriad I'm fairly certain it's still an issue of the behavior of the custom class name hook, though it's not clear based on how the Pull Quote block has evolved if it's still an issue that a user would actually encounter.

It's been a while since I looked at it, so aside from the failures noted in the prior review remarks, I can't recall exactly the state in which it had been left at. I'll try to see if I can revisit in the next couple weeks.

[aduth]

Looking at this once more: I don't think I ever felt particularly good about the temporary disabling of a filter as part of the implementation here. I think it's something which would be better if it was exposed at the level of the block API to generate the markup of a block without applying filters, either a separate lower-level function, or a new argument/option of the existing getSaveContent. Ultimately the problem this pull request seeks to resolve is that addParsedDifference is not operating on the baseline behavior of block serialization, but instead tampered by its own filtering.

I'm going to close this between a combination of (a) staleness, (b) non-ideal nature of the implementation. The reference issue #11352 remains open. Unit tests implemented here could prove portable to a better implementation.