Add figure as a directly nestable element to force_balance_tags #1678
Conversation
…block format breaking on sites with use_balanceTags option set
|
@glendaviesnz There are some unit tests for balanceTags. It looks like all that needs to be done is update this array for this change to be covered: wordpress-develop/tests/phpunit/tests/formatting/balanceTags.php Lines 8 to 16 in de822e8 |
Thanks, have updated the test. |
There was a problem hiding this comment.
I'll approve here for what it's worth!
This tests well for me.
✅ Install latest version of the gutenberg plugin and turn on the gallery experiment under the gutenberg experiments menu
✅ Add a gallery block with several images and save the post and reload and notice that the block is now invalid
✅Apply this patch to local dev site an add a new gallery block and this time you should be able to save and reload the post
src/wp-includes/formatting.php
Outdated
| @@ -2482,7 +2482,7 @@ function force_balance_tags( $text ) { | |||
| // Known single-entity/self-closing tags. | |||
| $single_tags = array( 'area', 'base', 'basefont', 'br', 'col', 'command', 'embed', 'frame', 'hr', 'img', 'input', 'isindex', 'link', 'meta', 'param', 'source' ); | |||
| // Tags that can be immediately nested within themselves. | |||
| $nestable_tags = array( 'blockquote', 'div', 'object', 'q', 'span' ); | |||
| $nestable_tags = array( 'blockquote', 'div', 'figure', 'object', 'q', 'span' ); | |||
There was a problem hiding this comment.
Only other comment I'd have is that this seems like it might be quite outdated. Should there be more added here to avoid future issues?
Things that might be missing:
<section><article>- the example here has immediate nesting https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article<aside><details>
I didn't spend a great deal of time researching this, so open to suggestions otherwise. I may also have missed some tags that could be included here.
There was a problem hiding this comment.
I'd also say it's fine to move ahead with committing the current change to get the gallery block moving forwards, and then follow-up with a separate PR with further improvements to the tags.
There was a problem hiding this comment.
Thanks @talldan - do you know what the general view is on the long term support of the likes of this option as it appears to be legacy and is only available for sites with 'initial_db_version' < 32453, so should it be limited maintenance to fix breaking issues, or kept up-to-date indefinitely with spec changes?
There was a problem hiding this comment.
When I checked, it looked like force_balance_tags is also used on comments by default.
It's a good question about other usages. If it's definitely legacy, I wonder, why update it for the gallery block? Wouldn't a WordPress installation receiving this patch also receive a db migration?
There was a problem hiding this comment.
If it's definitely legacy, I wonder, why update it for the gallery block?
My understanding is although it is legacy, any older sites that have the use_balanceTags option set still filter content through force_balance_tags despite any database migrations. This was the case in our wider testing of the refactored gallery. For older sites that had this flag set the new gallery structure was broken on save, so it seems like the only way to fix this is to at least add figure as in this patch, or to remove the use of force_balance_tags for post content saving completely - not sure what the implications of that would be?
There was a problem hiding this comment.
To me, making this change for the gallery probably supports the argument that the tags here should be updated for other use cases. There might already be situations where users can directly nest some of the tags mentioned above (custom or third-party blocks, Group block with the tag type option).
There was a problem hiding this comment.
Ok, that makes sense, have added the others in. Hard to tell if the nested tags one is complete as there doesn't appear to be list of directly nestable tags in the spec, you have to check each element, but the list looks like it covers the main ones now anyway.
src/wp-includes/formatting.php
Outdated
| @@ -2482,7 +2482,7 @@ function force_balance_tags( $text ) { | |||
| // Known single-entity/self-closing tags. | |||
| $single_tags = array( 'area', 'base', 'basefont', 'br', 'col', 'command', 'embed', 'frame', 'hr', 'img', 'input', 'isindex', 'link', 'meta', 'param', 'source' ); | |||
There was a problem hiding this comment.
Could also be updated:
https://developer.mozilla.org/en-US/docs/Glossary/Empty_element
There was a problem hiding this comment.
Have added the missing ones. Do you think we should remove the ones that are not listed at https://developer.mozilla.org/en-US/docs/Glossary/Empty_element now, or is it safest to just leave them there?
There was a problem hiding this comment.
I think it's ok to leave them there 👍
|
Committed via changeset https://core.trac.wordpress.org/changeset/52188. |
Description
The refactored gallery block uses a nested
structure, as recommended by w3c for image collections.If a site has the use_balanceTags option set to 1, then the Gallery block breaks on save as force_balance_tags restructures it from
<figure><figure></figure></figure>to<figure></figure><figure></figure>.This PR adds figure as as a directly nestable element, which it is under the current html specs.
Testing
Trac ticket: https://core.trac.wordpress.org/ticket/54130
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.