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: click event is fired correctly when focus has changed #20525

Merged
merged 2 commits into from
Mar 10, 2022

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Mar 8, 2022

User facing changelog

Click events are fired correctly when focus is changed on event handlers.

Additional details

  • Why was this change necessary? => When the focus is changed on event handlers, the newly focused object got the click event. This PR simulates this behavior.
  • What is affected by this change? => N/A

Any implementation details to explain?

Click event emitting is a bit different between enter and space.

  • Click event is fired on the newly-focused element when enter is pressed.
  • Click event is not fired on any element when space is pressed.

Before this PR, Cypress behavior is the opposite (not firing on enter and firing on space). This PR fixes this issue.

How has the user experience changed?

Before: Click event is not sent on enter and sent on space. => Buggy behavior.
After: Click event is sent on enter and not sent on space. => Correct behavior.

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)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?
  • [na] Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 8, 2022

Thanks for taking the time to open a PR!

@sainthkh sainthkh marked this pull request as ready for review March 8, 2022 02:34
@sainthkh sainthkh requested a review from a team as a code owner March 8, 2022 02:34
@sainthkh sainthkh requested review from jennifer-shehane and removed request for a team March 8, 2022 02:34
@flotwig flotwig requested a review from BlueWinds March 8, 2022 16:40
@jennifer-shehane jennifer-shehane removed their request for review March 8, 2022 16:40
@jennifer-shehane jennifer-shehane removed their request for review March 8, 2022 16:41
Copy link
Contributor

@BlueWinds BlueWinds left a comment

Choose a reason for hiding this comment

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

This is a fascinating and subtle interaction. The event logging style of tests make it really easy to understand exactly what's going on, since you can play with them directly in the AUT iframe.

@@ -332,21 +339,32 @@ export default function (Commands, Cypress, cy, state, config) {
updateTable(id, key, event, value)
}

if (event.type === 'keydown') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Because you only push events onto this array when type === keydown, there's no need to check the type again later. So this could be simplified to

 if (event.type === 'keydown') {
  keydownEventTargets.push(event.target);
}

// then below...

// When a space key is pressed on another element, the click event should not be fired.
keydownEventTargets.includes(target)

removing the need to define keydownFiredOnThisElement at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first, I logged all events. But I realized that it could slow down Cypress a lot. So, the code became this way.

Your code is much simpler.

Thanks!

@flotwig flotwig merged commit 19db0ce into cypress-io:develop Mar 10, 2022
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 14, 2022

Released in 9.5.2.

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

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Mar 14, 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.

None yet

3 participants