Bug: CSS custom property pfe-avatar--colors not working on pfe-avatar

[zeroedin]

Description of the issue

Pfe-avatar should allow the color set to be customizable through --pfe-avatar--colors. However, this feature is not currently working, and this.customColors() always returns undefined .

Impacted component(s)

• pfe-avatar

Steps to reproduce

1. Inspect element
2. $0.customColors = '#00000 #ffcc00 #eeeeee;'
3. $0.constructor._registerColors()

or

1. Set --pfe-avatars-colors: '#00000 #ffcc00 #eeeeee;' on :host in scss or custom CSS

Expected behavior

Expect $0.constructor.colors to include customColors Expect an avatar that has this.customColors to display in one of those colors as randomized by this._colorIndex

Possible fix

The following is a possible fix if it doesn't break any unexpected APIs I may be overlooking could easily be implemented in both branches.

Create class property colors with getter/setter:

static set colors(colors) {
  this._colors = colors;
}

static get colors() {
  return this._colors;
}

Remove property getter for customColors() on line 76-78 and replace it with instantiation in the constructor, also remove line 314 call to PfeAvatar._registerColors() and replace it with call in the constructor this.registerColors():

constructor() {
  super(PfeAvatar);
  
  this._initCanvas = this._initCanvas.bind(this);
  this.customColors = this.cssVariable("pfe-avatar--colors");
  this.registerColors();
}

Change class method static _registerColors() to instance method registerColors() on line 112.
Change this.colors = []; to PfeAvatar.colors = [] on line 113. Change this.defaultColors to PfeAvatar.defaultColors on line 114. Change this._registerColor(color) to class method call PfeAvatar._registerColor(color) on line 124 and 134.

registerColors() {
  PfeAvatar.colors = [];
  const contextColors = this.customColors || PfeAvatar.defaultColors;

  contextColors.split(/\s+/).forEach((colorCode) => {
    let pattern;
    switch (colorCode.length) {
      case 4: // ex: "#0fc"
        pattern = /^#([A-f0-9])([A-f0-9])([A-f0-9])$/.exec(colorCode);
        if (pattern) {
          pattern.shift();
          const color = pattern.map((c) => parseInt(c + c, 16));
          PfeAvatar._registerColor(color);
        } else {
          this.log(`[pfe-avatar] invalid color ${colorCode}`);
        }
        break;
      case 7: // ex: "#00ffcc"
        pattern = /^#([A-f0-9]{2})([A-f0-9]{2})([A-f0-9]{2})$/.exec(colorCode);
        if (pattern) {
          pattern.shift();
          const color = pattern.map((c) => parseInt(c, 16));
          PfeAvatar._registerColor(color);
        } else {
          this.log(`[pfe-avatar] invalid color ${colorCode}`);
        }
    }
  });

  return this.colors;
}

Change call to PfeAvatar.colors to this.colors in _registerColor() on Line 145:

static _registerColor(color) {
  this.colors.push({
    color1: `rgb(${color.join(",")})`,
    color2: `rgb(${this._adjustColor(color).join(",")})`,
  });
}

[bennypowers]

@mwcz what's the intention here? that I can register colours on the class and have them available to every instance? or that each instance can have it's own custom colours? or both?

[mwcz]

There's definitely a bug here. customColors() is an instance method but is being called from the static _registerColors, so it's returning undefined and falling back to defaultColors every time.

@bennypowers The intention was to have colors customizable on a page-wide or per-avatar basis. page-wide is way more important, so I'd be totally in favor of fixing that and throwing away per-avatar customization. Since per-avatar doesn't work anyway, removing it wouldn't even be a breaking change.

@zeroedin I think the reason customColors was returning undefined is you're running $0.customColors = on a property that doesn't have a setter, only a getter. To change customColors you need to set --pfe-avatar--colors: #ff0000 #00ff00 #0000ff; on the <pfe-avatar> itself or somewhere that will cascade to the avatar. I wasn't quite able to follow the fix you suggested since it seemed like you (just like my code!) have static methods calling instance methods and instance methods overwriting static properties (registerColors setes PfeAvatar.colors = []).

[zeroedin]

@mwcz Yes, at the time of this writing it believe it was my misunderstanding of how the customColors was intended to work. I think when you and I talked over google meet we had clarified that misunderstanding to my recollection. I also have a modified component written that cleans up on the static/instance method issue you are noting in a further exploration. That code just resides locally though I have not pushed it anywhere. I'll revisit and share if we deem it necessary or helpful.

I believe @bennypowers and I discussed leaving this issue open to review pfe-avatar later if a change of functionality was warranted after 2.0 release. That said, if there is a bug and the functionality is not as it should be compared to 1.* branch we should fix it.

[bennypowers]

@mwcz @zeroedin the CSS Paint API is very similar to the canvas API we're already using and it has a polyfill. I do believe that using this paint API along with CSS custom properties we could solve both the page-wide and per-element issues just with CSS:

html {
  --avatar-base-hue: 128;
}

pfe-avatar#special {
  --avatar-base-hue: 64;
}

or something

[mwcz]

@bennypowers Very cool, that's the first I've heard of the Paint API and it sounds like exactly what's we need: a way to draw something with JS and react to custom property changes.

It looks like Firefox and Safari may never implement it. I know Safari says "under development" but the webkit ticket has been dormant for four years. There's a good chance we'd be stuck with the polyfill forever. If the polyfill is really good, that may be fine.

[bennypowers]

Some things to consider:

Do we ship the component with the polyfill? probably not, right? but then we require users to load it? or have the component lazy load it as needed? neither of those are amazing options