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.

Sign up for GitHub

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

Jump to bottom

Menu: preventing default key event behaviours + adding menu tests #85

Merged
merged 8 commits into from
Oct 5, 2021
Merged

Menu: preventing default key event behaviours + adding menu tests #85

merged 8 commits into from
Oct 5, 2021

Conversation

roomman
Copy link

@roomman roomman commented Oct 1, 2021

This PR is very simple, calling the preventDefault and stopPropagation methods for key down events in the menu component. This prevents the default scrolling events when up and down arrows are used.

Fixes #80

Simon Zimmerman added 2 commits October 1, 2021 17:23
refactor: prevent default keydown events
2ec6df6
Merge branch 'add-menu-key-event-methods'
f0ecb7c 
* add-menu-key-event-methods:
  refactor: prevent default keydown events
@GavinJoyce
Copy link
Owner

Thanks for this @roomman. Perhaps you would add a test to ensure that we don't regress here in future?

Simon Zimmerman added 2 commits October 4, 2021 09:30
Merge branch 'upstream-master'
61dd60c 
* upstream-master:
  ember-cli-typescript blueprint forgot to install destroyable types
  Resolve prettier issues in non js/ts files, resolve lint warnings
  Convert utils/keyboard to TypeScript
  create a `tag-name-is-component` helper
  Add typechecking to CI
  Convert the dialog and the dialog stack provider to typescript
  Fix the blueprint output
  ember install ember-cli-typescript
  Fix remaining lint violations
  Resolve auto-fixable lints
  Add support for linting typescript
test(menu): it should be possible to open the menu with ArrowDown
06baba7
@roomman roomman changed the title Menu: preventing default key event behaviours Menu: preventing default key event behaviours + adding menu tests Oct 4, 2021
@GavinJoyce
Copy link
Owner

Looking great @roomman, nice one. There are some commented out lines with JSX in them, perhaps you could remove them and squash into a single commit when you're ready to merge?

@roomman
Copy link
Author

roomman commented Oct 4, 2021

Sorry @GavinJoyce that's supposed to be wip, committed to my local branch. My bad. Will submit the full PR when done 👍🏻

@GavinJoyce
Copy link
Owner

👍 sounds good, thanks

Simon Zimmerman added 3 commits October 4, 2021 15:40
test(menu): should not be able to open the menu when button is disabled
0b886b6 
Spotted these tests were missing when comapring with the vue implementation. Not sure how helpful they are given the browser defaults for disabled elements, but adding them for symmetry.
test(menu): ArrowDown navigation
a267878
test(menu): ArrowUp tests and other edits
9adc40f
@roomman
Copy link
Author

roomman commented Oct 4, 2021

@GavinJoyce here's the full PR as promised. I'll circle back and add tests for the remaining keydown modules in the next week or so, subject to workload.

Originally, I was supposed to be addressing your comment above, about protecting against regressing for event.preventDefault on the up and down events. Without adding something like sinon to facilitate spying on the event methods, I'm not sure how to approach this. Any thoughts - happy to write the test if you can signpost. 👍🏻

@GavinJoyce
Copy link
Owner

GavinJoyce commented Oct 4, 2021 •
edited
Loading

Just poking about in the dummy app, these changes seem to introduce a bug where pressing Enter on an active items doesn't close the menu. Pressing enter a second time throws an exception:

  1. Open the menu

Screenshot 2021-10-04 at 17 50 45

  1. Navigate to an item and press Enter - the menu stays open:

Screenshot 2021-10-04 at 17 50 53

  1. Press Enter a second time, an exception is thrown:

Screenshot 2021-10-04 at 17 50 59

@roomman
Copy link
Author

roomman commented Oct 4, 2021

@GavinJoyce looking into it now.

Merge branch 'upstream-master'
b9a45bc 
* upstream-master:
  ember-cli-typescript blueprint forgot to install destroyable types
  Resolve prettier issues in non js/ts files, resolve lint warnings
  Convert utils/keyboard to TypeScript
  create a `tag-name-is-component` helper
  Add typechecking to CI
  Convert the dialog and the dialog stack provider to typescript
  Fix the blueprint output
  ember install ember-cli-typescript
  Fix remaining lint violations
  Resolve auto-fixable lints
  Add support for linting typescript
@roomman
Copy link
Author

roomman commented Oct 4, 2021

Hi @GavinJoyce, so it turns out that the bug is unrelated to this PR, and was introduced in this commit.

My best guess is that it's related to the introduction of the {{focus-trap}} modifier, and appears to have resolved itself thanks to the work done by @far-fetched in #98. I've merged master into this PR and the bug has gone.

Are you happy for me to add some additional tests to the ArrowDown and ArrowUp modules to catch this scenario, despite them being additional to headless-ui test suite?

@roomman
Copy link
Author

roomman commented Oct 4, 2021

That said, I just wrote a test, on a branch where the bug is present, navigating to a menu item using ArrowDown and invoking with Enter and the test passes. 🤦🏻‍♂️

@GavinJoyce GavinJoyce merged commit ea9f165 into GavinJoyce:master Oct 5, 2021
@GavinJoyce
Copy link
Owner

Thanks @roomman, this is great!

@GavinJoyce GavinJoyce added the bug Something isn't working label Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Preventing default key event behaviours for menu
2 participants
@roomman @GavinJoyce