-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HTML API: CSS class name methods should behave according to quirks mode #7169
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
Changes from 12 commits
80c5983
0e1a917
5267774
ed8f34f
f2fa469
c103410
1435cf9
4f18368
2540406
9be0a32
cd43ef9
00e8aff
eb07339
a9e924d
1dc1752
8dc79bc
be6091b
07726b1
ec05abb
176604c
bb6d772
15d479e
8dbf6ab
926e7d6
f1f4224
559315d
df91415
21534f9
0eaa9af
3eaecd1
ab02ca4
cad5d62
62cbb1d
8217303
b417e11
618eec5
4c8db57
48e00c9
b099026
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -287,12 +287,14 @@ class WP_HTML_Processor extends WP_HTML_Tag_Processor { | |||||||||||||||||||||||||||||||||||||||||||||||
| * @since 6.4.0 | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @since 6.6.0 Returns `static` instead of `self` so it can create subclass instances. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @param string $html Input HTML fragment to process. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @param string $context Context element for the fragment, must be default of `<body>`. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @param string $encoding Text encoding of the document; must be default of 'UTF-8'. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @param string $html Input HTML fragment to process. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @param string $context Context element for the fragment, must be default of `<body>`. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @param string $encoding Optional. Text encoding of the document; must be default of 'UTF-8'. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @param string $document_mode Optional. Set document compatibility mode (quirks). Should be | ||||||||||||||||||||||||||||||||||||||||||||||||
| * 'quirks-mode' or 'no-quirks-mode'. Default: 'no-quirks-mode'. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @return static|null The created processor if successful, otherwise null. | ||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||
| public static function create_fragment( $html, $context = '<body>', $encoding = 'UTF-8' ) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| public static function create_fragment( $html, $context = '<body>', $encoding = 'UTF-8', $document_mode = WP_HTML_Processor_State::NO_QUIRKS_MODE ) { | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
Author
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. A fragment parser would typically inherit the context element's document's compatibility mode. I suspect we'll need to change how the context element is passed, but I don't think it will include information about it's document mode and this argument will remain helpful. See #7141.
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. do you think this is worth supporting right now? do we have enough warrant to include it? in what cases would we want to create a document fragment in quirks mode, and in those cases, how would we know? I can definitely see value in having this, but also I wonder if this inclusion will help people understand better what they need to be doing or add confusion. what do you think the consequences would be of simply not supporting a quirks-mode fragment parser? or at least of not having it in the primary function signature for creating the class?
Member
Author
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 was helpful and important for testing while working on these changes.
The fragment should use the same mode as the document for the context element. This should become clear when we work on Another option would be to adjust the full parser to handle doctype declarations and set the document_mode correctly according to the full HTML document. Then we could certainly omit these changes. I'm also reluctant to eagerly add more to this method signature.
Member
Author
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.
If the full parser supports quirks mode and we have
The mode should be based on the context element's document's mode. There's no way to provide that information to the fragment parser right now. The context element is currently passed to the fragment parser as an HTML-like string (only There are other ways to handle this, for example an instance method could create a fragment from a node and set quirks mode appropriately as well as handle things like reading attributes, namespace, etc. This is all best discussed in #7141. I am going to explore a change to handle quirks mode in the full parser based on the doctype declaration, that would be sufficient and we could remove this change.
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. I could see some value in having a method like I still suspect that it'll be basically universal that we don't work in the full parser mode within WordPress. almost no HTML-processing code has access to the full document, and so that's what I meant when assuming no-quirks mode. We just don't know even when doing
Member
Author
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. There may be value in allowing the compat mode to be changed on a processor. I'd like to leave those changes for consideration in their own PR. There was no way to change the compat mode before and I don't think we need to address that in this PR. |
||||||||||||||||||||||||||||||||||||||||||||||||
| if ( '<body>' !== $context || 'UTF-8' !== $encoding ) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -303,6 +305,10 @@ public static function create_fragment( $html, $context = '<body>', $encoding = | |||||||||||||||||||||||||||||||||||||||||||||||
| $processor->state->encoding = $encoding; | ||||||||||||||||||||||||||||||||||||||||||||||||
| $processor->state->encoding_confidence = 'certain'; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| if ( WP_HTML_Processor_State::QUIRKS_MODE === $document_mode ) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| $processor->state->document_mode = WP_HTML_Processor_State::QUIRKS_MODE; | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| // @todo Create "fake" bookmarks for non-existent but implied nodes. | ||||||||||||||||||||||||||||||||||||||||||||||||
| $processor->bookmarks['root-node'] = new WP_HTML_Span( 0, 0 ); | ||||||||||||||||||||||||||||||||||||||||||||||||
| $processor->bookmarks['context-node'] = new WP_HTML_Span( 0, 0 ); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -4636,6 +4642,30 @@ public function remove_class( $class_name ): bool { | |||||||||||||||||||||||||||||||||||||||||||||||
| return $this->is_virtual() ? false : parent::remove_class( $class_name ); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Transform a class name string to a comparable form. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||
| * When the document is in quirks mode, class names are transformed to | ||||||||||||||||||||||||||||||||||||||||||||||||
| * ASCII lowercase for comparison. In no quirks mode, class names are | ||||||||||||||||||||||||||||||||||||||||||||||||
| * compared as they are (case-sensitive). | ||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||
| * > When matching against a document which is in quirks mode, class names must be matched | ||||||||||||||||||||||||||||||||||||||||||||||||
| * > ASCII case-insensitively; class selectors are otherwise case-sensitive, only matching | ||||||||||||||||||||||||||||||||||||||||||||||||
| * > class names they are identical to. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @see https://www.w3.org/TR/selectors-4/#class-html | ||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @since 6.7.0 | ||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @param string $class_name The class name to transform. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @return string The transformed class name. | ||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||
| protected function comparable_class_name( string $class_name ): string { | ||||||||||||||||||||||||||||||||||||||||||||||||
| return WP_HTML_Processor_State::QUIRKS_MODE === $this->state->document_mode | ||||||||||||||||||||||||||||||||||||||||||||||||
| ? strtolower( $class_name ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| : $class_name; | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Returns if a matched tag contains the given ASCII case-insensitive class name. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -4645,7 +4675,7 @@ public function remove_class( $class_name ): bool { | |||||||||||||||||||||||||||||||||||||||||||||||
| * @return bool|null Whether the matched tag contains the given class name, or null if not matched. | ||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||
|
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. @sirreal I've reverted this change so we can consider it separately. I know it will be important during active format reconstruction, but I think that |
||||||||||||||||||||||||||||||||||||||||||||||||
| public function has_class( $wanted_class ): ?bool { | ||||||||||||||||||||||||||||||||||||||||||||||||
| return $this->is_virtual() ? null : parent::has_class( $wanted_class ); | ||||||||||||||||||||||||||||||||||||||||||||||||
| return $this->is_virtual() ? false : parent::has_class( $wanted_class ); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
Author
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 appears to be a small bug and isn't necessary in this PR. I'd be happy to make another PR if desired.
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. hm. I would consider this more of a bug in the type signature. with active format reconstruction it is possible for virtual nodes to have a class, but the
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. Bug introduced in #6753
Member
Author
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. That seems different from what the tag processor does, where "not on a tag" returns wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php Lines 1181 to 1203 in 830d66c
Member
Author
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.
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. thanks for following up on these comments. at some point, if we allow reading these this might change, as nodes created during active format reconstruction contain the attributes of their original tags. for now, though, I prefer having a distinction between "this tag does not have this class" and "it's not possible to answer if this tag has this class" |
||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -4663,6 +4693,7 @@ public function has_class( $wanted_class ): ?bool { | |||||||||||||||||||||||||||||||||||||||||||||||
| * // Outputs: "free <egg> lang-en " | ||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @since 6.6.0 Subclassed for the HTML Processor. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @since 6.7.0 Null bytes are replaced with the replacement character (U+FFFD). | ||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||
| public function class_list() { | ||||||||||||||||||||||||||||||||||||||||||||||||
| return $this->is_virtual() ? null : parent::class_list(); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
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.
The lines immediately above this about P > TABLE handling in quirks/no-quirks modes seem contradictory. That's directly related to tree-construction.
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've updated the wording. It indeed was self-contradictory