From 64f266949fcc3b1b6e173a42734ef3094d3802c2 Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Wed, 24 Jun 2026 09:59:04 -0700 Subject: [PATCH] Performance: Follow-up fixes to prepend_to_selector() optimization. See #76556 A recent optimization to `theme.json` code adds a fast-path to avoid attempting to split a selector list by commas, assuming that the absence of a parentheses means that all remaining commas are comma-tokens. However, there are several other CSS syntax forms which are common in selectors which would present commas that are not comma tokens, and the new optimization will lead to CSS selector corruption. This patch expands the list of potential syntax-form-openers to exclude the remaining domain of selector inputs which would be broken by the fast-path, and applies fixes to `split_selector_list()` which would have left failing tests when applying the bug-fix to the earlier work. Co-Authored-By: Joseph Scott Co-Authored-By: Weston Ruter --- backport-changelog/7.1/12306.md | 1 + lib/class-wp-theme-json-gutenberg.php | 170 ++++++++++++++++-- phpunit/class-wp-theme-json-selector-list.php | 122 +++++++++++++ phpunit/class-wp-theme-json-test.php | 2 +- 4 files changed, 277 insertions(+), 18 deletions(-) create mode 100644 phpunit/class-wp-theme-json-selector-list.php diff --git a/backport-changelog/7.1/12306.md b/backport-changelog/7.1/12306.md index 585138ccd6329b..d6ecee534e5e4c 100644 --- a/backport-changelog/7.1/12306.md +++ b/backport-changelog/7.1/12306.md @@ -1,3 +1,4 @@ https://github.com/WordPress/wordpress-develop/pull/12306 * https://github.com/WordPress/gutenberg/pull/76556 +* https://github.com/WordPress/gutenberg/pull/79499 diff --git a/lib/class-wp-theme-json-gutenberg.php b/lib/class-wp-theme-json-gutenberg.php index 10b9df4c9e5435..ef3d3cca012930 100644 --- a/lib/class-wp-theme-json-gutenberg.php +++ b/lib/class-wp-theme-json-gutenberg.php @@ -1304,8 +1304,26 @@ protected static function prepend_to_selector( $selector, $to_prepend ) { return $to_prepend . $selector; } - // Gate fast path, won't work for all selectors - if ( ! str_contains( $selector, '(' ) ) { + /** + * Check for an opportunity to skip the more-costly selector splitting. + * This should be possible if there are no comments, strings, functions, + * URLs, escapes, or comment declaration openers (CDOs). + * + * Note that this means the fast-path will not apply for selectors like + * the following incomplete list: + * + * - `[class ~= "wide"]` + * - `.wp-block:is(.is-style-a, .is-style-b)` + * - `:nth-child(1)` + * + * These syntax forms all present opportunities where a comma may not + * separate selectors. If none of the start characters are present, + * there should be no way for a comma to mean anything other than a + * comma token. The exception are syntax errors, which are not handled here. + * + * @see https://www.w3.org/TR/css-syntax-3/#parse-comma-separated-list-of-component-values + */ + if ( strlen( $selector ) === strcspn( $selector, '/\'"(<\\' ) ) { return $to_prepend . str_replace( ',', ',' . $to_prepend, $selector ); } @@ -1319,38 +1337,156 @@ protected static function prepend_to_selector( $selector, $to_prepend ) { } /** - * Splits a selector list by top-level commas. + * Splits a selector list into separate selectors. + * + * While selectors are joined by commas, not all commas separate top-level selectors. + * This method only separates top-level selectors, so some commas may appear inside + * strings, nested selectors, and comments. Leading and trailing CSS whitespace is + * trimmed from the returned list items. + * + * Non-selector content, such as comments, are retained in the list in the same item + * as the selector content they follow. + * + * Example: + * + * array( '.wp-block' ) === self::split_selector_list( '.wp-block' ); + * array( '.one', '.two' ) === self::split_selector_list( '.one, .two' ); + * + * // Nested selector lists are retained within their containing selector. + * array( ':is(.a, .b)', 'c' ) === self::split_selector_list( ':is(.a, .b), .c' ); + * + * // Commas within strings do not separate selectors. + * $selectors = self::split_selector_list( '[data-label="Save, continue"],.fallback' ); + * $selectors === array( '[data-label="Save, continue"]', '.fallback' ) + * + * array( 'lang(zh, "*-hant")', '.foo' ) === self::split_selector_list( 'lang(zh, "*-hant"), .foo' ); + * + * // Identifiers may contain escaped commas. + * array( '.foo\,bar', '.baz' ) === self::split_selector_list( '.foo\,bar,.baz' ); + * + * // Comments stay with the selector they follow. + * array( '.a /* a, the first *\/', '.b' ) === self::split_selector_list( '.a /* a, the first *\/,.b' ); + * + * @see https://www.w3.org/TR/selectors/#parse-selector + * @see https://www.w3.org/TR/css-syntax-3/ * * @param string $selector CSS selector list. * @return string[] Selectors. */ - protected static function split_selector_list( $selector ) { + protected static function split_selector_list( $selector ): array { if ( ! str_contains( $selector, ',' ) ) { return array( $selector ); } $selectors = array(); - $current_selector = ''; - $parentheses_depth = 0; $selector_length = strlen( $selector ); + $parentheses_depth = 0; + $at = 0; + $was_at = 0; - for ( $i = 0; $i < $selector_length; $i++ ) { - $char = $selector[ $i ]; + while ( $at < $selector_length ) { + $next_at = $at + strcspn( $selector, '/,\'"()<-\\', $at ); + if ( $next_at >= $selector_length ) { + break; + } - if ( '(' === $char ) { - ++$parentheses_depth; - } elseif ( ')' === $char && $parentheses_depth > 0 ) { - --$parentheses_depth; - } elseif ( ',' === $char && 0 === $parentheses_depth ) { - $selectors[] = $current_selector; - $current_selector = ''; + $next_cp = $selector[ $next_at ]; + + // Escaped syntax characters do not act as delimiters. + if ( '\\' === $next_cp ) { + $at = min( $next_at + 2, $selector_length ); + continue; + } + + /* + * Start of a parenthesized expression, which maintains a stack of parentheses. + * For the sake of this function, no selector list will be split inside parentheses. + * Therefore it’s possible to jump ahead until this list completes. + */ + if ( '(' === $next_cp || ')' === $next_cp ) { + $parentheses_depth += '(' === $next_cp ? 1 : -1; + $at = $next_at + 1; continue; } - $current_selector .= $char; + // Start of a string, which will be incorporated into the selector in which it’s found. + if ( "'" === $next_cp || '"' === $next_cp ) { + $end_of_string = $next_at + 1; + while ( $end_of_string < $selector_length ) { + $end_of_string += strcspn( $selector, "{$next_cp}\\", $end_of_string ); + if ( $end_of_string >= $selector_length ) { + break; + } + + $end_cp = $selector[ $end_of_string ]; + + // Skip escaped characters. + if ( '\\' === $end_cp ) { + $end_of_string = $end_of_string + 2; + continue; + } + + if ( $next_cp === $end_cp ) { + ++$end_of_string; + break; + } + + ++$end_of_string; + } + + $at = $end_of_string; + continue; + } + + // Start of a comment, which will be incorporated into the selector in which it’s found. + if ( '/' === $next_cp && ( $next_at + 1 ) < $selector_length && '*' === $selector[ $next_at + 1 ] ) { + $comment_end_at = strpos( $selector, '*/', $next_at + 1 ); + $is_terminated = false !== $comment_end_at; + $after_comment = $is_terminated ? $comment_end_at + 2 : strlen( $selector ); + $at = $after_comment; + continue; + } + + // Start of a CDO or CDC, which will be incorporated into the selector in which it’s found. + if ( + ( '<' === $next_cp && 0 === substr_compare( $selector, '', $next_at, 3 ) ) + ) { + $at = $next_at + ( '<' === $next_cp ? 4 : 3 ); + continue; + } + + // Everything else is either a comma token or part of a selector. + if ( ',' === $next_cp && 0 === $parentheses_depth ) { + /** + * Trim each selector so that downstream code doesn’t see whitespace + * as the first character in a selector and get confused. + * + * There is inconsistency in this because comments and other syntax + * are included which are also not part of the selector itself, but + * a tradeoff is made between removing common syntax which carries + * no meaning and rarer syntax which leaves auxiliary information. + * + * > A newline, U+0009 CHARACTER TABULATION, or U+0020 SPACE. + * > Note that U+000D CARRIAGE RETURN and U+000C FORM FEED are + * > not included in this definition, as they are converted + * > to U+000A LINE FEED during preprocessing. + * + * @see https://www.w3.org/TR/css-syntax/#whitespace + * @see https://www.w3.org/TR/css-syntax/#newline + */ + $selectors[] = trim( substr( $selector, $was_at, $next_at - $was_at ), " \t\n" ); + $at = $next_at + 1; + $was_at = $at; + continue; + } + + $at = $next_at + 1; } - $selectors[] = $current_selector; + if ( $was_at < $selector_length ) { + $selectors[] = substr( $selector, $was_at ); + } return $selectors; } diff --git a/phpunit/class-wp-theme-json-selector-list.php b/phpunit/class-wp-theme-json-selector-list.php new file mode 100644 index 00000000000000..e453999e8bddb3 --- /dev/null +++ b/phpunit/class-wp-theme-json-selector-list.php @@ -0,0 +1,122 @@ +getMethod( $method_name ); + if ( PHP_VERSION_ID < 80100 ) { + $method->setAccessible( true ); + } + + return $method->invokeArgs( null, $args ); + } + + /** + * @dataProvider data_split_selector_list + * + * @param string $selector CSS selector list. + * @param string[] $expected Expected selectors. + */ + public function test_split_selector_list( $selector, $expected ) { + $this->assertSame( + $expected, + self::invoke_theme_json_method( 'split_selector_list', $selector ) + ); + } + + /** + * @return array + */ + public function data_split_selector_list() { + return array( + 'simple selector list' => array( + 'selector' => 'h1,h2', + 'expected' => array( 'h1', 'h2' ), + ), + 'preserves whitespace around selectors' => array( + 'selector' => '.a , .b , .c', + 'expected' => array( '.a ', ' .b ', ' .c' ), + ), + 'selector function list argument' => array( + 'selector' => ':where(.a, .b),.c', + 'expected' => array( ':where(.a, .b)', '.c' ), + ), + 'nested selector functions' => array( + 'selector' => ':where(:not(.a, .b), .c),.d', + 'expected' => array( ':where(:not(.a, .b), .c)', '.d' ), + ), + 'attribute string containing comma' => array( + 'selector' => '[data-label="Save, continue"],.fallback', + 'expected' => array( '[data-label="Save, continue"]', '.fallback' ), + ), + 'escaped comma in identifier' => array( + 'selector' => '.foo\,bar,.baz', + 'expected' => array( '.foo\,bar', '.baz' ), + ), + 'escaped closing parenthesis in selector function' => array( + 'selector' => ':is(.a\), .b), .c', + 'expected' => array( ':is(.a\), .b)', ' .c' ), + ), + 'quoted function argument before top-level comma' => array( + 'selector' => ':lang(zh, "*-hant"),.foo', + 'expected' => array( ':lang(zh, "*-hant")', '.foo' ), + ), + 'escaped quote and comma inside string' => array( + 'selector' => '[data-x="\",inside"],.b', + 'expected' => array( '[data-x="\",inside"]', '.b' ), + ), + 'comment containing comma' => array( + 'selector' => '.a/*,*/,.b', + 'expected' => array( '.a/*,*/', '.b' ), + ), + ); + } + + /** + * @dataProvider data_prepend_to_selector_uses_safe_splitting + * + * @param string $selector CSS selector list. + * @param string $expected Expected prepended selector. + */ + public function test_prepend_to_selector_uses_safe_splitting( $selector, $expected ) { + $this->assertSame( + $expected, + self::invoke_theme_json_method( 'prepend_to_selector', $selector, '.scope ' ) + ); + } + + /** + * @return array + */ + public function data_prepend_to_selector_uses_safe_splitting() { + return array( + 'fast path for simple selector list' => array( + 'selector' => 'h1,h2', + 'expected' => '.scope h1,.scope h2', + ), + 'escaped comma does not trigger fast-path breakage' => array( + 'selector' => '.foo\,bar,.baz', + 'expected' => '.scope .foo\,bar,.scope .baz', + ), + 'quoted selector function argument is preserved' => array( + 'selector' => ':lang(zh, "*-hant"),.foo', + 'expected' => '.scope :lang(zh, "*-hant"),.scope .foo', + ), + ); + } +} diff --git a/phpunit/class-wp-theme-json-test.php b/phpunit/class-wp-theme-json-test.php index b7a644b4c23e2f..c4704ade4a41f3 100644 --- a/phpunit/class-wp-theme-json-test.php +++ b/phpunit/class-wp-theme-json-test.php @@ -8228,7 +8228,7 @@ public function data_prepend_to_selector() { 'to_prepend' => '.pre ', 'expected' => '.pre .a ,.pre .b ,.pre .c', ), - 'where with internal commas no top-level comma' => array( + 'where with internal commas and no top-level comma' => array( 'selector' => ':where(.a, .b)', 'to_prepend' => '.scope ', 'expected' => '.scope :where(.a, .b)',