Fix modeBarButtons issue

[nickmelnikov82]

As described in issue #1157, when dcc.Graph with the specified "modBarButtons" in the configuration is duplicated, an error occurs. The problem is that dash cannot pass a javascript function to the configuration.

Contributor Checklist

• I have broken down my PR scope into the following TODO tasks
  • Fix issue
  • Make test
• I have run the tests locally and they passed. (refer to testing section in contributing)
• I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

• I have added entry in the CHANGELOG.md
• If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
  • this GitHub #PR number updates the dash docs
  • here is the show and tell thread in Plotly Dash community

[alexcjohnson]

Is there a reason for the this.configModeBarButtons indirection? Looks like the idea might have been that from one update to the next if you removed modeBarButtons from the config (or removed config entirely?) the previous set of buttons would remain? I don't see why we'd want that, but also this wouldn't actually happen because this.getModeBarButtons will always return a truthy value - unlike in Python, in JS [] is truthy. Likewise we're always going to enter the if (this.configModeBarButtons) branch here.

So I'd suggest to remove this.configModeBarButtons entirely, and have this branch be if (modeBarButtons.length)

[nickmelnikov82]

This was done because the configuration is overwritten inside the component. If we have an empty parameter modeBarButtons then it remains empty and component get default config. But if there is a string configuration there, then after initialization the string is replaced with a JS object and the click function is not passed to the python code. So I made local storage for the string parameter so that it can be reused.

[alexcjohnson]

the configuration is overwritten inside the component

That's why we're cloning the config and copying in the original modeBarButtons spec, but I still don't see the purpose of this.configModeBarButtons. Can you show a situation (even better if it's a test!) where we need this bit of internal state to get the right behavior? I'm just wary of it holding on to a state we no longer want and thus introducing a new more subtle bug.

[nickmelnikov82]

You can check example code from issue #1157. The problem manifests itself when trying to copy or move a component. It turns out that such parameters modeBarButtons=[['resetScale2d']] are transferred during initilization, and such 'modeBarButtons': [[{'name': 'resetScale2d', '_cat': 'resetscale', 'attr': 'zoom', 'val': 'reset', 'icon': {'width': 928.6, 'height': 1000, 'path': 'm786 296v-267q0-15-11-26t-25-10h-214v214h-143v-214h-214q-15 0-25 10t-11 26v267q0 1 0 2t0 2l321 264 321-264q1-1 1-4z m124 39l-34-41q-5-5-12-6h-2q-7 0-12 3l-386 322-386-322q-7-4-13-4-7 2-12 7l-35 41q-4 5-3 13t6 12l401 334q18 15 42 15t43-15l136-114v109q0 8 5 13t13 5h107q8 0 13-5t5-13v-227l122-102q5-5 6-12t-4-13z', 'transform': 'matrix(1 0 0 -1 0 850)'}}]]}, are returned when copying the configuration.

[alexcjohnson]

OK - I think I understand what you're doing here now. But I strongly suspect there's a manifestation of this bug in plain plotly.js as well, so I'd much rather we address it there.

The problem is here where we mutate the input - which is the user-provided array, something we should not alter. Instead we should create a copy with the object inserted in place of the string. Would be a one-line change if we used ramda in plotly.js, but we don't...

@nickmelnikov82 I'm going to close this PR, if you want to solve it on the plotly.js side please go ahead! Otherwise we'll get it onto @archmoj's plate (or perhaps @emilykl's?) sometime soon.