search

mapbox / mapbox-gl-js

Interactive, thoroughly customizable maps in the browser, powered by vector tiles and WebGL
https://docs.mapbox.com/mapbox-gl-js/
Other
11.15k stars 2.21k forks source link

Color validation doesn't evaluate channel values #6467

Open tristen opened 6 years ago

tristen commented 6 years ago

It looks like invalid color values are passing validation!

mapbox-gl-style-spec version: 11.1.1

Steps to Trigger Behavior

import validateProperty from '@mapbox/mapbox-gl-style-spec/validate/validate_property'
import styleSpec from '@mapbox/mapbox-gl-style-spec/reference/v8.json'

const errors = validateProperty({
  key: '',
  layerType: 'line',
  objectKey: 'line-color',
  style: { glyphs: 'fake' },
  value: 'hsl(, 100%, 59%)', // There's no value for hue here
  styleSpec,
  valueSpec: styleSpec['paint_line']['line-color']
}, 'paint');

// []

Expected Behavior

An entry in the error array indicating the color value did not pass validation.

Actual Behavior

An empty error array returns resulting in no error found.

mollymerp commented 6 years ago

this appears to be a bug with our csscolorparser dependency – it only validates the number of arguments passed to the color types are valid, just that there are the correct number of args

https://github.com/deanm/css-color-parser-js/blob/0d3798690d7e52539f43155a07d3d0fd484e6b8b/csscolorparser.js#L166-L181

we have a PR open to change our color parser to d3 #6409 so that might fix this issue but its unclear if it will be merged. I'll ticket in the upstream repo.

mollymerp commented 6 years ago

errr looks like our current css parser hasn't been touched in 2 years 😬

tristen commented 6 years ago

oooo yeah. I recently swapped parsing in react-colorpickr out for color-string which has been working well so far!

import colorString from 'color-string'
colorString.get('hsl(0, 0, 0');

// null