Skip to content

Conversation

@cometkim
Copy link
Contributor

@cometkim cometkim commented Oct 1, 2021

Resolves #1093

Users can use the Shadow DOM instead of the original, through the data-shadow-dom property of the base element.

data-shadow-dom attribute should be open or closed

When Shadow DOM is enabled, it can protect the host styles by isolating style changes that occur inside the Worker.
Uses of <link> for external stylesheets inside the Worker are also isolated, so they also need to be moved inside the base element.


Note:

Using shadow dom may have side effects. I leave some issue I found here.

  1. In the React example, event listeners don't work at all. I'm guessing it's a React bug since there's no problem in the same Preact example.

  2. Shadow DOM is known to have very little performance penalty, but in high-load apps like DBMon seem to have frame drops quite noticiable actually.
    image

Users can use the Shadow DOM instead of the original, through the `data-shadow-root` property of the base element.

`data-shadow-dom` attribute should be `open` or `closed`, or implicitly use `open` when it is empty.

When Shadow DOM is enabled, it can protect the host styles by isolating style changes that occur inside the Worker.
Uses of `<link>` for external stylesheets inside the Worker are also isolated, so they also need to be moved inside the base element.

Resolves ampproject#1093
@kristoferbaxter
Copy link
Contributor

This looks amazing! I'll give it a proper review this week.

Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

One small comment. Otherwise the only thing this is missing is unit tests. Looks great

@cometkim cometkim requested a review from samouri October 13, 2021 15:54
@cometkim
Copy link
Contributor Author

cometkim commented Oct 13, 2021

Honestly, I'm not familiar with the environment of doing DOM integration tests here yet, I'll have to spend a bit of time this codebase.

Added a very simple one

@cometkim cometkim changed the title Added a flag to selectively enable shadow dom Added a flag to selectively using shadow dom Oct 20, 2021
@cometkim cometkim changed the title Added a flag to selectively using shadow dom Added a flag to selectively use shadow dom Oct 20, 2021
@samouri samouri merged commit d90e684 into ampproject:main Nov 1, 2021
@cometkim cometkim deleted the shadow-root branch November 2, 2021 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: Encapsulate styles using Shadow DOM

3 participants