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

Update critical request chains description to reflect inclusion of non-render-blocking requests #2324

Closed
fchristant opened this issue May 21, 2017 · 9 comments · Fixed by #4009

Comments

@fchristant
Copy link

Consider this more of a question or discussion rather than an issue. In some cases, I find this text in the Critical Request chain section to be somewhat misleading:

The Critical Request Chains below show you what resources are required for first render of this page.

Imagine a page with a CSS reference in the head, and all script references just before the end body tag. Lighthouse will report both the CSS reference and all scripts to be part of the critical request chain. Even if none of those scripts are required for actual rendering.

So technically, those scripts are not required for a first render of the page. They may be required, however, to finish DOM construction, but does that really block rendering of the first paint? It was my understanding that scripts put at the end of the body tag do not block progressive rendering of a page?

Another instance where I was confused is typekit's font service. By default their font loading is async, yet every request to their network is considered render-blocking by Lighthouse. Which seems incorrect, since the whole point of async font loading is to be non render-blocking?

I'm just trying to solidify my understanding of what is considered "critical" or render-blocking, because right now it seems that resources that do not block a first paint, are still marked as such.

@patrickhulce
Copy link
Collaborator

@fchristant you are correct that such scripts are not render blocking and we should update our text to reflect the set of resources we are highlighting there. Currently, it's the set of network requests (and their parents) that Chrome issued with a relatively "high priority" which typically includes stylesheets, fonts, and non-async scripts. Thanks for bringing up this inconsistency! 👍

@wardpeet
Copy link
Collaborator

Just to be clear we don't affect your score at all with this audit we only show this in the report as informative so you shouldn't worry about it too much but we are happy to get pointers on how to improve this audit.

@fchristant
Copy link
Author

Thanks @patrickhulce now I understand what is really meant in that report: high priority instead of actually being render-blocking, an important difference.

@wardpeet If that is the conclusion, the audit text should reflect that imho. I admit that I am micro managing the statements that Lighthouse outputs, but I do that for good reasons. I believe Lighthouse rapidly is going to be the go-to tool for web audits. This means millions of developers (and other stakeholders) are going to be looking at the reports and basing action on it. The precise language used in the report can steer them towards useful or completely needless action.

Since today's performance tuning almost completely focuses on rendering performance, I think this section is important.

@wardpeet
Copy link
Collaborator

@fchristant thanks for the feedback! We are aware on that and trying our best to improve the report as best as we can.

Can I ask what the biggest problem you're having troubles with not seeing it as informative. We don't show a score in front of it but a big blue icon
image

Perhaps move it to a collapsed section as we do with the passed audits?

@fchristant
Copy link
Author

Thanks for being open to feedback, @wardpeet
Fair point that it is a diagnostic, and not a scoring element. Still, a wrong or misleading diagnostic can lead to wrong conclusions and incorrect follow-up actions. So here goes:

My first point of critique is on this piece of text:

The Critical Request Chains below show you what resources are required for first render of this page.

As we just established, this statement is simply false. The overview shows high priority requests, including ones that are not blocking rendering. Therefore the title of this section is incorrect. Either the title should say this is about high priority requests, or the logic should change to only show requests that really are render-blocking a first paint.

My second point is linked to this, and concerns the advise that is associated with the section, as well as the length of the critical request chain which is reported. If the list this is based on is incorrect (since it includes non-blocking resources), how much confidence should I put in the advise and longest chain report being accurate?

The difference in practice can be huge. To illustrate the example I mentioned earlier: it has a single render-blocking resource (the CSS file) yet this section of the report shows a whopping 16. Where 15 of the requests are not render-blocking. Not exactly a subtle difference :)

@wardpeet
Copy link
Collaborator

I hear ya! I think it's wise for us to improve the naming and improve the audit itself!

Just in case they want a say in this.
cc @paulirish @patrickhulce @ebidel @brendankenny

@mflodin
Copy link

mflodin commented Jun 22, 2017

Not sure if this should be part of this issue or a separate one, but I found the sizes listed under Critical Request chain very confusing. They are all listed as having the same size, and doesn't seem to match any of the actual sizes.
screen shot 2017-06-22 at 10 35 29
screen shot 2017-06-22 at 11 01 41

@patrickhulce
Copy link
Collaborator

hey @mflodin thanks for the report! that's definitely a separate bug :) mind filing a new issue to track it?

@mflodin
Copy link

mflodin commented Jun 26, 2017

@patrickhulce Done! #2597

@patrickhulce patrickhulce changed the title What makes a critical request critical? Update critical request chains description to reflect inclusion of non-render-blocking requests Oct 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants