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

fix: Update .type(' ') to not emit clicks when the keyup event has been prevented #20156

Merged
merged 2 commits into from
Feb 12, 2022

Conversation

tbiethman
Copy link
Contributor

@tbiethman tbiethman commented Feb 11, 2022

User facing changelog

Same as #20067, not yet released

Additional details

Buttons that call preventDefault on their keyUp events do not emit a click event when pressed with the Spacebar. This PR adds the defaultPrevented check to make sure we're not sending more events than we expect.

How has the user experience changed?

Came across this issue when Space was being used to toggle open a headless-ui Popover; you can see their logic here: https://github.com/tailwindlabs/headlessui/blob/main/packages/%40headlessui-vue/src/components/popover/popover.ts#L315-L320. The implementation in develop will emit the click event regardless of the prevention, causing the Popover to close and then immediately reopen (as it implements handlers for both).

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?
  • Have new configuration options been added to the cypress.schema.json?

@tbiethman tbiethman requested a review from a team as a code owner February 11, 2022 19:47
@tbiethman tbiethman requested review from jennifer-shehane and removed request for a team February 11, 2022 19:47
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Feb 11, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Feb 11, 2022



Test summary

4379 0 54 0Flakiness 0


Run details

Project cypress
Status Passed
Commit 715fb6c
Started Feb 12, 2022 10:28 PM
Ended Feb 12, 2022 10:39 PM
Duration 11:23 💡
OS Linux Debian - 10.10
Browser Firefox 93

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

emilyrohrbough
emilyrohrbough previously approved these changes Feb 11, 2022
sainthkh
sainthkh previously approved these changes Feb 12, 2022
Copy link
Contributor

@sainthkh sainthkh left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

Rather than create all the situations in the fixture itself - it's better to directly setup the specific situation in the test so that it's self contained, easier to manage, doesn't rely on the fixture implementation, and doesn't leak/pollute into the test fixtures (which will get continuously modified over time). You can also bind directly to events from within the test.

I've gone ahead and pushed a commit that cleans this up - and did it to where all the inputs/buttons are looped through and individually tested.

Another thing I noticed is that when you only do cy.type(' ') - no argument appears in the - type command log. This is probably out of scope for this PR, but its worth mentioning because there's no visual way to tell what the user put into the cy.type(...) It could be one space or 100 spaces. I considered doing something like space but that wouldn't work well if you did cy.type(' foo ') it would be:

space space foo space space

Might be easier to just JSON.stringify(chars) if there are leading or trailing spaces which would give you:

" foo "

Or we could use "blank" spaces where the background color is different to indicate its an empty space. That's what assertion libraries do in stdout to make it clear during string comparisons.

@brian-mann brian-mann merged commit 2c88f0c into develop Feb 12, 2022
@brian-mann brian-mann deleted the 20067_follow-up-fix branch February 12, 2022 22:57
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Feb 15, 2022

Released in 9.5.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v9.5.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants