[Tabs] onChange fire even if value didn't change

[jjoselv]

Given option selectionFollowsFocus is true on Tabs component, when a Tab is clicked, onChange handler runs twice (one on focus and the other on clicked).

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Given option selectionFollowsFocus is true on Tabs component, when a Tab is clicked, onChange handler runs twice (one on focus and the other on clicked).

Expected Behavior 🤔

It should only run once (check if already selected maybe).

Steps to Reproduce 🕹

Click on each Tab
https://codesandbox.io/s/friendly-sound-30c2y?file=/index.js

Steps:

1. Click on each Tab

Tech	Version
Material-UI	v4.11.0
React	16.13.1
React DOM	16.13.1
Browser	Chrome 85

[eps1lon]

Thanks for the report. The underlying issue is that onChange is called whether the value changes or not: you can repeatedly click on the selected tab and onChange will be called (regardless of selectionFollowsFocus).

[jjoselv]

May I provide a PR?

[eps1lon]

May I provide a PR?

Sure, go for it. Don't forget to add a test.

[jjoselv]

Sure

[jjoselv]

Let me check for Contribution Guidelines

[oliviertassinari]

@jjoselv #20361 might help as a prior change in the same direction (sorry that the test isn't very descriptive)

[jjoselv]

Thanks olivier!

[jjoselv]

@oliviertassinari @eps1lon I'm having trouble making the test pass (select variable is not getting value true)
Can you take a look? github.com:jjoselv/material-ui.git at fix-on-change-fire-even-if-value-did-not-change

I run yarn run test inside material-ui/packages/material-ui

[jjoselv]

Maybe I have to let it elapse some time between clicks?

[oliviertassinari]

@jjoselv The value is controlled in your test, it stays on 0, even after the first click. Click on the first item instead

      fireEvent.click(getAllByRole('tab')[1]);
      expect(handleChange.callCount).to.equal(1);
      expect(handleChange.args[0][1]).to.equal(1);

      fireEvent.click(getAllByRole('tab')[0]);
      expect(handleChange.callCount).to.equal(1);

[jjoselv]

@oliviertassinari The thing is that I want to do the following test:
Initial Conditions
Tab 0 selected.

Act -> Click on Tab 1
Assert that handle change has been called once

Act -> Click on Tab 1 (again)
Assert that handle change has not been called (apart from that first time)

And that way the onChange handle does not run for already selected elements

[jjoselv]

I see now, you code checks the same but in a different way. Ok, I'll submit your proposal 👍

[jjoselv]

#22381