openwrt / luci

LuCI - OpenWrt Configuration Interface
Apache License 2.0
6.29k stars 2.51k forks source link

CSSTIDY: csstidy minifier is outdated and breaks CSS 2.1+ syntax. It is time to phase it out or replace it with a better tool. #5755

Closed okibcn closed 2 years ago

okibcn commented 2 years ago

Maintainer: @jow-

ISSUE: csstidy is a tool to compress the CSS code to optimize the size requirements of CSS files. However, it breaks features such as font faces, or any feature beyond CSS2.0. So, any other feature supported by CSS 2.1 or CSS 3.0 are destroyed by CSS tidy.

I see two options:

  1. leaving csstidy as an optional package, but removed from the default build. Compressed CSS code should actually be implemented in the theme packages instead of during the image build process.
  2. Replace this outdated tool with a new one, such as parcel-css or clean-css. Both are maintained and support all features without breaking the CSS code.

There is a recent good benchmark regarding CSS compression rate of the different CSS minifiers here: http://goalsmashers.github.io/css-minification-benchmark/

jow- commented 2 years ago

Does it break any of the officially supported themes? The suggested alternatives require Node. We need something in C/C++ or Python.

okibcn commented 2 years ago

No, it doesn't break any of the included themes, but it is breaking any potential new theme, forcing any developer to use the ancient CSS2 spec. The default application of csstidy is a problem when adding new themes. Limiting csstidy to official themes, and avoiding breaking the code of other themes using CSS 2.1, 3.0 or 4.0 could be another safe option. But I believe it is time to change the approach to these processors.

Seriously, CSStidy is very outdated. The best option is to remove the csstidy processing. The theme source should already provide the minified file. Actually, that will produce the minimum possible size using modern CSS optimization tools.

Think the other way around. Why continue using an old tool, a tool that is not as good as current tools, when there are better ways to reduce the size of the CSS files in the final image? At least, offer it as an option, but not as default.

jow- commented 2 years ago

Why continue using an old tool, a tool that is not as good as current tools, when there are better ways to reduce the size of the CSS files in the final image?

Because I didn't yet find a better alternative not requiring node.js in the toolchain.

okibcn commented 2 years ago

Well, why do we need that tool? Why not leave the CSS minified code to the theme maintainers? That could give them the freedom to choose the best possible minifier, ending up with a smaller package size than what csstidy could do. What makes that option worse than the current situation?

It is clear that CSS optimizers are most of them node.js for obvious reasons. It is very unlikely that CSStidy could catch up with the rest of the alternatives when it has been almost abandoned for 6 years. A port of any of the leading CSS minifiers to python or C/C++ is very unlikely too. Remaining with an obsolete tool that is not generating the most optimum code, and making it mandatory, is doing more harm than any other option.

As a side alternative, another option could be sending the CSS files to any of the multiple online optimizers during the build process. It would make it more flexible and compatible with CSS2.1, CSS3 and CSS4 files, and you would only require curl for that, wich is already part of the toolchain tools.

There are plenty of alternatives that are broken when added to the image builder, requiring to manually disable the infamous csstidy. There are plenty of beautiful themes beyond the OpenWrt database such as Argon, Infinity Freedom, Rosy, Neobird, Darkmatter, ifit, the beautiful jj, tano, pink, and many others. Officially limiting the creativity to CSS2, IMHO, is against the spirit of OpenWrt. And that should be the main reason to remove csstidy from the default toolchain, leaving it as an option.

jow- commented 2 years ago

Well, why do we need that tool?

To shrink the final size.

Why not leave the CSS minified code to the theme maintainers?

Because we don't want preprocessed artifacts in the source repository.

As a side alternative, another option could be sending the CSS files to any of the multiple online optimizers during the build process.

The build process has to work offline.

There are plenty of alternatives that are broken when added to the image builder

The affected themes could unset CONFIG_LUCI_CSSTIDY before including luci.mk. Example for bootstrap:

--- a/themes/luci-theme-bootstrap/Makefile
+++ b/themes/luci-theme-bootstrap/Makefile
@@ -11,6 +11,8 @@ LUCI_DEPENDS:=

 PKG_LICENSE:=Apache-2.0

+CONFIG_LUCI_CSSTIDY:=
+
 include ../../luci.mk

 # call BuildPackage - OpenWrt buildroot signature

Themes negatively affected by the minification can implement the above workaround.

jow- commented 2 years ago

See also commits 3646b0cd1f (master), 162defebe9 (22.03) and 59c838b52a (21.02) for a new, more official way to inhibit CSSTidy and other source minifications for those components that need it.

okibcn commented 2 years ago

See also commits 3646b0c (master), 162defe (22.03) and 59c838b (21.02) for a new, more official way to inhibit CSSTidy and other source minifications for those components that need it.

Thanks for the info. However, I really question the efficiency of these optimizers. There was a discussion regarding UPX binary compression and It was decided to remove it from the default toolchain since the squashfs already compress them and the benefit was minimal. I tested long ago the efficiency of upx with the micro editor. And removing upx was the right move, even a monster binary of 10MB doesn't really benefit from it.

Given this experience, I have just tested the image sizes with, and without CSStidy. These are the test configs for a device with limited storage, the Netgear R6020:

CONFIG_TARGET_ramips=y
CONFIG_TARGET_ramips_mt76x8=y
CONFIG_TARGET_ramips_mt76x8_DEVICE_netgear_r6020=y
CONFIG_PACKAGE_luci=y

and

CONFIG_TARGET_ramips=y
CONFIG_TARGET_ramips_mt76x8=y
CONFIG_TARGET_ramips_mt76x8_DEVICE_netgear_r6020=y
# CONFIG_LUCI_CSSTIDY is not set
CONFIG_PACKAGE_luci=y

This test illustrates the efficiency of using CSStidy in a file inside a squashfs that takes the image entropy way higher than what CSStidy can do. I compiled the image using the 22.03 branch and the final image size was exactly the same deactivating csstidy or not: 5899046 bytes. You can do this test yourself with the lua and js optimizer, but since the binaries optimization didn't bring any benefit, lua, js, and css minifiers will help even less.

I not only question the efficiency of the csstidy, but also the js and lua minifiers. We should really dig a little bit into this so we could simplify the toolchain. There is no point on maintaining a useless optimizer. Even more when the css optimizer is so outdated. Having a way to disable it is good, but not enough. Please, follow the same reasoning made with upx, be consistent with the decision, and remove csstidy from the default toolchain as it fails to improve the final size.

Carseason commented 1 year ago

js/css files built with vite are corrupted