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

chore(modal): add/update test cypress tests for modal #5081

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

seanforyou23
Copy link
Collaborator

What: Steps toward closing #3953

This PR adds adds a couple of new tests for modal which test to ensure focustrap is working as expected as well as that pressing escape key with an open modal would close it. I spent some time trying to also verify that pressing enter on the close button (or cancel/confirm) would also close the modal, but for some reason the trigger/type functions for cypress didn't actually close the modal. I tried several combinations of the following:

....trigger('keydown', { keyCode: 13, which: 13 });
....type('{enter}')

Any idea why these won't simulate the keypress event? The functionality does obviously work, just that cypress can't seem to invoke it. I could cheap out and go for click() which works fine, but the main point of the issue is to test keyboard interactivity so not sure that would fully exercise the right path in the code.

In any case, what's here should help prevent regressions to keyboard interactivity in the future, would be cool to get it merged and circle back to the pressing enter issue if possible.

I also fixed a small issue where one of the examples in the test suite was missing an accessible name.

Screen Shot 2020-10-29 at 9 28 59 PM

@patternfly-build
Copy link
Contributor

patternfly-build commented Oct 30, 2020

Copy link
Member

@jeff-phillips-18 jeff-phillips-18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should file an issue to circle back to attempt to get the return key to work.

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look myself and attempted a few things, but I'm seeing the same thing as you. I noticed that this issue exists in the cypress repo - I wonder if they're related and if we can add to that conversation.

@seanforyou23
Copy link
Collaborator Author

Yea that seems like a relevant issue to chime in on. I tried chaining the .type() and .trigger() methods off of .get(), .focused() and .tab() methods and they all had the same behavior. I'll tweak this PR a bit so there's a few lines in the cypress test that can be toggled on/off to compare how it works with click but not the keyboard events. Would that be enough to get these keyboard interaction related PRs merged, or should we leave them open? I can add the relevant info and instruction on how to reproduce in #5088 if you think that's enough - then reference that and this PR on a comment to that cypress thread. Just let me know what you prefer!

@tlabaj tlabaj merged commit 0e7bac0 into patternfly:master Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants