Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blurring the input field triggers an "onInputValueChange" event - with the changes being empty and it clears the value #598

Closed
johnjesse opened this issue Oct 10, 2018 · 7 comments

Comments

@johnjesse
Copy link
Contributor

  • downshift version: 3.0.0
  • node version:
  • npm (or yarn) version:

Relevant code or config

What you did and What happened:
I noticed in our own project using downshift that tabbing through a control raised a inputValueChange event when you leave the control, with an empty new input value

Here's a quick repro - having forked the downshift demo - demonstrating the issue

downshift bug

Reproduction repository:

https://codesandbox.io/s/6w37l8lm9z

Problem description:

it shouldn't be firing an event, the input value has not changed

Suggested solution:

@kentcdodds
Copy link
Member

kentcdodds commented Oct 10, 2018

Nice find. I think we just need to improve this if statement:

https://github.com/paypal/downshift/blob/3bdf3fd6df46ed4c9ecb3229c1dc5ec7f68fb53a/src/downshift.js#L364

Would you like to make a PR (with a test) for this?

@johnjesse
Copy link
Contributor Author

Yup, I can do that

@johnjesse
Copy link
Contributor Author

johnjesse commented Oct 10, 2018

@kentcdodds Actually, I'm not sure it's that simple, it looks like state is deliberately reset when the input is blurred - and reset sets the inputValue back to the value of the selected item.

https://github.com/paypal/downshift/blob/3bdf3fd6df46ed4c9ecb3229c1dc5ec7f68fb53a/src/downshift.js#L799-L801

https://github.com/paypal/downshift/blob/3bdf3fd6df46ed4c9ecb3229c1dc5ec7f68fb53a/src/downshift.js#L911-L917

So we fail the if statement above (on line 364) because isStateToSetFunction is true

I'm not really sure why it's resetting state, I guess because the user has left it wants to put things back to the starting state...

I'd suggest I fix it by changing reset to only reset the inputValue to the value of the selected item if there is a selectedItem, otherwise it should just stay as it is?
inputValue: selectedItem ? this.props.itemToString(selectedItem) : this.getState().inputValue

I can't see a reason why inputValue should be changing if the selectedItem is null

-- EDIT --

Although there's clearly a test that is expecting it to be reset

https://github.com/paypal/downshift/blob/3bdf3fd6df46ed4c9ecb3229c1dc5ec7f68fb53a/src/__tests__/downshift.props.js#L136-L144

so maybe that's not the right fix 🤔

@kentcdodds
Copy link
Member

Yeah, we definitely don't want to fail that test. There's a good reason for that. We'll have to be more creative. onInputValueChange is a special-case prop and I'd recommend using onStateChange if you can.

I think we could get away with removing the !isStateToSetFunction and just relying on what state looks like currently:

if (stateToSet.hasOwnProperty('inputValue') && this.getState().inputValue !== stateToSet.inputValue) { 

Give that a try. It's probably fine.

@johnjesse
Copy link
Contributor Author

So I don't think that will fix it because stateToSet.inputValue is '' in this case. The reset deliberately sets it back to an empty string so amending the check to look for differences in the current state and the stateToSet won't stop the event being fired.

Also, in this case onStateChange is fired too - with inputValue: ''. I've amended the code sandbox example to demonstrate.

I think I need to understand why the state is reset when you tab out of the input box. I think the loop for linking the inputValue to the selectedItem is slightly wrong. When a selected item is chosen then the inputValue should be set to that, but why would the inputValue be set to the selected item if the user typed something else into the input box in the meantime, and why should that be thrown away when the user leaves the control

It's probably because our use case is slightly different. I think I can fix it in my case by using the stateReducer and stopping the reset of inputValue for a blurInput event (which I've done).

so I guess the question is, is it deliberate to reset the inputValue to empty string when the user blurs the text input with and no value is selected. If it is then this isn't an issue and can be closed.

@kentcdodds
Copy link
Member

Hmmm... I think this is only an issue on tab, not on blur.... for some reason.

kapture 2018-10-11 at 8 09 23

@kentcdodds
Copy link
Member

I think I can fix it in my case by using the stateReducer and stopping the reset of inputValue for a blurInput event (which I've done).

Yes, that's what the stateReducer is for 👍

is it deliberate to reset the inputValue to empty string when the user blurs the text input with and no value is selected. If it is then this isn't an issue and can be closed.

Yes, this is deliberate.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants