From eba6438d791357de755bad86a64fa27872f0e8f2 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 28 Sep 2026 17:10:04 +0200 Subject: [PATCH] fix(overrides): give brand colour overrides the dark scopes, so every dark user sees one colour Every override sat in one :root block with !important. Nextcloud declares a chosen dark theme's colours on body, which a :root value never reaches, so a user who picked Dark theme kept core's colour while a user on System default with a dark OS saw the override. custom-overrides.css now also carries the two dark scopes of the generated dark stylesheets for every brand-layer colour override, with the value DarkPaletteService derives (the light value when it is not a colour literal). Component tokens are left out: both dark users already agree on them, and a body copy would outrank the primary-lock layer. The override import and the config bundle import read the :root block alone, so the dark copies in an exported file do not overwrite the light values. Fixes #698 --- lib/Controller/OverridesController.php | 3 +- lib/Service/ConfigBundleService.php | 2 +- lib/Service/CssParserService.php | 28 ++++ lib/Service/CustomOverridesService.php | 90 ++++++++++- lib/Service/DarkPaletteService.php | 34 +++- .../OverridesControllerValidationTest.php | 7 +- .../Unit/Service/ConfigBundleServiceTest.php | 2 +- .../CustomOverridesServiceDarkScopesTest.php | 150 ++++++++++++++++++ 8 files changed, 302 insertions(+), 14 deletions(-) create mode 100644 tests/Unit/Service/CustomOverridesServiceDarkScopesTest.php diff --git a/lib/Controller/OverridesController.php b/lib/Controller/OverridesController.php index 22da28b3..17ecd6ab 100644 --- a/lib/Controller/OverridesController.php +++ b/lib/Controller/OverridesController.php @@ -225,7 +225,8 @@ public function importOverrides(): JSONResponse { return new JSONResponse(['error' => 'Could not read uploaded file'], 400); } - $parsed = $this->cssParser->parseDeclarations($content); + // The light values only; an exported file also carries the dark blocks. + $parsed = $this->cssParser->parseOverridesFile(css: $content); if ($parsed === null) { return new JSONResponse( ['error' => 'No CSS custom property declarations found in the uploaded file'], diff --git a/lib/Service/ConfigBundleService.php b/lib/Service/ConfigBundleService.php index ff54ac9c..0dad0abe 100644 --- a/lib/Service/ConfigBundleService.php +++ b/lib/Service/ConfigBundleService.php @@ -556,7 +556,7 @@ private function validateOverridesSection(array $bundle, array &$errors): array return ['tokens' => [], 'skipped' => []]; } - $parsed = $this->cssParser->parseDeclarations(content: $css); + $parsed = $this->cssParser->parseOverridesFile(css: $css); if ($parsed === null) { $parsed = []; } diff --git a/lib/Service/CssParserService.php b/lib/Service/CssParserService.php index 9fb5db8c..3b24bd17 100644 --- a/lib/Service/CssParserService.php +++ b/lib/Service/CssParserService.php @@ -210,6 +210,34 @@ public function parseRootBlock(string $css): array { return []; }//end parseRootBlock() + /** + * Read the light values out of a custom-overrides file. + * + * The file custom-overrides.css carries its light values in a `:root` block + * and, for brand colour overrides, the same tokens again in two dark blocks. Reading every + * declaration in the file would let the dark values overwrite the light + * ones on import. So a file with a `:root` block is read from that block + * alone; a file of bare declarations (a hand-written import) is read whole. + * + * @param string $css The raw file content. + * + * @return array|null Token => light value, or null when the file declares nothing. + * + * @spec openspec/changes/authoring-token-value-types/tasks.md#task-2.2 + */ + public function parseOverridesFile(string $css): ?array { + if (preg_match('/:root\s*\{/', $css) === 1) { + $root = $this->parseRootBlock(css: $css); + if (empty($root) === true) { + return null; + } + + return $root; + } + + return $this->parseDeclarations(content: $css); + }//end parseOverridesFile() + /** * Parse hand-authored dark-mode declarations from a top-level * `@media (prefers-color-scheme: dark) { :root { ... } }` block. diff --git a/lib/Service/CustomOverridesService.php b/lib/Service/CustomOverridesService.php index 7e4a4cb2..3175ea9d 100644 --- a/lib/Service/CustomOverridesService.php +++ b/lib/Service/CustomOverridesService.php @@ -37,11 +37,15 @@ * It validates all token names against the TokenRegistry before writing. * * The CSS file format is strictly controlled: - * - Single :root {} block + * - One :root {} block with the light values, read back by read() + * - For every brand-layer colour override, the two dark scopes of the generated dark + * stylesheets with its derived dark value, so a user who chose the dark + * theme (whose colours Nextcloud declares on body) and a user whose system + * is dark see the same colour * - One declaration per line * - Each declaration carries !important so user overrides win the cascade over * the nldesign design-system stylesheets and Nextcloud core theming - * - No selectors other than :root + * - No selectors other than :root and the two dark scopes * * @spec openspec/changes/retrofit-2026-05-24-annotate-nldesign/tasks.md#task-8 * @spec openspec/changes/retrofit-2026-05-24-annotate-nldesign/tasks.md#task-28 @@ -54,6 +58,22 @@ */ class CustomOverridesService { + /** + * The scope of a user whose system is dark and who chose no theme (as in + * the generated dark stylesheets). + * + * @var string + */ + private const SYSTEM_DARK_SELECTOR = 'body:not([data-theme-light]):not([data-theme-dark])' + . ':not([data-theme-light-highcontrast]):not([data-theme-dark-highcontrast])'; + + /** + * The scope of a user who chose the dark theme (as in the generated dark stylesheets). + * + * @var string + */ + private const CHOSEN_DARK_SELECTOR = 'body[data-theme-dark],' . PHP_EOL . 'body[data-themes*=dark]'; + /** * The CSS file header comment. * @@ -75,15 +95,25 @@ class CustomOverridesService { */ private CssParserService $cssParser; + /** + * The dark palette, which derives a colour override's dark value exactly + * as the generated dark stylesheets do. + * + * @var DarkPaletteService + */ + private DarkPaletteService $darkPalette; + /** * Constructor. * * @param IAppManager $appManager The app manager. * @param CssParserService $cssParser CSS parser for :root block extraction. + * @param DarkPaletteService $darkPalette Derives each colour override's dark value. */ - public function __construct(IAppManager $appManager, CssParserService $cssParser) { + public function __construct(IAppManager $appManager, CssParserService $cssParser, DarkPaletteService $darkPalette) { $this->appManager = $appManager; $this->cssParser = $cssParser; + $this->darkPalette = $darkPalette; }//end __construct() /** @@ -277,10 +307,62 @@ private function buildCss(array $tokens): string { } $lines = $this->buildDeclarationLines(tokens: $tokens); + $css = $header . ':root {' . PHP_EOL . implode(PHP_EOL, $lines) . PHP_EOL . '}' . PHP_EOL; + + $darkLines = $this->buildDeclarationLines(tokens: $this->darkValues(tokens: $tokens)); + if (empty($darkLines) === true) { + return $css; + } - return $header . ':root {' . PHP_EOL . implode(PHP_EOL, $lines) . PHP_EOL . '}' . PHP_EOL; + // The same two scopes the generated dark stylesheets use. A user who + // chose the dark theme gets Nextcloud's dark colours declared on body, + // which a :root value never reaches; a body-level declaration wins for + // both kinds of dark user. + $css .= '@media (prefers-color-scheme: dark) {' . PHP_EOL + . ' ' . self::SYSTEM_DARK_SELECTOR . ' {' . PHP_EOL + . ' ' . implode(PHP_EOL . ' ', $darkLines) . PHP_EOL + . ' }' . PHP_EOL + . '}' . PHP_EOL + . self::CHOSEN_DARK_SELECTOR . ' {' . PHP_EOL + . implode(PHP_EOL, $darkLines) . PHP_EOL + . '}' . PHP_EOL; + + return $css; }//end buildCss() + /** + * The dark value of every brand-layer colour override: derived as the + * generated dark stylesheets derive it, or the light value when it is not + * a colour literal, so both kinds of dark user still see the same thing. + * + * Only the brand layer (Nextcloud's own variables) is split today, because + * only those does Nextcloud re-declare on body for a chosen theme. Component + * tokens are left out: both kinds of dark user already get the same body + * value from the generated dark stylesheet, and a body-level copy here would + * outrank the primary-lock layer, which locks them at :root. + * + * @param array $tokens Token name => light value. + * + * @return array Colour token name => dark value. + * + * @SuppressWarnings(PHPMD.StaticAccess) - TokenRegistry uses static methods by design + * + * @spec openspec/changes/authoring-token-value-types/tasks.md#task-2.2 + */ + private function darkValues(array $tokens): array { + $registry = TokenRegistry::getTokens(); + $dark = []; + foreach ($tokens as $name => $value) { + if (($registry[$name]['type'] ?? '') !== 'color' || ($registry[$name]['group'] ?? '') !== 'brand') { + continue; + } + + $dark[$name] = ($this->darkPalette->deriveDarkValue(token: $name, lightValue: $value, context: $tokens) ?? $value); + } + + return $dark; + }//end darkValues() + /** * Build individual CSS declaration lines from a token map. * diff --git a/lib/Service/DarkPaletteService.php b/lib/Service/DarkPaletteService.php index 8add6046..3fbf507c 100644 --- a/lib/Service/DarkPaletteService.php +++ b/lib/Service/DarkPaletteService.php @@ -314,22 +314,44 @@ public function deriveDarkDeclarations(array $lightDeclarations): array { // propagates through this alias in dark mode; the alternative is a // dark mode that only ever half-applies. $literal = $this->resolveAlias(value: $value, declarations: $lightDeclarations); - $rgba = $this->contrast->parseColorWithAlpha(value: $literal); - if ($rgba === null) { + $darkValue = $this->deriveDarkValue(token: $token, lightValue: $literal, context: $lightDeclarations); + if ($darkValue === null) { // Unparseable (gradient, keyword, size, font stack, url(), an // alias chain with no literal at the end) — skip. continue; } - // The dark channels come from the opaque colour; a translucent light - // value keeps its alpha, so an overlay stays an overlay in dark mode. - $dark[$token] = $this->deriveColorToken(token: $token, rgb: [$rgba[0], $rgba[1], $rgba[2]], lightDeclarations: $lightDeclarations) - . $this->alphaSuffix(alpha: $rgba[3]); + $dark[$token] = $darkValue; } return $this->regenerateRgbCompanions(lightDeclarations: $lightDeclarations, darkDeclarations: $dark); }//end deriveDarkDeclarations() + /** + * Derive the dark value of one colour literal, as the generated dark + * stylesheets do, so the token editor and the generator never disagree. + * + * The dark channels come from the opaque colour; a translucent light value + * keeps its alpha, so an overlay stays an overlay in dark mode. + * + * @param string $token The token name (decides text-class or surface-class). + * @param string $lightValue The light colour literal. + * @param array $context The light declarations around it, for the brand-primary exception. + * + * @return string|null The dark hex value, or null when the value is not a colour literal. + * + * @spec openspec/changes/authoring-token-value-types/tasks.md#task-2.3 + */ + public function deriveDarkValue(string $token, string $lightValue, array $context = []): ?string { + $rgba = $this->contrast->parseColorWithAlpha(value: $lightValue); + if ($rgba === null) { + return null; + } + + return $this->deriveColorToken(token: $token, rgb: [$rgba[0], $rgba[1], $rgba[2]], lightDeclarations: $context) + . $this->alphaSuffix(alpha: $rgba[3]); + }//end deriveDarkValue() + /** * Follow a `var()` alias chain to the literal it ends at. * diff --git a/tests/Unit/Controller/OverridesControllerValidationTest.php b/tests/Unit/Controller/OverridesControllerValidationTest.php index dd04a5bc..8c4ba0bf 100644 --- a/tests/Unit/Controller/OverridesControllerValidationTest.php +++ b/tests/Unit/Controller/OverridesControllerValidationTest.php @@ -14,12 +14,15 @@ namespace OCA\Thematiq\Tests\Unit\Controller; use OCA\Thematiq\Controller\OverridesController; +use OCA\Thematiq\Service\ContrastService; use OCA\Thematiq\Service\CssParserService; use OCA\Thematiq\Service\CustomOverridesService; +use OCA\Thematiq\Service\DarkPaletteService; use OCA\Thematiq\Service\ThemingAuditService; use OCP\App\IAppManager; use OCP\IRequest; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; /** * Thematiq#694: a save with an unknown token name or an unsafe value answered @@ -69,7 +72,9 @@ protected function setUp(): void { $appManager = $this->createMock(IAppManager::class); $appManager->method('getAppPath')->willReturn($this->appDir); - $this->overridesService = new CustomOverridesService($appManager, new CssParserService()); + $parser = new CssParserService(); + $darkPalette = new DarkPaletteService(new ContrastService(), $parser, $appManager, $this->createMock(LoggerInterface::class)); + $this->overridesService = new CustomOverridesService($appManager, $parser, $darkPalette); $this->overridesService->write(tokens: ['--color-primary' => '#000000']); $this->auditService = $this->createMock(ThemingAuditService::class); diff --git a/tests/Unit/Service/ConfigBundleServiceTest.php b/tests/Unit/Service/ConfigBundleServiceTest.php index e738457c..5be695ac 100644 --- a/tests/Unit/Service/ConfigBundleServiceTest.php +++ b/tests/Unit/Service/ConfigBundleServiceTest.php @@ -136,7 +136,7 @@ function (string $app, string $key, $value): void { $customTokenSetValidator = new CustomTokenSetValidator(); $logger = $this->createMock(LoggerInterface::class); - $this->overridesService = new CustomOverridesService($appManager, $cssParser); + $this->overridesService = new CustomOverridesService($appManager, $cssParser, new DarkPaletteService($contrast, $cssParser, $appManager, $logger)); $this->customTokenSetService = new CustomTokenSetService( $appManager, $config, diff --git a/tests/Unit/Service/CustomOverridesServiceDarkScopesTest.php b/tests/Unit/Service/CustomOverridesServiceDarkScopesTest.php new file mode 100644 index 00000000..8be296a6 --- /dev/null +++ b/tests/Unit/Service/CustomOverridesServiceDarkScopesTest.php @@ -0,0 +1,150 @@ +appDir = sys_get_temp_dir() . '/thematiq-698-' . bin2hex(random_bytes(4)); + mkdir($this->appDir . '/css', 0777, true); + + $appManager = $this->createMock(IAppManager::class); + $appManager->method('getAppPath')->willReturn($this->appDir); + $parser = new CssParserService(); + $this->darkPalette = new DarkPaletteService(new ContrastService(), $parser, $appManager, $this->createMock(LoggerInterface::class)); + $this->service = new CustomOverridesService($appManager, $parser, $this->darkPalette); + }//end setUp() + + protected function tearDown(): void { + foreach (glob($this->appDir . '/css/*') ?: [] as $file) { + unlink($file); + } + + rmdir($this->appDir . '/css'); + rmdir($this->appDir); + parent::tearDown(); + }//end tearDown() + + /** + * The declarations inside the block that follows a selector. + * + * @param string $css The file content. + * @param string $selector The block's selector. + * + * @return array Token => value, `!important` stripped. + */ + private function block(string $css, string $selector): array { + $start = strpos($css, $selector . ' {'); + $this->assertNotFalse($start, 'custom-overrides.css has no block for ' . $selector); + $open = strpos($css, '{', $start); + $close = strpos($css, '}', $open); + preg_match_all('/(--[A-Za-z0-9_-]+)\s*:\s*([^;]+);/', substr($css, $open + 1, $close - $open - 1), $matches, PREG_SET_ORDER); + $result = []; + foreach ($matches as $match) { + $result[$match[1]] = trim(str_replace('!important', '', $match[2])); + } + + return $result; + }//end block() + + /** + * A colour override gets the same derived dark value in both dark scopes, + * so both kinds of dark user see one colour. + */ + public function testDarkScopesWritten(): void { + $this->service->write(tokens: ['--color-primary' => '#154273', '--border-radius' => '4px']); + $css = $this->service->getRawContent(); + + $expected = $this->darkPalette->deriveDarkValue(token: '--color-primary', lightValue: '#154273'); + $this->assertNotSame('#154273', $expected, 'a dark primary is derived, not the light value'); + + $systemDark = $this->block(css: $css, selector: self::SYSTEM_DARK); + $chosenDark = $this->block(css: $css, selector: self::CHOSEN_DARK); + + $this->assertSame(['--color-primary' => $expected], $systemDark); + $this->assertSame(['--color-primary' => $expected], $chosenDark); + $this->assertStringContainsString('@media (prefers-color-scheme: dark)', $css); + $this->assertMatchesRegularExpression('/--color-primary: [^;]+ !important;/', substr($css, (int)strpos($css, 'body[data-theme-dark]'))); + }//end testDarkScopesWritten() + + /** + * A component token gets no dark copy: the generated dark stylesheet already + * gives both kinds of dark user the same body value, and a body-level copy + * here would outrank the primary-lock layer. + */ + public function testComponentTokensGetNoDarkCopy(): void { + $this->service->write(tokens: ['--color-primary' => '#154273', '--nldesign-component-header-background-color' => '#112233']); + $css = $this->service->getRawContent(); + + $this->assertArrayNotHasKey('--nldesign-component-header-background-color', $this->block(css: $css, selector: self::CHOSEN_DARK)); + }//end testComponentTokensGetNoDarkCopy() + + /** + * An exported file imports back to its light values, not the dark copies + * that follow them in the file. + */ + public function testAnExportedFileReadsBackItsLightValues(): void { + $this->service->write(tokens: ['--color-primary' => '#154273']); + + $this->assertSame( + ['--color-primary' => '#154273'], + (new CssParserService())->parseOverridesFile(css: $this->service->getRawContent()) + ); + $this->assertSame(['--x' => '1px'], (new CssParserService())->parseOverridesFile(css: '--x: 1px;')); + }//end testAnExportedFileReadsBackItsLightValues() + + /** + * The light values still read back from the :root block alone. + */ + public function testLightValuesReadBack(): void { + $this->service->write(tokens: ['--color-primary' => '#154273', '--border-radius' => '4px']); + + $this->assertSame(['--color-primary' => '#154273', '--border-radius' => '4px'], $this->service->read()); + }//end testLightValuesReadBack() + + /** + * A colour the palette cannot parse keeps its light value in the dark + * scopes, so the two kinds of dark user still agree. + */ + public function testAnUnparseableColourKeepsItsValueInBothDarkScopes(): void { + $this->service->write(tokens: ['--color-primary' => 'var(--brand-blue)']); + $css = $this->service->getRawContent(); + + $this->assertSame(['--color-primary' => 'var(--brand-blue)'], $this->block(css: $css, selector: self::CHOSEN_DARK)); + $this->assertSame(['--color-primary' => 'var(--brand-blue)'], $this->block(css: $css, selector: self::SYSTEM_DARK)); + }//end testAnUnparseableColourKeepsItsValueInBothDarkScopes() +}//end class