dark-mode: Add a proposal for dark mode extension icons

[solomonkinard]

Proposal for #229

[xeenon]

Thanks for writing this up! This is looking good, but I have some overall concerns and questions that should be addressed.

[solomonkinard]

All comments addressed in the patch response. Thanks.

[oliverdunk]

Thanks for writing this up - we've been through so many iterations here that it's nice to have a single place to track what we're thinking. Left some initial thoughts.

[carlosjeurissen]

@solomonkinard Thanks for putting this together!

The window.matchMedia('(prefers-color-scheme: dark)') trick would not work in serviceWorkers and more importantly, it would not work for dark browser themes.

Take for example the following theme:
https://chromewebstore.google.com/detail/mafbdhjdkjnoafhfelkjpchpaepjknad

This theme would want light-colored extension icons or they might be unreadable. Are browsers expected to use the "dark" variant of the extension icons in this situation?

A bit hesitant on the readability of yet another nesting object. A potential different approach would be to use a flat structure like this:

"icon_variants": [{
  "any": "16.svg",
  "type": "image/svg+xml"
}, {
  "16": "mono16.png",
  "32": "mono32.png",
  "type": "image/png",
  "purpose": "monochrome",
}, {
  "16": "dark16@2x.png",
  "32": "dark32@2x.png",
  "type": "image/png",
  "color_scheme": "dark",
  "density": "2dppx"
}, {
  "16": "light16.png",
  "32": "light32.png",
  "type": "image/png",
  "color_scheme": "light"
}, {
  "16": "default16.png",
  "32": "default32.png",
  "type": "image/png"
}]

This way we open ourselves up for future additions like the showcased monochrome (which would be helpful for Safari as they derive monochrome icons from normal extension icons currently). And a density property to handle details.

The browser would then pick the first object it understands and has matching conditions. This would allow extension developers to do several calls to setIcons with different syntaxes hoping the browser accept it.

This would also allow extension developers to specify both SVG and PNG icons and let the browser use SVG if supported, else fall back to PNG.

In the same fashion, dynamically setted icons could take imageData or dynamic svgs (as proposed by @xeenon here #462):

[{
  "16": imageDataSize16,
  "32": imageDataSize32,
  "type": ImageData
}, {
  "16": svgElement16,
  "32": svgElement32,
  "type": SVGElement
}]

Having the same syntax for dynamic and declarative icons seems ideal for developers.

Another benefit would be for a browser to pick the default icon in case the browser theme is neither dark nor light.

[carlosjeurissen]

@carlosjeurissen, are you comfortable approving this? You are the only one left with a pending review and would be nice to fix that before landing 😄

Yeah, I was still waiting for some of my suggestions to be incorporated in the document.

There remains one aspect which we did not cover yet. How should browsers handle mixed types within the same icon group?
Think about something like:

{
  "16": "raster.png",
  "24": "raster.jpg",
  "36": "vector.svg",
  "48": ImageData
}

My initial thought on this would be ignore the full icon group and give a warning in the manifest when loading.

[xeenon]

There remains one aspect which we did not cover yet. How should browsers handle mixed types within the same icon group?

My initial thought on this would be ignore the full icon group and give a warning in the manifest when loading.

@carlosjeurissen How would you specify ImageData in the manifest? The manifest only supports paths. Maybe you mean JS APIs?

I think having the APIs only support one type per group (path or ImageData) seems best and is slightly easier to implement.

Supporting different image formats in one group is fine for Safari, we can support a mix of SVG and bitmap images.

[carlosjeurissen]

@carlosjeurissen How would you specify ImageData in the manifest? The manifest only supports paths. Maybe you mean JS APIs?

The example was meant for both the manifest in setIcon. As for both we need to consider what to do with mixed types. ImageData is indeed not supported in the manifest.

I think having the APIs only support one type per group (path or ImageData) seems best and is slightly easier to implement.

One type per group is critical for simplifying the implementation of the fallback system.

Supporting different image formats in one group is fine for Safari, we can support a mix of SVG and bitmap images.

This might be true for Safari, but this can be problematic for Chrome as they currently do not support SVGs. However it is quite unlikely a developer would even mix SVGs with PNGs. @solomonkinard @oliverdunk Since this seems like a Chrome-only issue. What are your thoughts here?

[dotproto]

This should be ["light", "dark"] and any other color_scheme we might want to add in the future. It specificially is not "light" as default.

Fixed.

@dotproto The path for any does not need to be an SVG, it can be a large bitmap that is scaled as needed for example. So this can drop SVGPath.

Ah, my mistake. Thanks for pointing that out. I've fixed this in my comment.

[oliverdunk]

In trying to move to TypeScript, I think we've introduced some new issues. Unfortunately those make the intention of the API design unclear enough that I think they need to be blockers. Left some initial feedback - while I very much appreciate the TypeScript types I also wouldn't be against just removing them so we can avoid leaving this PR open for too long.

[solomonkinard]

The TypeScript schema section that Simeon recommended was removed. See #585 (review) for context. The PR was approved two weeks ago and awaiting merge; the TypeScript schema addition was added a week ago. Anyone, please share if there are any more comments blocking this PR from being merged. The latest PRs address all of the latest questions.

I think having the APIs only support one type per group (path or ImageData) seems best and is slightly easier to implement.

@solomonkinard @oliverdunk Since this seems like a Chrome-only issue. What are your thoughts here?

One type per group seems reasonable to make implementation easier. The PR has been updated in e1b23de.

[carlosjeurissen]

One type per group seems reasonable to make implementation easier. The PR has been updated in e1b23de.

Sounds good. Yet we probably want to specify what would happen when mixed types are present. Should the whole icon group be marked as invalid? Should it throw an error in setIcon? Should it reject the whole manifest?

[solomonkinard]

Yet we probably want to specify what would happen when mixed types are present. Should the whole icon group be marked as invalid? Should it throw an error in setIcon? Should it reject the whole manifest?

Added the following to the relevant section:

If more than one type is present in an icon group,
   the group can be ignored, marked as invalid and optionally show a warning
   during installation.

[solomonkinard]

All merge-blocking comments should have been resolved at this point. If not, please advise. Otherwise, awaiting merge. Thanks.

[dotproto]

… I also wouldn't be against just removing them so we can avoid leaving this PR open for too long.

No objections on my end. I'm a bit late on saying this, but I think it makes the most sense to land what we've all agreed on and introduce more explicit TypeScript types in a follow-on PR.

Thanks for all your work on this, @solomonkinard!