-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 all 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 |
|---|---|---|
|
|
@@ -1080,7 +1080,7 @@ private function step_initial(): bool { | |
| case 'html': | ||
| $doctype = $this->get_doctype_info(); | ||
| if ( null !== $doctype && 'quirks' === $doctype->indicated_compatability_mode ) { | ||
| $this->state->document_mode = WP_HTML_Processor_State::QUIRKS_MODE; | ||
| $this->compat_mode = WP_HTML_Tag_Processor::QUIRKS_MODE; | ||
| } | ||
|
|
||
| /* | ||
|
|
@@ -1095,7 +1095,7 @@ private function step_initial(): bool { | |
| * > Anything else | ||
| */ | ||
| initial_anything_else: | ||
| $this->state->document_mode = WP_HTML_Processor_State::QUIRKS_MODE; | ||
| $this->compat_mode = WP_HTML_Tag_Processor::QUIRKS_MODE; | ||
| $this->state->insertion_mode = WP_HTML_Processor_State::INSERTION_MODE_BEFORE_HTML; | ||
| return $this->step( self::REPROCESS_CURRENT_NODE ); | ||
| } | ||
|
|
@@ -2448,7 +2448,7 @@ private function step_in_body(): bool { | |
| * > has a p element in button scope, then close a p element. | ||
| */ | ||
| if ( | ||
| WP_HTML_Processor_State::QUIRKS_MODE !== $this->state->document_mode && | ||
| WP_HTML_Tag_Processor::QUIRKS_MODE !== $this->compat_mode && | ||
| $this->state->stack_of_open_elements->has_p_in_button_scope() | ||
| ) { | ||
| $this->close_a_p_element(); | ||
|
|
@@ -4938,6 +4938,10 @@ public function remove_class( $class_name ): bool { | |
| * | ||
| * @since 6.6.0 Subclassed for the HTML Processor. | ||
| * | ||
| * @todo When reconstructing active formatting elements with attributes, find a way | ||
| * to indicate if the virtually-reconstructed formatting elements contain the | ||
| * wanted class name. | ||
| * | ||
| * @param string $wanted_class Look for this CSS class name, ASCII case-insensitive. | ||
| * @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 |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -511,6 +511,32 @@ class WP_HTML_Tag_Processor { | |
| */ | ||
| protected $parser_state = self::STATE_READY; | ||
|
|
||
| /** | ||
| * Indicates if the document is in quirks mode or no-quirks mode. | ||
| * | ||
| * Impact on HTML parsing: | ||
| * | ||
| * - In `NO_QUIRKS_MODE` (also known as "standard mode"): | ||
| * - CSS class and ID selectors match byte-for-byte (case-sensitively). | ||
| * - A TABLE start tag `<table>` implicitly closes any open `P` element. | ||
| * | ||
| * - In `QUIRKS_MODE`: | ||
| * - CSS class and ID selectors match match in an ASCII case-insensitive manner. | ||
| * - A TABLE start tag `<table>` opens a `TABLE` element as a child of a `P` | ||
| * element if one is open. | ||
| * | ||
| * Quirks and no-quirks mode are thus mostly about styling, but have an impact when | ||
| * tables are found inside paragraph elements. | ||
| * | ||
| * @see self::QUIRKS_MODE | ||
| * @see self::NO_QUIRKS_MODE | ||
| * | ||
| * @since 6.7.0 | ||
| * | ||
| * @var string | ||
| */ | ||
| protected $compat_mode = self::NO_QUIRKS_MODE; | ||
|
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. Can we not bring over the previous comment here, and update it with the correction to the contradictory statement you mentioned?
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. Is bb6d772 sufficient or do you think there should be more information on the property?
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. having it on the constants is a good requirement. I can still see value in having at least a summary here, though I know we're not consistent everywhere in this. what I'm thinking about is someone hovering over the let me stew on it. maybe I can work at it some as well
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've brought the comment over and updated the wording to make it clearer. wanted to copy it here so that when people hover over |
||
|
|
||
| /** | ||
| * Indicates whether the parser is inside foreign content, | ||
| * e.g. inside an SVG or MathML element. | ||
|
|
@@ -1155,6 +1181,8 @@ public function class_list() { | |
|
|
||
| $seen = array(); | ||
|
|
||
| $is_quirks = self::QUIRKS_MODE === $this->compat_mode; | ||
|
|
||
| $at = 0; | ||
| while ( $at < strlen( $class ) ) { | ||
| // Skip past any initial boundary characters. | ||
|
|
@@ -1169,13 +1197,11 @@ public function class_list() { | |
| return; | ||
| } | ||
|
|
||
| /* | ||
| * CSS class names are case-insensitive in the ASCII range. | ||
| * | ||
| * @see https://www.w3.org/TR/CSS2/syndata.html#x1 | ||
| */ | ||
| $name = str_replace( "\x00", "\u{FFFD}", strtolower( substr( $class, $at, $length ) ) ); | ||
| $at += $length; | ||
| $name = str_replace( "\x00", "\u{FFFD}", substr( $class, $at, $length ) ); | ||
sirreal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if ( $is_quirks ) { | ||
| $name = strtolower( $name ); | ||
| } | ||
| $at += $length; | ||
|
|
||
| /* | ||
| * It's expected that the number of class names for a given tag is relatively small. | ||
|
|
@@ -1205,10 +1231,14 @@ public function has_class( $wanted_class ): ?bool { | |
| return null; | ||
| } | ||
|
|
||
| $wanted_class = strtolower( $wanted_class ); | ||
| $case_insensitive = self::QUIRKS_MODE === $this->compat_mode; | ||
|
|
||
| $wanted_length = strlen( $wanted_class ); | ||
| foreach ( $this->class_list() as $class_name ) { | ||
| if ( $class_name === $wanted_class ) { | ||
| if ( | ||
| strlen( $class_name ) === $wanted_length && | ||
| 0 === substr_compare( $class_name, $wanted_class, 0, strlen( $wanted_class ), $case_insensitive ) | ||
| ) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
@@ -2296,6 +2326,23 @@ private function class_name_updates_to_attributes_updates(): void { | |
| */ | ||
| $modified = false; | ||
|
|
||
| $seen = array(); | ||
| $to_remove = array(); | ||
| $is_quirks = self::QUIRKS_MODE === $this->compat_mode; | ||
| if ( $is_quirks ) { | ||
| foreach ( $this->classname_updates as $updated_name => $action ) { | ||
| if ( self::REMOVE_CLASS === $action ) { | ||
| $to_remove[] = strtolower( $updated_name ); | ||
| } | ||
| } | ||
| } else { | ||
| foreach ( $this->classname_updates as $updated_name => $action ) { | ||
| if ( self::REMOVE_CLASS === $action ) { | ||
| $to_remove[] = $updated_name; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Remove unwanted classes by only copying the new ones. | ||
| $existing_class_length = strlen( $existing_class ); | ||
| while ( $at < $existing_class_length ) { | ||
|
|
@@ -2311,25 +2358,23 @@ private function class_name_updates_to_attributes_updates(): void { | |
| break; | ||
| } | ||
|
|
||
| $name = substr( $existing_class, $at, $name_length ); | ||
| $at += $name_length; | ||
|
|
||
| // If this class is marked for removal, start processing the next one. | ||
| $remove_class = ( | ||
| isset( $this->classname_updates[ $name ] ) && | ||
| self::REMOVE_CLASS === $this->classname_updates[ $name ] | ||
| ); | ||
| $name = substr( $existing_class, $at, $name_length ); | ||
| $comparable_class_name = $is_quirks ? strtolower( $name ) : $name; | ||
| $at += $name_length; | ||
|
|
||
| // If a class has already been seen then skip it; it should not be added twice. | ||
| if ( ! $remove_class ) { | ||
| $this->classname_updates[ $name ] = self::SKIP_CLASS; | ||
| // If this class is marked for removal, remove it and move on to the next one. | ||
| if ( in_array( $comparable_class_name, $to_remove, true ) ) { | ||
| $modified = true; | ||
| continue; | ||
| } | ||
|
|
||
| if ( $remove_class ) { | ||
| $modified = true; | ||
| // If a class has already been seen then skip it; it should not be added twice. | ||
| if ( in_array( $comparable_class_name, $seen, true ) ) { | ||
| continue; | ||
| } | ||
|
|
||
| $seen[] = $comparable_class_name; | ||
|
|
||
| /* | ||
| * Otherwise, append it to the new "class" attribute value. | ||
| * | ||
|
|
@@ -2350,7 +2395,8 @@ private function class_name_updates_to_attributes_updates(): void { | |
|
|
||
| // Add new classes by appending those which haven't already been seen. | ||
| foreach ( $this->classname_updates as $name => $operation ) { | ||
| if ( self::ADD_CLASS === $operation ) { | ||
| $comparable_name = $is_quirks ? strtolower( $name ) : $name; | ||
| if ( self::ADD_CLASS === $operation && ! in_array( $comparable_name, $seen, true ) ) { | ||
| $modified = true; | ||
|
|
||
| $class .= strlen( $class ) > 0 ? ' ' : ''; | ||
|
|
@@ -3932,8 +3978,29 @@ public function add_class( $class_name ): bool { | |
| return false; | ||
| } | ||
|
|
||
| $this->classname_updates[ $class_name ] = self::ADD_CLASS; | ||
| if ( self::QUIRKS_MODE !== $this->compat_mode ) { | ||
| $this->classname_updates[ $class_name ] = self::ADD_CLASS; | ||
| return true; | ||
| } | ||
|
|
||
| /* | ||
| * Because class names are matched ASCII-case-insensitively in quirks mode, | ||
| * this needs to see if a case variant of the given class name is already | ||
| * enqueued and update that existing entry, if so. This picks the casing of | ||
| * the first-provided class name for all lexical variations. | ||
| */ | ||
| $class_name_length = strlen( $class_name ); | ||
| foreach ( $this->classname_updates as $updated_name => $action ) { | ||
| if ( | ||
| strlen( $updated_name ) === $class_name_length && | ||
| 0 === substr_compare( $updated_name, $class_name, 0, $class_name_length, true ) | ||
| ) { | ||
| $this->classname_updates[ $updated_name ] = self::ADD_CLASS; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| $this->classname_updates[ $class_name ] = self::ADD_CLASS; | ||
| return true; | ||
| } | ||
|
|
||
|
|
@@ -3953,10 +4020,29 @@ public function remove_class( $class_name ): bool { | |
| return false; | ||
| } | ||
|
|
||
| if ( null !== $this->tag_name_starts_at ) { | ||
| if ( self::QUIRKS_MODE !== $this->compat_mode ) { | ||
| $this->classname_updates[ $class_name ] = self::REMOVE_CLASS; | ||
| return true; | ||
| } | ||
|
|
||
| /* | ||
| * Because class names are matched ASCII-case-insensitively in quirks mode, | ||
| * this needs to see if a case variant of the given class name is already | ||
| * enqueued and update that existing entry, if so. This picks the casing of | ||
| * the first-provided class name for all lexical variations. | ||
| */ | ||
| $class_name_length = strlen( $class_name ); | ||
| foreach ( $this->classname_updates as $updated_name => $action ) { | ||
| if ( | ||
| strlen( $updated_name ) === $class_name_length && | ||
| 0 === substr_compare( $updated_name, $class_name, 0, $class_name_length, true ) | ||
| ) { | ||
| $this->classname_updates[ $updated_name ] = self::REMOVE_CLASS; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| $this->classname_updates[ $class_name ] = self::REMOVE_CLASS; | ||
| return true; | ||
| } | ||
|
|
||
|
|
@@ -4350,6 +4436,37 @@ public function get_doctype_info(): ?WP_HTML_Doctype_Info { | |
| */ | ||
| const COMMENT_AS_INVALID_HTML = 'COMMENT_AS_INVALID_HTML'; | ||
|
|
||
| /** | ||
| * No-quirks mode document compatability mode. | ||
| * | ||
| * > In no-quirks mode, the behavior is (hopefully) the desired behavior | ||
| * > described by the modern HTML and CSS specifications. | ||
|
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. whence comes this quote? and what does "hopefully" communicate to someone trying to figure out the purpose of this mode?
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. even though the Tag Processor doesn't parse HTML differently because of these, it would be nice to still discuss the impact of TABLE elements closing an open P. these constants will still provide hover-documentation inside the HTML Processor code, and could inform someone, who is poking around and reading the docs, that this has such an impact.
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 PR moves this, it was introduced in Processor State in #6972 / [58779]. I'm not sure where it came from originally.
I struggled with how to document different behavior without being confusing or too length. I'll add some notes again and see what you think.
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. See bb6d772 for notes about quirks-mode.
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. Found it; this is why I need review and scrutiny on my own contributions. 🙃 https://developer.mozilla.org/en-US/docs/Web/HTML/Quirks_Mode_and_Standards_Mode |
||
| * | ||
| * @see self::$compat_mode | ||
| * @see https://developer.mozilla.org/en-US/docs/Web/HTML/Quirks_Mode_and_Standards_Mode | ||
| * | ||
| * @since 6.7.0 | ||
| * | ||
| * @var string | ||
| */ | ||
| const NO_QUIRKS_MODE = 'no-quirks-mode'; | ||
|
|
||
| /** | ||
| * Quirks mode document compatability mode. | ||
| * | ||
| * > In quirks mode, layout emulates behavior in Navigator 4 and Internet | ||
| * > Explorer 5. This is essential in order to support websites that were | ||
| * > built before the widespread adoption of web standards. | ||
| * | ||
| * @see self::$compat_mode | ||
| * @see https://developer.mozilla.org/en-US/docs/Web/HTML/Quirks_Mode_and_Standards_Mode | ||
| * | ||
| * @since 6.7.0 | ||
| * | ||
| * @var string | ||
| */ | ||
| const QUIRKS_MODE = 'quirks-mode'; | ||
|
|
||
| /** | ||
| * Indicates that a span of text may contain any combination of significant | ||
| * kinds of characters: NULL bytes, whitespace, and others. | ||
|
|
||
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