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

fix: ignore stylesheets fetching when is a inline style #4138

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

EduSantosBrito
Copy link
Contributor

Closes: #4137

This fixes the issue when there's a inline style tag inside the shadowRoot. Since the inline style doesn't have a href, there's no need to fetch it. This is usual on Lit components.

@EduSantosBrito EduSantosBrito requested a review from a team as a code owner August 25, 2023 11:51
@CLAassistant
Copy link

CLAassistant commented Aug 25, 2023

CLA assistant check
All committers have signed the CLA.

@straker
Copy link
Contributor

straker commented Aug 28, 2023

Thanks for the pull request. I would think that the filter method filerStyleAndLinkAttributesInDocumentFragment would catch the inline style tag and filter it out of the loop, thus never getting into this problem. A simple example shows that this is what happens. Do you know why in your case the filter is letting the inline style tag through?

<div id="shadow"></div>
<script>
    const root = shadow.attachShadow({ mode: 'open' });
    root.innerHTML = `
      <!-- this is filtered out of the getStylesheetsFromDocumentFragment loop -->
      <style>
      a { color: red }
      </style>
      <a href="#">hello world</a>
  `;
</script>

@WilcoFiers
Copy link
Contributor

@EduSantosBrito Quick ping, have you had a chance to look at Steve's comment?

@EduSantosBrito
Copy link
Contributor Author

Hey! I have time this week to look at it. I was on holiday last week hahaha

@EduSantosBrito
Copy link
Contributor Author

Ok, after debugging for some hours I found that document.implementation.createHTMLDocument breaks HTMLStyleElement.sheet. There's some issues opened related to that jsdom/jsdom#3179

This makes sense why the browser using selenium-webdriver was working and the test using jsdom wasn't.

Quick ping @WilcoFiers @straker

@straker
Copy link
Contributor

straker commented Sep 5, 2023

That makes sense. Thanks for the looking into that for us.

Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

Just updating the comment to reflect the investigations on when and why this is needed.

lib/core/utils/preload-cssom.js Outdated Show resolved Hide resolved
EduSantosBrito and others added 2 commits September 5, 2023 16:26
Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
@straker
Copy link
Contributor

straker commented Sep 5, 2023

Approved for security.

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.

Shadow Root with inline style fails to fetch cssom
4 participants