Elements with display: none are considered above the fold

[rungta]

I’m facing an issue where elements that have display: none applied on them are considered above the fold and in the critical path, resulting in the inclusion of all their CSS styles. For example, I have a <button class="example"> in my footer which is hidden by default and may optionally be made visible by JavaScript. This is resulting in CSS for button and .example etc. to be included.

Looked around the code and I suspect this happens because getBoundingClientRect returns the top value as 0 for hidden elements, falsely giving the impression that they are visible above the fold.

https://github.com/pocketjoso/penthouse/blob/e61253edfe91262f14b173955e0196b4411d4c4d/src/browser-sandbox/pruneNonCriticalSelectors.js#L25

[pocketjoso]

This check is indeed why elements with display:none get included. Is there any particular reason why this element needs to be in the HTML, if it's not used until JS (optionally) activates it? Unless there is something else going on, it seems better to remove it from the HTML and create it via JS instead, as this would reduce the size of your html. I'm asking because penthouse would have to do more work if we should avoid including display: none element styles in the critical css, so I want to make sure there are good use cases for this first.

[rungta]

Hi @pocketjoso, thanks for responding. Here are some of my use cases for having HTML elements that are not visible during the initial render:

• Content in accordions or tabs where all the non-active tabs are hidden
• Selectively hiding or showing elements based on feature detection
• Enabling bits of UI once JS kicks-in (eg carousel left/right buttons, Ajax form submit button), etc.

While some websites do use JS for rendering content (React, Vue etc.), I believe a lot of websites also simply follow the separation of concerns principle by keeping all content in the HTML and using JS / CSS for tweaking availability by adding/removing classes, styles or attributes. We certainly do that.

Unless there is something else going on, it seems better to remove it from the HTML and create it via JS instead, as this would reduce the size of your html.

This is a valid suggestion. However, given that Penthouse is a library for general usage and does not come with prescriptions about markup style, it might be worth accommodating for the reality that web pages do contain elements with display:none.

I'm asking because penthouse would have to do more work if we should avoid including display: none element styles in the critical css

How about checking for a { width: 0, height: 0 } match in the return value of element.getBoundingClientRect()? Would that increase the work by a lot?

[pocketjoso]

Thanks for the follow up and the use cases you presented.

How about checking for a { width: 0, height: 0 } match in the return value of element.getBoundingClientRect()? Would that increase the work by a lot?

The problem is that we cannot discern whether an element is in view or not just by the fact that the bbox call yields top: 0, width: 0, height: 0. If the element is actually in view and we remove its styles from the critical css it will break the page, as the element will show, and without any styles.

Penthouse would have to unset some applied styles, or start using IntersectionObserver instead. In either case this would require some work and decent amount of testing to ensure it doesn't slow down the critical css generation too much - given that Penthouse iterates over a very high number of elements.

Given the amount of work for the relatively small return (pruning styles for non critical display:none elements), I don't think I will get around to this anytime soon. If you're interested though, feel free to open a PR!

[michaelchart]

We would also benefit from display:none elements being removed. In our scenario, we've got a 'mega nav' where the navigation is all loaded in the initial page load, and is beneficial for SEO. The nav is display:none at the start until the burger icon is clicked,, but there are a lot of classes and CSS that could be removed from the critical CSS.

For now, we can just add lots of ignore rules, or change our CSS from being display;none to something like position:absolute; top:99999px; visibility:hidden; etc

[pocketjoso]

Thanks for the info. I will look into it eventually, but not likely in the short term.