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: Fire appropriate event when cy.type(). #8255

Merged
merged 7 commits into from
Sep 10, 2020

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Aug 12, 2020

User facing changelog

cy.type() now fires KeyboardEvent, not Event.

It didn't because it had to support Chrome < 63. But Chrome 63 isn't in the lastest list of chrominum. I guess we don't have to support it any more.

Additional details

Why was this change necessary?

  • cy.type() fires Event class and it doesn't match the behavior of the Browsers.
  • When Event class is overridden and you cannot fix the code of the site, cy.type() fails.

What is affected by this change?

N/A

Any implementation details to explain?

I tried to simply revive the commented-out code, but there were some problems in Firefox.

  • Firefox doesn't support TextInput event. So, we had to check if it's a valid Event constructor and fallback to KeyboardEvent.
  • When KeyboardEvent with Enter key is sent to an input field in a form in Firefox, submit event is automatically triggered. I had to fix problems related with it.

How has the user experience changed?

N/A

PR Tasks

  • Have tests been added/updated?
  • Has the original issue been tagged with a release in ZenHub?
  • [N/A] Has a PR for user-facing changes been opened in cypress-documentation?
  • [N/A] Have API changes been updated in the type definitions?
  • [N/A] Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 12, 2020

Thanks for taking the time to open a PR!

@sainthkh sainthkh changed the title Fire appropriate event when cy.type(). fix: Fire appropriate event when cy.type(). Aug 12, 2020
@sainthkh sainthkh marked this pull request as ready for review August 13, 2020 09:50
@kuceb
Copy link
Contributor

kuceb commented Aug 14, 2020

@sainthkh we should always be able to workaround cases where the AUT overwrites some native class that we need, we backup the getter/setter or prototype as soon as the page is loaded before the AUT has time to overwrite it, see https://github.com/cypress-io/cypress/blob/composite-error/packages/driver/src/dom/elements.ts#L264

Is there any other reason we need to use KeyboardEvent? As far as I know the only real impact should be if AUT code is using instanceOf logic e.g. if (e instanceOf KeyboardEvent) {...}

Edit: looking at the tests and the related issues, it makes sense to move to KeyboardEvent, thanks for the PR!

@kuceb
Copy link
Contributor

kuceb commented Aug 14, 2020

@sainthkh this should fix #5650 no?

Comment on lines 188 to 200
it('should trigger KeyboardEvent, not Event, for event listeners', () => {
cy.visit('fixtures/issue-5650.html')

cy.get('#test-input').type('A')
cy.get('#result').contains('isKeyboardEvent: true')
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest something like (to keep test smaller, faster):

Suggested change
it('should trigger KeyboardEvent, not Event, for event listeners', () => {
cy.visit('fixtures/issue-5650.html')
cy.get('#test-input').type('A')
cy.get('#result').contains('isKeyboardEvent: true')
})
it('should trigger KeyboardEvent, not Event, for event listeners', () => {
cy.$$('input:first').on('keydown', (e) => {
if (e instanceOf e.currentTarget.ownerDocument.defaultView.KeyboardEvent) return
throw new Error('event was not instanceOf KeyboardEvent')
})
cy.get('input:first').type('A')
})

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 tried to reuse the test created in #5650. But I prefer your way.

But the code above has 2 problems:

  1. e is not a native event. To access it, we need to use e.originalEvent.

It's also important to note that the event object contains a property called originalEvent, which is the event object that the browser itself created. jQuery wraps this native event object with some useful methods and properties, but in some instances, you'll need to access the original event via event.originalEvent for instance. This is especially useful for touch events on mobile devices and tablets.

  1. done function should be used. Otherwise, the test would pass without checking keydown event. Or fail later when Cypress is running other tests.

I fixed these 2 problems and updated the test.

// promise in the submit command it will break again
// consider changing type to a Promise and juggle logging
return cy.now('submit', form, { log: false, $el: form })
// In Firefox, submit event is automatically fired
Copy link
Contributor

Choose a reason for hiding this comment

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

nice comments 👍

@sainthkh
Copy link
Contributor Author

As I wrote in the comment, there are 3 problems in #5650:

  • KeyboardEvent
  • documentation about iframe in the Test Runner AUT.
  • cy.trigger()

KeyboardEvent is solved here. Doc issue is solved at cypress-io/cypress-documentation#3073. And I'm currently coping with cy.trigger() issue and #5650 will be closed with it.

@sainthkh sainthkh force-pushed the issue-6125 branch 2 times, most recently from 893e521 to b24a4a4 Compare August 17, 2020 06:27
@sainthkh
Copy link
Contributor Author

kitchensink tests failed because of the change in #7782.

@jennifer-shehane
Copy link
Member

@sainthkh sigh, yah, these should pass but I think they fail for outside contributors because the run doesn't have access to env var NEXT_DEV_VERSION. Let me try to rerun.

@sainthkh
Copy link
Contributor Author

Flaky failure.

@sainthkh sainthkh mentioned this pull request Aug 26, 2020
4 tasks
@chrisbreiding chrisbreiding requested review from kuceb and removed request for chrisbreiding September 10, 2020 17:54
@kuceb kuceb merged commit 7c79a42 into cypress-io:develop Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants