Re: [w3c/manifest] Add `color_scheme_dark` member and themeable members (PR #1205)

@Copilot commented on this pull request.

## Pull request overview

Adds support for defining dark-mode theme overrides in the Web App Manifest by introducing a `color_scheme_dark` member and identifying which members are “themeable” (and thus eligible for dark-mode overrides).

**Changes:**
- Marks `icons`, `theme_color`, and `background_color` (and shortcut `icons`) as “themeable members”.
- Defines `manifest/color_scheme_dark` and `shortcut item/color_scheme_dark`, including processing steps.
- Hooks `Process the \`color_scheme_dark\` member` into the main manifest processing algorithm.





---

💡 <a href="/w3c/manifest/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

> +            [=themeable members=] as keys, and their values as the theme
+            overrides.

The `color_scheme_dark` definition says its keys can be themeable members *and* their corresponding localizable members (e.g., `icons_localized`), but the “Theming themeable members” paragraph says the map accepts only themeable members as keys. Please align the wording so it consistently includes the `*_localized` companion keys (or redefine them as themeable) to avoid ambiguity about what keys are allowed.
```suggestion
            [=themeable members=] and their corresponding [=localizable members=]
            as keys, with their values as the theme overrides.
```

> @@ -1497,6 +1504,105 @@ <h3>
           </ol>
         </section>
       </section>
+      <section>
+        <h3>
+          `color_scheme_dark` member
+        </h3>
+        <p>
+          The [=manifest's=] <code><dfn data-export="" data-dfn-for=
+          "manifest">color_scheme_dark</dfn></code> member is an [=ordered
+          map=] whose keys are the names of [=themeable members=] and their
+          corresponding [=localizable members=], while the values are their
+          corresponding theme overrides.
+        </p>
+        <p>
+          A <dfn>themeable member</dfn> is a [=manifest=] member that can be

`themeable member` is defined as “a manifest member that can be themed”, but the term is also used for `shortcut item/icons`, which is not a top-level manifest member. Please broaden the definition so it applies to members of nested manifest structures (e.g., shortcut items) as well, or avoid using the same term for shortcut-item members.
```suggestion
          A <dfn>themeable member</dfn> is a member of the [=manifest=] or of
          a nested manifest structure (such as a shortcut item) that can be
```

> +            </li>
+            <li>If |colorScheme| is not an [=ordered map=], return.
+            </li>
+            <li>Let |processedColorScheme:ordered map| be a new [=ordered
+            map=].
+            </li>
+            <li>[=Map/Set=] |manifest|["color_scheme_dark"] to
+            |processedColorScheme|.
+            </li>
+            <li>[=Process a color member=] passing |colorScheme|,
+            |processedColorScheme|, "theme_color".
+            </li>
+            <li>[=Process a color member=] passing |colorScheme|,
+            |processedColorScheme|, "background_color".
+            </li>
+            <li>[=Process image resources=] passing |colorScheme|["icons"],

`Process image resources` always sets `map[member]` to a new (possibly empty) list before validating `images` (see `Processing image resources` at index.html:2364-2370). Calling it unconditionally here means that if `color_scheme_dark.icons` is absent, the processed dark-scheme override will still contain an empty `icons` list, which prevents inheriting the main `icons` and can effectively remove icons in dark mode. Consider only invoking `Process image resources` when `"icons"` exists in `|colorScheme|` (and is a list), so absence cleanly falls back to the light-scheme icons.
```suggestion
            <li>If |colorScheme|["icons"] [=map/exist=] and is a [=list=], then
            [=Process image resources=] passing |colorScheme|["icons"],
```

> +              <li>[=Process image resources=] passing |colorScheme|["icons"],
+              |processedColorScheme|, |manifest URL|, and "icons".

Same inheritance issue for shortcut-item theming: `Process image resources` always sets `processedColorScheme["icons"]` to an empty list even when `color_scheme_dark.icons` is missing/invalid (index.html:2364-2370). This makes a present-but-partial `shortcut item/color_scheme_dark` override wipe out the base shortcut icons. Add a presence/type check for `|colorScheme|["icons"]` before processing so missing `icons` inherits from the light-scheme shortcut icons.
```suggestion
              <li>If "icons" [=map/exists=] in |colorScheme| and
              |colorScheme|["icons"] is a [=list=], then [=Process image
              resources=] passing |colorScheme|["icons"], |processedColorScheme|,
              |manifest URL|, and "icons".
```

-- 
Reply to this email directly or view it on GitHub:
https://github.com/w3c/manifest/pull/1205#pullrequestreview-3724287504
You are receiving this because you are subscribed to this thread.

Message ID: <w3c/manifest/pull/1205/review/3724287504@github.com>