Making them mandatory only made sense when we only supported those 3 properties. Now we support many more properties, so these shouldn't be mandatory.

What do you think about this?

Some investigation into this: removing the check that makes the 3 properties mandatory doesn't work well. There is a theme existence check in LightweightThemeConsumer.jsm based on headerURL, and not specifying that makes the check falsy. Also, a lot of lightweight theme CSS is based on the assumption that accentcolor/textcolor/headerURL are always defined.

A very simple fix would be adding: if (!details.images || !details.images.headerURL) { details.images.headerURL = ""; } to ext-theme.js and removing the checks making accentcolor/textcolor/headerURL mandatory. A better fix would be refactoring LightweightThemeConsumer.jsm to stop using `active = !!aData.headerURL` as "theme is active" check, but that's quite complicated to do with the current setup unfortunately.

These properties are mandatory, because we don't want theme authors to create invalid Light-Weight Themes. If you want the properties to become optional, then we need to move this validation to ext-theme.js: 1. If only LWT properties are present in the manifest, kick off validation that none of them is missing. Ex.: When only 'accentcolor' is defined in the manifest, then we should assume that it's trying to 'behave' like a LWT, thus we should fail validation, saying 'textcolor' and 'headerURL' are also required in this case. 2. Exempt the theme.update() function from this requirement & skip the validation step. LWTs are manifest-only (near future, I know), so we needn't impose this restriction to other theme API consumers.

I am fine with removing the requirements once we can confirm that themes without these will not look broken on our tier 1 platforms (linux, osx, and windows). We should also confirm that AMO can handle and display them fine.

This is actually an essential part for chrome compatibility.

headerURL was taken care of in bug 1404688

Both the patch in bug 1404688 and the refactoring in bug 1417883 made this much easier. The accentcolor/textcolor is decoupled from the rest of LWTConsumer.jsm.

Comment on attachment 8991000 [details] Bug 1413144 - Make accentcolor and textcolor optional. https://reviewboard.mozilla.org/r/255962/#review262842 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/extensions/parent/ext-theme.js:91 (Diff revision 2) > - defaultTheme = this; > + defaultTheme = this; > - } > + } > - onUpdatedEmitter.emit("theme-updated", this.details, this.windowId); > + onUpdatedEmitter.emit("theme-updated", this.details, this.windowId); > > + let lwtData = { > + theme: this.lwtStyles Error: Missing trailing comma. [eslint: comma-dangle] ::: toolkit/components/extensions/parent/ext-theme.js:297 (Diff revision 2) > } > } > > static unload(windowId) { > - let lwtStyles = { > - headerURL: "", > + let lwtData = { > + theme: null Error: Missing trailing comma. [eslint: comma-dangle]

Comment on attachment 8991000 [details] Bug 1413144 - Make accentcolor and textcolor optional. https://reviewboard.mozilla.org/r/255962/#review262850 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/extensions/parent/ext-theme.js:89 (Diff revision 1) > - defaultTheme = this; > + defaultTheme = this; > - } > + } > - onUpdatedEmitter.emit("theme-updated", this.details, this.windowId); > + onUpdatedEmitter.emit("theme-updated", this.details, this.windowId); > > + let lwtData = { > + theme: this.lwtStyles Error: Missing trailing comma. [eslint: comma-dangle] ::: toolkit/components/extensions/parent/ext-theme.js:295 (Diff revision 1) > } > } > > static unload(windowId) { > - let lwtStyles = { > - headerURL: "", > + let lwtData = { > + theme: null Error: Missing trailing comma. [eslint: comma-dangle]

Comment on attachment 8991000 [details] Bug 1413144 - Make accentcolor and textcolor optional. https://reviewboard.mozilla.org/r/255962/#review262946 ::: toolkit/components/extensions/parent/ext-theme.js:94 (Diff revision 3) > + if (this.windowId) { > + lwtData.window = > + getWinUtils(windowTracker.getWindow(this.windowId)).outerWindowID; Can you combine this with the block at line 83? ::: toolkit/modules/LightweightThemeConsumer.jsm:211 (Diff revision 3) > + for (let icon of ICONS) { > + let value = aData.icons ? aData.icons[`--${icon}--icon`] : null; Thanks, it looks like you did this to fix a bug with _update in which we weren't removing an icon that was set in a previous update.

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/bf951826537f Make accentcolor and textcolor optional. r=jaws

Hello, is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag. Thanks!

I've updated https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/theme#colors, and also the the compat data: https://github.com/mdn/browser-compat-data/pull/2772, although the compat data isn't deployed to the Wiki yet. Marking dev-doc-complete, but please let me know if we need anything else.