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 deletion of custom block classes #8232

Merged
merged 2 commits into from Aug 11, 2018

Conversation

Projects
None yet
4 participants
@johngodley
Contributor

johngodley commented Jul 26, 2018

If you add a custom class to a block and then remove that custom class it will flag the block as invalid. This is because the custom class becomes part of the block attributes, and is then added back by the 'custom class name' filter. The expected HTML then doesn't match the current HTML, and a warning is triggered.

You can reproduce the problem as follows:

  1. Create a quote block
  2. Edit the HTML of the quote block and add another class in the <blockquote>:
    <blockquote class="wp-block-quote newclass”><p>fdsfsdfsdd</p></blockquote>
  3. Tab out of the editor and notice that no warning is shown
  4. Edit HTML again and remove the newclass
  5. Tab out and trigger a validation failure, even though the HTML is the default

This may or may not be expected behaviour, but it seems like removing the custom class shouldn't trigger a warning, and so I've treated it as a bug.

This PR ensures the block contains the default class names + whatever classes are currently in the HTML, ignoring any custom ones in the block itself.

The unit tests have been beefed up to cope with several edge case situations

Fixes #8131

Note that I don't fully understand the context of this change, and I may totally have misunderstood the code. The change keeps the existing unit tests running while fixing the additional cases of removing classes. I'm not sure if this is the purpose of the code, so the fix may be misplaced.

How has this been tested?

Existing unit tests work, and additional unit tests have been added. Manual verification will show the problem in different situations:

  • Add a custom class to a block without a default class and then remove it. To test, manually add a class to a paragraph block:
    <p class="test">paragraph</p>
    And then remove the class to trigger an invalid block warning
  • Add a custom class to a block with a default class and then remove the class - explained above
  • Add several custom classes to a block and remove one of them. To test, manually add several classes to a quote block:
    <blockquote class="wp-block-quote a b c"><p>fdsfsdfsdd</p></blockquote>
    Then remove class b to trigger an invalid block warning. A variation of above, but worth testing

Types of changes

This is a bug fix that changes some core block HTML code and so could potentially be a breaking fix.

Checklist:

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

@johngodley johngodley added the Blocks label Jul 26, 2018

@johngodley johngodley changed the title from Fix deletion of custom block classes to WIP Fix deletion of custom block classes Jul 26, 2018

Fix deletion of custom block classes
If you add a custom class to a block and then remove that custom class it would flag the block as invalid. The reason is that the custom class became part of the blocks ‘default’ attributes, and it would then add it back and this then the expected HTML wouldn’t match the current HTML.

This change ensures the block contains the default class names + whatever classes are currently in the HTML, ignoring any custom ones in the block itself. It also handles the
situation where the block's default class doesn't exist in the HTML

The unit tests have been beefed up to cope with several edge case situations

@johngodley johngodley changed the title from WIP Fix deletion of custom block classes to Fix deletion of custom block classes Jul 28, 2018

@johngodley

This comment has been minimized.

Show comment
Hide comment
@johngodley

johngodley Jul 28, 2018

Contributor

@aduth, sorry to drag you in but with your work in #7538 you may be best placed to comment on this

Contributor

johngodley commented Jul 28, 2018

@aduth, sorry to drag you in but with your work in #7538 you may be best placed to comment on this

@aduth aduth self-requested a review Aug 3, 2018

@aduth

aduth approved these changes Aug 3, 2018

Thanks for the ping @johngodley . I agree this is a bug. I think there might be an easier approach here in retrieving the custom classes as simply the difference between innerHTML and the default block (i.e. a block serialized with a className attribute of undefined). I've done this in 1f2a7a1 . All tests still pass.

Let me know what you think. On my end, this is good to go 👍

@johngodley

This comment has been minimized.

Show comment
Hide comment
@johngodley

johngodley Aug 11, 2018

Contributor

That's great, thanks! It seemed like there should be a simpler way but I wasn't entirely sure what happened beyond this code.

Merging.

Contributor

johngodley commented Aug 11, 2018

That's great, thanks! It seemed like there should be a simpler way but I wasn't entirely sure what happened beyond this code.

Merging.

@johngodley johngodley merged commit b5490a6 into master Aug 11, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.28% but relative coverage increased by +48.86% compared to a975273
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@johngodley johngodley deleted the fix/remove-custom-class branch Aug 11, 2018

@mtias mtias added this to the 3.6 milestone Aug 16, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 18, 2018

Contributor

It looks like this behavior regressed custom classes for dynamic blocks because they don't have any serialized content #9991

Contributor

youknowriad commented Sep 18, 2018

It looks like this behavior regressed custom classes for dynamic blocks because they don't have any serialized content #9991

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment