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

Styles inserted via CSSStyleSheet API don't survive cloning in iframe mode #207

Closed
bensmithett opened this issue Jul 29, 2022 · 8 comments · Fixed by #488
Closed

Styles inserted via CSSStyleSheet API don't survive cloning in iframe mode #207

bensmithett opened this issue Jul 29, 2022 · 8 comments · Fixed by #488
Labels
question Further information is requested
Milestone

Comments

@bensmithett
Copy link

bensmithett commented Jul 29, 2022

Describe the bug

When cloning style nodes in iframe mode (#196) styles inserted using the CSSStyleSheet API don't make it across.

I'm using Fela which uses CSSStyleSheet.insertRule.

StackOverflow answer that suggests looping through to copy each rule: https://stackoverflow.com/a/56156757

If I get some time before you get to this I'll poke around to see if there's a better way. Maybe something like grabbing document.styleSheets and using adoptedStyleSheets in the iframe might work?

Reproduction

https://stackblitz.com/edit/ladle-pxajhf?file=src%2Fbasic.stories.jsx

The text should be blue. When you set the story width it's red.

Environment

I don't think it's environment specific but I'm testing on

  • OS: macOS
  • Browser: chrome & firefox
  • Version: latest
@bensmithett bensmithett added the needs triage needs to be reviewed label Jul 29, 2022
@tajo tajo added question Further information is requested and removed needs triage needs to be reviewed labels Jul 29, 2022
@tajo
Copy link
Owner

tajo commented Jul 29, 2022

Yea, I had a similar issue with styletron.

I had to write a bit hacky code to "retarget" the engine so it starts adding styles to the correct document, you should be able to do something very similar since Fela is pretty much identical:

https://github.com/uber/baseweb/blob/master/.ladle/components.tsx#L23-L40

Frankly, I couldn't come up with a more elegant solution other than just reloading the page when turning on/off the iframe. If anyone has some ideas... adoptedStyleSheets looks interesting - it could simplify this code but you'll still need to handle the CSS in JS engine targeting the correct document after the switch. Btw, you can't observe mutations for CSSStyleSheet - so no ad-hoc synchronization is possible.

@bensmithett
Copy link
Author

bensmithett commented Jul 31, 2022

Thanks for the pointer, here's how I adapted that for Fela in a way that lets you switch the frame on/off without needing to reload:

function getDocument(story) {
  const iframe = document.querySelector(`[title='Story ${story}']`)
  return iframe && iframe.contentDocument ? iframe.contentDocument : document
}

export const Provider ({ children, globalState }) => {
  const doc = getDocument(globalState.story)
  const felaRenderer = useMemo(() => createRenderer(), [doc])

  return (
    <RendererProvider renderer={renderer} targetDocument={doc}>
      {children}
    </RendererProvider>
  )
}

This could be nicer without needing to write the custom getDocument...

  • Possible to directly pass in the current story document on globalState or something?
  • ...or maybe even just put a consistent identifier on the iframe (e.g id='ladle-iframe') so you don't need to target the story title?

you'll still need to handle the CSS in JS engine targeting the correct document after the switch

I might be misunderstanding adoptedStylesheets but if the idea is that documents/shadow roots can share live updating stylesheets, you could possibly avoid that. Just always target the root document, then adopt in the iframe when you have an iframe? But it'd only work with constructed stylesheets which probably isn't much good for most people.

@tajo
Copy link
Owner

tajo commented Aug 1, 2022

Possible to directly pass in the current story document on globalState or something?

I had a version passing it into the Provider at one point. Also, there's probably only a single iframe anyway, so you could just query that.

I might be misunderstanding adoptedStylesheets but if the idea is that documents/shadow roots can share live updating stylesheets

That would be great. I will test it out. That would remove any need to custom setup CSS in JS libs.

@tajo
Copy link
Owner

tajo commented Aug 4, 2022

Tried to use adoptedStylesheets. It seems you can't grab <style> element from the parent and do iframe.document.adoptedStyleSheets = [styleElement.sheet].

It throws:

Uncaught DOMException: Failed to set the 'adoptedStyleSheets' property on 'Document': Can't adopt non-constructed stylesheets.

The adapted sheet needs to be created/shared through

new CSSStyleSheet()

So that rules out some type of seamless integration on the background. Not sure if most CSS in JS libraries even provide CSSStyleSheet instance directly instead of just adding style DOM nodes in a set container.

Also, this whole thing doesn't work in Safari.

@bensmithett
Copy link
Author

Good to know, appreciate you giving it a try 🙏

I'm happy with the workaround. Would you like me to PR a docs update mentioning it or something?

@tajo
Copy link
Owner

tajo commented Aug 5, 2022

Yea, I think it would nice to document it here with some examples: https://ladle.dev/docs/width

I am also considering to provide some onIframeChange callback passed through the Provider to simplify the user setup.

@twhitbeck
Copy link

I believe a project using Chakra UI which uses emotion for styles also suffers from this problem.

@tajo tajo added this to the v3 milestone Sep 4, 2023
@tajo
Copy link
Owner

tajo commented Sep 11, 2023

I have a solution for this now, should work for all CSS in JS libs without any strange workarounds by users. Will be released in V3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants