gutenberg icon indicating copy to clipboard operation
gutenberg copied to clipboard

Block Library: Refactor core blocks to use HTML Tag Processor

Open gziolo opened this issue 2 years ago • 4 comments

What?

The full proposal for the new HTM Tag Processor API is available at https://make.wordpress.org/core/2022/08/19/a-new-system-for-simply-and-reliably-updating-html-attributes/.

Why?

This branch presents the usage of the new HTML Tag Processor API introduced by @adamziel and @dmsnell in https://github.com/WordPress/gutenberg/pull/42485.

How?

Updated core blocks:

  • Cover
  • Gallery
  • Image
  • Site Logo

Testing Instructions

All updated core blocks should work as before.

gziolo avatar Aug 12 '22 10:08 gziolo

I rebase this branch with trunk. Refactored code to use the new class name WP_HTML_Tag_Processor. One of the core blocks Post Featured Image got refactored and it no longer is necessary to use Tag Processor there.

I wrapped all values passed to set_attribute with esc_attr to present how this would look in practice if we leave it up to developers.

gziolo avatar Sep 27 '22 08:09 gziolo

Esc_attr shouldn’t be needed thanks to https://github.com/WordPress/gutenberg/pull/44447. Let’s remove it to avoid escaping the value twice.

adamziel avatar Sep 27 '22 10:09 adamziel

Esc_attr shouldn’t be needed thanks to #44447. Let’s remove it to avoid escaping the value twice.

Nice, it still might be necessary to use some sanitization in some cases so we need to have a good documentation that covers all possible scenarios.

gziolo avatar Sep 27 '22 10:09 gziolo

@adamziel and @dmsnell, I think this PR depends on how https://github.com/WordPress/gutenberg/pull/44447 ends up handling escaping attributes. It would be great if you could bring it to the finish line. 1983b8f removes esc_attr usage, so if we need to add it back, we only need to remove this commit.

gziolo avatar Sep 28 '22 16:09 gziolo

If we want these in WordPress 6.2 we should hurry and merge them, otherwise I don't think it matters if we merge sooner or later.

dmsnell avatar Feb 01 '23 05:02 dmsnell

I refreshed this PR to use the latest API. It doesn't matter if we include it in WordPress 6.2. Actually, it's probably better to test it in the plugin first.

gziolo avatar Feb 03 '23 10:02 gziolo

Flaky tests detected in 9bb0a03c79edc589290f171bba851ead654020ed. Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4231586927 📝 Reported issues:

  • #39787 in specs/editor/various/multi-block-selection.test.js

github-actions[bot] avatar Feb 21 '23 10:02 github-actions[bot]

Testing

Cover block

✅ Minimum height style applied correctly

Screenshot 2023-02-21 at 12 17 56

Image block

✅ Aria label and target applied correctly

Screenshot 2023-02-21 at 12 48 51

Social Icon

✅ Still properly handles rel and target attributes

Screenshot 2023-02-21 at 12 53 43

gziolo avatar Feb 21 '23 11:02 gziolo

Still looking good 👍

adamziel avatar Feb 21 '23 11:02 adamziel

I'd like to know if hurry was the only reason the regex for removing the link from the site logo was left as is? That regex was what was flagged as incorrect and used as one of the reasons for including the tag processor in the first place :). If that is the only reason, that is not a problem! No complaints. I am only asking to learn if there was another reason or if I can continue with replacing the regex.

carolinan avatar Feb 23 '23 11:02 carolinan

I'd like to know if hurry was the only reason the regex for removing the link from the site logo was left as is?

HTML Tag Processor doesn't support removing HTML tags as of today, it only allows operations on HTML attributes.

gziolo avatar Feb 23 '23 11:02 gziolo

Aha! Thank you @gziolo. We need to wait then.

carolinan avatar Feb 23 '23 11:02 carolinan