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

feat: support beforeinput event. #8411

Merged
merged 22 commits into from Oct 21, 2020
Merged

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Aug 26, 2020

User facing changelog

  • Support beforeinput event to solve slate.js issue.

Additional details

Why was this change necessary?

Cypress cannot be used to test some applications that use beforeinput events like slate.js

What is affected by this change?

N/A

Any implementation details to explain?

  • slatejs uses inputType to decide what to do. So, I had to decide it based on the key combo and the spec.
  • noUpdate option had to be introduced to cy.type(). Because slatejs handles its own state and the text updated by Cypress causes errors.
  • I had to use the plaintext example. Because slatejs uses React and creating a webpack bundle is big.
  • Firefox test will fail. Because this PR needs the dispatching event fix code in fix: Fire appropriate event when cy.type(). #8255.

How has the user experience changed?

You can test slatejs with Cypress.

PR Tasks

  • Have tests been added/updated?
  • Has the original issue been tagged with a release in ZenHub?
  • Has a PR for user-facing changes been opened in cypress-documentation? => coming soon after the approval
  • 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 26, 2020

Thanks for taking the time to open a PR!

packages/driver/src/cy/commands/actions/type.js Outdated Show resolved Hide resolved
@@ -801,6 +803,7 @@ export class Keyboard {
let eventConstructor = 'KeyboardEvent'
let cancelable = true
let addModifiers = true
let inputType = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will add the inputType to every event since it's not initialized as undefined. Since we don't test for extra properties we should add a test for that.

I'll take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It should be written in that way.

Comment on lines 10 to 16
.type(testString, { noUpdate: true })

cy.contains('[data-slate-string="true"]', testString)
.should('be.visible')

cy.get('[data-slate-editor="true"]')
.type('{ctrl}{shift}{backspace}', { release: false, noUpdate: 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 tried removing the noUpdate options here and the test still passed, is there a test case that fails w/o the option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. noUpdate isn't necessary.

I added it because there were some cases when cy.type() breaks the html structure inside slatejs. It seems that it was because beforeinput part was buggy.

Copy link
Contributor

Choose a reason for hiding this comment

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

the reason you previously needed noUpdate was due to the event not being set as cancelable https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/beforeinput_event, fixed now so we will respect a cancelled event and not perform the default action

@kuceb
Copy link
Contributor

kuceb commented Oct 15, 2020

made changes, I added tests in type_spec for beforeinput and verified it passes the slate spec before removing it, since the event tests cover it.

Copy link
Contributor

@kuceb kuceb left a comment

Choose a reason for hiding this comment

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

@sainthkh if you could look at the changes in the last commit and give them a review, we can merge

Copy link
Contributor Author

@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. I only did some minor changes.

  • Fixed the test failures.
  • Removed console.log comments.

@sainthkh
Copy link
Contributor Author

  • firefox failures: flaky
  • webpack: from develop branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cy.type does not fire beforeInput event / does not work with slate.js
3 participants