Unique colors for expanded traces

[rreusser]

This PR clears color defaults between supplyDefault passes so that expanded traces get unique colors.

[rreusser]

@etpinard @alexcjohnson I implemented this as discussed. It turned out to be pretty simple, but these things usually have unintended consequences, so glad to get feedback if I'm missing any caveats. I've added an image test and a supplyDefaults test, though maybe that's a bit redundant.

[alexcjohnson]

would it be worthwhile to cache colorAttrs in _module so we don't need to crawl the schema with every supplyDefaults?

[rreusser]

Good idea. Will do.

[rreusser]

Current status: Lots of test failures due to subtle color changes. Debugging the source to determine whether desired or not.

[etpinard]

No need for a new file here.

Can you move this to either transform_groupby_test.js or the poorly-named transform_multi_test.js?

[etpinard]

would be better for sure, but I'm ok with this for now, as this will never get called for transform-less traces.

Maybe we start thinking about adding transform module categories (similar to our trace module categories) to help us out.

[alexcjohnson]

It's a hypothetical for now, since groupby is currently the only one that can yield more than one trace... but my rationale for suggesting this was that it's precisely this condition that breaks default colors. We'll see what happens if/when we add more such transforms, if this is insufficient then a category system is a great idea!

[etpinard]

@rreusser looking good!

I didn't notice any red flags in those failed tests. 👌 for updating them.

[rreusser]

@etpinard Your initial sense was correct. To get the sequence correct and to preserve existing behavior, I had to move this into the groupby code instead of calling it directly from supplyDataDefaults. That makes it a general-purpose function that still has to be called manually where needed (since the styling pass from within the transform is kinda custom), so I left it in plots.js. Waiting to confirm that it passes the original and new tests, otherwise I think I'm content.

[rreusser]

I've removed a redundant image mock and transferred making sure the colors are correct over to the jasmine test. Looks like it's green, so I don't have anything further to add to this PR.

[alexcjohnson]

Looks great to me! One more question though: Are there other trace types that make sense with groupby that we should include useExpandedTraceDefaultColor in (and test)? The ones that look like they should be groupable to me are bar, box, choropleth, histogram (perhaps not 2d histogram? grouping makes sense but the presentation is tricky), and all the other scatter types.

[etpinard]

Are there other trace types that make sense with groupby that we should include useExpandedTraceDefaultColor

In theory, all attributes that get coerced using defaultColor should be declared with useExpandedTraceDefaultColor: true.

I'll make sure they're handled properly in this PR.

---

By the way, as object keys can't be minified, would anyone be opposed to shortening useExpandedTraceDefaultColor to something smaller (e.g. colorDflt)?

Even better, I think we don't even have to add another attribute field: all valType: 'color' attribute in traces that don't have a dflt field set should --- in theory --- be useExpandedTraceDefaultColor: true.

[alexcjohnson]

Even better, I think we don't even have to add another attribute field: all valType: 'color' attribute in traces that don't have a dflt field set should --- in theory --- be useExpandedTraceDefaultColor: true.

Interesting - I think this is valid (in traces only, of course, not in layout, but we're only crawling trace modules anyway) so yeah, go for it!

[alexcjohnson]

attribute in traces that don't have a dflt field set

Does coerce complain if it sees either zero or two defaults? Either if the attribute has a dflt but we override it in the coerce call, or if neither the attribute nor the coerce call contains a default? Should it? That would make this distinction more robust.

And even if we decide this is a good idea, not sure if we want it to happen all the time or just patch it in during testing somehow.

[etpinard]

Interesting - I think this is valid

Replacing

(attr.useExpandedTraceDefaultColor)

by

(attr.valType === 'color' && attr.dflt === undefined)

on this line, gives the following colorAttrs list:

 ['line.color', 'fillcolor', 'marker.colorbar.tickfont.color', 'marker.colorbar.titlefont.color', 'marker.line.color', 'marker.gradient.color', 'marker.color', 'textfont.color', 'error_y.color', 'error_x.color']

which includes a few attributes that should have had useExpandedTraceDefaultColor: true (e.g. error_y.color) and a few others like marker.colorbar.tickfont.color that inherit from layout.font.color.

Clearing attributes that inherit from layout.font.color doesn't have any side-effects (apart from a few extra items in that for-loop), so I think this solution is the way to go. It's definitely a lot more robust than manually having to list useExpandedTraceDefaultColor: true for all traces. I hope agree.

[etpinard]

so I think this solution is the way to go.

All done in 25c2951.

@alexcjohnson ready to 💃 ?

[alexcjohnson]

Very nice. 💃

[rreusser]

Great solution re: not having to manually set, @etpinard. Thanks! 💃