Material-ui: [Accordion] Expand icon button calls onChange twice per click

[Hivemind9000]

The expand icon button calls the onChange function twice per click.

It was tripping me up when I was trying to perform an action when the panel was expanded (and only wanted the action performed once).

Expected Behavior

Should only call onChange once (i.e. when the state of the panel changes).

Current Behavior

Calls it twice per state change (ONLY when the expanded button is clicked - it calls only once when the summary section is clicked, which is inconsistent). Upsets things when trying to perform an action based on a state change (ends up doing it twice). Can be worked around by maintaining a local instance variable and comparing with the provided expanded parameter.

onExpandChange = (event, expanded) => {
  if (expanded && expanded != this.prevExpanded) {
    // Call server and do other stuff
  }
  this.prevExpanded = expanded;
}

Steps to Reproduce (for bugs)

Example of behavior (to show double call):

CodeSandbox Example Link

[kgregory]

When ExpansionPanelSummary is included, it also uses the provided onChange handler, so it will be called twice in your example. The bug in your example is that you aren't considering the second parameter that is passed to the onChange handler: expanded

See this fork of your codesandbox

In your example, you were toggling a class instance variable whenever it was called. The updated example has modified toggleExpand (and the button's click handler) so that the expanded parameter, which comes from the internal state of the component, is used to update your state.

This approach is used in the Controlled Accordion demo.

[Hivemind9000]

@kgregory Thanks. The example I provided with the class instance variable was only to catch the double call. I have edited my initial post to explain a little more accurately why it is a problem.

The double call is a little counter intuitive (and I can't think of any situation where it would make sense - even the Controlled Accordion demo doesn't seem to need it). Note that it only calls twice when you click the expanded button. If you click the summary section away from the button, it only calls once.

Everything behaves as expected if all you are doing is handling something keyed off the expanded parameter value, but not if you are doing something that is acting upon a state transition. i.e. if I do a server call when the component is expanded, I'll end up calling it twice. Of course I could maintain a class instance variable (as I did above) and do a comparison to avoid the double up, but it seems a little unnecessary and unexpected.

Maybe some note in a docs/demo might help others?

[kgregory]

You’re right, it shouldn’t call twice. Reopening.

[Hivemind9000]

@kgregory Thanks again. What led me down this path was another problem I was trying to debug. I've finally managed to replicate it in codesandbox. The problem seems to be if I set the expanded prop with a variable that isn't a component state, the expanded prop is updated with the correct value, but the panel doesn't respond (even though it is definitely re-rendered). I had a quick look using the React debug tools and I can see the prop being updated, but the internal expanded state of the component is not being changed.

You can check it out here.

The situation I am encountering this (in my code) is where I have wrapped the component and am passing the expanded property down via props (like a HoC).

I wondered if it may have been related to this issue. If not, should I create another thread?

--- Update ---

If I changed the line

<ExpansionPanel expanded={this.exp} onChange={this.toggleExpand}>

to

<ExpansionPanel expanded={(this.exp) ? true : false} onChange={this.toggleExpand}>

It works. Not entirely sure why this works. See this codesandbox.

[kgregory]

@hivemind9000 you’re initializing this.exp to undefined

[oliviertassinari]

The issue comes from the fact that we listen twice to the onClick event. I don't think that it's needed. Worse, it's introducing an unexpected behavior. We can simply remove this line:
https://github.com/mui-org/material-ui/blob/fd7facc447dea934704be92d208e6268a48c9d02/src/ExpansionPanel/ExpansionPanelSummary.js#L137
And rely on event bubbling.

[Hivemind9000]

@kgregory Interesting, I didn't realize the isControlled flag was only set in the componentWillMount phase. I can see it does that also in SwitchBase and Tooltip, but not in Input. Input allows control to be switched from internal to external at any stage of the component lifecycle.

isControlled(props = this.props) {
  return hasValue(props.value);
}

This is really a question for you and @oliviertassinari - what is considered best practice? It would be good to have a consistent approach (which could be noted in the documentation eventually). My preference is the approach in the input component, as it otherwise can trip up the developer who doesn't know about how this framework approaches controlled components. Unless there's a serious downside to changing control after instantiation (looking through the code I couldn't see one).

My case is a little unique as I'm building a development framework on top of react and material-ui that enables apps to be built without JavaScript (using a high-level declarative expression language instead). My problem is that some of the "variables" used in expressions may not be initialized by the user prior to the component being instantiated, leading to this unexpected behavior (to the user at least). I can try and catch it somehow, but would prefer "it just works"... 👍

[kgregory]

@hivemind9000 I agree with you. Input uses the preferred pattern.

[kgregory]

I will try to make some time for this issue tonight.

[oliviertassinari]

@kgregory Thank you. To be noted, the PR only fix:

Expand icon button calls onChange twice per click

I think that you surfaced an interesting inconsistency issue regarding the controlled behavior of the Input component.