vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
619 stars 167 forks source link

Auto load all style sheets under custom theme folder #12907

Open tarekoraby opened 2 years ago

tarekoraby commented 2 years ago

Describe your motivation

The custom theming mechanism using @Theme automatically loads a styles.css stylesheet under the theme sub-folder. Most often, however, one needs to organize the styles into several other stylesheets, which would then have to be manually imported from within styles.css.

Describe the solution you'd like

Automatically import all the stylesheets that exist under the custom theme folder.

Artur- commented 2 years ago

See #9741

tarekoraby commented 2 years ago

I don't get the justification for using one "magic css file" that is mentioned in #9741:

The benefit of this is that the developer gets to have explicit control over what is loaded and in which order by having to import the css themselves to the file

If everything under the custom theme folder is auto-loaded (as I'm suggesting here), and the developer really wants to choose what gets imported and in which order, then they are still free to use the good old @CssImport and UI.getPage().addStyleSheet().

Why would we make the custom theming mechanism more cumbersome for what's IMO the typical use case in which I would want everything under my custom theme folder imported?

knoobie commented 2 years ago

The difference comes from the import order. Now I (as developer) can say that I want to import b.css before a.css inside the styles.css with the automatic including of all files it's always a.css before b.css or something else I can't change.

tarekoraby commented 2 years ago

Thanks, @knoobie! But how often does the typical Flow user find it necessary to control the ordering of imports? Is this such a common use case to justify imposing the inconvenience of manual imports on all Flow users? Or would we be maximizing the happiness of the many if the few users with the edge use-case are asked to use something like `UI.getPage().addStyleSheet()``?

knoobie commented 2 years ago

I see where your concern comes from; but I haven't personally seen many(any?) people complaining about the current way of it.

The difference with UI.getPage().addStyleSheet would be, that those css files aren't bundled/minimized like all other sheets that are now part of the merged styles.css.

Additionally, the current way is already battle proven and known by the community since V8 and earlier and somebody could even argue, that automatic import could be a security problem - see https://cheatsheetseries.owasp.org/cheatsheets/Securing_Cascading_Style_Sheets_Cheat_Sheet.html for more details why it's bad. (now the developer can add an admin.css file inside the theme folder and import it if needed)

tarekoraby commented 2 years ago

To my mind, manually importing style sheets is no more secure than automatically importing them. As far as I can tell, it's only the ability to control the importing order that speaks in favor of the manual approach, and that is not typically needed in Flow apps IMO.

Still, Flow could ideally do both: respect the importing order of style sheets if any is found inside styles.css. Otherwise, auto import everything.

knoobie commented 2 years ago

To my mind, manually importing style sheets is no more secure than automatically importing them.

Manually importing style sheets with UI.getPage().addStyleSheet is more secure than @import inside the styles.css or automatic importing / bundling of all style sheets inside /themes/xxx/ - it's just another argument I found while writing my last response, it wasn't really related to the import order inside styles.css.

I wanted to mention it, because I've seen client applications where even the access to specific theme-subfolder was access controlled (e.g. themes/xxx/admin/admin.css was only accessible for users with the admin role) and adding the style sheet only if needed.

Don't get me wrong; I'm not totally against this - but I would prefer some way of disabling auto load / get the old behaviour back.