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

FOUC when loading CSS #291

Closed
jaysoo opened this issue Jun 15, 2017 · 9 comments
Closed

FOUC when loading CSS #291

jaysoo opened this issue Jun 15, 2017 · 9 comments

Comments

@jaysoo
Copy link
Contributor

jaysoo commented Jun 15, 2017

Scenario

Given the following element: (See https://github.com/jaysoo/example-react-helmet)

  <Helmet
    style={[{
      cssText: `
        .message {
          text-align: center;
          font-size: 48px;
          color: magenta;
        }
      `
    }]}
  />
  <p className="message">
    Hello World!
  </p>
</div>

I know this is a contrived example, and not how you should be styling React apps. The real world example involves allowing user customization via custom CSS.

Expected

The element is rendered, and the user sees "Hello World!" in large, magenta text.

Actual

The user will see a flash of unstyled content (FOUC) before the <style> is inserted into <head>. This seems to be caused by react-helmet v5's use of requestIdleCallback, which forces the head tag insertion to happen after the rendered element has been updated in the DOM.

Note: This does not happen with version 3.x.

fouc

Workaround

I don't see any obvious workarounds, unless something clever is done using onChangeClientState. Suggestions welcomed.

@jaysoo
Copy link
Contributor Author

jaysoo commented Jun 15, 2017

Maybe a new prop like defer or blocking on the tags could control whether tags are updated synchronously or asynchronously? I'd be happy to do a PR for this if it is desirable.

@doctyper
Copy link
Contributor

@jaysoo Interesting use-case. The delay is meant to avoid multiple writes to document.head, but it seems it had an unintended side-effect.

I'm not opposed to a defer={false} opt-out. Let's see what @cwelch5 says.

@jaysoo
Copy link
Contributor Author

jaysoo commented Jun 16, 2017

@doctyper Cool. Yeah it might not be a common use-case, but interested to see what you folks think.

@jaysoo
Copy link
Contributor Author

jaysoo commented Jun 20, 2017

@cwelch5 Any thoughts? I can open a PR with a deferred: boolean prop and some tests around it. Let me know if this is okay.

@cwelch5
Copy link
Contributor

cwelch5 commented Jun 20, 2017

@jaysoo Hi, sorry for the delay. Just want to confirm some details and try to understand the issue, because we've had a FOUC in version 3.x and we tried to fix in 5.x. Is the Helmet example from above rendered in the root component of your app? If it is, then the requestIdleCallback may have introduced an issue. BUT, if this example is contained in a subcomponent of the app, there are multiple renders of Helmet on the client side, one of which is the root render that wouldn't include your style. requestIdleCallback was supposed to address this issue and instead of adding more API, I'd like to understand how we can use the callback to correctly fix this.

Oh, and is this server-side rendered as well?

Thanks for the info.

@jaysoo
Copy link
Contributor Author

jaysoo commented Jun 20, 2017

@cwelch5 The example I provided is for FOUC on the root component.

With SSR, this isn't an issue since the <style> is already in the . This is problem is purely for when SSR is off -- at least from what I can see.

@cwelch5
Copy link
Contributor

cwelch5 commented Jun 20, 2017

@jaysoo It sounds like requestIdleCallback may have exacerbated the delay that you see. It would make sense to have some sort of defer flag then. I can see it as two ways to go, either the flag defers all the Helmet tags and they all render as immediately as possible (like < 5.x), or the flag is configured per tag. In that option, all tags would wait for the idle callback, except for those with the flag would go immediately. I think I'd lean towards option 2. What do you think? And thanks for the PR!

@jaysoo
Copy link
Contributor Author

jaysoo commented Jun 20, 2017

@cwelch5 I agree, it would be better to have it configured per tag instead of the whole thing. I'll see if I can get a PR in today or tomorrow. Thanks!

@jaysoo
Copy link
Contributor Author

jaysoo commented Jun 22, 2017

@cwelch5 I was able to get something working in this PR #297. I'd be glad to hear any feedback.

jaysoo pushed a commit to jaysoo/react-helmet that referenced this issue Jun 22, 2017
cwelch5 added a commit that referenced this issue Jul 26, 2017
feat: Adds support for synchronously updated tags (Closes #291)
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

No branches or pull requests

3 participants