From a0447b8d45a0b96a89eb48d67fbf52d1748c0cb7 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Mon, 15 Jan 2024 11:05:12 +0100 Subject: [PATCH] Remove useless shouldOverride --- packages/alfa-style/src/style.ts | 50 ++++++++--------------- packages/alfa-style/src/variable.ts | 19 ++++----- packages/alfa-style/test/style.spec.tsx | 24 +++++++++++ packages/alfa-style/test/variable.spec.ts | 47 +++++++++------------ 4 files changed, 69 insertions(+), 71 deletions(-) diff --git a/packages/alfa-style/src/style.ts b/packages/alfa-style/src/style.ts index 8782a15d4b..deace9f9ce 100644 --- a/packages/alfa-style/src/style.ts +++ b/packages/alfa-style/src/style.ts @@ -20,10 +20,10 @@ import { Context } from "@siteimprove/alfa-selector"; import { Slice } from "@siteimprove/alfa-slice"; import * as element from "./element/element"; -import * as node from "./node/node"; import { Longhand } from "./longhand"; import { Longhands } from "./longhands"; +import * as node from "./node/node"; import { Shorthand } from "./shorthand"; import { Shorthands } from "./shorthands"; @@ -36,6 +36,13 @@ type Name = Longhands.Name; * @public */ export class Style implements Serializable { + /** + * Build a style from a list of declarations. + * + * @remarks + * Declarations must be in pre-sorted in decreasing Cascade order. + * Prefer using Style.from(), which has fewer assumptions. + */ public static of( styleDeclarations: Iterable, device: Device, @@ -49,7 +56,8 @@ export class Style implements Serializable { /** * First pass, substitute all variable by their definition */ - const declaredVariables = Variable.gather(declarations, shouldOverride); + // First step: gather all variable declarations. + const declaredVariables = Variable.gather(declarations); // Second step: since CSS variables are always inherited, and inheritance // takes precedence over fallback, we can merge the current variables with @@ -68,6 +76,10 @@ export class Style implements Serializable { /** * Second pass: Resolve cascading properties using the cascading variables * from the first pass. + * + * Since declarations have been sorted in decreasing cascade order by the + * cascade, the first value we encounter for each property is the correct + * one for the cascaded value. */ let properties = Map.empty(); @@ -75,9 +87,7 @@ export class Style implements Serializable { const { name, value } = declaration; if (Longhands.isName(name)) { - const previous = properties.get(name); - - if (shouldOverride(previous, declaration)) { + if (properties.get(name).isNone()) { for (const result of parseLonghand( Longhands.get(name), value, @@ -96,9 +106,7 @@ export class Style implements Serializable { variables, )) { for (const [name, value] of result) { - const previous = properties.get(name); - - if (shouldOverride(previous, declaration)) { + if (properties.get(name).isNone()) { properties = properties.set( name, Value.of(value, Option.of(declaration)), @@ -378,32 +386,6 @@ export namespace Style { export const { isRendered, isVisible } = node; } -/** - * The "next" declaration should override the previous if: - * - either there is no previous; or - * - next is important and previous isn't. - * This supposes that the declarations have been pre--ordered in otherwise - * decreasing precedence. - * - * @privateRemarks - * This is not correct since importance of declaration reverses precedence of UA - * and author origins. - * {@link https://github.com/Siteimprove/alfa/issues/1532} - * - * @internal - */ -export function shouldOverride( - previous: Option>, - next: Declaration, -): boolean { - return previous.every( - (previous) => - next.important && - previous.source.every((declaration) => !declaration.important), - ); - // return false; -} - function parseLonghand( property: Longhands.Property[N], value: string, diff --git a/packages/alfa-style/src/variable.ts b/packages/alfa-style/src/variable.ts index 2dfa2ad8dd..9a8db46602 100644 --- a/packages/alfa-style/src/variable.ts +++ b/packages/alfa-style/src/variable.ts @@ -1,6 +1,5 @@ import { Array } from "@siteimprove/alfa-array"; -import { Component, Keyword, Lexer } from "@siteimprove/alfa-css"; -import { Token } from "@siteimprove/alfa-css"; +import { Component, Keyword, Lexer, Token } from "@siteimprove/alfa-css"; import type { Declaration } from "@siteimprove/alfa-dom"; import { Iterable } from "@siteimprove/alfa-iterable"; import { Map } from "@siteimprove/alfa-map"; @@ -26,8 +25,10 @@ export namespace Variable { /** * Gather variables that are declared on the declarations. + * + * @remarks * The same variable may be declared several times, so we rely on - * declarations being pre-ordered by decreasing specificity and only take + * declarations being pre-ordered by decreasing precedence and only take * the first declaration. * * This builds a map from variable names to their lexed value @@ -35,10 +36,10 @@ export namespace Variable { **/ export function gather( declarations: Array, - shouldOverride: ( - previous: Option>, - next: Declaration, - ) => boolean, + // shouldOverride: ( + // previous: Option>, + // next: Declaration, + // ) => boolean, ): DefinitionMap { let currentVariables: DefinitionMap = Map.empty(); @@ -46,9 +47,7 @@ export namespace Variable { declaration.name.startsWith("--"), )) { const { name, value } = declaration; - const previous = currentVariables.get(name); - - if (shouldOverride(previous, declaration)) { + if (currentVariables.get(name).isNone()) { currentVariables = currentVariables.set( name, Value.of(Lexer.lex(value), Option.of(declaration)), diff --git a/packages/alfa-style/test/style.spec.tsx b/packages/alfa-style/test/style.spec.tsx index 3cfd364174..54010841af 100644 --- a/packages/alfa-style/test/style.spec.tsx +++ b/packages/alfa-style/test/style.spec.tsx @@ -199,6 +199,30 @@ test(`#cascaded() expands a var() function`, (t) => { }); }); +test("#cascaded() prefer important var() declarations", (t) => { + const element =
; + + h.document( + [element], + [ + h.sheet([ + h.rule.style("div", { overflowX: "var(--hidden)" }), + h.rule.style(".foo", { "--hidden": "hidden !important" }), + // Higher specificity, but not important + h.rule.style("#foo", { "--hidden": "visible" }), + ]), + ], + ); + + t.deepEqual(cascaded(element, "overflow-x"), { + value: { + type: "keyword", + value: "hidden", + }, + source: h.declaration("overflow-x", "var(--hidden)").toJSON(), + }); +}); + test(`#cascaded() expands a var() function with a fallback`, (t) => { const element =
; diff --git a/packages/alfa-style/test/variable.spec.ts b/packages/alfa-style/test/variable.spec.ts index 1088b4a943..0e7dd2bff6 100644 --- a/packages/alfa-style/test/variable.spec.ts +++ b/packages/alfa-style/test/variable.spec.ts @@ -3,7 +3,6 @@ import { test } from "@siteimprove/alfa-test"; import { Declaration } from "@siteimprove/alfa-dom"; import { Variable } from "../src/variable"; -import { shouldOverride } from "../src/style"; const varFoo = (value: string, important: boolean = false) => Declaration.of("--foo", value, important); @@ -13,16 +12,18 @@ const propFoo = Declaration.of("foo", "foo"); const propBar = Declaration.of("bar", "bar"); test("gather() builds an empty map when there are no declarations", (t) => { - const actual = Variable.gather([], shouldOverride); + const actual = Variable.gather([]); t.deepEqual(actual.toJSON(), []); }); test("gather() gather all declaration starting with --", (t) => { - const actual = Variable.gather( - [propFoo, varFoo("foo"), varBar("bar"), propBar], - shouldOverride, - ); + const actual = Variable.gather([ + propFoo, + varFoo("foo"), + varBar("bar"), + propBar, + ]); t.deepEqual(actual.toJSON(), [ [ @@ -42,16 +43,13 @@ test("gather() gather all declaration starting with --", (t) => { ]); }); -test("gather() prefers the first declaration at same importance", (t) => { - const actual = Variable.gather( - [ - varFoo("foo"), - varBar("bar", true), - varFoo("not foo"), - varBar("not bar", true), - ], - shouldOverride, - ); +test("gather() prefers the first declaration", (t) => { + const actual = Variable.gather([ + varFoo("foo"), + varBar("bar", true), + varFoo("not foo"), + varBar("not bar", true), + ]); t.deepEqual(actual.toJSON(), [ [ @@ -71,28 +69,23 @@ test("gather() prefers the first declaration at same importance", (t) => { ]); }); -test("gather() prefers important declaration", (t) => { - const actual = Variable.gather( - [varFoo("not foo"), varFoo("foo", true)], - shouldOverride, - ); +test("gather() does not consider precedence of declarations", (t) => { + // gather() doesn't care about precedence and requires its input to be sorted + const actual = Variable.gather([varFoo("notfoo"), varFoo("foo", true)]); t.deepEqual(actual.toJSON(), [ [ "--foo", { - value: [{ type: "ident", value: "foo" }], - source: { name: "--foo", value: "foo", important: true }, + value: [{ type: "ident", value: "notfoo" }], + source: { name: "--foo", value: "notfoo", important: false }, }, ], ]); }); test("flatten() expands variable definitions", (t) => { - const map = Variable.gather( - [varFoo("var(--bar)"), varBar("hello")], - shouldOverride, - ); + const map = Variable.gather([varFoo("var(--bar)"), varBar("hello")]); t.deepEqual(Variable.flatten(map).toJSON(), [ [