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

Handle keydown/up events with no key data. #622

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xaviershay
Copy link

@xaviershay xaviershay commented Apr 25, 2020

Checklist

  • The code has been run through pretty yarn run pretty
  • The tests pass on CircleCI
  • You have referenced the issue(s) or other PR(s) this fixes/relates-to
  • The PR Template has been filled out (see below)
  • Had a beer/coffee because you are awesome

What?

In certain repeatable yet rare cases I haven't quite been able to narrow down (at least involving the Tab key), the keydown and keyup events are not in fact KeyboardEvents and do not contain key data. From this screenshot, you can see two bogus events but then also a legitimate one.

diagrams-event-error

While I haven't been able to figure out a minimal repro, my project is open source. Repro instructions in that project (after yarn install && yarn start) for Chrome on Ubuntu:

  1. Drag "Blank" node from sidebar to canvas.
  2. Double click title to edit.
  3. Type "Gr" which should then show an autocomplete for "Green Circuit" (maybe you have to have edited the Green Circuit box first!? This is a browser suggestion, not done by the code).
  4. Position the cursor over the autocomplete suggestion.
  5. Press tab.

Why?

The errors in the screenshot above cause a full screen react error that breaks the application.

How?

With as little code as possible and a helpful comment.

Feel good image:

good dog

(ps this project is dope)

@dylanvorster
Copy link
Member

yeah i would prefer to get to the bottom of this tbh.

@xaviershay
Copy link
Author

xaviershay commented Apr 25, 2020

Fair.

Did some more research, I believe this is expected behaviour. I think this change was responsible for it in Chrome.

Here is a minimal repro, so long as the input name has autocomplete-able values in your browser two events will be generated when tab-completing (one invalid, one for Tab): https://codesandbox.io/s/infallible-bardeen-1n1vg A hypothesis I had is that the "fake" keydown event is generated when the input contents changes as a result of the autofill, which led to discovery of above issue.

To be clear, here is a screenshot of the autocomplete in Chrome:
autocomplete

Based on the above, my current conclusion is that this is behaviour we should expect - there doesn't seem to be a sane key value that Chrome could be providing while meeting their other criteria from that issue (though empty string is probably better than undefined?).

I think this implies that it is not "safe" to cast event as KeyboardEvent based solely on type. I don't know anything about typescript though so am not sure if there is a better workaround possible than the one I proposed.

If you think that sounds reasonable, I can update the comment appropriately. If not, would appreciate guidance on next steps. Thanks for taking a look 🙏

@pradeep199896
Copy link

Hey @xaviershay @dylanvorster . I too facing the same issue. Did you find any tackle to it? It would be helpful for any suggestions.

@tdangkhoa0303
Copy link

Hi @xaviershay @dylanvorster, is there any update on this?

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

Successfully merging this pull request may close these issues.

None yet

4 participants