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

Mismatch in computed styles after critical #541

Open
espoal opened this issue Aug 5, 2022 · 6 comments
Open

Mismatch in computed styles after critical #541

espoal opened this issue Aug 5, 2022 · 6 comments

Comments

@espoal
Copy link

espoal commented Aug 5, 2022

After using critical, there's a mismatch in the computed styles:

Screenshot from 2022-08-05 16-31-36

Without critical, and with critical

If you patiently inspect the styles, you see there's a mismatch in the computed styles, especially border-radius

I tried to fix the issue by using forceInclude to no avail. It seems that when splitting the styles, the uncritical.css gets precedence over the inlined styles.

Btw: Amazing project. I took a commercial dashboard, whose load time was around 10 second, and I brought it to 2 seconds with a lot of work (ssr, lazy load, minification...) and then to 0.8 seconds with critical. This npm had by far the most impact, with the least amount of work, it should be advertised everywhere.

@bezoerb
Copy link
Collaborator

bezoerb commented Aug 10, 2022

Hey @espoal, thanks for creating this issue. Can you provide a "not working" example? I can't spot any differences in the provided links

@espoal
Copy link
Author

espoal commented Aug 11, 2022

Hey @bezoerb thank you for your quick reply.

Sorry I was playing with the repo and inadvertently deleted the route in firebase. Try again, and try ctrl+shift+r, because I'm also testing a service worker and I messed up a bit the caching.

And just to make it even more clear: the calendar SVG in the datepicker overlaps the date after using critical.
Untitled drawing

If you want to look at the output is here, and if you want to look how I generated it it's here.

If you want to try locally just fix the hard paths (/home/mamluk/....) in libs/critical/index.mjs, I was too lazy to use dynamic paths in a test :D

@bezoerb
Copy link
Collaborator

bezoerb commented Aug 11, 2022

I tried to debug on my local machine but i'm missing the firebase token ;)
Nevertheless, i think the problem is caused by the extract: true setting.
This leads to the following:

<style>
...
.pl-9 {
    padding-left: 2.25rem;
}
...
</style>
<link rel="stylesheet" href="/uncritical.css" media="all" onload="this.media='all'">

uncritical.css contains

[type=text] {
  padding-left: 0.75rem;
}

but the pl-9 class is removed because of extract which then leads to your css beeing in the wrong order. Classes and attribute selectors have the same specificity so the latter wins and the input gets a padding-left value of 0.75rem.

I think it's currently not possible to place the <link rel="stylesheet" href="/uncritical.css" media="all" onload="this.media='all'"> before the <style> tag but doing so would most likely result in other styles being messed up.

You can try playing around with the different inlining strategies provided by inline-critical. You can change the strategy by passing a config object for inline instead of true|false but i guess the results will stay wrong.

critical.generate({
  ...
  inline: {
    strategy: 'default' 
  },
  ...

For now i would suggest removing the extract option and trying the 'default' strategy even if i'm wondering why the [type=text] styles are missing in the critical css. In that case they should be removed from the uncritical.css, too.
Maybe @pocketjoso has more insights on this :)

@bezoerb
Copy link
Collaborator

bezoerb commented Aug 30, 2022

@espoal: Any updates on this?

@espoal
Copy link
Author

espoal commented Sep 9, 2022

@bezoerb sorry I was stuck with some backend topics, I will try to answer before monday.

@espoal
Copy link
Author

espoal commented Sep 14, 2022

@bezoerb I'm sorry for the super late reply, and thank you very much for keeping this alive, I really appreciate it.

Unfortunately time is scarce, and my attention had to be diverted on some backend topics.

About the firebase issue: in theory no token is needed. Have you installed firebase emulator? what errors exactly should show?
My repo should be an open source tutorial so my coworkers (and others) can follow it for new projects. If any token is asked for, then there must be a bug somewhere.

Unfortunately I tried all your tips without much success. In the end I decided that service workers are good enough: even though they are much much more complex, they provide similar benefits in term of loading. If we could make this work though, that would be amazing and I could backport it in my coworkers projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants