Bug 2068024 - Stylelint: create the no-background-without-text-color rule r=hjones,desktop-theme-reviewers

The rule ships `meta.fixable: false`; bug 2068773 adds the fix, holding it off on the annotated sites with the disable comment stylelint already honours for fixes.

Differential Revision: https://phabricator.services.mozilla.com/D322597

.stylelintrc.js

@@ -279,6 +279,7 @@ module.exports = {
     // unrelated errors in the same query.
     "media-query-no-invalid": null,
     "stylelint-plugin-mozilla/media-query-no-invalid": true,
+    "stylelint-plugin-mozilla/no-background-without-text-color": true,
     "stylelint-plugin-mozilla/no-base-design-tokens": true,
     "stylelint-plugin-mozilla/no-has-selector": true,
     "stylelint-plugin-mozilla/use-design-tokens": true,

docs/code-quality/lint/linters/stylelint-plugin-mozilla/rules/no-background-without-text-color.md

@@ -0,0 +1,160 @@
+# no-background-without-text-color (stylelint)
+
+This rule requires a text color from any declaration block that paints its
+background with a design token that has a paired text color token.
+
+A block that paints a background claims a surface, and it owes that surface a
+text color. Without one, the text takes whatever color an unrelated ancestor
+happens to supply, and nothing guarantees the two have sufficient contrast in
+every theme, in dark mode, under `prefers-contrast`, or under `forced-colors` —
+high contrast mode, which applies to chrome windows on Windows and to `about:`
+pages on every platform.
+
+The counterpart of the background token is the text color the surface was
+designed for, so the rule's message names it;
+[use-paired-color-tokens](use-paired-color-tokens.md) is what checks a
+combination once both halves are declared.
+
+## Rule Scope
+
+The rule reports a block whose winning `background` or `background-color`
+declaration reads a background token that has a paired text color token, and
+which sets no `color`. A background token without a counterpart is meant to
+combine with whatever the surface inherits, so it makes no claim to report.
+
+Blocks selected by a state — `:hover`, `:focus`, `[open]`, `[disabled]` and
+their kin — are exempt, along with blocks nested inside one. A state variant
+usually restyles an element its base rule has already given a text color, and
+that base rule is generally a flat sibling the rule cannot reach. A negated
+state such as `:not(:hover)` names the base state itself, so a block selected
+that way owes the surface a text color and is reported.
+
+Read that as a gap in the rule, not as permission. Declare both halves in a
+state variant too: `--button-text-color-hover` and `--button-text-color` are
+separate tokens that can resolve to entirely different values under
+`forced-colors`, so a state that repaints the background and inherits the base
+rule's text color is not safe there.
+
+Declarations directly inside an at-rule are checked as their own block, since a
+`@media` query can paint a background the rule around it does not. The element
+is the same one either way, so a `color` on the rule covers what the at-rule
+paints:
+
+```css
+.card {
+  color: var(--panel-text-color);
+
+  @media -moz-pref("browser.nova.enabled") {
+    /* Fine: the color above applies to this element in every query. */
+    background-color: var(--panel-background-color);
+  }
+}
+```
+
+A nested *rule* matches a different element and stands on its own.
+
+A block can hand the surface a text color through a custom property instead of a
+`color` declaration, which is how a component that renders the text in its own
+shadow tree takes one. Defining a paired text token, or a property that reads
+one, satisfies the rule:
+
+```css
+.new-badge {
+  background-color: var(--badge-background-color-filled);
+  --badge-text-color: var(--badge-text-color-filled);
+}
+```
+
+The rule does not require the reverse: a block that sets only a text color is
+usually a descendant of the element painting the background, which the rule
+cannot see.
+
+## Examples of incorrect usage for this rule
+
+```css
+#header {
+  background-color: var(--sidebar-background-color);
+}
+```
+
+## Examples of correct usage for this rule
+
+```css
+#header {
+  background-color: var(--sidebar-background-color);
+  color: var(--sidebar-text-color);
+}
+```
+
+```css
+.toolbar-button:hover {
+  background-color: var(--button-background-color-hover);
+  color: var(--button-text-color-hover);
+}
+```
+
+## Autofix functionality
+
+None yet;
+[bug 2068773](https://bugzilla.mozilla.org/show_bug.cgi?id=2068773) adds it. The
+counterpart is fixed by the design system rather than chosen by the author,
+which is why the message can name it, and the rule only reports where that
+counterpart exists, so the declaration to insert is determined. What the block
+alone does not say is whether the surface takes its text color from elsewhere on
+purpose, where inserting one is a silent rendering change. Such a block carries
+a disable comment, which stylelint honours for fixes as well as reports — so the
+fix goes in ungated and the disable comment described below is what holds it
+off.
+
+## Disabling the rule
+
+**Prefer declaring the color.** The tempting disable is the one that reasons
+about what the block contains — this element holds no text, its icon takes the
+color from a `fill` below, a child element colors itself. Those are all easy to
+falsify: markup changes, `forced-colors` turns a translucent wash into an opaque
+`ButtonFace`, and the next person to put a text node in the element inherits
+whatever happens to be there. Declaring the counterpart costs one line and is
+usually value-neutral in the default theme.
+
+A `::part()` is not an exception to that. A `::part()` rule in the outer tree
+overrides the component's own declaration for that part, so a block repainting a
+part's background sets the `color` in the same block:
+
+```css
+.section-context-menu.context-menu-open > moz-button::part(button) {
+  background-color: var(--button-background-color-ghost-hover);
+  color: var(--button-text-color-ghost-hover);
+}
+```
+
+`moz-select.css` is the in-tree example to follow: its
+`panel-item[selected]::part(button)` sets `background-color` and `color` together
+on the element `panel-item.css` gives `background-color: transparent` and
+`color: inherit`.
+
+Where a component remaps a background token into its own palette, remap the text
+counterpart beside it. Pairing the remapped background with the palette's own
+text token instead is what
+[use-paired-color-tokens](use-paired-color-tokens.md) rejects:
+
+```css
+panel-list {
+  --button-background-color-hover: var(--smartwindow-panel-item-background-color-hover);
+  --button-text-color-hover: var(--panel-list-text-color);
+}
+```
+
+That leaves the disable worth writing, for a block where declaring the color
+would do damage or would not resolve at all:
+
+```css
+/* The tracker count below takes the box text color; a color here would recolor
+   the shield. */
+/* stylelint-disable-next-line stylelint-plugin-mozilla/no-background-without-text-color */
+background-color: var(--urlbar-box-background-color);
+```
+
+The comment must name what setting a color here would break, or where the color
+comes from instead, in terms a reviewer can check. If the surface should have a
+text color of its own but the token does not exist, file a bug for the missing
+token and reference it from a `TODO`.

docs/code-quality/lint/linters/stylelint-plugin-mozilla/rules/use-paired-color-tokens.md

@@ -15,7 +15,10 @@ in the theme the author did not try.
 The rule only reports a declaration block that sets both a background color and
 a text color, because a block that sets one of them takes the other from
 somewhere the rule cannot see: an ancestor, a sibling rule, or another
-pseudo-element.
+pseudo-element. Whether a block that paints a surface owes it a text color at
+all is
+[no-background-without-text-color](no-background-without-text-color.md)'s
+question.
 
 Within such a block it reports two things:
 

tools/lint/stylelint/stylelint-plugin-mozilla/rules/index.mjs

@@ -5,6 +5,7 @@
  */
 
 import mediaQueryNoInvalid from "./media-query-no-invalid.mjs";
+import noBackgroundWithoutTextColor from "./no-background-without-text-color.mjs";
 import noBaseDesignTokens from "./no-base-design-tokens.mjs";
 import noBrowserRefsInToolkit from "./no-browser-refs-in-toolkit.mjs";
 import noHasSelector from "./no-has-selector.mjs";
@@ -13,6 +14,7 @@ import usePairedColorTokens from "./use-paired-color-tokens.mjs";
 
 export default {
   "media-query-no-invalid": mediaQueryNoInvalid,
+  "no-background-without-text-color": noBackgroundWithoutTextColor,
   "no-base-design-tokens": noBaseDesignTokens,
   "no-browser-refs-in-toolkit": noBrowserRefsInToolkit,
   "no-has-selector": noHasSelector,

tools/lint/stylelint/stylelint-plugin-mozilla/rules/no-background-without-text-color.mjs

@@ -0,0 +1,185 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+import stylelint from "stylelint";
+import {
+  backgroundToText,
+  customPropertiesRead,
+  findColorDeclarations,
+  isCustomPropertyDefinition,
+  namespace,
+  textToBackground,
+} from "../helpers.mjs";
+
+const {
+  utils: { report, ruleMessages, validateOptions },
+} = stylelint;
+
+let ruleName = namespace("no-background-without-text-color");
+let messages = ruleMessages(ruleName, {
+  noTextColor: (background, text) =>
+    `"${background}" should be used with a text color; add "color: var(${text})", or disable the rule with a comment saying where the text color comes from.`,
+});
+let meta = {
+  url: "https://firefox-source-docs.mozilla.org/code-quality/lint/linters/stylelint-plugin-mozilla/rules/no-background-without-text-color.html",
+  fixable: false,
+};
+
+// A state variant usually restyles an element its base rule has already given a
+// text color, so state selectors are exempt. That base rule is generally a flat
+// sibling (`.foo:hover {}` beside `.foo {}`) rather than a nesting parent, which
+// is why this keys on the selector rather than walking up.
+const STATE_PSEUDO_CLASS =
+  /:(?:active|checked|current|default|disabled|enabled|focus|focus-visible|focus-within|hover|in-range|indeterminate|invalid|open|out-of-range|past|paused|placeholder-shown|playing|popover-open|read-only|read-write|target|user-invalid|user-valid|valid|visited|-moz-broken|-moz-drag-over|-moz-focusring|-moz-window-inactive)\b/;
+
+// Chrome markup carries the states a pseudo-class cannot express as attributes
+// instead, so `menulist[disabled]` is a state variant of `menulist`.
+const STATE_ATTRIBUTE =
+  /\[\s*(?:active|aria-checked|aria-current|aria-disabled|aria-expanded|aria-pressed|aria-selected|busy|checked|disabled|focused|open|pressed|selected)\s*[\]~^|$*=]/;
+
+/**
+ * Drops the argument of every `:not()`, innermost first. A negated state names
+ * the base state rather than a variant of it, so `&:not(:hover)` is the rule
+ * that owes the surface a text color, not one that inherits it from elsewhere.
+ *
+ * @param {string} selector
+ * @returns {string}
+ */
+let stripNegations = selector => {
+  for (let previous; selector != previous; ) {
+    previous = selector;
+    selector = selector.replace(/:not\([^()]*\)/g, "");
+  }
+  return selector;
+};
+
+/**
+ * Whether a selector says the text color is not this block's to declare.
+ *
+ * @param {string} selector
+ * @returns {boolean}
+ */
+let isExemptSelector = selector => {
+  let states = stripNegations(selector);
+  return STATE_PSEUDO_CLASS.test(states) || STATE_ATTRIBUTE.test(states);
+};
+
+/**
+ * Whether the selector of a block, or of a block it is nested in, exempts it.
+ *
+ * @param {object} block - A PostCSS Rule or AtRule.
+ * @returns {boolean}
+ */
+let isExempt = block => {
+  for (let node = block; node; node = node.parent) {
+    if (
+      node.type == "rule" &&
+      isExemptSelector(node.raws.selector?.raw ?? node.selector)
+    ) {
+      return true;
+    }
+  }
+  return false;
+};
+
+/**
+ * Whether the element a block styles is given a text color. An at-rule nested
+ * in a rule styles that rule's element, so a `color` the rule sets applies to
+ * the at-rule's surface too; a nested rule matches a different element.
+ *
+ * @param {object} block - A PostCSS Rule or AtRule.
+ * @returns {boolean}
+ */
+let hasTextColor = block => {
+  for (let node = block; node; node = node.parent) {
+    if (findColorDeclarations(node).text) {
+      return true;
+    }
+    if (node.type != "atrule") {
+      return false;
+    }
+  }
+  return false;
+};
+
+/**
+ * Whether a block gives the surface a text color through a custom property,
+ * which is how a component rendering the text in its own shadow tree takes
+ * one.
+ *
+ * @param {object} block - A PostCSS Rule or AtRule.
+ * @returns {boolean}
+ */
+let definesTextColor = block =>
+  block.nodes.some(
+    node =>
+      node.type == "decl" &&
+      isCustomPropertyDefinition(node) &&
+      (textToBackground.has(node.prop) ||
+        customPropertiesRead(node.value).some(token =>
+          textToBackground.has(token)
+        ))
+  );
+
+/**
+ * Reports a declaration block that paints a paired background token and sets
+ * no text color, leaving the surface to inherit one no theme guarantees the
+ * contrast of. Only the declarations that win the cascade within the block are
+ * considered.
+ *
+ * @param {object} block - A PostCSS Rule or AtRule.
+ * @param {object} result - The PostCSS result to report to.
+ */
+let checkBlock = (block, result) => {
+  let { background } = findColorDeclarations(block);
+  if (
+    !background ||
+    hasTextColor(block) ||
+    definesTextColor(block) ||
+    isExempt(block)
+  ) {
+    return;
+  }
+
+  let [paired] = customPropertiesRead(background.value).filter(token =>
+    backgroundToText.has(token)
+  );
+  if (!paired) {
+    return;
+  }
+
+  report({
+    message: messages.noTextColor(paired, backgroundToText.get(paired)),
+    node: background,
+    result,
+    ruleName,
+  });
+};
+
+let ruleFunction = primaryOption => {
+  return (root, result) => {
+    let validOptions = validateOptions(result, ruleName, {
+      actual: primaryOption,
+      possible: [true],
+    });
+
+    if (!validOptions) {
+      return;
+    }
+
+    // Declarations nest directly inside an at-rule as well as inside a rule,
+    // which is the shape of every generated token sheet. A statement at-rule
+    // such as @namespace has no block and so no nodes at all.
+    root.walk(node => {
+      if ((node.type == "rule" || node.type == "atrule") && node.nodes) {
+        checkBlock(node, result);
+      }
+    });
+  };
+};
+
+ruleFunction.ruleName = ruleName;
+ruleFunction.messages = messages;
+ruleFunction.meta = meta;
+export default ruleFunction;

tools/lint/stylelint/stylelint-plugin-mozilla/tests/no-background-without-text-color.tests.mjs

@@ -0,0 +1,211 @@
+/**
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+// Bug 1948378: remove this exception when the eslint import plugin fully
+// supports exports in package.json files
+// eslint-disable-next-line import/no-unresolved
+import { testRule } from "stylelint-test-rule-node";
+import stylelint from "stylelint";
+import noBackgroundWithoutTextColor from "../rules/no-background-without-text-color.mjs";
+
+let plugin = stylelint.createPlugin(
+  noBackgroundWithoutTextColor.ruleName,
+  noBackgroundWithoutTextColor
+);
+let {
+  ruleName,
+  rule: { messages },
+} = plugin;
+
+testRule({
+  plugins: [plugin],
+  ruleName,
+  config: [true],
+  fix: false,
+  accept: [
+    {
+      code: ".a { background-color: var(--button-background-color); color: var(--button-text-color); }",
+      description: "A background token with a text color.",
+    },
+    {
+      code: ".a { color: var(--button-text-color); }",
+      description: "A block that paints no background claims no surface.",
+    },
+    {
+      code: ".a { background-color: var(--color-accent-primary); }",
+      description: "A base color is not a background token.",
+    },
+    {
+      code: ".a { background-color: var(--text-color-deemphasized); }",
+      description:
+        "A token with no background counterpart guarantees nothing to pair with.",
+    },
+    {
+      code: ".a { background-color: transparent; }",
+      description: "A background that paints nothing claims no surface.",
+    },
+    {
+      code: ".a { background-color: var(--my-local-background); }",
+      description: "A local custom property is not a token.",
+    },
+    {
+      code: ".a { --box-background: var(--button-background-color); }",
+      description: "Defining a custom property is not painting a surface.",
+    },
+    {
+      code: ".a:hover { background-color: var(--button-background-color-hover); }",
+      description:
+        "A state variant takes the text color from the rule it varies.",
+    },
+    {
+      code: ".a { &:hover { background-color: var(--button-background-color-hover); } }",
+      description: "A nested state variant is one as well.",
+    },
+    {
+      code: ".a:hover { .b { background-color: var(--button-background-color-hover); } }",
+      description: "A block nested inside a state variant is covered too.",
+    },
+    {
+      code: "menulist[disabled] { background-color: var(--button-background-color); }",
+      description: "A state carried as an attribute is a state variant.",
+    },
+    {
+      code: '.a[aria-expanded="true"] { background-color: var(--button-background-color); }',
+      description: "An ARIA state attribute is one as well.",
+    },
+    {
+      code: ".a:not(.b):is(:hover, [open]) { background-color: var(--button-background-color); }",
+      description: "A state beside a negation still exempts the block.",
+    },
+    {
+      code: ".a { background-color: var(--button-background-color); color: inherit; }",
+      description: "Any text color satisfies the rule, token or not.",
+    },
+    {
+      code: ".a { color: CanvasText; @media (prefers-contrast) { background-color: var(--button-background-color); } }",
+      description:
+        "An at-rule paints the surface of the rule around it, which has a text color.",
+    },
+    {
+      code: ".a { color: var(--panel-text-color); background-color: var(--panel-background-color); }",
+      description: "Declaration order does not matter.",
+    },
+    {
+      code: "@namespace url(http://www.w3.org/1998/Math/MathML);",
+      description: "A statement at-rule has no block to check.",
+    },
+    {
+      code: ".a[type='text'] { background-color: var(--button-background-color); color: var(--button-text-color); }",
+      description: "An attribute that is not a state is not a carve-out.",
+    },
+    {
+      code: ".a { background-color: var(--badge-background-color-filled); --badge-text-color: var(--badge-text-color-filled); }",
+      description:
+        "A paired text token handed to a component through a custom property.",
+    },
+    {
+      code: ".a { background-color: var(--button-background-color-ghost); --button-text-color-ghost: currentColor; }",
+      description: "Defining the paired text token itself counts as well.",
+    },
+  ],
+  reject: [
+    {
+      code: ".a { background-color: var(--sidebar-background-color); }",
+      message: messages.noTextColor(
+        "--sidebar-background-color",
+        "--sidebar-text-color"
+      ),
+      description: "A block that paints a surface and names no text color.",
+      line: 1,
+      column: 6,
+    },
+    {
+      code: ".a { background: var(--panel-background-color) no-repeat; }",
+      message: messages.noTextColor(
+        "--panel-background-color",
+        "--panel-text-color"
+      ),
+      description: "The background shorthand paints a surface too.",
+      line: 1,
+      column: 6,
+    },
+    {
+      code: ".a { background-color: light-dark(var(--button-background-color-hover), var(--button-background-color-hover)); }",
+      message: messages.noTextColor(
+        "--button-background-color-hover",
+        "--button-text-color-hover"
+      ),
+      description: "A token nested in light-dark() paints a surface.",
+      line: 1,
+      column: 6,
+    },
+    {
+      code: ".a { @media (prefers-contrast) { background-color: var(--sidebar-background-color); } }",
+      message: messages.noTextColor(
+        "--sidebar-background-color",
+        "--sidebar-text-color"
+      ),
+      description:
+        "Declarations nested directly inside an at-rule are checked.",
+      line: 1,
+      column: 34,
+    },
+    {
+      code: ".a { background-color: var(--sidebar-background-color); color: var(--sidebar-text-color); &::after { background-color: var(--panel-background-color); } }",
+      message: messages.noTextColor(
+        "--panel-background-color",
+        "--panel-text-color"
+      ),
+      description:
+        "A pseudo-element paints its own surface and inherits nothing from its host block.",
+      line: 1,
+      column: 102,
+    },
+    {
+      code: ".a { --box-radius: 4px; background-color: var(--sidebar-background-color); }",
+      message: messages.noTextColor(
+        "--sidebar-background-color",
+        "--sidebar-text-color"
+      ),
+      description:
+        "A custom property that carries no text color does not excuse the block.",
+      line: 1,
+      column: 25,
+    },
+    {
+      code: ".a { background-color: var(--button-background-color); @media (prefers-contrast) { color: CanvasText; } }",
+      message: messages.noTextColor(
+        "--button-background-color",
+        "--button-text-color"
+      ),
+      description:
+        "A text color set only inside a nested at-rule leaves the surface without one outside it.",
+      line: 1,
+      column: 6,
+    },
+    {
+      code: ".a { &:not(:hover) { background-color: var(--button-background-color); } }",
+      message: messages.noTextColor(
+        "--button-background-color",
+        "--button-text-color"
+      ),
+      description:
+        "A negated state names the base state, which owes the surface a text color.",
+      line: 1,
+      column: 22,
+    },
+    {
+      code: ".a:not([open]) { background-color: var(--button-background-color); }",
+      message: messages.noTextColor(
+        "--button-background-color",
+        "--button-text-color"
+      ),
+      description: "A negated state attribute is no exemption either.",
+      line: 1,
+      column: 18,
+    },
+  ],
+});