Option 'forceExclude'

[futureweb]

Hey there,
would it be possible to implement a 'forceExclude' Option just like 'forceInclude'?
Just noticed a few Styles I don't really need on initial Rendering as they aren't visible.
Guess this would make a good addition to this already pretty cool Project?! ;-)
thx, bye from Austria
Andreas Schnederle-Wagner

[pocketjoso]

This sounds like a bug. Something that is not visible in the critical viewport should by definition not be part of the critical css. Do you have a concrete example with a public url and a class you are not expecting to be included, that I can debug? Le mar. 6 mars 2018 10:20, Andreas Schnederle-Wagner < [email protected]> a écrit :

…

Hey there, would it be possible to implement a 'forceExclude' Option just like 'forceInclude'? Just noticed a few Classes I don't really need on initial Rendering as they aren't visible. Guess this would make a good addition to this already pretty cool Project?! ;-) thx, bye from Austria Andreas Schnederle-Wagner — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#243>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG3BANqO4x_guDGCS3JgzRnu-BXxgRenks5tblT3gaJpZM4SeW6N> .

[futureweb]

example: https://www.futureweb.at/?testCriticalCSS=1 (only critical css is loaded here)
I don't need all those blueimp Child Styles within critical CSS.

[pocketjoso]

The reason they are showing up is because the blueimp-gallery element is in the critical viewport (position absolute/fixed, top,left,bottom,right: 0) (a version of the problem in the readme: https://github.com/pocketjoso/penthouse#early-dom-content-moved-out-of-critical-viewport-via-css). I understand your situation: you set display: none; opacity: 0 on this element, so you cannot see it, hence you think the styles should not be considered critical. Penthouse just cannot automatically make such inferences.

You can make this style disappear from the critical css by avoiding setting top: 0 until you start showing the gallery (make sure it's outside of the critical viewport).

So in the end this is and edge case based on your styles. Let me know if you test my solution and how it works for you.

I don't really want to add in a forceExclude feature to support - I prefer trying to explain why this happens and how to fix it. I'll look into updating the readme to cover this case (element considered critical even though visually hidden).

[futureweb]

Hey Jonas,
sorry for the late reply - was quite busy last week! ;-)

The Problem here is that those Elements are set by a 3rd-Party Module (https://github.com/blueimp/Gallery) and not by me - and one can imagine that I can't edit the CSS delivered with those Modules (... welcome to upgrade hell ...), and I guess that's not the only 3rd-Party-Module which does so?! :-/

I can understand why you try to avoid adding a forceExclude Option - but IMHO for such edge cases it could be helpful to give penthouse some "hints" what to exclude to get smaller critical file size ...
(guess having such an Option at Hand when don't you need it wouldn't harm anyone - instead of not having such a Option when needed like in this case ... ;-)

Maybe you give the Option a chance? ;-)

bye from sunny Austria
Andreas

[pocketjoso]

Okay I'll consider it.

While it's not there, I imagine you could still do something like:

• either not initilize the gallery until you after page load, possibly even leaving the blueimp gallery markup out of the initial html
• or override the default styles, adding ~ position: relative to the gallery for default styles, then adding back position: fixed when it's showing.

Either of these would take the gallery element out of the critical viewport and remove it from the critical css.

I have understood your point though that this is a bit more work for you and that you are dependent on your library so not in full control. I will research the use case a bit more and possibly add in forceExclude or ~~ ignore to next release.

[futureweb]

Hey Jonas,

sounds good - thx! ;-)

As for your suggestions - leaving out the Gallery Markup (or modify in any other way) isn't that easy as it's automatically added by our SAAS CMS to every Page (including JS code to call it as simple as adding a class to an image and so on ...) ... would affect thousands of pages at once ... would need really ugly exceptions ... "if above fold optimizations and this and that is activated then don't insert it into page footer ..."

For me a forceExclude Option is pretty much the same as the forceInclude Option - I - as a coder - can override the behaviour of a Programm if needed / if I think I know it better than the Program ... ;-)

bye out of the Mountains of Austria
Andreas

[pocketjoso]

Okay, I'm for adding it as an option in the end. I will do it when I have time, or if someone you or someone else wants to do it, it should be a relatively easy change to make in a PR.

[Odisseya]

Hi, excluding option will be much usefull to handy remove rules from display: none elements below the fold. For instanse, rules for lazyloading images:

.no-js img[data-src],
.no-js source[data-srcset] {
  display: none;
}

as a result, a lot of unnecessary code with images styles in critical CSS. @pocketjoso, thanks for your work! Please consider about this.

[pocketjoso]

@Odisseya These rules should only be kept in the critical css if they apply for elements that show above the fold, i.e. in the critical viewport. If that's not the case, can you give me a (public) URL where you see this unwanted behavior so I can check what's happening?

[Odisseya]

@pocketjoso There are few instances, with 2 versions (2 URLs) in each of them:

1. With critical css — includes unwanted behavior.
2. Pure — without critical css.

Instance#1
URL: http://odisseya.github.io/criticalcss/1/
Contains below the fold elements with «display:none» declaration, so such elements with all their css rules were kept in critical css (unwanted behavior). If you remove the «display:none» declaration — elemens will not be included in critical css.

Instance#2
URL: http://odisseya.github.io/criticalcss/2/
The lazyloading images with «display:none» (from whole page) are keeping in the critical css.

[pocketjoso]

Okay I understand the case now, thanks for the clear examples.

It's not safe for Penthouse to remove any display: none rules because Penthouse does not know if such a rule removal would cause the element to appear (unstyled) in the critical viewport. I missed explaining this earlier in the thread.

Thus there are situations where end users can further trim there critical css if they have specific knowledge for their site(s) about display: none usage. So yes this would be a good use case for forceExclude.

So I repeat what I said before - I am for adding it, but to be transparent it's not at the top of my own TODO list. If anyone else wants to create a PR for it, that would make it appear in Penthouse sooner. The logic should follow the existing forceInclude property quite closely.

[futureweb]

Well ... I never touched Node before in my life ... but I think/hope/guess the PR should work ... more or less ... ;-)
PR: #289

Unfortunately I was not able to test it as Test Env always failed with: "Cannot find module 'source-map' from 'source-map-support.js'" (Guess I'm affected by jestjs/jest#8291)

[pocketjoso]

Available now in [email protected]:
https://github.com/pocketjoso/penthouse/releases/tag/v2.2.0