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

The type command should not click on focused elements inside of shadow roots. #26198

Closed
kgroat opened this issue Mar 23, 2023 · 7 comments · Fixed by #26207
Closed

The type command should not click on focused elements inside of shadow roots. #26198

kgroat opened this issue Mar 23, 2023 · 7 comments · Fixed by #26207
Assignees
Labels
pkg/driver This is due to an issue in the packages/driver directory topic: shadow dom Issues when testing shadow dom

Comments

@kgroat
Copy link
Contributor

kgroat commented Mar 23, 2023

Current behavior

When you attempt to .type() inside an element that is inside a shadow root, it will always perform the .click() even if the element has focus.

This is even an issue if you have includeShadowDom set to true.

Desired behavior

Either check inside of shadow roots for the focused element, or allow the .click() behavior to be turned off using an option inside the .type() command.

Test code to reproduce

https://github.com/kgroat/cypress-type-repro

Cypress Version

12.8.1

Node version

16.16.0

Operating System

MacOS 13.2.1

Debug Logs

No response

Other

No response

@lmiller1990
Copy link
Contributor

Hi, thanks for the minimal reproduction. Just to make it even more minimal, it looks like we can remove some code - can you confirm this still represents your use case? https://github.com/kgroat/cypress-type-repro/pull/1/files.

I also made a minimal non web components/shadow example, to see what happens - it does indeed pass:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta http-equiv="X-UA-Compatible" content="IE=edge">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <title>Document</title>
</head>
<body>
  
  <input id="input" type='text' onclick="handleClick()" />
  <div id="div" tabindex="-1" class="popup">
    I am a popup
  </div>

  <script id="no-shadow-dom">
    const $ = sel => document.querySelector(sel)

    const $input = $("#input")
    const $div = $("#div")

    document.addEventListener('click', (e) => {
      console.log('click')
      handleDocClick(e)
    });

    function handleDocClick(e) {
      console.log(e.target)
      if (e.target === $input || e.target === $div) return
      open = false;
      $div.focus();
    }

    let open = false

    async function handleClick() {
      if (open) return;
      console.log('set')
      open = true;
      $div.setAttribute('tabindex', 0)
      $div.classList.add('open')
      $div.focus();
    }
  </script>
  
  <style>
    .popup { 
      display: none; 
    }

    .open { 
      display: block; 
    }
  </style>

</body>
</html>

It looks like there was some somewhat related work in #7847.

I'd say the best option here would be "Either check inside of shadow roots for the focused element".

@lmiller1990 lmiller1990 added pkg/driver This is due to an issue in the packages/driver directory topic: shadow dom Issues when testing shadow dom routed-to-e2e labels Mar 24, 2023
@kgroat
Copy link
Contributor Author

kgroat commented Mar 24, 2023

Your PR does take out some unnecessary code; I'll go ahead and merge it. That said, the test in there I think we should keep the .focus() before typing, otherwise I think the test will always fail, even if this issue is fixed.

Also, yes, the code above seems to pass the test, which is why I believe this has to do with shadow dom. Checking inside the shadow roots would be my preferred option as well.

I think I've tracked a solution down.

Should I open a PR with my proposed solution? I've verified it using yarn start & pulling in my reproduction case.

@lmiller1990
Copy link
Contributor

Oh nice, I was about to ask if you wanted to make a PR. If you think you've got a fix, go ahead and make a PR. I'll need to kick off the CI since you are a third party, so you could add me as a reviewer.

Lmk if you need any other help navigating the code base, adding a reproduction, or anything else. Thanks!

@kgroat
Copy link
Contributor Author

kgroat commented Mar 24, 2023

I'm not sure how to invite you to be a reviewer, but you can see the PR above. I've marked it as a draft while I add a test for the scenario. I see that there are only e2e tests in the driver package. How would I run those in order to verify my test works? I tried yarn test-e2e but it just says the command isn't found.

@kgroat
Copy link
Contributor Author

kgroat commented Mar 24, 2023

Figured out that I could just yarn start & drag the driver package in to test it.

@lmiller1990
Copy link
Contributor

lmiller1990 commented Mar 24, 2023

You can run yarn watch in packages/runner to rebuild the runner (which includes the packages/driver) on save.

You can do yarn cypress:open in packages/driver to open Cypress and develop the driver (so you would do yarn watch in one terminal, and yarn cypress:open` in another).

You can also do yarn dev at the top level and go from there, but that runs a lot of other processes and watchers, which you probably don't need for this PR.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 28, 2023

Released in 12.9.0.

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

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Mar 28, 2023
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: shadow dom Issues when testing shadow dom
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants