Fix incorrect input reset #354

jduthon · 2018-03-02T09:29:37Z

What:
Bug with selecting the input text which might lead to reseting the state

How:
I added a onFocus on the input, which is responsible with the onBlur to keep a isInputFocused boolean updated.
If the input is focused, we do not want to reset the state and fire the onOuterClick prop.
It seemed to me that was a clearer way to go than what was discussed in the issue.
Though if you consider it not the right way, feel free to ask for edit.

Checklist:

Documentation N/A
Tests
Ready to be merged
Added myself to contributors table

…nputBlur

codecov-io · 2018-03-02T14:12:17Z

Codecov Report

Merging #354 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #354   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         329    330    +1     
  Branches       84     84           
=====================================
+ Hits          329    330    +1

Impacted Files	Coverage Δ
src/downshift.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dec961...1805e6a. Read the comment docs.

austintackaberry

I like this solution better as well! 👍

kentcdodds

Looking good! Let's try something simpler first.

kentcdodds · 2018-03-02T16:12:03Z

src/downshift.js

@@ -827,7 +840,8 @@ class Downshift extends Component {
        if (
          (event.target === this._rootNode ||
            !this._rootNode.contains(event.target)) &&
-          this.getState().isOpen
+          this.getState().isOpen &&
+          !this.isInputFocused


What if instead we just change this to:

// above: const {document} = this.props.environment // this line (!this.inputId || document.getElementById(this.inputId) !== document.activeElement)

Then I think we could remove all the focus logic.

Want to give that a try? The tests shouldn't need to change if it works.

I like it, and looks like it's working.
It is breaking the test though as it seems like enzyme doesn't really support the document.activeElement enzymejs/enzyme#1128
I believe I can just go ahead and set the document.activeElement in my test.
And what if instead of looking for the getElementById we would do as simple text check :
(!this.inputId || this.inputId !== document.activeElement.id) ?

Well seems like the testing might get a bit hairier with the document.activeElement solution.
To use document.activeElement from what I read we might need to use JSDOM to simulate this.

And what if instead of looking for the getElementById we would do as simple text check:
(!this.inputId || this.inputId !== document.activeElement.id)

That's fantastic.

Yeah, go ahead and set document.activeElement manually in your tests. Kinda weird, but should be ok. If you want you could try to add a browser test via cypress:

https://github.com/paypal/downshift/blob/1dec961ad5e787ad29556c85afe8ddc8bb05968f/cypress/integration/basic.js

Learn more about cypress here.

I think we should have both, one for the quick feedback loop and the other to ensure it works without jsdom

If you can't get it to work with the jest tests then that's fine. Add an /* istanbul ignore next */ comment on the line above this condition and it should prevent coverage being recorded for that line and we'll cover it using cypress 👍

Awesome 👍

Let me know if you need a hand :)

Oh, and to start up cypress, run npm run test:cypress:dev. I should probably add that to the contributing docs... Would you like to?

Hmmm not so sure I have the time atm, but I might do it later if you want.
I believe the cypress test is up, it's pretty cool and quite straightforward.
I tested with npm run test:cypress though, hope it's equivalent.

hi guys! how did u set up the document.activeelement in enzyme? im using activeElement in my component code. However, in my enzyme testing, it won't recognize the activeelement. i kept getting undefined.

If you read the thread you commented on you'll see that we didn't do it either....

kentcdodds

Sweet, I've verified with the netlify preview that the bug has been fixed, the cypress tests look good and they're passing! Well done! Thank you for your work on this :D

jduthon · 2018-03-02T17:25:21Z

You are welcome.
Thanks a lot for downshift, it's pretty sweet!

kentcdodds · 2018-03-02T17:26:32Z

Actually, now I'm starting to wonder if we should change this to be more generic... How about this:

this._rootNode.contains(document.activeElement)

This way if the active element is the button or really anything else within the component it wont reset. I'm not 100% certain this is what we're looking for, but it's something to think about and maybe try... Would you like to give it a try?

kentcdodds · 2018-03-02T17:27:02Z

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

jduthon · 2018-03-04T10:33:36Z

@kentcdodds what would be the advantage to make this more generic.
I can only really think of the case where the input is focused where our scenario can happen (a mouse up outside of the element should not reset). Not so sure what would happen in the case of the button, I am not really familiar with the button globally though.
Also we might introduce unwanted behaviour if we were to do this?
And performance wise the current implementation is faster than the contains would be, but well, this is a very shallow point and if there is any reason for the more generic version it would not make any sense to hold this.

Let me know if you want to go for the contains version, I believe it should work for sure.
And thanks a lot for adding me to the contributors! :)

austintackaberry · 2018-03-04T11:49:30Z

Using this as an example...

Let's say the user types out 'Luke Skywalker' and does a mouseDown on the menu toggle button. Then they change their mind and do a mouseUp not on the button.

Do we want that to reset? I would say no which would suggest that the contains version is the way to go.

Separately though, what if the user clicks on the label? I would think the ideal behavior would be the same as onOuterClick.

kentcdodds · 2018-03-04T13:57:45Z

The thing is that downshift has absolutely 0 opinions about what you render within it. So some implementation could add a second input that serves to further filter the list of items. We wouldn't want to have this same bug on that second input. That's just one example. The use cases are wide and varied. Also, I think that this change would make the solution more robust :)

kentcdodds · 2018-03-04T13:59:21Z

Separately though, what if the user clicks on the label? I would think the ideal behavior would be the same as onOuterClick.

That's a fair point.... Hmmm.... I'm not certain what to do here. @austintackaberry, would you mind opening a new issue that outlines the problem and arguments for and against for us to discuss this further? That way we can get more people to weigh in.

austintackaberry · 2018-03-05T02:01:55Z

Not at all!

…ownshift-js#354) * Fixes downshift-js#350 : Selecting input text can lead to state reset * Updated contributors * Makes sense to switch the isInputFocused bool first thing in handle_inputBlur * Added test * Better way to handle knowing if the input is focused * Updated test * chore: update .all-contributorsrc

jduthon added 4 commits March 2, 2018 10:22

Fixes downshift-js#350 : Selecting input text can lead to state reset

e3a0763

Updated contributors

df33de4

Makes sense to switch the isInputFocused bool first thing in handle_i…

4118ae9

…nputBlur

Added test

37d3edc

jduthon changed the title ~~Fix/incorrect input reset~~ Fix incorrect input reset Mar 2, 2018

austintackaberry reviewed Mar 2, 2018

View reviewed changes

kentcdodds reviewed Mar 2, 2018

View reviewed changes

jduthon added 2 commits March 2, 2018 18:12

Better way to handle knowing if the input is focused

aeae54d

Updated test

2e7766f

kentcdodds previously approved these changes Mar 2, 2018

View reviewed changes

chore: update .all-contributorsrc

1805e6a

kentcdodds dismissed their stale review via 1805e6a March 2, 2018 17:23

kentcdodds merged commit 7ba6870 into downshift-js:master Mar 2, 2018

austintackaberry mentioned this pull request Mar 5, 2018

Default behavior for mouseUp event #356

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect input reset #354

Fix incorrect input reset #354

jduthon commented Mar 2, 2018 •

edited

Loading

codecov-io commented Mar 2, 2018 •

edited

Loading

austintackaberry left a comment

kentcdodds left a comment

kentcdodds Mar 2, 2018 •

edited

Loading

jduthon Mar 2, 2018

jduthon Mar 2, 2018

kentcdodds Mar 2, 2018

kentcdodds Mar 2, 2018

kentcdodds Mar 2, 2018

kentcdodds Mar 2, 2018

jduthon Mar 2, 2018

yfchina143 Aug 19, 2019

kentcdodds Aug 19, 2019

kentcdodds left a comment

jduthon commented Mar 2, 2018

kentcdodds commented Mar 2, 2018

kentcdodds commented Mar 2, 2018

jduthon commented Mar 4, 2018

austintackaberry commented Mar 4, 2018 •

edited

Loading

kentcdodds commented Mar 4, 2018

kentcdodds commented Mar 4, 2018

austintackaberry commented Mar 5, 2018

Fix incorrect input reset #354

Fix incorrect input reset #354

Conversation

jduthon commented Mar 2, 2018 • edited Loading

codecov-io commented Mar 2, 2018 • edited Loading

Codecov Report

austintackaberry left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

jduthon commented Mar 2, 2018

kentcdodds commented Mar 2, 2018

kentcdodds commented Mar 2, 2018

jduthon commented Mar 4, 2018

austintackaberry commented Mar 4, 2018 • edited Loading

kentcdodds commented Mar 4, 2018

kentcdodds commented Mar 4, 2018

austintackaberry commented Mar 5, 2018

jduthon commented Mar 2, 2018 •

edited

Loading

codecov-io commented Mar 2, 2018 •

edited

Loading

kentcdodds Mar 2, 2018 •

edited

Loading

austintackaberry commented Mar 4, 2018 •

edited

Loading