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: cy.type(' ') fires click event on button-like elements. #20067

Merged
merged 2 commits into from
Feb 10, 2022

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Feb 7, 2022

User facing changelog

cy.type(' ') fires click event on button-like elements.

Additional details

  • Why was this change necessary? => When button-like elements get focus and space key is pressed, click event is fired. Cypress doesn't follow the browser behavior.
  • What is affected by this change? => N/A

Implementation details to explain.

  • Unlike enter, click event by space key is generated on all button-like elements(<button> tag, button, submit, reset, image, checkbox, radio input).
  • But as for radio, click event is not sent when it is already checked.
  • This change adds many event-related tests to type_spec.js. I decided to move event-related tests to type_events_spec.js.
  • I changed filename from type-enter.html to click-event-by-type.html because it better suits the purpose.

How has the user experience changed?

Before: click event is not sent.
After: click event is sent.

PR Tasks

  • Have tests been added/updated?
  • [na] 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 Feb 7, 2022

Thanks for taking the time to open a PR!

@sainthkh sainthkh marked this pull request as ready for review February 7, 2022 03:05
@sainthkh sainthkh requested a review from a team as a code owner February 7, 2022 03:05
@sainthkh sainthkh requested review from jennifer-shehane and removed request for a team February 7, 2022 03:05
@sainthkh
Copy link
Contributor Author

sainthkh commented Feb 7, 2022

For some reason, system-tests always fail for me.

@@ -0,0 +1,710 @@
const { _, $ } = Cypress
Copy link
Member

Choose a reason for hiding this comment

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

Can we break these tests out in a different PR? I can't tell what tests were added for the changes you made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I temporarily removed type_events_spec.js. I'll bring it back in the next PR.

@@ -1,19 +1,19 @@
<!DOCTYPE html>
Copy link
Member

@emilyrohrbough emilyrohrbough Feb 7, 2022

Choose a reason for hiding this comment

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

With your previous changes in #19726, was #7318 implemented? and was #8267 fixed?

Copy link
Contributor Author

@sainthkh sainthkh Feb 8, 2022

Choose a reason for hiding this comment

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

I read those 2 issues. And below are my opinions:

About #7318

I think #19541 and #7318 are separate issues. #7318 wants to listen to search event with cy.type('{enter}'). But the problem is that search event is not a standard event as @jennifer-shehane wrote in the issue.

I guess we can ignore this in this PR.

About #8267

Yes and no.

There are 2 problems in #8267.

  1. cy.get('#button').focus().type('{enter}') should trigger click event. => It's solved by fix: send click event with cy.type('{enter}'). #19726.
  2. cy.get('#button').type('f') should not send click event. => It's not solved and the debate isn't ended.

As in the comment of #8767, we should decide the current behavior of cy.type first.

It assumes that click is required before typing into an element. But as we all know, "click" isn't the only way to make elements get focus.

I believe #8267 should be handled after the decision about this has been made.

@jennifer-shehane jennifer-shehane removed their request for review February 9, 2022 16:26
cy.get('#target-input-radio').should('be.checked')
})

it('radio does not fire click event when it is checked', () => {
Copy link
Member

@emilyrohrbough emilyrohrbough Feb 9, 2022

Choose a reason for hiding this comment

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

This test does appear to fire a click event based on the assertion however the test title indicates it shouldn't be fired.

Is this relying on the state of radio fires click event when it is not checked? If not, it appears to be the same tests as radio fires click event when it is not checked. with a different assertion order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Radio button has 2 states: unchecked (empty circle), checked (filled circle). When you focus the empty radio button (unchecked) and press the space bar, it is filled (checked) and click event is sent.

But when we press the space bar again, click event is not fired because the radio button is already checked.

To make it clear, I've edited the code like below:

// We're clicking here first to make the radio element checked.
cy.get(`#target-input-radio`).click().type(' ')

// item 0 is click event. It's fired because we want to make sure our radio button is checked.
cy.get('li').eq(1).should('have.text', 'keydown')
cy.get('li').eq(2).should('have.text', 'keypress')
cy.get('li').eq(3).should('have.text', 'keyup')

cy.get('#target-input-radio').should('be.checked')

Comment on lines 333 to 338
// Firefox sends a click event when the Space key is pressed.
!Cypress.isBrowser('firefox') &&
Copy link
Member

Choose a reason for hiding this comment

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

I think I'm misunderstanding, but If Firefox is the only browser that sends a click event with a space key, shouldn't this check be validating that the browser is Firefox? If this was chrome or Edge, this should send a click event that the browser wouldn't actually send giving the test a false-positive.

Cypress.isBrowser('firefox')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike Chrome, Firefox automatically sends click event. Because of that, when we remove ! here, Chrome does not send a click event and Firefox sends click events twice.

This is the code to prevent this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added comment like below:

// Firefox sends a click event when the Space key is pressed.
// We don't want send it twice.
!Cypress.isBrowser('firefox') &&

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation. I was concerned we were giving people a false-positive by sending a click event for chrome if it sent automatically.

@emilyrohrbough
Copy link
Member

The comment linked in the summary state:

It's also important if the target changes i.e. calling .focus() in the keydown event the click event is triggered on the new target.

If the target changes from a onKeyDown callback on the element the space key was triggered on, does this change account and no-opt the click event?

@sainthkh
Copy link
Contributor Author

@emilyrohrbough That comment has not been implemented yet. I need to validate the problem by writing a test about it first. It might be the part-4 of this series.

Because part 3 is creating type_event_spec.js I rolled back above.

@emilyrohrbough emilyrohrbough merged commit 5d77abc into cypress-io:develop Feb 10, 2022
tgriesser added a commit that referenced this pull request Feb 14, 2022
* develop:
  feat: gray out the path to system node in cypress run header (#20121)
  feat: redesign server errors (#20072)
  test: fix awesome-typescript-loader test and remove test-binary job (#20131)
  fix: Fix issues with stack traces and command log in Chrome 99 (#20049)
  fix: `cy.type(' ')` fires click event on button-like elements. (#20067)
  fix: `change`, `input` events are not fired when the same option is selected again. (#19623)
  build: publish vue3 on latest (#20099)
  chore: release @cypress/webpack-preprocessor-v5.11.1
  chore: release @cypress/webpack-dev-server-v1.8.1
  fix: detect newly added specs in dev-server compilation (#17950)
  chore: Remove pkg/driver //@ts-nocheck part 3 (#19837)
  chore: set up semantic-pull-request GitHub Action (#20091)
  chore: release @cypress/react-v5.12.2
  fix: remove nullish coalescing in js files to support node 12 (#20094)
  docs: update @cypress/webpack-preprocessor links (#19902)
  refactor: use aliases instead of meta (#19566)
@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.

3 participants