From e70a55fc3124c4261df69e61496d378cca0110ec Mon Sep 17 00:00:00 2001 From: Jake Hotson Date: Mon, 27 Jan 2025 23:12:48 +0000 Subject: [PATCH 1/4] [BUGFIX] Render RGB functions with "modern" syntax if required The "legacy" syntax does not allow a mixture of `percentage`s and `number`s for the red, green and blue components. So if `rgb`/`rgba` functions that have such a mixture are rendered in the "legacy" syntax, an invalid property value will result. An `OutputFormat` option to use the "modern" syntax throughout will be added later (see #801). --- CHANGELOG.md | 1 + src/Value/Color.php | 64 ++++++++++++++++++++++++++++++++++ tests/Unit/Value/ColorTest.php | 5 --- 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ca3c21c3..3cc0fd2ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Please also have a look at our - Partial support for CSS Color Module Level 4: - `rgb` and `rgba`, and `hsl` and `hsla` are now aliases (#797} - Parse color functions that use the "modern" syntax (#800) + - Render RGB functions with "modern" syntax when required (#840) - Add a class diagram to the README (#482) - Add more tests (#449) diff --git a/src/Value/Color.php b/src/Value/Color.php index 5dd832a61..c88dc06a3 100644 --- a/src/Value/Color.php +++ b/src/Value/Color.php @@ -231,6 +231,8 @@ public function render(OutputFormat $outputFormat): string && $this->allComponentsAreNumbers() ) { return $this->renderAsHex(); + } elseif ($this->shouldRenderInModernSyntax()) { + return $this->renderInModernSyntax($outputFormat); } return parent::render($outputFormat); @@ -282,4 +284,66 @@ private function renderAsHex(): string return '#' . ($canUseShortVariant ? $result[0] . $result[2] . $result[4] : $result); } + + /** + * The "legacy" syntax does not allow RGB colors to have a mixture of `percentage`s and `number`s. + */ + private function shouldRenderInModernSyntax(): bool + { + $function = $this->getRealName(); + if ($function !== 'rgb' && $function !== 'rgba') { + return false; + } + + $hasPercentage = false; + $hasNumber = false; + foreach ($this->aComponents as $key => $value) { + if ($key === 'a') { + // ALpha can have units that don't match those of the RGB components in the "legacy" syntax. + // So it is not necessary to check it. It's also always last, hence `break` rather than `continue`. + break; + } + if (!$value instanceof Size) { + // Unexpected, unknown, or modified via the API + return false; + } + $unit = $value->getUnit(); + // `switch` only does loose comparison + if ($unit === null) { + $hasNumber = true; + } elseif ($unit === '%') { + $hasPercentage = true; + } else { + // Invalid unit + return false; + } + } + + return $hasPercentage && $hasNumber; + } + + /** + * @return non-empty-string + */ + private function renderInModernSyntax(OutputFormat $outputFormat): string + { + \end($this->aComponents); + if (\key($this->aComponents) === 'a') { + $alpha = $this->aComponents['a']; + $componentsWithoutAlpha = \array_diff_key($this->aComponents, ['a' => 0]); + } else { + $componentsWithoutAlpha = $this->aComponents; + } + + $arguments = $outputFormat->implode(' ', $componentsWithoutAlpha); + if (isset($alpha)) { + $arguments = $outputFormat->implode( + $outputFormat->spaceBeforeListArgumentSeparator('/') . '/' + . $outputFormat->spaceAfterListArgumentSeparator('/'), + [$arguments, $alpha] + ); + } + + return $this->getName() . '(' . $arguments . ')'; + } } diff --git a/tests/Unit/Value/ColorTest.php b/tests/Unit/Value/ColorTest.php index 2b533a7f8..5c502f824 100644 --- a/tests/Unit/Value/ColorTest.php +++ b/tests/Unit/Value/ColorTest.php @@ -86,8 +86,6 @@ public static function provideValidColorAndExpectedRendering(): array 'rgb(0 119 0)', '#070', ], - // The "legacy" syntax currently used for rendering does not allow a mixture of percentages and numbers. - /* 'modern rgb with percentage R' => [ 'rgb(0% 119 0)', 'rgb(0% 119 0)', @@ -112,7 +110,6 @@ public static function provideValidColorAndExpectedRendering(): array 'rgb(0 60% 0%)', 'rgb(0 60% 0%)', ], - //*/ 'modern rgb with percentage components' => [ 'rgb(0% 60% 0%)', 'rgb(0%,60%,0%)', @@ -131,7 +128,6 @@ public static function provideValidColorAndExpectedRendering(): array 'rgb(0 119 0 / 50%)', 'rgba(0,119,0,50%)', ], - /* 'modern rgba with percentage R' => [ 'rgb(0% 119 0 / 0.5)', 'rgba(0% 119 0/.5)', @@ -144,7 +140,6 @@ public static function provideValidColorAndExpectedRendering(): array 'rgb(0 119 0% / 0.5)', 'rgba(0 119 0%/.5)', ], - //*/ 'modern rgba with percentage RGB' => [ 'rgb(0% 60% 0% / 0.5)', 'rgba(0%,60%,0%,.5)', From 148f26592f3b3ce1df12106b77ed2f6e7e3daf43 Mon Sep 17 00:00:00 2001 From: JakeQZ Date: Mon, 27 Jan 2025 23:52:05 +0000 Subject: [PATCH 2/4] Correct typo in comment Co-authored-by: Oliver Klee --- src/Value/Color.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Value/Color.php b/src/Value/Color.php index c88dc06a3..1c24492b5 100644 --- a/src/Value/Color.php +++ b/src/Value/Color.php @@ -299,7 +299,7 @@ private function shouldRenderInModernSyntax(): bool $hasNumber = false; foreach ($this->aComponents as $key => $value) { if ($key === 'a') { - // ALpha can have units that don't match those of the RGB components in the "legacy" syntax. + // Alpha can have units that don't match those of the RGB components in the "legacy" syntax. // So it is not necessary to check it. It's also always last, hence `break` rather than `continue`. break; } From e966c063dd330e52d6dd6fec730005d190efe0fa Mon Sep 17 00:00:00 2001 From: Jake Hotson Date: Wed, 29 Jan 2025 01:36:57 +0000 Subject: [PATCH 3/4] Changes suggested in review References to add to final commit message: - https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb#formal_syntax - https://www.w3.org/TR/css-color-4/#rgb-functions --- src/Value/Color.php | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/Value/Color.php b/src/Value/Color.php index 1c24492b5..ba24e6c2e 100644 --- a/src/Value/Color.php +++ b/src/Value/Color.php @@ -231,7 +231,9 @@ public function render(OutputFormat $outputFormat): string && $this->allComponentsAreNumbers() ) { return $this->renderAsHex(); - } elseif ($this->shouldRenderInModernSyntax()) { + } + + if ($this->shouldRenderInModernSyntax()) { return $this->renderInModernSyntax($outputFormat); } @@ -287,11 +289,23 @@ private function renderAsHex(): string /** * The "legacy" syntax does not allow RGB colors to have a mixture of `percentage`s and `number`s. + * + * The "legacy" and "modern" monikers are part of the formal W3C syntax. + * See the following for more information: + * - {@link + * https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb#formal_syntax + * Description of the formal syntax for `rgb()` on MDN + * }; + * - {@link + * https://www.w3.org/TR/css-color-4/#rgb-functions + * The same in the CSS Color Module Level 4 W3C Candidate Recommendation Draft + * } (as of 13 February 2024, at time of writing). */ private function shouldRenderInModernSyntax(): bool { - $function = $this->getRealName(); - if ($function !== 'rgb' && $function !== 'rgba') { + static $colorFunctionsThatWithMixedValueTypesCannotBeRenderedInLegacySyntax = ['rgb', 'rgba']; + $colorFunction = $this->getRealName(); + if (!\in_array($colorFunction, $colorFunctionsThatWithMixedValueTypesCannotBeRenderedInLegacySyntax, true)) { return false; } @@ -327,21 +341,19 @@ private function shouldRenderInModernSyntax(): bool */ private function renderInModernSyntax(OutputFormat $outputFormat): string { - \end($this->aComponents); - if (\key($this->aComponents) === 'a') { + // Maybe not yet without alpha, but will be... + $componentsWithoutAlpha = $this->aComponents; + \end($componentsWithoutAlpha); + if (\key($componentsWithoutAlpha) === 'a') { $alpha = $this->aComponents['a']; - $componentsWithoutAlpha = \array_diff_key($this->aComponents, ['a' => 0]); - } else { - $componentsWithoutAlpha = $this->aComponents; + unset($componentsWithoutAlpha['a']); } $arguments = $outputFormat->implode(' ', $componentsWithoutAlpha); if (isset($alpha)) { - $arguments = $outputFormat->implode( - $outputFormat->spaceBeforeListArgumentSeparator('/') . '/' - . $outputFormat->spaceAfterListArgumentSeparator('/'), - [$arguments, $alpha] - ); + $separator = $outputFormat->spaceBeforeListArgumentSeparator('/') + . '/' . $outputFormat->spaceAfterListArgumentSeparator('/'); + $arguments = $outputFormat->implode($separator, [$arguments, $alpha]); } return $this->getName() . '(' . $arguments . ')'; From ecaccc5e5f5162d1d5867ec438ee5f44bf793bf0 Mon Sep 17 00:00:00 2001 From: Jake Hotson Date: Thu, 30 Jan 2025 02:53:59 +0000 Subject: [PATCH 4/4] Further changes suggested in review --- src/Value/Color.php | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/Value/Color.php b/src/Value/Color.php index ba24e6c2e..af6efa956 100644 --- a/src/Value/Color.php +++ b/src/Value/Color.php @@ -303,9 +303,7 @@ private function renderAsHex(): string */ private function shouldRenderInModernSyntax(): bool { - static $colorFunctionsThatWithMixedValueTypesCannotBeRenderedInLegacySyntax = ['rgb', 'rgba']; - $colorFunction = $this->getRealName(); - if (!\in_array($colorFunction, $colorFunctionsThatWithMixedValueTypesCannotBeRenderedInLegacySyntax, true)) { + if (!$this->colorFunctionMayHaveMixedValueTypes($this->getRealName())) { return false; } @@ -317,7 +315,7 @@ private function shouldRenderInModernSyntax(): bool // So it is not necessary to check it. It's also always last, hence `break` rather than `continue`. break; } - if (!$value instanceof Size) { + if (!($value instanceof Size)) { // Unexpected, unknown, or modified via the API return false; } @@ -336,6 +334,19 @@ private function shouldRenderInModernSyntax(): bool return $hasPercentage && $hasNumber; } + /** + * Some color functions, such as `rgb`, + * may have a mixture of `percentage`, `number`, or possibly other types in their arguments. + * + * Note that this excludes the alpha component, which is treated separately. + */ + private function colorFunctionMayHaveMixedValueTypes(string $function): bool + { + $functionsThatMayHaveMixedValueTypes = ['rgb', 'rgba']; + + return \in_array($function, $functionsThatMayHaveMixedValueTypes, true); + } + /** * @return non-empty-string */