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.

Sign up for GitHub

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

Jump to bottom

Clicking nested element in disabled button invokes button's onclick handler #28788

Closed
nils4cosee opened this issue Jan 24, 2024 · 3 comments · Fixed by #28807
Closed

Clicking nested element in disabled button invokes button's onclick handler #28788

nils4cosee opened this issue Jan 24, 2024 · 3 comments · Fixed by #28807
Labels
good first issue Good for newcomers pkg/driver This is due to an issue in the packages/driver directory topic: cy.click 🖱 type: bug

Comments

@nils4cosee
Copy link

Current behavior

When "clicking" a nested element of a disabled button, the onclick-handler of this button is run (in Chrome or Firefox, not in Electron)

<body>
    <button disabled onclick="document.querySelector('div').innerText = 'Button clicked'">
        <span>Click me</span>
    </button>
    <div>

    </div>
</body>
</html>

Test:

it("does not invoke the onclick handler of a disabled parent button", () => {
    cy.visit("index.html")
    cy.contains("span", "Click me").click()
    cy.contains("Button clicked").should('not.exist')
})

image

Desired behavior

The behavior of

cy.contains("span", "Click me").click()

should be the same as

cy.contains("button", "Click me").click()

it only runs the onclick-handler if the button is not disabled and also waits for the button to be not disabled anymore.

Test code to reproduce

See my repo with the code https://github.com/nils4cosee/cypress-click-span-in-disabled-button

Cypress Version

13.6.1

Node version

18.15.0

Operating System

macOS 13.6

Debug Logs

Output of 


DEBUG=cypress:* npm run cypress:run -- --browser chrome --headless >debug.log 2>&1

is too long, according to GitHub. Please tell me if you really need it



### Other

_No response_
@jennifer-shehane jennifer-shehane added type: bug topic: cy.click 🖱 stage: ready for work The issue is reproducible and in scope good first issue Good for newcomers labels Jan 24, 2024
@jennifer-shehane
Copy link
Member

@nils4cosee Thank you for the clear reproducible example. This is surely a bug. We're open to a pull request to fix this. This bug is in the cypress/packages/driver package within this repo.

I feel like the logic may be in the mouse.ts logic? https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cy/mouse.ts

cy.click logic is defined here: https://github.com/cypress-io/cypress/blob/a11y-fixes-1/packages/driver/src/cy/commands/actions/click.ts#L259

A test case would be added here to test this new behavior.: https://github.com/cypress-io/cypress/blob/a11y-fixes-1/packages/driver/cypress/e2e/commands/actions/click.cy.js

@jennifer-shehane jennifer-shehane added the pkg/driver This is due to an issue in the packages/driver directory label Jan 24, 2024
@TheoAnastasiadis
Copy link
Contributor

Apparently, disabled elements on Chrome do not prevent their children from firing "click" events. see this codepen.

Maybe it is because according to the HTML spec, "actually disabled" elements can only ever be buttons, inputs, selects, textareas, options etc. ie. elements that must not have interactive content descendants.

My idea would be to check if the mouse down or mouse up elements have an "actually disabled" parent, and skip the click command if they do. Will open a PR shortly.

TheoAnastasiadis added a commit to TheoAnastasiadis/cypress that referenced this issue Jan 26, 2024
@TheoAnastasiadis
fixes cypress-io#28788 -- mouse.ts logic
0bb361b 
If mouseUp element or mouseDown element or commonAncestor element is :disabled, click event should be prevented
TheoAnastasiadis added a commit to TheoAnastasiadis/cypress that referenced this issue Jan 26, 2024
@TheoAnastasiadis
fixes cypress-io#28788 -- e2e test
622d4f3 
tests that no click events are registered when click happens on child of disabled element
TheoAnastasiadis added a commit to TheoAnastasiadis/cypress that referenced this issue Jan 26, 2024
@TheoAnastasiadis
fixes cypress-io#28788 -- mouse.ts logic
6c8943b 
If mouseUp element or mouseDown element have an "actually disabled" parent, the click event should not be registered
@TheoAnastasiadis TheoAnastasiadis mentioned this issue Jan 26, 2024
Merged
1 task
jennifer-shehane added a commit that referenced this issue Feb 27, 2024
@TheoAnastasiadis @AtofStryker
@jennifer-shehane @cacieprins
fix: Disabled elements being clickable (#28807)
5d42406 
* fixes #28788 -- mouse.ts logic
If mouseUp element or mouseDown element or commonAncestor element is :disabled, click event should be prevented

* fixes #28788 -- e2e test
tests that no click events are registered when click happens on child of disabled element

* fixes #28788 -- mouse.ts logic
If mouseUp element or mouseDown element have an "actually disabled" parent, the click event should not be registered

* refactoring: minor name changes for readability

* fixes #24322 --added changelog entry

* Update packages/driver/src/cy/mouse.ts

Co-authored-by: Bill Glesias <bglesias@gmail.com>

* Update packages/driver/cypress/e2e/commands/actions/click.cy.js

Co-authored-by: Bill Glesias <bglesias@gmail.com>

* Update packages/driver/src/cy/mouse.ts

Co-authored-by: Bill Glesias <bglesias@gmail.com>

* Update packages/driver/src/cy/mouse.ts

Co-authored-by: Bill Glesias <bglesias@gmail.com>

* update mouse.ts -- unexpected token

* docs: moved entry to 13.6.5

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update packages/driver/cypress/e2e/commands/actions/click.cy.js

Co-authored-by: Bill Glesias <bglesias@gmail.com>

* Update cli/CHANGELOG.md

Co-authored-by: Bill Glesias <bglesias@gmail.com>

* lint: fixed 2 linting errors

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

* style: removed trailing space

567:1  error  Trailing spaces not allowed  no-trailing-spaces

* Update CHANGELOG.md

---------

Co-authored-by: Bill Glesias <bglesias@gmail.com>
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Co-authored-by: Cacie Prins <cacieprins@users.noreply.github.com>
@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 removed the stage: ready for work The issue is reproducible and in scope label Mar 13, 2024
@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.
Assignees
No one assigned
Labels
good first issue Good for newcomers pkg/driver This is due to an issue in the packages/driver directory topic: cy.click 🖱 type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Disabled elements being clickable TheoAnastasiadis/cypress
3 participants
@jennifer-shehane @TheoAnastasiadis @nils4cosee