[lumo] Overlay styles are duplicated if combo box is used together with any other overlay-based component

[jouni]

Problem

If you just use <vaadin-select>, <vaadin-date-picker> or any other component that internally uses an element based on <vaadin-overlay>, styles are as expected.

When you import and use <vaadin-combo-box> together with any of them, the styles for the overlays are duplicated. There’s no visible effect from that, but it looks ugly in Devtools, and it certainly doesn't increase performance.

The reason is that combo box imports vaadin-overlay/theme/lumo/vaadin-overlay.html which adds a theme module for vaadin-overlay which includes the lumo-overlay style module.

At the same time, other components do not import vaadin-overlay/theme/lumo/vaadin-overlay.html but instead define their own theme modules for their corresponding specialized overlay elements such as <vaadin-select-overlay> and include the lumo-menu-overlay style module which then includes the lumo-overlay style module.

As theme modules are included from the inherited component as well, the overlays end up getting the lumo-overlay style module twice.

Possible solutions

1. Do not import vaadin-overlay/theme/lumo/vaadin-overlay.html from combo-box. This only fixes the symptom, and it will break again if some other component would want to import the plain <vaadin-overlay> element and use it directly.

2. Let all components import the Lumo styled overlay (like combo box does), and remove the include="lumo-overlay ..." from the lumo-menu-overlay theme module.

3. Add logic to ThemableMixin to avoid including the same theme module multiple times. There’s a small risk of breaking something, as this could cause the priority of some selectors to change in users’ apps.

[lkraav]

What would you want to go with as the top choice?

[jouni]

Hi @lkraav!

My original choice was 1, and there were no duplicated styles initially as <vaadin-overlay> was unstyled by itself.

But then we decided that the bare <vaadin-overlay> should also have some styles by default, and we didn’t pay attention to this issue resulting from it.

Now I see 2 as the correct solution. It’s, unfortunately, the solution which requires the most code changes. Not a huge effort, but needs work across multiple repos and updating versions.

3 would be good to implement as well, but as there’s a small risk for breaking some apps, I’m not sure it’s worth it.

[lkraav]

Now I see 2 as the correct solution. It’s, unfortunately, the solution which requires the most code changes. Not a huge effort, but needs work across multiple repos and updating versions.

Thanks for your thoughts.

So for a new project, it would make sense to fork relevant component repos and just do this change while upstream is working on integrating it across all?

[jouni]

Sorry for the long delay in responding.

I’m not sure I understood what you meant (my git terminology is a bit lacking). But I think branches for the relevant components would be the way to go. I would let the core dev team handle the integration.

[jouni]

@tomivirkki, do you have any instructions/suggestions on how @lkraav could contribute here?

[lkraav]

@tomivirkki your thoughts? Let me know if my question is unclear in some way.

[tomivirkki]

Not sure if we can do much about 3. Themable mixin already prevents duplicate includes for style modules but I guess the issue here is about the includes that occur within the style modules themselves.

So that leaves us with option 2, unless we come up with something else

[lkraav]

@tomivirkki is this a problem inherent to HTML imports? Would we be able to better control this if everything was ES modules?