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.

Sign up for GitHub

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

Jump to bottom

Head childs doesn't have to be evaluated in Maximum Child Elements #6806

Closed
marcmrf opened this issue Dec 14, 2018 · 8 comments
Closed

Head childs doesn't have to be evaluated in Maximum Child Elements #6806

marcmrf opened this issue Dec 14, 2018 · 8 comments
Labels
bug good first issue P2

Comments

@marcmrf
Copy link

marcmrf commented Dec 14, 2018

Provide the steps to reproduce

  1. Run LH on https://andro4all.com/
  2. Check "Avoid an excessive DOM size" point on Diagnostics.

What is the current behavior?

Right now is failing due to Maximum Child Elements, 79, which is bigger than the 60 recommended. The parent element is the head.

What is the expected behavior?

Most of the elements placed in the head are metas, pre-connects, fetch's... As the number of the elements in the head doesn't affect to rendering performance, they have to be omitted in this check.

Environment Information

  • Affected Channels: Devtools
  • Lighthouse version: Lighthouse 4.0.0-beta
  • Operating System: macOS
@patrickhulce
Copy link
Collaborator

A fair point thanks for filing @marcmrf!

@ebidel any reason you can remember that you included the head elements long, long ago? :)

@midzer
Copy link
Contributor

midzer commented Dec 14, 2018

So it seems we're only looking for render relevant nodes under <body>. Might be an idea to filter them by using nodeInBody(node) function located right now in font-size.js

@ebidel
Copy link
Contributor

ebidel commented Dec 14, 2018

The original goal of the audit was to raise awareness to apps that create excessive DOM trees which can put a burden on layout/style recalcs and the general advise we got from the Chrome team was < 60 nodes per parent. I'm not sure there was any reason in particular we included <head> elements, other than for completeness. Possibly call out usage of old school meta/link tags which are no longer needed.

@ebidel
Copy link
Contributor

ebidel commented Dec 14, 2018

The other thing might be to avoid loads of markup in head. Means more markup to parse on a slow mobile connections, possible blocking inline scripts and other resources, and a slower first render. But those things could be captured in other perf audits.

@paulirish
Copy link
Member

Yeah lets only include elements inside the body.

@midzer
Copy link
Contributor

midzer commented Feb 13, 2019

I would be happy to have a look on this issue if no newcomer is interested

@patrickhulce
Copy link
Collaborator

Sounds good @midzer thanks!

@midzer midzer mentioned this issue Feb 13, 2019
Merged
@exterkamp exterkamp mentioned this issue Apr 30, 2019
Closed
@brendankenny
Copy link
Member

Fixed by #7241

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug good first issue P2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@paulirish @ebidel @brendankenny @patrickhulce @midzer @marcmrf