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

Add Shadow DOM support to @percy/dom #280

Closed
wachunga opened this issue Dec 11, 2020 · 32 comments · Fixed by #1165
Closed

Add Shadow DOM support to @percy/dom #280

wachunga opened this issue Dec 11, 2020 · 32 comments · Fixed by #1165
Labels
✨ enhancement New feature or request ❄️ on ice Waiting for API / upstream support

Comments

@wachunga
Copy link

Nothing rendered in the shadow-dom, even if open, shows up in percy-cypress snapshots.

The below cypress test passes fine.

Repo:

<html>
  <head>
    <script lang="javascript" defer>
      document.addEventListener('DOMContentLoaded', () => {
        const contentEl = document.querySelector('#content');
        const shadow = contentEl.attachShadow({mode: 'open'});
        const paragraphEl = document.createElement('p');
        paragraphEl.textContent = 'Hey Percy! Can you see this?';
        shadow.appendChild(paragraphEl);
      });
    </script>
  </head>
  <body>
    <div>
      <h1>Shadow DOM repro</h1>
      <div id="content"></div>
    </div>
  </body>
</html>
it.only("shadow dom test", () => {
  cy.visit(`/`);
  cy.contains("Hey Percy!");
  cy.document().then((doc) => console.log(doc.documentElement.outerHTML));
  cy.percySnapshot();
});

The outerHTML logged out does not include the text, and apparently that's how percy gets the DOM...

Any suggestions on workarounds? Getting rid of shadow-dom is not an option for my application.

@wachunga
Copy link
Author

wachunga commented Dec 11, 2020

Forgot to mention: there's a similar issue with Cypress snapshots (shown on hover) not including shadow-dom:
cypress-io/cypress#8843

There's a repo linked there that makes it easy to repro.

@Robdel12 Robdel12 transferred this issue from percy/percy-cypress Apr 1, 2021
@Robdel12 Robdel12 changed the title percy-cypress does not capture anything in shadow-dom Add Shadow DOM support to @percy/dom Apr 1, 2021
@Robdel12 Robdel12 added the ✨ enhancement New feature or request label Apr 1, 2021
@Robdel12
Copy link
Contributor

Robdel12 commented Apr 1, 2021

Thanks for the issue! This is currently not supported across all of our SDKs. In the near future we'll be picking this up now that there's a method for capturing open shadowroots in the DOM: https://web.dev/declarative-shadow-dom/#serialization

But it does require a complete refactor to how @percy/dom works now.

@jeffchew
Copy link

jeffchew commented Apr 6, 2021

I'd like to +1 this too. We started out with percy-storybook which appears to be fine with at least the base snapshots, but we have recently started hooking into puppeteer to capture click states, etc. The only way any content appears to render is with enableJavascript set to true, which only renders the initial state of the page but does not capture any subsequent states/e2e tests like click states, form entering, etc.

@wachunga
Copy link
Author

@Robdel12 That's great news. I have confirmed that Cypress supports this already with the following change (when using Chrome browser, but not yet Electron):

  on("before:browser:launch", (browser = {}, launchOptions) => {
    launchOptions.args.push("--enable-experimental-web-platform-features");
    return launchOptions;
  });

Can you confirm that doesn't help until percy-dom knows what to do with <template shadowroot="open\>?

Also, if you have ideas for workarounds, that would really help my team.

@Robdel12
Copy link
Contributor

Sadly there isn't anything that can be done until we do the work to update @percy/dom. We have to change how we clone the DOM -- Shadow DOM doesn't respect/work with cloning the full DOM tree (document.cloneNode(true)). We'll have to rewrite use element.getInnerHTML({includeShadowRoots: true}) which could have other ripple impacts on how the DOM is captured/serialized/persisted. Not a small change :)

@wwilsman
Copy link
Contributor

Did some preliminary research on this with Chrome 90 recently out with the declarative API enabled by default.

  1. Even with the new declarative API, the cloned document does not seem to contain shadow roots. While the document does contain the templates needed to create shadow roots, it seems the roots themselves aren't actually created unless the cloned templates are rendered. This makes it so we have the same current issue of not being able to serialize shadow root contents (such as nested input elements or canvases).

  2. It doesn't looks like these new template shadow roots will be able to be re-rendered trivially (without JS) unless the browser also supports the new declarative API. While chromium browsers have only recently shipped with support enabled by default, our other main rendering browser, FireFox, doesn't seem to have support on their roadmap quiet yet (that I can find).

Even when support is more widely available and that isn't an issue, we'll still have to figure out the first issue of how to properly serialize shadow root trees. Maybe that involves an even bigger refactor to walking the DOM tree and building the clone ourselves, but that's an even larger task than refactoring to use getInnerHTML.

@jeffchew
Copy link

Just checking if there's been any progress here at all?

@Robdel12
Copy link
Contributor

Robdel12 commented Jun 24, 2021

Hey @jeffchew! Based on @wwilsman's research (the comment right above) it doesn't look like this will be something we'll be able to implement for a while. The web APIs just aren't there and what is starting to come out only works/exists in Chrome (behind flags, depending on what version your tests are running in, default in Chrome 90).

We'd love to support this but it seems to be a case of Google running out in front of everyone (other browser vendors).

@Robdel12 Robdel12 added the ❄️ on ice Waiting for API / upstream support label Jul 15, 2021
kodiakhq bot pushed a commit to carbon-design-system/carbon-for-ibm-dotcom that referenced this issue Sep 10, 2021
)

### Related Ticket(s)

Refs #6139

### Description

This PR fully moves the existing `test-e2e` package to a dedicated cypress test setup for web components and react storybooks. A few tests have been included for `masthead`, `locale-modal`, and `leadspace`. They are largely focused on snapshot testing, though percy has been disabled for now as rendering of dynamic states are still not supported in percy for web components (see percy/cli#280).

Actual cypress tests can be created now though, and also the current tests uses the built-in cypress screenshot feature:

![image](https://user-images.githubusercontent.com/1641214/132697052-2943ccdf-81c5-450d-999e-c39e557a3d05.png)

And video that shows how the tests are run:

https://user-images.githubusercontent.com/1641214/132697174-a778c055-d3d8-4e01-877d-bd58c2aa7491.mp4

### Changelog

**New**

- Cypress testing for web components and react storybook

**Changed**

- disabled video recording by default to speed up tests

**Removed**

- Removed `test-e2e` package
@jeffchew
Copy link

Just keeping this on the radar if possible. We are increasing our e2e test coverage on our end and so far can only test with React (which we plan to deprecate sometime in the future and only support web components).

@jeffchew
Copy link

Just another FYI, we tried upgrading @percy/storybook to v4, and it ended up breaking all of the snapshots since from what I understand uses @percy/cli under the hood in this version:

https://percy.io/538fc19a/Carbon-for-IBM.com-Web-Components/builds/14269858/changed/805810435?browser=chrome&browser_ids=18%2C20&subcategories=unreviewed%2Cchanges_requested&viewLayout=side-by-side&viewMode=new&width=1280&widths=375%2C1280

All snapshots are unstyled, and a number of them also just went missing.

@Robdel12
Copy link
Contributor

Robdel12 commented Nov 30, 2021

You'll want to re-enable JavaScript with Storybook & WC's. https://github.com/percy/percy-storybook#unexpected-diffs

Edit: FWIW, there won't be any progress on this until the Web's APIs for WC's matures (as described here.)

@mmun
Copy link

mmun commented Apr 25, 2022

Hey Percy folks!

Shadow DOM support is important for us at Addepar because our design system is based on Stencil which makes heavy use of shadow DOM.

We've developed a fork of @percy/cli (and to a smaller extent @percy/ember) that supports shadow DOM. They're publicly accessible at https://github.com/Addepar/percy-cli and https://github.com/Addepar/percy-ember. We've open sourced the forks as a proof of concept for the developers of Percy, as well as for other Percy users to use if they are in a similar situation as us.

Here are some details of the implementation. I'm happy to answer any questions.

Caveats

As discussed in the comments above, we're leaning on the experimental Declarative Shadow DOM available in Chrome. This means that:

  • 🚨 It only works in Chrome 🚨 Specifically Chrome 90+, or Chrome 85+ behind a flag. See https://chromestatus.com/feature/5191745052606464.
  • It may break at any time in the future if Chrome changes its API or drops support for the feature entirely.

Implementation notes

cloneNode

cloneNode(true) does not include shadow roots. We reimplement a recursive cloneNode that can handle this.

adoptedStylesheets

The adoptedStylesheets property is commonly used in component frameworks using shadow DOM because it lets you efficiently reuse stylesheet objects across multiple shadow roots (i.e. across multiple instances of a shadow DOM-based web component).

It's a known issue that adoptedStylesheets are not currently serialized by the APIs exposed in the Declarative Shadow DOM feature. See for example whatwg/dom#831 (comment), https://github.com/mfreed7/declarative-shadow-dom/blob/master/README.md#other-unanswered-questions and WICG/webcomponents#939.

We resolve this issue by inlining a <style> tag into each shadow root. There are more efficient ways to handle this serialization but it would require running a bit of setup code in the document on Percy's servers before snapshots are taken and that's not currently a feature Percy offers.

@IgnaceMaes
Copy link

@mmun That's amazing, thanks for looking into this.

At @OTA-Insight we're in the exact same situation as the design system is built with Stencil using Shadow DOM and the main application is written in Ember. Not having Shadow DOM available in the screenshot is blocking us from adopting Percy.

I've tested our setup using the forks you've linked and it fixes the issue 🙌

Right now the forks are built locally. When linking them directly in package.json in the following way it results in an error from the @percy/cli package: error Can't add "@percy/cli": invalid package version undefined.. Any idea if there is a way to fix this?

"@percy/cli": "https://github.com/Addepar/percy-cli.git#master",
"@percy/ember": "https://github.com/Addepar/percy-ember.git#master",

It would be great if this could make its way to the official package.

@mmun
Copy link

mmun commented Apr 26, 2022

@IgnaceMaes Yeah, we're still thinking about the best way to handle this. One option is to use something like https://gitpkg.vercel.app/.

As an aside, you'll probably run into a Stencil-specific bug ("Sharing constructed stylesheets in multiple documents is not allowed") with the current implementation. This will cause your tests to fail despite the screenshots uploading correctly. We'll add a workaround for that soon.

@Robdel12
Copy link
Contributor

Robdel12 commented May 2, 2022

👋🏼 Thanks for the info @mmun! We'll likely only be interested in accepting something like this behind a flag so it's opt-in. There's too many caveat's for us to go all in on this, the API will need to stabilize a lot more before we feel ready for that.

@mixonic
Copy link

mixonic commented May 5, 2022

There's too many caveat's for us to go all in on this

I mean, the caveat today is that Percy doesn't work with Web Components (w/ Shadow DOM) rendered by a JS framework (or maybe generally) at all, which seems like a pretty large caveat.

Regardless, your call over at Percy 👍. What does adding this functionality behind an opt-in flag look like? Is there prior art or an example? We would love to have this upstream even if it is opt-in so that we can get off our forks.

@wwilsman
Copy link
Contributor

wwilsman commented May 5, 2022

That fork does look promising! We'll look into adopting this officially behind a config option in the coming weeks. Currently, the only option that gets forwarded to the DOM serialization script is enableJavaScript. A new option will likely follow a similar pattern, and we'll have to verify that all of our SDKs will correctly forward that option to the script.

@mmun
Copy link

mmun commented May 5, 2022

@wwilsman Awesome. Thanks for taking a look. My team is available to collaborate if needed.

@jeffchew
Copy link

Would love this as a config option! Thank you @mmun for the work you put into your project for this! This would hopefully unblock us on our end!

@Robdel12
Copy link
Contributor

@jeffchew with storybook you can enable JS safely and it will render the story from scratch in our browsers (so the shadow DOM will get built). It's the only SDK that this will work well with

@jeffchew
Copy link

Thank you @Robdel12 , it turns out after digging into the storybook side there might be an unrelated issue to this that we are still blocked by, but we are also using percy with e2e testing in Cypress where we currently cannot do with web components (which relates more to this issue).

The unrelated bug with storybook seems to also affect our React based storybook, where most snapshots are not rendering (there is a timeout error of some kind) and I already reached out to our account executive and Lee V. about this (but not sure if there is an issue open for this).

@Robdel12
Copy link
Contributor

Robdel12 commented May 17, 2022

Ah! @jeffchew I remember debugging this and shipping a feature to help. I remember there were ad requests that were hanging asset discovery up (or maybe tracking pixels). You can exclude those requests from asset discovery and that should hopefully stop the timeouts from happening on your storybook builds https://docs.percy.io/docs/debugging-sdks#disallowing-requests

@jeffchew
Copy link

Thanks @Robdel12 ! We did try that but still running into issues. Should I create a new issue to track this as this is different from the Shadow DOM support? I can share logs on what I'm seeing but looks like JS assets themselves are getting blocked in Percy.

@Robdel12
Copy link
Contributor

For sure! I'd open a support discussion on the storybook SDK

@jeffchew
Copy link

@Robdel12 issue opened! percy/percy-storybook#565

@snake-py

This comment was marked as duplicate.

@andrei-musnikov

This comment was marked as duplicate.

@IgnasCi

This comment was marked as duplicate.

@andreapigatto

This comment was marked as duplicate.

@branislav-remen

This comment was marked as duplicate.

@itsjwala
Copy link
Contributor

Hey folks!

we've shadow DOM capturing in our roadmap. Due to some OKR reshuffling, it was delayed. Please expect beta by earlier Jan 2023.

To workaround this till then you may enable javascript in Percy's config and use https://github.com/percy/percy-storybook/ SDK.

@itsjwala itsjwala linked a pull request Jan 5, 2023 that will close this issue
@itsjwala
Copy link
Contributor

Hey folks!

Percy now supports capturing Shadow DOM available in the Alpha release of CLI 🏗️. Please refer to this documentation for trying it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request ❄️ on ice Waiting for API / upstream support
Projects
None yet
Development

Successfully merging a pull request may close this issue.