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: Disabled elements being clickable #28807

Merged
merged 24 commits into from
Feb 27, 2024

Conversation

TheoAnastasiadis
Copy link
Contributor

Additional details

Click commands on children of disabled elements still produce click events.
For example the following;

<button disabled>
  <span>Click Me!</span>
</button>
cy.contains("Click Me!").click() // this triggers `click` events (at least on Chrome).

Even though the button element is disabled.

How has the user experience changed?

Little

PR Tasks

If mouseUp element or mouseDown element or commonAncestor element is :disabled, click event should be prevented
tests that no click events are registered when click happens on child of disabled element
If mouseUp element or mouseDown element have an "actually disabled" parent, the click event should not be registered
@CLAassistant
Copy link

CLAassistant commented Jan 26, 2024

CLA assistant check
All committers have signed the CLA.

@cypress-app-bot
Copy link
Collaborator

@TheoAnastasiadis TheoAnastasiadis changed the title Disabled click fix: Disabled elements being clickable Jan 26, 2024
@jennifer-shehane
Copy link
Member

@TheoAnastasiadis Thanks for the contribution. Please see this failing check for details about adding a changelog entry: https://github.com/cypress-io/cypress/actions/runs/7670111753/job/20905527788?pr=28807

Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

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

Hi @TheoAnastasiadis! Thank you for the contribution PR. I had some issues getting the added code in mouse.ts to work correctly. Were you able to run the code changes against the cypress test or was this a best guess effort? Either way I think we can get this running with the suggestions below.

packages/driver/cypress/e2e/commands/actions/click.cy.js Outdated Show resolved Hide resolved
packages/driver/src/cy/mouse.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/mouse.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/mouse.ts Outdated Show resolved Hide resolved
cli/CHANGELOG.md Outdated Show resolved Hide resolved
@TheoAnastasiadis
Copy link
Contributor Author

Hi @TheoAnastasiadis! Thank you for the contribution PR. I had some issues getting the added code in mouse.ts to work correctly. Were you able to run the code changes against the cypress test or was this a best guess effort? Either way I think we can get this running with the suggestions below.

this was embarrassing 😳 is ok now, what should the release date of 13.6.5 be?

@jennifer-shehane
Copy link
Member

@TheoAnastasiadis 2 weeks from the previous release is the target, so 2/13/2024

@AtofStryker
Copy link
Contributor

Hi @TheoAnastasiadis! Thank you for the contribution PR. I had some issues getting the added code in mouse.ts to work correctly. Were you able to run the code changes against the cypress test or was this a best guess effort? Either way I think we can get this running with the suggestions below.

this was embarrassing 😳 is ok now, what should the release date of 13.6.5 be?

no worries @TheoAnastasiadis! We've all been there before 🙂 . CI should be running against the PR now.

Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

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

Besides the changelog edits I think we are good to go! Going to wait for CI test feedback then will re review

cli/CHANGELOG.md Outdated Show resolved Hide resolved
cli/CHANGELOG.md Outdated

**Bugfixes:**

Fixed an issue where `.click()` commands on children of disabled elements would still produce "click" events -- even without `{ force: true }`. Fixes [#28788](https://github.com/cypress-io/cypress/issues/28788).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Fixed an issue where `.click()` commands on children of disabled elements would still produce "click" events -- even without `{ force: true }`. Fixes [#28788](https://github.com/cypress-io/cypress/issues/28788).
- Fixed an issue where `.click()` commands on children of disabled elements would still produce "click" events -- even without `{ force: true }`. Fixes [#28788](https://github.com/cypress-io/cypress/issues/28788).

@AtofStryker AtofStryker self-requested a review February 6, 2024 15:28
TheoAnastasiadis and others added 5 commits February 15, 2024 22:39
Co-authored-by: Bill Glesias <bglesias@gmail.com>
Co-authored-by: Bill Glesias <bglesias@gmail.com>
565:1  error  Expected indentation of 8 spaces but found 10  indent
567:7  error  Expected blank line before this statement      padding-line-between-statements
567:1  error  Trailing spaces not allowed  no-trailing-spaces
@AtofStryker AtofStryker self-requested a review February 16, 2024 14:18
cacieprins
cacieprins previously approved these changes Feb 27, 2024
@cacieprins cacieprins dismissed their stale review February 27, 2024 15:17

Needs changelog updates before merge

@jennifer-shehane jennifer-shehane merged commit 5d42406 into cypress-io:develop Feb 27, 2024
67 checks passed
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 13, 2024

Released in 13.7.0.

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

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

Clicking nested element in disabled button invokes button's onclick handler
6 participants