Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for custom elements #49

Merged
merged 5 commits into from
Dec 13, 2024
Merged

Conversation

trotzig
Copy link
Contributor

@trotzig trotzig commented Dec 13, 2024

The DOM snapshot hasn't been handling custom elements up until now. This PR will change that by adding a process to inline the shadow roots as regular elements. Happo workers will then recreate the shadow root and inject content and styling into them.

@trotzig trotzig requested a review from lencioni December 13, 2024 11:56

expect(snapshot.assetUrls).toEqual([
{
url: '/nested-custom-elements/avatar.jpeg',
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is 1.33 MiB, which is a pretty large binary blob to add to the repo. Let's be sure to make it smaller in the commit it is added before merging, so we don't end up unnecessarily bloating this repo so much.

Copy link
Contributor

Choose a reason for hiding this comment

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

I resized and re-exported this in my preview app, and it is now 106 KiB

Comment on lines 52 to 57
const href =
element.tagName.toLowerCase() === 'image' && element.getAttribute('href');
const href = element.getAttribute('href');
Copy link
Contributor

Choose a reason for hiding this comment

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

This will grab <a href. Maybe that's fine, but I want to make sure it is before proceeding. It might be safer to change the element tag name check to include link.

It is also worth noting that link elements are used for more than just adding stylesheets (e.g. favicons, preloading assets of any kind, etc). Since we are just working on stylesheets here, maybe it makes sense to scope this to rel="stylesheet"? Maybe it makes sense to get more than that at some point, but it might be best to reduce risk by keeping this tightly scoped for this change.

We should probably cover the desired behavior for <a href and non-stylesheet links in our tests if we aren't already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! I’ll update this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the commit and added a link in the regular elements test. That will cover the extraction of asset urls and make sure that we don't incorrectly extract assets from <a href="...">.

This is an attempt at fixing one of the most disabling issues with our
Cypress and Playwright integrations. Since we rely on DOM snapshots,
supporting custom elements has turned out to be tricky. To fix that, I'm
doing the following:

- Detect shadow roots as part of the DOM snapshot
- Inline the shadow root html as a hidden element (happo-shadow-content)

On the Happo worker, we will
- Query the DOM for potential `happo-shadow-content` elements
- Create a shadow root and inject the shadow content in the new shadow
  root.

It's very possible that I've missed some details of how shadow roots and
custom elements work. We can address them as they show up.
@lencioni lencioni force-pushed the dom-snapshot-custom-element branch from e3fba92 to 06f105b Compare December 13, 2024 14:39
So far we've only handled <style> elements inside shadow roots. That's
not the only way you can style shadow roots however. We have to handle
<link href> and adopted stylesheets as well.

For <link href>, add the asset to the list of assetUrls. This will make
it part of the asset package and in turn make it available on the Happo
workers.

For adopted stylesheets, inline them as <style> elements.
So that we don't run these 4 times (each node version).
@trotzig trotzig force-pushed the dom-snapshot-custom-element branch from 06f105b to d1cb9e3 Compare December 13, 2024 17:56
Because we are asserting on the assetUrls being collected, having a link
here will ensure that getElementAssetUrls() doesn't incorrectly return
assets linked to.
@trotzig trotzig merged commit 7e184dd into main Dec 13, 2024
5 checks passed
@trotzig trotzig deleted the dom-snapshot-custom-element branch December 13, 2024 18:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants