Make accentcolor and textcolor optional

RESOLVED FIXED in Firefox 63

Status

enhancement
P5
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla63
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [ntim-intern-project])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
What do you think about this?
Flags: needinfo?(mdeboer)
Flags: needinfo?(jaws)
(Assignee)

Comment 2

2 years ago
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.
Assignee: nobody → ntim.bugs
(Assignee)

Comment 3

2 years ago
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.
Flags: needinfo?(mdeboer)

Updated

2 years ago

Updated

2 years ago
Priority: -- → P5
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.
Flags: needinfo?(jaws)
(Assignee)

Updated

a year ago
Blocks: themingapi-polish
No longer blocks: themingapi
(Assignee)

Updated

a year ago
Assignee: ntim.bugs → nobody
(Assignee)

Comment 6

a year ago
This is actually an essential part for chrome compatibility.
Blocks: themingapi-chrome
No longer blocks: themingapi-polish
Component: WebExtensions: Frontend → WebExtensions: Themes
(Assignee)

Updated

a year ago
Summary: Make accentcolor, textcolor, headerURL optional → Make accentcolor, textcolor optional
(Assignee)

Comment 7

a year ago
headerURL was taken care of in bug 1404688
Summary: Make accentcolor, textcolor optional → Make accentcolor and textcolor optional
(Assignee)

Updated

a year ago
Assignee: nobody → ntim.bugs
(Assignee)

Comment 8

a year ago
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.

Updated

10 months ago
Product: Toolkit → WebExtensions
Whiteboard: [ntim-intern-project]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

10 months ago
mozreview-review
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 12

10 months ago
mozreview-review
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 hidden (mozreview-request)

Comment 14

10 months ago
mozreview-review
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.
Attachment #8991000 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

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

Comment 19

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf951826537f
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Updated

9 months ago
Keywords: dev-doc-needed

Comment 20

9 months ago
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!
Flags: needinfo?(ntim.bugs)
(Assignee)

Updated

9 months ago
Flags: needinfo?(ntim.bugs) → qe-verify-
(Assignee)

Updated

9 months ago
Depends on: 1476970
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.
You need to log in before you can comment on or make changes to this bug.