/insights tab check boxes don't work

[KyleJu]

https://wpt.fyi/insights check boxes don't work really well. e.g. when I unselect WebKitGTK, the query isn't updated.

[stephenmcgruer]

Having missed that @Hexcles had self-assigned this, I poked at this the afternoon because I assumed it would be easy to fix. It wasn't, but here's what I learnt.

1. this.splice is not notifying listeners of the selected property in BrowserMultiPicker

In selectedChanged, when a browser is re-added to the list we splice it on via: this.splice('selected', this.selected.length, 0, browser);. For some reason this does not trigger the data binding update to others in the Anomalies class. Changing the splice to this.selected = this.selected.concat([browser]); does work.

You can see this by re-ticking an unselected browser. It does nothing.

2. The default value for others in Anomalies should be AllBrowserNames not DefaultBrowserNames

This speaks for itself, mostly. This gets covered up because Anomalies also passes down AllProducts into the BrowserMultiPicker, which then changes selected that is data-bound back to others anyway, but should be cleaned up.

(Can we just not have defaults for properties, like, ever? I feel like defaults keep causing problems and if possible we should force our embedder to specify everything always...)

3. BrowserMultiPicker is pretty broken

This is hard to summarize, but let's give it a go. The first big problem is that it only binds selectedChanged events to an initial set of paper-checkboxs that exist during ready():

  ready() {
    super.ready();
    this.shadowRoot.querySelector('dom-repeat').render();
    this.shadowRoot.querySelectorAll('paper-checkbox').forEach(c => {
      c.addEventListener('change', e => this.selectedChanged(c.value, e));
    });
  }

This means that if we ever have more than the original number of paper-checkboxs, the new ones will be ignored. But that's 'ok', we're created with a default set AllProducts - 1 from Anomalies and all we ever do is swap in products when you change the 'base' comparison one, right?

Nope! Because (I think...) products has a default value of DefaultProducts.map(p => Object.assign({}, p)), we initially run ready() with num(DefaultProducts) check-boxes in existence... which is one less than the num(AllProducts - 1 item) check-boxes we eventually end up with.. So we never even bind that last checkbox (WebKitGTK).

If we didn't have a default value for products at all in BrowserMultiPicker, we would have spotted this bug instantly I think. Kill all defaults! (My opinion).

EDIT: Also, if dom-repeat ever garbage-collected a check-box for whatever reason, we would not add a listener to a new item either. So even if we always had the same number of items, we ran a risk every time we changed the set.

[Hexcles]

Sorry I wasn't able to get to this after all.

[Hexcles]

FWIW, I agree with Stephen that extensive use of default values is confusing and masks mistakes.