-
Notifications
You must be signed in to change notification settings - Fork 3.3k
add some shadow dom support #7469
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
Conversation
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
59af811
to
a3d34ca
Compare
Marking it as ready to review so i can hopefully get some feedback. It needs catching up from latest dev branch, and there's failing integration tests i haven't managed to track down yet. But the functionality is there and works just fine as far as i've seen. Yet to add unit tests but this needs reviewing so i don't go down the rabbit hole for no reason. |
2a1e1f4
to
c74290f
Compare
Hi, I am looking into working with Cypress for some form automation testing and I'm very glad to see people are taking up this task. How would you say progress is going in regards to Cypress' ability in your additions with detecting if elements in the shadow-root are clickable? Or even just progress in general? I am still relatively new to Cypress, so I am unsure how much help I would be in the review process to move this along in that regard; but you're doing great work that would benefit a lot of people, including myself, and I would love to help where I can. |
Really at this point i just need to get the tests passing and have someone from the cypress team review the changes. I think what could be useful is to hear from people like yourself if the suggested api makes sense:
I've also changed mouse events to be |
I think that is the exact API that most of us are looking for, and I think the format for the shadow condition being an optional parameter makes sense with keeping theme of the currently used Cypress API. And if mouse actions are being composed the same way as if they were in the browser, I wouldn't see any issue outside of that. |
Is there a ballpark amount of time you think this will take to be implemented for use? I'm looking at other options for the time being, but I'd love to use Cypress if possible! |
We're reviewing this PR right now and coming up with a more detailed plan, given the awesome work that @43081j has done. |
That's great to hear! From looking through this issue for the last two days, it seems like not being able to see the shadow dom has been one of the only roadblocks for a lot of people in using Cypress. Waiting patiently yet eagerly to hear about this. Great work to @43081j once again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finished looking through the PR with @chrisbreiding. We'd like to release this behind an experimental feature flag as soon as possible.
Before we can release, we need to:
- fix the regressions introduced (I'm starting this now)
- add unit tests (I would love @43081j to collaborate on this. He has more context about the code written and the Shadow-DOM specific cases covered)
- create an experimental feature flag and add it to the docs (@chrisbreiding or I can do this)
- gate the code behind this feature flag
After #1
and #2
are completed, I expect the code will look a bit different. At this point, we'll re-review and clean up the code for nits, etc.
thanks a lot of taking the time to review it. happy to write the unit tests once i have a dig through the current ones to get my head around it. i had a lot of trouble getting cypress to build locally (windows, WSL) so haven't been able to easily debug the remaining test failures. i did step through it plenty, which helped me fix most of the failures but these last few are tough. they seem to be something to do with visibility. im sure like the other tests i figured out, it'll be some minor thing i've missed out in one of my changes. |
@43081j there's a lot of changes in dom/elements.js that we were suspicious of. I haven't started debugging them. I pinged you on twitter if you wanted to collab in realtime and I can help you get your env up. |
i added an example shadow() test just to try it out but my local env is failing miserably so i haven't run them. ill do the tests properly tomorrow when i switch machine 👍 |
Will this only work for native shadow DOM? In other words, if the browser being used supports shadow DOM itself, then this will work, but not necessarily with Shadow DOM polyfills or other potentially special implementations of encapsulation/Shadow DOM. I really don't know enough about it to tell from the code, but I know this can be a thing. Figured I'd ask to see if maybe it can help clarify in what cases this may be able to be used. |
Shadow dom polyfills/shims will fill in the same API @JasonFairchild , so to cypress it should be transparent as long as your app includes the right polyfill. |
I am thrilled to see the progress made in the last few days on this topic. Is there a relative time frame for when this feature flag could be ready for use? And outside of that, is there anything that people like myself, who are very much interested in using Cypress for future company testing, can do to help with this process? |
} | ||
|
||
const findShadowRoots = (root: Node): Node[] => { | ||
const doc = root.getRootNode({ composed: true }) as Document |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use a state method for getting this to use the real document
@cbowlinger We're not going to commit to a specific date. We just got off the phone with @43081j who plans to bring this in for a landing after adding more tests and some examples in our recipe repository. We plan to help him do that. Releasing this will coincide with a Cypress binary release date which happens every two weeks. |
3be576e
to
4a262fe
Compare
…into shadows-everywhere
…nto shadows-everywhere
Happy Friday! We have a PR for the docs open here. We don't usually provide detailed docs for experimental features, but the types and recipes will help you. We are waiting to choose a flag name using active voice (e.g. instead of We'll update all the references of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES
The shadow dom support issue never seems to go away, and the only PR that tried to do anything about it seems to have gone stale.
So here's some kind of draft of what I had in my head back when the issue was discussed a bit more (#144 ).
Changes
shadow()
Allows you to explicitly traverse into an element's shadow root.
includeShadowDom
Allows you to tell cypress to ignore shadow boundaries, i.e. traverse beyond them.
Keep in mind this means you would not be able to do something like
find('.imInOneRoot .imInAnother')
because it'd be a nasty idea to have cypress parse that and figure it out.mouse events
Mouse events are now
composed: true
by default, just like the browser. This allows clicks on elements inside shadow roots to bubble upwards into the outer document.isAttached
We now use
.isConnected
to detect if a given element is attached or not, rather than guessing based on parent traversal.clicks are deep
Clicks will now click the deepest element at the point you click, not the element in the document where you click.
This basically makes no difference to non-shadow-contained elements.
For shadow contained elements, it means they will be clicked rather than their host.
Example Usage
Given the following DOM
You can do:
get(selector)
behaves differently:This is on purpose, as mentioned earlier. It is better to use
get
for selecting a particular element, thenfind
within its shadow root.Keep in mind, it will always be more performant to have direct selectors rather than boundary-ignoring selectors, i.e. the
.shadow().find(...)
option is always better than usingincludeShadowDom
.