👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @alshakerM! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@gziolo would it be possible to add some general guidance for how we approach maintenance in the element package (our abstraction layer on top of React)?

I think the overall approach is sound, but I do wonder how we handle updating of these properties. Do we normally audit major React upgrades?

Looking specifically at the history of this file, we diverged a bit for wp_texturize support.

For reference serialization in React is abstracted in a renderer https://github.com/facebook/react/blob/cae635054e17a6f107a39d328649137b83f25972/packages/react-dom/src/server/ReactDOMStringRenderer.js#L31-L39 I haven't traced all the way to where they handle attribute normalization yet, but if I'm understanding this correctly, the main bit of work here is making sure these values are aligned.

@gwwar, I think this is the place where React handles normalization - https://github.com/facebook/react/blob/main/packages/react-dom/src/shared/DOMProperty.js#L448-L452

See also my comment #38553 (comment). This PR should fix the issue for SVG, but there are more attributes we need to align with React 17. At the same time, as @gwwar pointed out, we need to ensure that they get saved the same way on the server. There is an existing unit test that asserts that wp_kses_post doesn't change the HTML in https://github.com/WordPress/gutenberg/blob/trunk/phpunit/class-block-fixture-test.php.

So browsing through React code:

possibleStandardNames.js appears to be used to warn on mismatched names in dev mode. There's also DOMProperty which is meant to stay in sync with possibleStandardNames.js. Comments in the file note that properties are mostly scraped from MDN documentation.

My overall recommendations here would be:

Update getNormalAttributeName

Using a list that includes attributes noted in possibleStandardNames and DOMProperty. We can diverge a bit in how we store the properties, but they should match what React 17 checks for.

Update the test suite to add cases for the following:

• Test SVG/HTML attributes that should be kebab-case serialize properly. (for example data- and aria-)
• Test SVG/HTML attributes that should be camelCase serialize properly and with the correct capitalization.
• Test that we can't serialize reserved React attributes like children or dangerouslySetInnerHTML
• Add similar tests for server serialization. @gziolo noted that https://github.com/WordPress/gutenberg/blob/trunk/phpunit/class-block-fixture-test.php might be a good starting point.

Overall, I think there's a good amount of flexibility here for the implementation @alshakerM. The main idea is that we're matching the same attributes as React does. Let us know if you had any questions and please do ping us when you'd like us to take another look.

If you'd prefer, I can also commit some suggestions to the branch directly too.

Thank you so much everyone for the feedback!

I addressed the following:

Using a list that includes attributes noted in possibleStandardNames and DOMProperty. We can diverge a bit in how we store the properties, but they should match what React 17 checks for.

I created two new lists for untypical attributes (case-sensitive or have colons). Both derived from React. For more compact code and more robust lookup, I created the maps between lowercase => correct case in runtime.

Test SVG/HTML attributes that should be kebab-case serialize properly. (for example data- and aria-)

If I understand correctly, this seems to be already covered in tests.

Test SVG/HTML attributes that should be camelCase serialize properly and with the correct capitalization.

I implemented support for this, and added tests for it.

Test that we can't serialize reserved React attributes like children or dangerouslySetInnerHTML

I'm not sure the kind of failure we should be aiming for. Could you elaborate? Also, feel free to push whatever code you like to this PR!

Add similar tests for server serialization. @gziolo noted that
https://github.com/WordPress/gutenberg/blob/trunk/phpunit/class-block-fixture-test.php might be a good starting point.

Honestly, PHP isn't my forte, but I'm happy to dig deeper if you want me.

Overall, this is looking very nice @alshakerM! I also think we might be able to move the PHP testing in a follow up PR.

cc @Mamaduka @gziolo in case you had some high level feedback, before I manually test.

If I understand correctly, this seems to be already covered in tests

Excellent, good eye! I didn't have the full file open when writing the comment.

I'm not sure the kind of failure we should be aiming for. Could you elaborate? Also, feel free to push whatever code you like to this PR!

There are some properties we can set on the react component that we don't want to be serialized in html output.

Some of these cases are also already covered, though we can double check that our list is up to date with https://github.com/facebook/react/blob/5d08a24c21dcb3d42ea628ce3f94bdbd0d432756/packages/react-dom/src/shared/DOMProperty.js#L239-L252 On a second look I don't think we need to worry about them specifically in this PR.

Honestly, PHP isn't my forte, but I'm happy to dig deeper if you want me.

To get php-unit tests to run, here's one way of doing so:

• Install docker
• Install wp-env (a local wp env)

npm -g i @wordpress/env
wp-env start
npm run test-php # lint + units
# or 
npm run test-unit-php # php unit tests

Whenever a new core block is added, a few fixtures are generated and added to test/integration/fixtures/blocks. This lets us easily check that we can parse the block, check that it's internal representation matches what we expect, and that we can serialize the block again without errors.

The idea here for the test is that given a block that saves SVG as part of its block content, we should be able to parse it, have the correct in-memory representation, and be able to serialize it again with the correct attribute casing. For example viewBox stays as viewBox and not viewbox or view-box

<!-- test:examplesvg -->
<svg viewBox="0 0 1 1" preserveAspectRatio="slice"></svg>
<!-- /test:examplesvg -->

@gziolo do we have any core blocks that serialize SVG? I took a quick look, but I couldn't find any. We might need to create a test block otherwise.

I think we can pass on new PHP tests here, but let me know if I'm missing some information on other filters that may run. I tested the following locally, and a few things stood out on behavior:

• do_blocks appears to leave the block's innerHTML alone, even if the properties are not valid.
• By default posts are run through wp_kses, meaning that SVG content is stripped from a post if the user does not have an admin role
• When passed a whitelist of attributes (typically via the wp_kses_allowed_html filter), things behave as we expect (output is not changed on re-serialization). Note that camelCase properties need to be whitelisted as lowercase or else it will not work. For example viewbox vs viewBox in the whitelist array.

	function test_attribute_casing() {
		$test_block_serialized = '<!-- test:examplesvg --><svg viewBox="0 0 30 10" xmlns="http://www.w3.org/2000/svg"><line x1="0" y1="1" x2="30" y2="1" stroke="black" /><line x1="0" y1="3" x2="30" y2="3" stroke="black" stroke-dasharray="4" /></svg><!-- /test:examplesvg -->';
		$kses_defaults = wp_kses_allowed_html( 'post' );

		$svg_args = array(
			'svg'   => array(
				'class' => true,
				'xmlns' => true,
				'viewbox' => true, // Must be lower case!
			),
			'line'     => array( 'x1' => true, 'x2' => true, 'y1' => true, 'y2' => true, 'stroke' => true, 'stroke-dasharray' => true ),
		);
		$allowed_tags = array_merge( $kses_defaults, $svg_args );
		$serialized = do_blocks( wp_kses( $test_block_serialized, $allowed_tags ) );
		$this->assertSame( $test_block_serialized, $serialized );
	}

@gziolo or @Mamaduka I'd appreciate a +1 review on this, but I'll try landing this this week otherwise if folks don't have any other additional feedback.

You don't have to wait for me. I have been following the progress on the PR but I didn't have a chance to take a closer look.

do_blocks appears to leave the block's innerHTML alone, even if the properties are not valid.

I'm not sure how you tested it, but if in isolation from WordPress then I would expect that to happen. I think it changes with filters and might behave differently depending on the editor's screen.

By default posts are run through wp_kses, meaning that SVG content is stripped from a post if the user does not have an admin role

Yes, it's a known limitation and one of the reasons why SVGs weren't used in the saved content of the Social Link block.

Note that camelCase properties need to be whitelisted as lowercase or else it will not work. For example viewbox vs viewBox in the whitelist array.

It reminds me that @pento added some WordPress core changes while working on #35611 where updated unit tests to run block fixtures through KSES. He changed the implementation in WordPress core to make everything work as documented in https://core.trac.wordpress.org/ticket/54261, so maybe we should look at updating how the whitelist works as a follow up.

Sounds good @gziolo. Let's try landing, and we can keep eyes out for any necessary follow ups. Thanks @alshakerM for the contribution! ✨

Congratulations on your first merged pull request, @alshakerM! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

It would be great to open a follow-up PR that adds an entry to the CHANGELOG in @wordpress/elements as this might have some implications on how some blocks validate. Let's hope it's going to impact these blocks in a good way only 😄

https://github.com/WordPress/gutenberg/blob/trunk/packages/element/CHANGELOG.md

Example:

### Bug Fix

- Ensure that the package uses the latest version of React types ([#37365](https://github.com/WordPress/gutenberg/pull/37365)).

Thanks for the reminder @gziolo I added a changelog note in #39141

@alshakerM checking back to see if you can confirm your WordPress.org username so we can ensure you are properly credited in the WordPress 6.0 release coming out on May 24th. You can use the links above or share information directly with me and I'll ensure it's included correctly within the release. And thanks again for your contribution, it's greatly appreciated!

@alshakerM any chance you can confirm your WordPress.org account so that we can properly credit you in the WordPress 6.0 release next week?