HTML API: Add class name utilities has_class() and class_list()
#5096
Conversation
This file contains 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
dmsnell
force-pushed
the
html-api/add-class-name-utilities
branch
from
219de8a
to
8d49680
Compare
|
Unit tests are currently failing with what seems to be related to this change. |
dmsnell
force-pushed
the
html-api/add-class-name-utilities
branch
from
8d49680
to
b2b68f5
Compare
|
@ockham I saw those errors but assumed they were unrelated. I'm running them again rebased against an updated |
dmsnell
force-pushed
the
html-api/add-class-name-utilities
branch
from
bfbb7c9
to
74ae887
Compare
|
@ockham quick update: I found some code calling the Tag Processor to search by class name, but it was searching for the whole class attribute including multiple class names. this is something we ran into early on in the adoption: the expectation that so what broke is that when improving the I believe we want to maintain the new behavior as it's going to be a faulty endeavor to search by class name with multiple classes. simple reordering of class names will break that. I'm going to update the code that depends on this behavior. |
dmsnell
force-pushed
the
html-api/add-class-name-utilities
branch
5 times, most recently
from
790fdd3
to
354fc0e
Compare
ockham
reviewed
Aug 30, 2023
There was a problem hiding this comment.
block-supports code is unfortunately also duplicated in Gutenberg -- outside of lib/compat (e.g. layout.php). We'll likely need to update that file, and make sure that any viable combination of Core and Gutenberg versions will continue to work 😅
There was a problem hiding this comment.
Started an update in WordPress/gutenberg#54075.
I'm thinking we can push the layout change first into Gutenberg and delay this PR. Since Gutenberg relies on gutenberg_-prefixed functions, are we going to face problems or does that kind of free us as long as the same change is applied to Core?
dmsnell
added a commit
to WordPress/gutenberg
that referenced
this pull request
Aug 30, 2023
…lasses. In #45364 (WordPress/wordpress-develop#3976) the Block Supports was extended to add layout class names using the HTML API, new in WordPress 6.2. The initial patch opened up two opportunities to refine the code, however: - There are multiple instances of the `WP_HTML_Tag_Processor` created when a single one suffices. (There is an exception in that a second processor is necessary in order to find an inner block wrapper). - The code relies on the incidental fact that searching by a whitespace-separated list of class names works if the class names in the target tag are in the same order. In this patch the use of the HTML API is refactored to address these opportunities and clean up a few places where there could be stronger consistency with other use patterns of the HTML API: - Multiple instances of the Tag Processor have been combined to remove overhead, extra variables, and code confusion. The new flow is more linear throughout the function instead of branching. - Updated HTML is returned via `get_updated_html()` instead of casting to a string. - The matching logic to find the inner block wrapper has been commented and the condition uses the null-coalescing operator now that WordPress requires PHP 7.0+. - When attempting to find the inner block wrapper at the end, a custom comparison is made against the `class` attribute instead of relying on `next_tag()` to find a tag with the given set of class names. The last refactor is important as a preliminary step to WordPress/wordpress-develop#5096 where `has_class()` and `class_list()` methods are being introduced to the Tag Processor. In that patch the implicit functionality of matching `'class_name' => 'more than one class'` is removed since that's not a single class name, but many.
dmsnell
added a commit
to WordPress/gutenberg
that referenced
this pull request
Sep 1, 2023
…lasses. In #45364 (WordPress/wordpress-develop#3976) the Block Supports was extended to add layout class names using the HTML API, new in WordPress 6.2. The initial patch opened up two opportunities to refine the code, however: - There are multiple instances of the `WP_HTML_Tag_Processor` created when a single one suffices. (There is an exception in that a second processor is necessary in order to find an inner block wrapper). - The code relies on the incidental fact that searching by a whitespace-separated list of class names works if the class names in the target tag are in the same order. In this patch the use of the HTML API is refactored to address these opportunities and clean up a few places where there could be stronger consistency with other use patterns of the HTML API: - Multiple instances of the Tag Processor have been combined to remove overhead, extra variables, and code confusion. The new flow is more linear throughout the function instead of branching. - Updated HTML is returned via `get_updated_html()` instead of casting to a string. - The matching logic to find the inner block wrapper has been commented and the condition uses the null-coalescing operator now that WordPress requires PHP 7.0+. - When attempting to find the inner block wrapper at the end, a custom comparison is made against the `class` attribute instead of relying on `next_tag()` to find a tag with the given set of class names. The last refactor is important as a preliminary step to WordPress/wordpress-develop#5096 where `has_class()` and `class_list()` methods are being introduced to the Tag Processor. In that patch the implicit functionality of matching `'class_name' => 'more than one class'` is removed since that's not a single class name, but many.
dmsnell
added a commit
to WordPress/gutenberg
that referenced
this pull request
Sep 4, 2023
…lasses. In #45364 (WordPress/wordpress-develop#3976) the Block Supports was extended to add layout class names using the HTML API, new in WordPress 6.2. The initial patch opened up two opportunities to refine the code, however: - There are multiple instances of the `WP_HTML_Tag_Processor` created when a single one suffices. (There is an exception in that a second processor is necessary in order to find an inner block wrapper). - The code relies on the incidental fact that searching by a whitespace-separated list of class names works if the class names in the target tag are in the same order. In this patch the use of the HTML API is refactored to address these opportunities and clean up a few places where there could be stronger consistency with other use patterns of the HTML API: - Multiple instances of the Tag Processor have been combined to remove overhead, extra variables, and code confusion. The new flow is more linear throughout the function instead of branching. - Updated HTML is returned via `get_updated_html()` instead of casting to a string. - The matching logic to find the inner block wrapper has been commented and the condition uses the null-coalescing operator now that WordPress requires PHP 7.0+. - When attempting to find the inner block wrapper at the end, a custom comparison is made against the `class` attribute instead of relying on `next_tag()` to find a tag with the given set of class names. The last refactor is important as a preliminary step to WordPress/wordpress-develop#5096 where `has_class()` and `class_list()` methods are being introduced to the Tag Processor. In that patch the implicit functionality of matching `'class_name' => 'more than one class'` is removed since that's not a single class name, but many.
dmsnell
force-pushed
the
html-api/add-class-name-utilities
branch
from
621ca75
to
01d0e77
Compare
dmsnell
added a commit
to WordPress/gutenberg
that referenced
this pull request
Sep 19, 2023
See introduction in WordPress/wordpress-develop#5096
This patch adds two new public methods to the HTML Tag Processor: - `has_class()` indicates if a matched tag contains a given CSS class name. - `class_list()` returns a generator to iterate over all the class names in a matched tag. Included in this patch is a refactoring of the internal logic when matching a tag to reuse the new `has_class()` function. Previously it was relying on optimized code in the `matches()` function which performed byte-for-byte class name comparison. With the change in this patch it will perform class name matching on the decoded value, which might differ if a class attribute contains character references. These methods may be useful for running more complicated queries based on the presence or absence of CSS class names. The use of these methods avoids the need to manually decode the class attribute as reported by `$process->get_attribute( 'class' )`.
dmsnell
force-pushed
the
html-api/add-class-name-utilities
branch
from
01d0e77
to
1521aa8
Compare
oh darn I missed this, and I won't be able to address it before you're active. I'll try and prioritize it tomorrow and see if it's something small |
dmsnell
commented
Sep 26, 2023
|
tests seem to be passing now after my last update. |
|
I was curious if Fortunately, it seems to work: diff --git a/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php b/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php
index 4469f90c4f..4db85070e7 100644
--- a/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php
+++ b/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php
@@ -2070,6 +2070,20 @@ HTML;
$this->assertSame( array( 'one' ), $found_classes, 'Visited multiple copies of the same class name when it should have skipped the duplicates.' );
}
+ public function test_class_list_finds_unflushed_class_names() {
+ $p = new WP_HTML_Tag_Processor( '<div class="one">' );
+ $p->next_tag();
+
+ $p->add_class( 'two' );
+
+ $found_classes = array();
+ foreach ( $p->class_list() as $class ) {
+ $found_classes[] = $class;
+ }
+ $this->assertSame( array( 'one', 'two' ), $found_classes, 'Failed to report class names added via add_class.' );
+ }
+
+
/**
* @ticket 59209
*I won't add this test for now, but we might want to consider adding a bit of coverage in a follow-up for how changes made by |
ockham
approved these changes
Sep 26, 2023
|
Committed to Core in https://core.trac.wordpress.org/changeset/56703. |
this is the OO principle of delegation at work here. by the way, I had the same thought and double-checked at some point early on. as long as don't reach into our own internals inside our internals then all these invariants should remain in place. good thinking for double-checking this. |
86 tasks
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Status:
Trac ticket: #59209
This patch adds two new public methods to the HTML Tag Processor:
has_class()indicates if a matched tag contains a given CSS class name.class_list()returns a generator to iterate over all the class names in a matched tag.Included in this patch is a refactoring of the internal logic when matching a tag to reuse the new
has_class()function. Previously it was relying on optimized code in thematches()function which performed byte-for-byte class name comparison. With the change in this patch it will perform class name matching on the decoded value, which might differ if a class attribute contains character references.These methods may be useful for running more complicated queries based on the presence or absence of CSS class names. The use of these methods avoids the need to manually decode the class attribute as reported by
$process->get_attribute( 'class' ).