IconVariants: Correct spelling & address svg and empty icon_variants #775

solomonkinard · 2025-03-03T20:48:14Z

An edge case came up tho. What if an extension which has only SVG icons defined in icon_variants gets loaded into Chrome once Chrome implemented icon_variants. What would it do? Will it fall back to the legacy icons?

#585 (comment)

This PR aims to address three questions before merge:

How will browsers that don't support .svg handle extensions with only "any" defined as an .svg file?
Can "all" be set with a key other than an .svg file, such as .png?
Should icon_variants error if it's supplied in the manifest, but it's of the wrong type or it has the correct type but there isn't at least one valid icon_variant inside?

solomonkinard · 2025-03-03T20:54:07Z

Thought:

AFAIK, if a browser doesn't currently have .svg support, but only "any" was supplied with it, then the extension should load without error. Perhaps a warning will suffice. Ideally there would be svg support. However, if that's not available the question is whether to a) fall back to "icons" or b) treat the extension as if it doesn't have any icons at all. I think that b) would be ideal for consistency. WDYT? Also, any thoughts on the other questions. Thanks.

oliverdunk · 2025-03-04T13:07:50Z

If an extension specifies icon_variants but there are no supported variants, I could see an argument for either a warning or an error. That is something the developer can and should avoid (by specifying a lower priority variant with a commonly supported file type, like a PNG).

Without having thought too much about it, I don't think we need to fallback to the icons key. I'm also not against that though.

@xeenon, do you know how this is handled in Safari?

xeenon · 2025-03-04T18:12:26Z

@oliverdunk We do record an warning / soft error if icon_variants is empty or the specified icons are not valid, file missing, etc.

Rob--W · 2025-03-04T23:23:16Z

An extension without an icon is valid, so as the error handling mode I would prefer no icon over a hard error.

I wouldn't mind falling back to icons for compatibility, since that would match the behavior of the extension in browsers that don't recognize icon_variants. I don't feel too strongly about this though.

carlosjeurissen

For browsers not supporting SVG icons falling back to manifest.icons would be great for webcompat. However doing so would make the case for icon_variants as a replacement for icons weaker.

Safari currently falls back to manifest.icons in case manifest.action.icon_variants is malformed, however it skips manifest.action.icons. As @xeenon pointed out in #775 (comment) introducing the fallback would be a behavioural change.

carlosjeurissen · 2025-03-05T19:27:44Z

proposals/dark_mode_extension_icons.md

@@ -166,10 +166,10 @@ effectively have a wild-card for `color_schemes`. For example, `["*"]` is valid.
 **Misc**
 1. If the top-level `icon_variants` key is provided, the top level `icons` key
 will be ignored.
-1. `icon_variants` will not cause an error in the event that it is invalid.
+1. `icon_variants` will not cause an error if it's invalid.


Suggested change

1. `icon_variants` will not cause an error if it's invalid.

1. `icon_variants` will not cause hard errors if no icon groups are found or specific icon groups have unknown properties. It may cause a hard error if icon groups are not of type object.

carlosjeurissen · 2025-03-05T19:30:51Z

proposals/dark_mode_extension_icons.md

 Instead, only the faulty icon group(s) will be ignored, with an optional
 warning. Warnings are preferred over errors because they're more adaptable to
-changes in the future.
+changes in the future. If none of the icon groups are valid, should this error?


Suggested change

changes in the future. If none of the icon groups are valid, should this error?

changes in the future. If none of the icon groups are valid, agents are expected to fallback to the `icons` property in `manifest.json`. If `icons` is missing, agents are expected to use a default error and not give a hard error.

This behaviour matches the current Safari implementation and reliefs extension authors from having to declare additional fallbacks in icon_variants for browsers which do not support SVG for example. We may want to revisit this in MV4+ when the icons property might be deprecated/discontinued.

Safari does not fallback to icons. We have separate fallback to the containing app's icon if we can't get the icon from icon_variants / icons. I would be fine adding fallback to icons but it would require a change in our current shipping behavior.

@xeenon thanks for clarifying. You are right. In my tests, invalid manifest.action.icon_variants would fallback to manifest.icons if invalid. Which led me to the false conclusion this also happens for manifest.icon_variants. In that case this is up for discussion once again. For browsers not supporting SVG icons falling back to manifest.icons would be great for webcompat. However doing so would make the case for icon_variants as a replacement for icons weaker.

carlosjeurissen · 2025-03-05T19:36:05Z

proposals/dark_mode_extension_icons.md

@@ -200,7 +200,8 @@ starting with smaller sizes if available, retreating to the nearest larger size.
 1. The `"16"` is a size in `{"16": "icon.png"}` and any number can be used as a
 size, per the browser’s icon requirements. The word `"any"` can also be used in
 place of a number to represent an icon that can be shown at any size (usually
-vector images). The icon size used by the browser will be determined as follows:
+vector images). Does that mean that "any" will only have an svg and not a png?


Suggested change

vector images). Does that mean that "any" will only have an svg and not a png?

vector images).

As currently with the manifest.icons key, you can already specify icons in a size not matching the size specified, I do not see a reason to not support the any keyword for raster images.

IconVariants: Correct spelling & address svg and empty icon_variants

0e783df

solomonkinard requested a review from carlosjeurissen March 3, 2025 20:50

solomonkinard mentioned this pull request Mar 3, 2025

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

Merged

solomonkinard requested review from dotproto, oliverdunk, xeenon and Rob--W March 3, 2025 20:54

carlosjeurissen suggested changes Mar 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IconVariants: Correct spelling & address svg and empty icon_variants #775

IconVariants: Correct spelling & address svg and empty icon_variants #775

solomonkinard commented Mar 3, 2025

solomonkinard commented Mar 3, 2025

oliverdunk commented Mar 4, 2025

xeenon commented Mar 4, 2025

Rob--W commented Mar 4, 2025 •

edited

Loading

carlosjeurissen left a comment •

edited

Loading

carlosjeurissen Mar 5, 2025

carlosjeurissen Mar 5, 2025

xeenon Mar 5, 2025 •

edited

Loading

carlosjeurissen Mar 5, 2025

carlosjeurissen Mar 5, 2025

	1. `icon_variants` will not cause an error if it's invalid.
	1. `icon_variants` will not cause hard errors if no icon groups are found or specific icon groups have unknown properties. It may cause a hard error if icon groups are not of type object.

	changes in the future. If none of the icon groups are valid, should this error?
	changes in the future. If none of the icon groups are valid, agents are expected to fallback to the `icons` property in `manifest.json`. If `icons` is missing, agents are expected to use a default error and not give a hard error.

	vector images). Does that mean that "any" will only have an svg and not a png?
	vector images).

IconVariants: Correct spelling & address svg and empty icon_variants #775

Are you sure you want to change the base?

IconVariants: Correct spelling & address svg and empty icon_variants #775

Conversation

solomonkinard commented Mar 3, 2025

solomonkinard commented Mar 3, 2025

oliverdunk commented Mar 4, 2025

xeenon commented Mar 4, 2025

Rob--W commented Mar 4, 2025 • edited Loading

carlosjeurissen left a comment • edited Loading

Choose a reason for hiding this comment

carlosjeurissen Mar 5, 2025

Choose a reason for hiding this comment

carlosjeurissen Mar 5, 2025

Choose a reason for hiding this comment

xeenon Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

carlosjeurissen Mar 5, 2025

Choose a reason for hiding this comment

carlosjeurissen Mar 5, 2025

Choose a reason for hiding this comment

Rob--W commented Mar 4, 2025 •

edited

Loading

carlosjeurissen left a comment •

edited

Loading

xeenon Mar 5, 2025 •

edited

Loading