-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix performance regression in the block-level custom CSS feature #11686
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
Closed
+83
−19
Closed
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
79df64b
Update BASE_TAG default version to 6.8.0
mukeshpanchal27 57bd3b6
Merge branch 'WordPress:trunk' into trunk
mukeshpanchal27 98aa939
Return early if no custom CSS class is provided
mukeshpanchal27 391f5b7
Update default BASE_TAG version to 6.7.0
mukeshpanchal27 ac2597e
Merge branch 'trunk' into perf/10777
mukeshpanchal27 7e5a7d0
Merge branch 'trunk' into perf/10777
mukeshpanchal27 a331593
Refactor custom CSS support validation logic
mukeshpanchal27 68c4f7e
Merge branch 'trunk' into perf/10777
mukeshpanchal27 439b39f
Apply suggestions from code review
mukeshpanchal27 87d952c
Merge branch 'trunk' into perf/10777
westonruter a291f83
Guard against malformed attrs from parsed block
westonruter d637df3
Guard against non-string className in attrs
westonruter fba41c3
Harden types for wp_render_custom_css_class_name()
westonruter 62781ae
Omit irrelevant types from phpdoc for wp_render_custom_css_support_st…
westonruter 0bbcfe2
Merge branch 'trunk' into perf/10777
mukeshpanchal27 aa22994
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter 2fc9025
Use phpdoc tag for function reference
westonruter 5eb8c72
Block Supports: Short-circuit custom CSS class rendering with a cheap…
westonruter bd93fe8
Ensure wp-custom-css-* class is not the suffix of another class after…
westonruter a89feac
Leverage HTML Tag Processor to parse out wp-custom-css-* class name
westonruter e002ecf
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter 3313806
Add test cases
westonruter 833170a
Use strtok() instead of HTML Tag Processor
westonruter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2604,9 +2604,9 @@ function unregister_block_style( $block_name, $block_style_name ) { | |
| * @since 5.8.0 | ||
| * @since 6.4.0 The `$feature` parameter now supports a string. | ||
| * | ||
| * @param WP_Block_Type $block_type Block type to check for support. | ||
| * @param string|array $feature Feature slug, or path to a specific feature to check support for. | ||
| * @param mixed $default_value Optional. Fallback value for feature support. Default false. | ||
| * @param WP_Block_Type|null $block_type Block type to check for support. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is because the return value of |
||
| * @param string|array $feature Feature slug, or path to a specific feature to check support for. | ||
| * @param mixed $default_value Optional. Fallback value for feature support. Default false. | ||
| * @return bool Whether the feature is supported. | ||
| */ | ||
| function block_has_support( $block_type, $feature, $default_value = false ) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@dmsnell I'd appreciate your feedback on this approach.
This ensures correctness, as it won't incorrectly match
my-wp-custom-css-0acafc53as previously due to the\bword boundary matching at hyphens.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.
Note the additional computation involved for
WP_HTML_Tag_Processoris guarded against via thestr_contains( $class_name_attr, 'wp-custom-css-' )check above.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.
Should we also add a unit test to validate the edge cases that
WP_HTML_Tag_Processorcovers (vs the regex)?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.
@ramonjd citing your comment here from WordPress/gutenberg#78217 (review) to keep reviews on the canonical PR:
Yes, the intention is to avoid reinventing the wheel for parsing class names. Leveraging the HTML Tag Processor as a utility like this is something I learned from @sirreal when populating text in a script or style tag. I recall @dmsnell also intended to expose a static method that provided a wrapper to do this parsing.
Alternatively, I first tried just improving the regex in bd93fe8 but then I decided to go down the route of using the tag processor.
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.
I believe that's #10043 proposing a class list utility.
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.
I took a look at this and did some testing. I think this HTML round-trip to extract the class name is safe to avoid here.
Consider this non-blocking feedback. There are merits to using the tag processor for greater consistency and correctness guarantees.
Given the performance focus of this PR, I'll leave feedback on alternative strictly for the purpose of your consideration. I'm happy to leave the final call up to @westonruter. There shouldn't be any downside to using the tag processor here aside from some possibly redundant overhead.
$block_contentcontains HTML. The class attribute there appears to have passed throughesc_attr().$class_name_attris a plain string, it does not include escaped HTML and is not encoded.$class_name_attris a plain text representation.Given we don't need normalization at this phase and we'll later add the plaintext class name using the
add_class()method, I think we can use some simpler string whitespace splitting that matches HTML rules for a set of space-separated tokens (like the class attribute).In short, I think it's safe to save some additional overhead and split like this:
strtok()has some pitfalls, so a regular expression pattern should also work, it just needs to align with how class names are matched according to standards:Again, please take this as non-blocking feedback. Any of the 3 approaches (tag processor or my suggestions here) seem correct and have different tradeoffs.
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.
@ramonjd Added "prefixed custom CSS class" test case in 3313806 which tests the bug and verifies the fix.
@sirreal Thank you. I've implemented
strtok()in 833170a.