-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Search block: refactor to use HTML Tag Processor #51273
Conversation
@@ -60,7 +60,9 @@ window.addEventListener( 'DOMContentLoaded', () => { | |||
searchButton.addEventListener( 'keydown', ( e ) => { | |||
hideSearchField( e ); | |||
} ); | |||
searchLabel.addEventListener( 'click', handleButtonClick ); | |||
if ( searchLabel ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unrelated but necessary bugfix to evaluate this PR.
Size Change: +3 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
Flaky tests detected in 3124b38. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5190717255
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Refactor to use tag processor for label, input, and button. * Fix searchLabel null bug. * Use add_class instead.
$label_classes = array( 'wp-block-search__label' ); | ||
if ( ! empty( $typography_classes ) ) { | ||
$label_classes[] = $typography_classes; | ||
$label = new WP_HTML_Tag_Processor( sprintf( '<label %1$s>%2$s</label>', $inline_styles['label'], $label_inner_html ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're missing an opportunity here to rely on the safety of the HTML API by sprintf-ing the label. what's the shape of $inline_styles['label']
? do we expect it to be something like style="…"
this is okay because that content is safely sourced, but we could also set it dynamically.
// 012345678
// $inline_styles['label'] === ` style="…"`
$style = substr( $inline_styles['label'], 8, -1 );
$label = new WP_HTML_Tag_Processor( '<label>' );
$label->next_tag();
$label->set_attribute( 'style', $style );
$label->set_attribute( 'for', $input_id );
…
$label_markup = $label->get_updated_html() . $label_inner_html . '</label>';
a similar comment applies below. in general it would be great if we avoided merging safe and unsafe domains, that is, it would best to avoid using sprintf
to add HTML syntax inside of the tag processor string if we can help it.
I'm curious if doing this causes any double-escaping issues since inline styles are already escaped. if that's the case we might need to see if we can account for that in the HTML API.
otherwise: nice work - I like the refactor
This PR refactors the Search block's rendering of the
input
,label
, andbutton
to use the HTML Tag Processor introduced in 6.2.By using the tag processor, the resulting code is more readable, reliable, and performant.
Part of #51207
Testing Instructions
Take the following search block markup and paste it into a post/page/template.
Check out this PR, verify each form still renders as expected.