Fix setting tickwidth, tickcolor, ticklen, linecolor and possibly more attributes via template

[archmoj]

Fixes #4852 & fixes #4906 by revisiting Lib.coerce2 function.

TODO:

• figure out if changes in 11fd23c needed? That mock uses plotly.py template which is important.

updtae: 11fd23c baseline change is due to issue #4906 which is fixed by this PR as well.
demo: before vs after

@plotly/plotly_js

[alexcjohnson]

    var attr = nestedProperty(attributes, attribute).get();

From coerce (line 364 above) - then the attr || {} below would be unnecessary - attr will always exist.

But I'm a little concerned whether the logic below (theDefault !== undefined && theDefault !== propOut) is robust - what if your template explicitly provides the same value as the default? If valIn does that, we return propOut, but given the logic here if the template does that we'll return false.

Could we instead make a base function like _coerce that returns both the value and where it came from (default, template, or container), then have exports.coerce just ignore that second part and return the value, whereas exports.coerce2 returns false if the value came from a default, otherwise the value?

Actually isn't there another already existing problem with coerce2? Looks like if you provide an invalid valIn, the prop will be set to the default value but it'll look like you set it explicitly. The base function approach would fix that problem too.

[archmoj]

@alexcjohnson thanks for the review.
Please find the commits below.

[alexcjohnson]

Hmm, I'm not sure about this, but at this point I think the best way to handle it will be extensive jasmine testing.

I see we actually do have a test that contradicts one thing I was hoping to "fix":

plotly.js/test/jasmine/tests/lib_test.js

Line 781 in be70865

it('should set and return the default if the user input is not valid', function() {

Do we have a good reason for that behavior? It seems contrary to what we do with templates, as well as contrary to what I think the purpose of coerce2 should be, which would be stated something like:

should set the default and return false if the user input is not valid

@archmoj do you see any issue if we make such a change? I feel like that test is locking down a bug rather than useful behavior.

(while we're at it there's a piece of copypasta in that test

plotly.js/test/jasmine/tests/lib_test.js

Lines 795 to 798 in be70865

	expect(colOut).toBe('rgba(0, 0, 0, 0)');
	expect(sizeOut).toBe(outObj.testMarker.testSize);
	expect(sizeOut).toBe(20);
	expect(sizeOut).toBe(outObj.testMarker.testSize);

- should be 2 colOut lines and 2 sizeOut lines)

So what I'd like to see is a test covering each of the valTypes used by coerce2, and for each one:

• valid input in the container
• valid input in the container but changing type (ie '2' to 2) if such a thing exists for this valType
• invalid input in the container OR no input in the container, with:
  • no input in the template
  • invalid input in the template
  • valid input in the template
  • valid input in the template but changing type (ie '2' to 2) if such a thing exists for this valType

I suspect getting this to work for all valTypes may not be worthwhile - especially since there are things like colorscale where even the default may be "fixed" by the coercion process, and subplotid where the default affects which input values are allowed. But we should be able to find a way to make it work for the ones we actually use for coerce2

[archmoj]

@alexcjohnson thanks for the review. Please see cf7c061 and 21b86cd.

[archmoj]

@alexcjohnson wondering what should be the result of this test?
https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/axes_test.js#L1791-L1797
Shouldn't it be

expect(yaxis.ticklen).toBe(undefined);
expect(yaxis.tickwidth).toBe(undefined);
expect(yaxis.tickcolor).toBe(undefined);
expect(yaxis.ticks).toBe('');

?

[archmoj]

@alexcjohnson this PR is ready for another review.
Please see adc36cd.

[nicolaskruchten]

Can I get a sense of how much more time would be spent to finish this PR? I didn't add this to the 1.54.2 milestone so I don't want it to hold up the release ;)

[archmoj]

Can I get a sense of how much more time would be spent to finish this PR? I didn't add this to the 1.54.2 milestone so I don't want it to hold up the release ;)

Possibly a couple of hours. But this is tricky and it may require a patch of its own.
So I am moving this & related issues to the next milestone.

[alexcjohnson]

Ah I see - shouldValidate is really just used to improve performance for regular coerce by skipping these extra validation steps. Very nice.

[alexcjohnson]

That said, is this particular line actually necessary? Since this step is overwriting valIn, after this we're going to treat it as though it came from the container, so on line 406 (or 398) we'll reset it to 'c'. In practice we don't care about this for now - given the coerce2 logic all we care about is truthy (specified) or falsy (default), but in case that ever changes, seems like we should either:

• remove this line and pick a single truthy value for src so future users don't think 'c' vs 't' is accurate
• or fix lines 398 and 406 to preserve a pre-existing 't', something like:

// 398
src: src || 'c'
//406
if(!src && valOut !== undefined && shouldValidate && validate(valIn, attr)) src = 'c';

[archmoj]

Good call. Revised in 24d1da9.

[alexcjohnson]

Looks great - nice tests, and you're absolutely right that axes_test was locking down behavior we don't actually want.

Just one small comment https://github.com/plotly/plotly.js/pull/4904/files?short_path=275b5ba#r441249204 then this is ready to go! 💃