Bug-2001318 example showing SVG icon reacting to theme

[rebloor]

Description

This is the example suggested in Bug 2001318 Some SVG pageAction icons have a poor contrast due to built-in CSS filter.

Compared to the suggested example this version is my grade to Manifest V3 and has some minor copy edit changes.

Motivation

This example demonstrates how to use the prefers-color-scheme media query to adapt an SVG icon to dark and light themes.

Related issues and pull requests

Related MDN documentation changes in TBC.

[rpl]

@rebloor thanks for taking care also of converting the example into an MV3 extension, the PR looks pretty good but there are a couple of issues to fix to make the example to be installing successfully (it currently doesn't due to the issue on the manifest.json side) and to improve a little bit the behavior when "the event page is terminated and respawned on the action button clicks while the extension page isn't already in an open tab", and a link to be fixed in the README.md.

[rpl]

issue: the url associated to Bug 2016509 link got cut out

[rpl]

nit/thought: not an actual issue and I'm actually not 100% sure if this is just my own habit and convention, but I would have put the (See Bug 2001318) before the period that ends the previous phrase and moved the period after the close parenthesis, I mean something like ... that use multiple colors (see Bug 2001318)..

[rpl]

issue: this part of the manifest isn't currently valid JSON (it looks like browser_specific_settings isn't closed and the commas at the end of line 14 isn't allowed in valid JSON), let's also format it so that it is easier to see how the open and closed curly brackets match each other:

diff --git a/themed-icons/manifest.json b/themed-icons/manifest.json
index b7a90b8..2af23e4 100644
--- a/themed-icons/manifest.json
+++ b/themed-icons/manifest.json
@@ -5,14 +5,14 @@
   "name": "themed-icons",
   "version": "1.0",
   "homepage_url": "https://github.com/mdn/webextensions-examples/tree/master/themed-icons",
-  "browser_specific_settings": 
-    { "gecko": 
-      {
-        "id": "themed-icons@mozilla.org",
-        "data_collection_permissions": {
-          "required": ["none"]
-       },
-     },
+  "browser_specific_settings": {
+    "gecko": {
+      "id": "themed-icons@mozilla.org",
+      "data_collection_permissions": {
+        "required": ["none"]
+      }
+    }
+  },
 
   "icons": { "32": "prefers-color-scheme-icon.svg" },

[rpl]

issue(kind of minor, but still incorrect):

• Now that the extension is an MV3 extension, the background script are going to be non-persistent (a.k.a. "event page"), and so the behavior described in this inline comment doesn't actually match the actual behavior anymore.
• There is also a race that is likely to happen when the event page will get terminated and the user triggers it to be respawned by clicking on the action/pageAction buttons, in that case this call will be triggered (as a side effect of the previously terminated event page being respawned to handle the action button click event) around the same time we will be triggering the switchToNextTheme event handler to be handling the action click event itself. Due to this race it is not unlikely we would end up opening the extension page twice in two tabs

You should be able to trigger the race with the following STR:

• install the add-on
• close any tab that has the add-on extension page url (e.g. the one opened when the add-on is installed, and any other ones if there are more than one already)
• make sure to not have any Add-on Debugging Developer Toolbox attached to this test extension (because that would prevent the event page from being terminated on idle)
• open about:debugging and click "Terminate background script" and wait for the "Background script" state to be "stopped" (unless it is already stopped due to getting past the idle timeout that will be auto-terminating the background script)
• once the "Background script" state to be "stopped", click on the add-on action button (either the pageAction or the action button, it doesn't matter for this STR)
  • Expected behavior: only one tab to be opened on the extension page
  • Actual behavior: two tabs gets opened on the extension page

Even if the race doesn't happen consistently (it was fairly consistent for me when I tried the STR described above on one of my dev machines though), I feel we should still adjust this part to account for the event page scripts to be executed again when the event page is respawned and adjust the inline comment.

e.g. I think that a reasonable approach would be to use browser.runtime.onInstalled to restrict this call to the first startup after being installed and adjust the inline comment accordingly:

diff --git a/themed-icons/background.js b/themed-icons/background.js
index 630ec8a..b717005 100644
--- a/themed-icons/background.js
+++ b/themed-icons/background.js
@@ -28,5 +28,7 @@ async function switchToNextTheme() {
 browser.pageAction.onClicked.addListener(switchToNextTheme);
 browser.action.onClicked.addListener(switchToNextTheme);
 
-// Open or select the extension test page on addon startup.
-openOrSelectTab(true);
+// Open the the extension page in a new tab after the add-on is installed.
+browser.runtime.onInstalled.addListener(() => {
+  openOrSelectTab(true);
+});

[rebloor]

Thanks @rpl change is made

[LeoMcA]

Change to examples.json looks fine

[rpl]

Thanks @rebloor, the updated version looks good to me too.