Remove useless shouldOverride

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<Style.JSON> {
+  /**
+   * 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<Declaration>,
     device: Device,
@@ -49,7 +56,8 @@ export class Style implements Serializable<Style.JSON> {
     /**
      * 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,16 +76,18 @@ export class Style implements Serializable<Style.JSON> {
     /**
      * 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<Name, Value>();
 
     for (const declaration of declarations) {
       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<Style.JSON> {
           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<T>(
-  previous: Option<Value<T>>,
-  next: Declaration,
-): boolean {
-  return previous.every(
-    (previous) =>
-      next.important &&
-      previous.source.every((declaration) => !declaration.important),
-  );
-  // return false;
-}
-
 function parseLonghand<N extends Longhands.Name>(
   property: Longhands.Property[N],
   value: string,

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,29 +25,29 @@ 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
    * i.e. "--foo: lorem ipsum" becomes "foo => [lorem, ipsum]"
    **/
   export function gather(
     declarations: Array<Declaration>,
-    shouldOverride: <T>(
-      previous: Option<Value<T>>,
-      next: Declaration,
-    ) => boolean,
+    // shouldOverride: <T>(
+    //   previous: Option<Value<T>>,
+    //   next: Declaration,
+    // ) => boolean,
   ): DefinitionMap {
     let currentVariables: DefinitionMap = Map.empty();
 
     for (const declaration of declarations.filter((declaration) =>
       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)),

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 = <div class="foo" id="foo" />;
+
+  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 = <div />;
 

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(), [
     [