[EnhancedButton] Fix onClick event being fired twice on "Enter Key" press

[karaggeorge]

This PR solves this issue: #9344

This is my first PR to this repo, so I'm not quite familiar with the process. The bug doesn't exist on v1x. Only for v0.20.0. I created my branch from that, but I can only create this PR against master or v1-beta.

The issue was that the event was fired once from the normal onClick proc which happens naturally when space/enter is pressed on a focused button, and then once more from the keyDown and keyUp events in the EnhancedButton component manually. That's why the problem showed up in all the buttons. Removing those extra checks and event fires solved the issue.

I wasn't able to write a failing test for the bug, since Enzyme event simulations call the handlers directly and don't actually simulate browser events.

[mbrookes]

You need to select the master branch.

[mbrookes]

Also, please update the commit message to match the revised PR title - it help keep the commit log clean, and simplifies preapring the release notes. Thanks!

[karaggeorge]

@mbrookes thank you for the help. I renamed the commit message

[oliviertassinari]

The pull request has been inactive for a month. I'm closing. Maybe someone will review it later.

[m2mathew]

This is an odd bug. @karaggeorge, when I apply your changes within our app, I can see that the double-fire for the onClick handler is always reduced to 1. Way to go!

But, under normal circumstances without these changes, it is sometimes firing once, sometimes twice. Even using the same higher order component that calls the same underlying onClick, with exactly the same signature. I am going to go ahead and merge this. Thanks for being patient with us, @karaggeorge! 💯

[djbuckley]

This breaks the tests no as the synthetic keyboard clicks no longer count.

481 passing (5s)
1 pending
1 failing

1. should fire the click handler if keyboard focused and the >enter or space keys are hit:

   AssertionError: expected 0 to equal 1

   • expected - actual

   -0
   +1

   at Function.assert.strictEqual (node_modules\chai\lib\chai\interface\assert.js:178:32)
   at Context. (src/internal/EnhancedButton.spec.js:298:12)

I'm currently trying to find the actual root cause of the issue so this can be reverted and the tests run as planned.
The test could be easily "fixed", but I don't think it should be bodged just to get round this. Unless someone from the maintenance team can provide a reason to update the tests that have been there 2 years.

Personally I think it would be better to get to the root cause.

[oliviertassinari]

It's essentially why I do no longer want to have to deal with v0.x. The test coverage is low. Every single change can lead to unexpected issues. I think that it's too much risk to accept significant pull-requests. I'm happy @djbuckley is looking into it.

[djbuckley]

right .... I've been looking at this all day I think I can now see individual subpixels changing on my screen now .... 😵

The actual EnhancedButton was not malfunctioning at all. When the handleKeyUp fires the handleClick, a second event is generated that is trapped by the handleClick. I haven't been able to work out what is generating that second event 😕

so ...

I have a patch that basically saves the timestamp of the last event processed by handleClick. If the current event being processed has the same timestamp then its ignored.
The tests pass.
I feel its a bit of a bodge as its not fixed the route cause, but it is handling the issue without dropping any functionality.

...thoughts?

handleClick = (event) => {
const bumpTime = this.state.lastHandledEventTime + 1;
const eventTime = event ? event.hasOwnProperty('timeStamp') ? event.timeStamp : bumpTime : bumpTime;  // this is for the chance that no event is provided

if (eventTime !== this.state.lastHandledEventTime) {
  this.setState({lastHandledEventTime: eventTime});
  this.cancelFocusTimeout();
  if (!this.props.disabled) {
    tabPressed = false;
    this.removeKeyboardFocus(event);
    this.props.onClick(event);
  }
 }
};

Incidentally the fork we have been using has +95 unit tests in it, I'll see what can be merged into the master branch (stuff thats not specific to the changes that are in our fork)... just to increase coverage I checked and the tests are specific to the stuff we introduced 😞

[caseychoiniere]

@djbuckley I'm curious if you think this might actually make it into the the Material-UI v0.20 master branch at some point? I'm working on updating an app from v0.16.7 and this is definitely a stopper for me. Thanks for your work on it.

[djbuckley]

@caseychoiniere I've put the change in as a pull request, so .... theoretically it should come through.
I haven't found the root cause of the problem though, so this is more of a mitigation patch than a fix.

If anyone finds a better way to keep the functionality originally there without this mitigation... please write it up