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 flicker in. #147

Open
abhiaiyer91 opened this issue Oct 3, 2016 · 26 comments
Open

Styles flicker in. #147

abhiaiyer91 opened this issue Oct 3, 2016 · 26 comments

Comments

@abhiaiyer91
Copy link

Hey there,

Loving aphrodite so far. I keep noticing that when my components render on the screen there is a small flicker. The component is first unstyled then styled. I am not using SSR.

Is there a way to preload css functions? Or am I just doing something wrong?

@xymostech
Copy link
Contributor

Hello there @abhiaiyer91!

Can you give me a little bit more information about the environment you're seeing the flicker in? What browser you're testing in, are you using React/some other framework, etc? We don't see flicker on the Khan Academy site.

@abhiaiyer91
Copy link
Author

Hey there @xymostech!

I'm seeing this during development/production in Google Chrome. I'm using React and MeteorJS. I'm wondering whats going on!!

@abhiaiyer91
Copy link
Author

Let me see if I can get a video!

@abhiaiyer91
Copy link
Author

@xymostech heres a video!

http://screencast.com/t/rbFxm4qNv

@jlfwong
Copy link
Collaborator

jlfwong commented Oct 6, 2016

We have yet to be able to reproduce this in code --if there's any way you can find a minimal reproducible case and put it up somewhere that we can run, it would be incredibly helpful in diagnosing this issue.

This has been reported a few times now, but nobody's been able to produce code that consistently gives this behavior in any environment.

If you can give an example of this happening, I'd love to figure out the root cause and fix it.

@zgotsch
Copy link
Contributor

zgotsch commented Oct 8, 2016

We have seen something similar at Spring. I think one of my coworkers was looking into it. I'll see if he got anywhere with his investigation.

@mattkrick
Copy link

@zgotsch @abhiaiyer91 do you guys use aphrodite for fonts by chance? Here's what I found:

  • Every time aphrodite adds something to the style tag, the entire tag must be reparsed + re-evaluated
  • If you use something like font-family, then the @font-face is re-evaluated & the sources are refetched on every update (thankfully from the cache). This can be confirmed by looking at your Networks tab in the Chrome Debugger & counting the font files. Aphrodite protects against injecting it twice, but it can't protect from the browser reparsing it.
  • reloading the font (if it's not base64-inlined) causes a text flicker every time you add a new style, which means you'll see a lot of flickers for new actions, but as state permutations get explored, fewer injections occur & you see it flicker less & less.

Then again, I could be doing something wrong, too 😄

The solution is there in #83 (which compliments #95 quite nicely).

@abhiaiyer91
Copy link
Author

Hey @mattkrick thanks for the info! Super interesting. We are def using font family via aphrodite. Gonna look into this a bit more.

@abhiaiyer91
Copy link
Author

@mattkrick this is indeed my issue.

@mattkrick
Copy link

mattkrick commented Oct 9, 2016

@abhiaiyer91 I'm working on a PR right now to fix this, although it's closer to a rewrite & has breaking changes, so I'm not sure it'll get merged. Here's the approach:

  • On the server, business as usual
  • On the client, user insertRule instead of mutating the style tag innerHTML.
  • insertRule only accepts rules that the browser understands, so we can either use a try/catch block (performance penalty) or we can only generate the styles that the browser needs. The latter is the right way.
  • Use window.getComputedStyle(document.documentElement, '') to figure out what properties the browser supports. & prefix accordingly. This is faster than prefixAll and now we don't need to send inline-styles-prefixer to the client. that's a ~3k savings.
  • Now that we're super performant, we can hit the DOM synchronously, which means styles are predictable (still benchmarking this, but at a glance, using insertRule synchronously is way faster than appending stuff to a style tag async). This would solve Styles are injected after componentDidUpdate. #76

@jlfwong
Copy link
Collaborator

jlfwong commented Oct 9, 2016

Glad we've perhaps narrowed down the issue!

A minimal reproducible test case would still be much appreciated, because then @xymostech and I can perhaps take a look too

@mattkrick
Copy link

@jlfwong here's the easiest way to see it:

  • open chrome dev console >> network tab >> Font
  • https://action-staging.parabol.co
  • log in: username: graphql@prbl.co, password: 123123
  • look at all them fonts!
  • hit refresh
  • watch the text flicker in
  • put your mouse over the card in the done column
  • watch text flicker
  • click the card to edit it
  • watch text flicker

code sauce: https://github.com/parabolinc/action

Minimal proof that the problem is what I say it is:

  • don't save the styleTag, instead, for every flush, inject the buffer into a new styleTag

@xymostech
Copy link
Contributor

xymostech commented Oct 10, 2016

Use window.getComputedStyle(document.documentElement, '') to figure out what properties the browser supports. & prefix accordingly. This is faster than prefixAll and now we don't need to send inline-styles-prefixer to the client. that's a ~3k savings.

@mattkrick This doesn't work on the server side. The reason we prefix for all browsers is so we can server-side render styles without knowing what prefixes the client need.

EDIT: Just kidding I didn't read your comment. Carry on!

@jlfwong
Copy link
Collaborator

jlfwong commented Oct 10, 2016

Yesss, thank you @mattkrick! Glad to have a reproducible test case. It seems like this shouldn't be too hard to get to a much smaller test case, but knowing that this problem seems to be font-specific is super helpful.

Based on the descriptions here, I think we can probably fix this with a less heavy handed approach: we should just put @font-face declarations in a different <style> tag, and I suspect that'll fix it if everyones' descriptions in this thread are correct.

@abhiaiyer91
Copy link
Author

abhiaiyer91 commented Oct 10, 2016

@mattkrick So after doing some refactoring, my problem still exists but its not as bad as it was. I removed aphrodite refs in regards to font family, and the flicker is not as bad as it was. The answer is indeed in your plan above.

@mattkrick
Copy link

@abhiaiyer91 true true, while fonts are by far the most expensive thing to re-evaluate, reparsing and re-evaluating the style tag for every new style isn't a sustainable path for a large app.

I like @jlfwong's idea of 2 style tags, but once the primary style tag grows large enough, we're back where we started. IE10+ support ~4000 style tags IIRC, but keeping em all in 1 sounds a little cleaner IMO.

@chendo
Copy link

chendo commented Oct 11, 2016

Just confirmed that our flicking issue is also due to loading in a font-face with Aphrodite. Moving the fonts into a normal stylesheet and referencing them in Aphrodite has no flickering.

@mattkrick Much thanks for figuring this out!

@mattkrick
Copy link

mattkrick commented Oct 11, 2016

@chendo a fix is coming soon! If you can, please go ahead & add this to your dependencies in package.json:
"aphrodite": "https://github.com/mattkrick/aphrodite/tarball/e6dfa5559cbc6adbb6b7c8b4c432c8024d14738c"
Then run npm i.
This is a pretty large change to the codebase so the more folks that can poke it with a stick the better!

@chendo
Copy link

chendo commented Oct 11, 2016

@mattkrick Tried to do that but running into the following:

ERROR in ./~/react-with-styles-interface-aphrodite/lib/aphroditeInterface.js
Module not found: Error: Cannot resolve module 'aphrodite/lib/inject' in /Users/chendo/the_app/node_modules/react-with-styles-interface-aphrodite/lib
 @ ./~/react-with-styles-interface-aphrodite/lib/aphroditeInterface.js 7:14-45

ERROR in ./~/react-with-styles-interface-aphrodite/lib/aphroditeInterface.js
Module not found: Error: Cannot resolve module 'aphrodite' in /Users/chendo/the_app/node_modules/react-with-styles-interface-aphrodite/lib
 @ ./~/react-with-styles-interface-aphrodite/lib/aphroditeInterface.js 5:17-37

We need to ship super soon so we'll probably go with pulling out the font-face stuff into a separate stylesheet for now.

@mattkrick
Copy link

@chendo ahhh yeah, you'll wanna build it after install. (cd node_modules/aphrodite && npm run build:main)

@chendo
Copy link

chendo commented Oct 13, 2016

@mattkrick Ahhh, I'll try that when I have the time next.

I just tested the refactored setup on an iPhone 5S and I'm still getting a slight flicker even without @font-face in Aphrodite.

@mattkrick
Copy link

@chendo yup, that's expected. The problem won't go away unless you don't reparse/re-evaluate every rule on every update. Fixed in #157

@niksajanjic
Copy link

niksajanjic commented Nov 3, 2016

There's something similar happening in case of transition CSS rule:

{
  ...someOtherRules,
  transition: 'all 0.3s',
}

Lets say all my buttons have transition because I want to slowly change styling whether it's hovered, active, disabled or normal. The thing is that Aphrodite doesn't style button immediately, but instead it has transition. This is not seen on initial load of a page which is good, but it does happen when I go to another route in SPA application using react-router. When I switch to another route my buttons all have transition only for that first time. If I move from the page and get back they don't get transitioned again, but that is because it's styles are already included in Aphrodite style tag from the previous time I was on that page.

This is unwanted behavior. So if there is a way to force Aphrodite to calculate all possible styles in my application (I'm using webpack/babel) and insert all styles at once on my initial load I'm guessing that would clear this problem.

I haven't yet tried animations but I have a feeling similar thing will happen.

@acomito
Copy link

acomito commented Nov 8, 2016

I'm also getting some flickering with client-side routing + meteor and aphrodite. I have my fonts declared in a css file and use them in my js via fontFamily.

@tomduncalf
Copy link

Just to add, I'm seeing flickering/flashes of unstyled content in Safari 9 - only with some styles, not all of them, but it's pretty noticeable. Not seen it in any other browsers but as Safari 9 is reasonably important for the site, I may have to rip out Aphrodite and replace with something else :(

@whirmill
Copy link

There's just one thing that I can't figure out and it's driving me crazy:
In my react application i've declared two different themes that uses two different font-faces:
The first one describes a paid and self-hosted font;
The second one describes a free font hosted by google fonts.

Using the theme with the self-hosted assets, a re-fetch is performed on the first update and the application flickers,
Using the theme with the google fonts hosted assets, no re-fetch is performed and the application doesn't flickr.

I really can't understand why they behave so differently.

Here's my font faces configuration:

export default {
  fakt: {
    light: {
      fontFamily: 'Fakt Soft Pro',
      src: `local(\'Fakt Soft Pro Lt\'), local(\'FaktSoftPro-Light\'), url('${assetsPath}/fonts/FaktSoftPro-Blond.eot?#iefix') format('embedded-opentype'), url('${assetsPath}/fonts/FaktSoftPro-Blond.woff') format('woff'), url('${assetsPath}/fonts/FaktSoftPro-Blond.ttf') format('truetype')`,
      fontWeight: '300',
      fontStyle: 'normal'
    },
    normal: {
      fontFamily: 'Fakt Soft Pro',
      src: `local(\'Fakt Soft Pro Bnd\'), local(\'FaktSoftPro-Blond\'), url('${assetsPath}/fonts/FaktSoftPro-Normal.eot?#iefix') format('embedded-opentype'), url('${assetsPath}/fonts/FaktSoftPro-Normal.woff') format('woff'), url('${assetsPath}/fonts/FaktSoftPro-Normal.ttf') format('truetype')`,
      fontWeight: '400',
      fontStyle: 'normal'
    },
    semibold: {
      fontFamily: 'Fakt Soft Pro',
      src: `local(\'Fakt Soft Pro SmBd\'), local(\'FaktSoftPro-SemiBold\'), url('${assetsPath}/fonts/FaktSoftPro-Medium.eot?#iefix') format('embedded-opentype'), url('${assetsPath}/fonts/FaktSoftPro-Medium.woff') format('woff'), url('${assetsPath}/fonts/FaktSoftPro-Medium.ttf') format('truetype')`,
      fontWeight: '600',
      fontStyle: 'normal'
    },
    bold: {
      fontFamily: 'Fakt Soft Pro',
      src: `local(\'Fakt Soft Pro Bd\'), local(\'FaktSoftPro-Bold\'), url('${assetsPath}/fonts/FaktSoftPro-SemiBold.eot?#iefix') format('embedded-opentype'), url('${assetsPath}/fonts/FaktSoftPro-SemiBold.woff') format('woff'), url('${assetsPath}/fonts/FaktSoftPro-SemiBold.ttf') format('truetype')`,
      fontWeight: '800',
      fontStyle: 'normal'
    }
  },
  montserrat: {
    light: {
      fontFamily: 'Montserrat',
      src: 'local(\'Montserrat Regular\'), local(\'Montserrat-Regular\'), url(https://fonts.gstatic.com/s/montserrat/v10/zhcz-_WihjSQC0oHJ9TCYPk_vArhqVIZ0nv9q090hN8.woff2) format(\'woff2\')',
      fontWeight: '400',
      fontStyle: 'normal'
    },
    normal: {
      fontFamily: 'Montserrat',
      src: 'local(\'Montserrat Regular\'), local(\'Montserrat-Regular\'), url(https://fonts.gstatic.com/s/montserrat/v10/zhcz-_WihjSQC0oHJ9TCYPk_vArhqVIZ0nv9q090hN8.woff2) format(\'woff2\')',
      fontWeight: '400',
      fontStyle: 'normal'
    },
    semibold: {
      fontFamily: 'Montserrat',
      src: 'local(\'Montserrat SemiBold\'), local(\'Montserrat-SemiBold\'), url(https://fonts.gstatic.com/s/montserrat/v10/q2OIMsAtXEkOulLQVdSl06VlZKEoJGujTpfWnQT9bUY.woff2) format(\'woff2\')',
      fontWeight: '600',
      fontStyle: 'normal'
    },
    bold: {
      fontFamily: 'Montserrat',
      src: 'local(\'Montserrat SemiBold\'), local(\'Montserrat-SemiBold\'), url(https://fonts.gstatic.com/s/montserrat/v10/q2OIMsAtXEkOulLQVdSl06VlZKEoJGujTpfWnQT9bUY.woff2) format(\'woff2\')',
      fontWeight: '600',
      fontStyle: 'normal'
    }
  }
}

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 a pull request may close this issue.

10 participants