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

Inconsistant and invalid Events Typing using cypress #5650

Closed
avallete opened this issue Nov 9, 2019 · 17 comments · Fixed by #8305 or #8255
Closed

Inconsistant and invalid Events Typing using cypress #5650

avallete opened this issue Nov 9, 2019 · 17 comments · Fixed by #8305 or #8255
Labels
pkg/driver This is due to an issue in the packages/driver directory topic: cy.type ⌨️

Comments

@avallete
Copy link
Contributor

avallete commented Nov 9, 2019

Current behavior:

Testing a code who used event instanceof KeyboardEvent, I've seen some strange behavior, digging up, it seems that there is some strange inconsistencies between the events triggered and also into the way they are sent to the eventListener.

Desired behavior:

Consistent behavior:

  • .type
  • .trigger({..., type: 'keydown'})
  • .get('#element').then(element.dispatchEvent(new KeyboardEvent('keydown', ...)))

Should all send a KeyboardEvent type event.

And the eventListener:

element.addEventListener('keydown', (event) => (event instanceof KeyboardEvent));

Should always return true.

Steps to reproduce: (app code and test code)

I've tested three different methods: cy.type, cy.trigger, cy.get('#element').then( element.dispatchEvent).

And two different eventListener for each test:

  1. One directly into a <script> inside the html page itself
  2. One created into the test itself using cy.get -> addEventListener

You can find MWE here: https://github.com/avallete/cypress-test-tiny-event-bug

Simply run:

npm install
npm run cypress:open

# Then run the test and check the console.log inside the debuging tools.
# Each test is a specific case the 2 first show that event is type Event instead of KeyboardEvent.
# The third test show the inconsistency of the same eventListener if it's declared inside the test or outside (into the html <script>).

Versions

3.6.1, Linux, Chromium 78 / Electron 73

@avallete
Copy link
Contributor Author

avallete commented Nov 9, 2019

The issue also seem to concern 'mousedown' and MouseEvent, except that .click seem to send MouseEvent to one of the two listener. But .trigger doesn't.

I've added the tests for the MouseEvent to the MWE repo.

@jennifer-shehane
Copy link
Member

Hey @avallete, I'm trying to narrow down exactly what the issue is from what you provided. Does the below summarize the totality of the issue?

When using .type(), .trigger() or dispatching an event directly from an element returned from a Cypress command:

  • The mousedown event that is triggered not an instanceof MouseEvent
  • The keydown event that is triggered in Cypress not an instanceof KeyboardEvent

Please clarify if I have missed something else. Thanks.

@avallete
Copy link
Contributor Author

avallete commented Nov 13, 2019

Hey @avallete, I'm trying to narrow down exactly what the issue is from what you provided. Does the below summarize the totality of the issue?

When using .type(), .trigger() or dispatching an event directly from an element returned from a Cypress command:

  • The mousedown event that is triggered not an instanceof MouseEvent
  • The keydown event that is triggered in Cypress not an instanceof KeyboardEvent

Please clarify if I have missed something else. Thanks.

I'll just add from my last comment that:

When using the .click() Cypress command, the event that is triggered is not an instanceof MouseEvent.

And also the strange behavior is that between eventListener in different places into the code (directly put into cypress it(){...} or into <script>...</script>) seem to receive not the same values and instanceof seem to behave in different ways between these two scopes.

But yeah @jennifer-shehane , you have summarized it way better than I did, thank's.

@cypress-bot cypress-bot bot added the stage: needs investigating Someone from Cypress needs to look at this label Nov 13, 2019
@jennifer-shehane jennifer-shehane added pkg/driver This is due to an issue in the packages/driver directory topic: cy.type ⌨️ labels Nov 14, 2019
@avallete
Copy link
Contributor Author

Investigating a little bit on this issue, I've been able to found what seem to be the root of the behaviour I describe on this issue:

event = new win['Event'](eventType, eventOptions)

Replacing the win['Event'] by win['KeyboardEvent'] on this line, make the .type function to properly send an event which is instanceof KeyboardEvent into the tests.

But looking on the comment on top of this line, this fix may lead to some regression on Chrome < 63.

@sainthkh
Copy link
Contributor

sainthkh commented Aug 7, 2020

I tried it with Cypress 4.12.1 + 'Event' => 'KeyboardEvent'. But something interesting happened.

console.log says it's a KeyboardEvent, but the instanceof KeyboardEvent is false.

Here are the screenshots.

Screenshot from 2020-08-07 10-33-25

Something similar happens to MouseEvent.

Screenshot from 2020-08-07 10-33-16

It's a bit hard to figure out what made the difference.

@avallete
Copy link
Contributor Author

avallete commented Aug 7, 2020

I had opened this issue so long ago that I had completely forgot about it.

Related to your comment @sainthkh I've refactored my MEW to:

  1. Bump cypress to the latest version
  2. Split the different executions contexts of the instanceof (inside the running test, inside the html web page script)
  3. Rename the tests accordingly to the execution context actually tested.
  4. Instead of just logging the result, make the cypress tests actually fail if the instanceof is not true in any of the context (since it's the attended behaviour)

If you don't want to clone the MWE repository, here is the videos of the running tests to see what's wrong.

KeyboardEvent tests:

specjs

MouseEvent tests:

specmousejs

FYI, the expected behaviour for both KeyboardEvent and MouseEvent can be summarize into three assertions who are the following ones:

  1. When you add an eventListener to an element you've retrieved inside your test with cypress, and that an event occur, this event should be an instanceof (MouseEvent or KeyboardEvent, depending on the test).
  2. When an event is dispatched to the html page, the eventListener inside the <script> tag of the page, should see an event which is 'instanceof' (MouseEvent or KeyboardEvent, depending on the test). And so, set the text 'isKeyboardEvent|isMouseEvent' to true inside the '#result' div.
  3. Whatever if you use different methods (.click(), .trigger(), .type(), .get(element).then(elem => elem.dispatchEvent())) the behaviour should be consistent between all the raised events.

I hope this addition may help to understand more clearly what's going on here.

@sainthkh
Copy link
Contributor

Finally, I understood what's going on.

You provided us 9 tests for each event. I read them and learned that the last 3 are duplicates of the above 6. So, all we have to check was those 6.


When we run the tests as-is with 4.12.0, only the test no. 3 passes. Other 5 fail. With the change to 'Event' to 'KeyboardEvent' you mentioned above, the test no.4 passes.

Now, we have 4 tests to fix.


When I saw the console log, the thing made me curious was this:

trigerred event show from static/html file ! 
                received key: '65'
                is instanceof  KeyboardEvent ? : (false)
                event type: (keydown)
                event constructor: function KeyboardEvent() { [native code] }
                event object: 
KeyboardEvent {isTrusted: false, key: "", code: "", location: 0, ctrlKey: false, …}

instanceof KeyboardEvent is false. But the object is KeyboardEvent.

This problem happens with the test no. 1 and 6. And I finally learned why.

It's because they use different KeyboardEvent.

To run the test application, Cypress uses iframe. When we write test code with KeyboardEvent, KeyboardEvent itself came from the test runner. While event object is the instance of the "application(inside iframe)" KeyboardEvent.

So, the test 1 and 6 should be written like below:

it('should trigger KeyboardEvent with .type inside Cypress event listener', (done) => {
  cy.visit('fixtures/issue-5650-2.html')
  console.log('test: should trigger KeyboardEvent with .type inside Cypress event listener')

  cy.window().then((win) => {
    cy.get('#test-input').then((jQueryElement) => {
      let elemHtml = jQueryElement.get(0)

      elemHtml.addEventListener('keydown', (event) => {
        console.log(`trigerred event show from cypressListener file !
                  received key: '${event.keyCode}'
                  is instanceof  KeyboardEvent ? : (${event instanceof KeyboardEvent})
                  event type: (${event.type})
                  event constructor: ${event.constructor}
                  event object: 
              `, event)

        expect(event instanceof win['KeyboardEvent']).to.be.true
        done()
      })
    })
  })

  cy.get('#test-input').type('A')
})

it('should trigger KeyboardEvent with .get.then .dispatchEvent on htmlElement inside html script event listener', () => {
  cy.visit('fixtures/issue-5650-2.html')
  console.log('test: should trigger KeyboardEvent with .get.then .dispatchEvent on htmlElement inside html script event listener')
  cy.window().then((win) => {
    cy.get('#test-input').then((jQueryElement) => {
      let elemHtml = jQueryElement.get(0)
      let kbEvent = new win['KeyboardEvent']('keydown', {
        keyCode: 65,
        which: 65,
        shiftKey: false,
        ctrlKey: false,
      })

      elemHtml.dispatchEvent(kbEvent)
    })
  })

  cy.get('#result').contains('isKeyboardEvent: true')
})

Check that I used cy.window() to bring iframe KeyboardEvent.


Finally, tests 2 and 4 are not fixed. The problem is here:

const dispatch = (target, eventName, options) => {
const event = new Event(eventName, options)
// some options, like clientX & clientY, must be set on the
// instance instead of passing them into the constructor
_.extend(event, options)
return target.dispatchEvent(event)
}

cy.trigger doesn't take the type of event class into account.

@sainthkh
Copy link
Contributor

Before I go fix cy.trigger(), I want to ask this question. When does this consistency necessary? Can I get some real world use cases?

I'm asking this because to solve this problem, we need to add a big table of event name and event object types based on this.

@avallete
Copy link
Contributor Author

Hi there,

As you mentioned, the last 3 tests are effectively a combination of the check in both (html script and tests runner) contexts. Which as you say, if all the above tests work, should also work. I do realize know that it may be confusing, sorry for that.

Nice spot about the iframe and the use of win['KeyboardEvent']. It may be worthy to add this trick somewhere into the documentation since this problem should occurs for any instanceof or === checks on window instantiated objects.

On the question about 'why it is necessary ?':

  1. To me it isn't, at first this issue was really about the fact that .type and .click wasn't instanceof the proper event. I do believe that the .trigger issue may even be handled in another issue.
  2. Still, I do believe that calling .trigger('keydown') or .type should produce the same kind of event, and that, according to the MDN, those events should have to proper types (as the ones who would be normally triggered by an user of the tested application.).

@sainthkh
Copy link
Contributor

sainthkh commented Aug 11, 2020

There are 3 problems in this issue:

@jennifer-shehane
Copy link
Member

jennifer-shehane commented Aug 12, 2020

@sainthkh This seemed to be a use case that someone encountered in their app, where they were checking instanceof MouseEvent in their app code, so that cypress didn't behave the same. #8137

But yah, as you mentioned, this trigger thing may be a separate issue. Feel free to reopen that issue if it's the case.

Also, maybe related??? #3686

@sainthkh
Copy link
Contributor

@jennifer-shehane I guess so. After #8255 is done, I'll research it more deeply.

@tobmaster
Copy link

I am not very experienced in the cypress codebase yet. But shouldn't the action "type" emit a KeyboardEvent instead of an HTMLEvent?

const change = document.createEvent('HTMLEvents')

That way getModifierState() could work correctly?
The modifieres still need to be set on the event I assume.

We currently have this problem in the project where we handle modifiers and it fails cause the method is not there. And including a workaround for cypress in the application code is kinda awkward.
Maybe I am wrong here but this seems to be connected to this issue.

@tobmaster
Copy link

Ah I now also read all of #8255 So I assume my comment is obsolete then. Sorry for the noise

@cypress-bot cypress-bot bot added stage: work in progress stage: needs review The PR code is done & tested, needs review and removed stage: needs investigating Someone from Cypress needs to look at this stage: work in progress labels Aug 18, 2020
@anp
Copy link

anp commented Aug 23, 2020

Before I go fix cy.trigger(), I want to ask this question. When does this consistency necessary? Can I get some real world use cases?

I'm running into this issue because I have some Rust code which uses typed bindings to DOM APIs. The bindings provide a number of type definitions generated from WebIDL, and it's possible to cast between JS types. By default those casts use instanceof checks before returning to catch common bugs.

I have event handler helpers that take a typed callback and perform the conversion under the hood for the user. Elsewhere there are definitions mapping strings like "keydown" to types so that the callbacks always have the correct type. This works quite nicely "in production" so far but breaks when using cypress to enter text due to this issue.

I've had mixed results with removing the checks as it seems the bindings may rely on prototypes for doing their property lookups. The easiest fix that'd allow me to use cypress would be to emit an event with the correct prototype in its chain.

EDIT: I should also note that #8255 will address the most common cases.

@cypress-bot cypress-bot bot added stage: work in progress stage: needs review The PR code is done & tested, needs review and removed stage: needs review The PR code is done & tested, needs review stage: work in progress labels Aug 24, 2020
@cypress-bot cypress-bot bot added stage: needs review The PR code is done & tested, needs review and removed stage: work in progress labels Aug 31, 2020
@cypress-bot cypress-bot bot added stage: pending release and removed stage: needs review The PR code is done & tested, needs review labels Aug 31, 2020
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 31, 2020

The code for this is done in cypress-io/cypress#8305, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Sep 1, 2020

Released in 5.1.0.

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

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Sep 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg/driver This is due to an issue in the packages/driver directory topic: cy.type ⌨️
Projects
None yet
6 participants