Fix bug wherein unsetting and resetting fill-outline-color causes TypeError to be thrown

[anandthakker]

mapbox-gl-js version: master

Using setPaintProperty(..., 'fill-outline-color', undefined) ( to 'unset' this paint property), and then setting it again to a value causes an exception to be thrown.

Example:

var style = {
  "version": 8,
  "name": "Mapbox Dark",
  "sources": {
    "composite": {
      "url": "mapbox://mapbox.mapbox-terrain-v2,mapbox.mapbox-streets-v7",
      "type": "vector"
    }
  },
  "sprite": "mapbox://sprites/mapbox/dark-v9",
  "glyphs": "mapbox://fonts/mapbox/{fontstack}/{range}.pbf",
  "layers": [
    {
      "id":"background",
      "type":"background",
      "layout":{},
      "paint":{"background-color":"hsl(55, 1%, 20%)"}
    },
    {
      "id": "barrier_line-land-polygon",
      "type": "fill",
      "source": "composite",
      "source-layer": "barrier_line",
      "filter": [
        "all",
        [
          "==",
          "$type",
          "Polygon"
        ],
        [
          "==",
          "class",
          "land"
        ]
      ],
      "layout": {},
      "paint": {
        "fill-color": "hsl(55, 1%, 20%)",
        "fill-outline-color": "hsl(55, 1%, 20%)"
      }
    }
  ],
  "created": 0,
  "modified": 0,
  "owner": "mapbox",
  "id": "dark-v9",
  "draft": false
}
var map = window.map = new mapboxgl.Map({
    container: 'map',
    zoom: 2,
    center: [-77.01866, 38.888],
    style: style,
    hash: style
});

var set = false
setInterval(function () {
    var value = set ? 'hsl(55, 1%, 20%)' : undefined;
    console.log(set ? 'set' : 'unset')
    map.setPaintProperty('background', 'background-color', set ? 'white' : 'black')
    map.setPaintProperty('barrier_line-land-polygon', 'fill-outline-color', value)
    set = !set
}, 1000);

Error:

Uncaught TypeError: Cannot read property '0' of undefined
    at StyleTransition.interpolate.color [as interp] (http://localhost:9966/dist/mapbox-gl-dev.js:18233:25)
    at StyleTransition.calculate (http://localhost:9966/dist/mapbox-gl-dev.js:10245:17)
    at FillStyleLayer.getPaintValue (http://localhost:9966/dist/mapbox-gl-dev.js:9699:31)
    at FillStyleLayer.getPaintValue (http://localhost:9966/dist/mapbox-gl-dev.js:9978:55)
    at FillStyleLayer.recalculate (http://localhost:9966/dist/mapbox-gl-dev.js:9795:46)
    at Style._recalculate (http://localhost:9966/dist/mapbox-gl-dev.js:8921:19)
    at Map._render (http://localhost:9966/dist/mapbox-gl-dev.js:16353:24)

[anandthakker]

I think this is fundamentally a problem involving how default values are handled for paint properties, but I haven't exactly pinpointed the root cause why it's manifesting. My sense from stepping through the debugger is that it's some kind of timing issue with transitions, but that's as much as I've determined so far.

[anandthakker]

A crumb of evidence supporting the theory that it's a timing issue: when I put a breakpoint, say, here in StyleLayer, I never see the exception being raised -- even if I just immediately hit the 'continue' button each time the breakpoint trips.

[anandthakker]

Okay. I think the underlying issue here is about the special-case handling for default fill-outline-color values, e.g. here in fill_style_layer:

    getPaintValue(name, globalProperties, featureProperties) {
        if (name === 'fill-outline-color' && this.getPaintProperty('fill-outline-color') === undefined) {
            return super.getPaintValue('fill-color', globalProperties, featureProperties);
        } else {
            return super.getPaintValue(name, globalProperties, featureProperties);
        }
    }

Transitioning either to or from undefined leads to a problem, but it manifests differently for each direction:

• When we're supposed to be transitioning to fill-outline-color: undefined, we take the first branch in the code above, which means that even if fill-outline-color was supposed to transition from some value to default, it actually just immediately goes to the default (fill-color) value at the beginning of the transition.
• When we're supposed to be transitioning from fill-outline-color: undefined to a set value, we take the second branch. That is the case first reported in this ticket: when StyleTransition tries to interpolate, it fails, because the spec.default value from which we're transitioning is undefined.

[anandthakker]

Possible fixes for this, ranked in order of my personal preference:

1. Include an additional hack in FillStyleLayer#getPaintValue to short-circuit transitions in the second case (the exception-throwing one): that is, when facing a fill-outline-color transition from undefined to a target value, just snap immediately to the target value. This isn't "correct", but it would make the second case incorrect in the same way that the first case is... and in a way that doesn't throw.
2. Don't fix: require users to explicitly set fill-outline-color on both sides of a transition.
3. Introduce analogous if (name === 'fill-outline-color') forks into StyleTransition to 'correctly' fix transitioning to/from default fill-outline-color. This might require some fairly significant hackery, because we'd need to somehow send fill-color information (for both the previous value and the target value) into StyleTransitions.

@jfirebaugh @lucaswoj @mourner thoughts?

[anandthakker]

^ I meant to say: the reason I ranked the 'correct' fix last is that @mourner indicated the plan was probably to remove fill-outline-color entirely, so that I figured the extra code complexity/weirdness would not be worth it.

[anandthakker]

@mourner indicated the plan was probably to remove fill-outline-color entirely, so that I figured the extra code complexity/weirdness would not be worth it.

Reference: mapbox/mapbox-gl-style-spec#240

I'm going to proceed with option 1 above unless there are any objections.

[mourner]

@anandthakker yes, that sounds like a good option.