Use namespaced class names for CSS classes on the host element

[laymonage]

Hey all.

I've spent a few hours debugging why we're not seeing the notifications for old versions and PR builds with the new Notification add-on. It turns out that the element is there, but Bootstrap (which our theme uses) hides it because it has the toast class:

See the opacity: 0; at the bottom right of the screenshot. That comes from Bootstrap, and it requires toast to be initialised via JS (hence the initial state has opacity: 0;). You can check here: https://sphinx-wagtail-theme--295.org.readthedocs.build/en/295/

Debugging this was quite hard because the notification immediately disappears after 5s. I had to overwrite the JS and increase the timeout to investigate this.

Is there a reason why the custom element needs to rely on CSS classes in the host element? Could you perhaps use something else, e.g. data attributes?

Or, if you want to use CSS classes, I think they should be namespaced, e.g. prefixed with rtd- so that it doesn't conflict with the theme's CSS (and can still be overridden if that's the desired goal).

Thanks!

P.s. love the addons – DocDiff in particular is really awesome.

[humitos]

Thanks for opening this issue. I understand this should not happen since the notification is a Web Component with its shadow DOM is closed. I will need to take a deeper look to understand what's going on.

Is there a reason why the custom element needs to rely on CSS classes in the host element? Could you perhaps use something else, e.g. data attributes?

@agjohnson shouldn't the web component ignore this external modification? Is it maybe because we left the shadow DOM open somehow?

[agjohnson]

I can't see the notification on this example, so can't tell you what is happening yet.

But yes, the elements we inject are inside a shadow DOM and should not inherit CSS styling from the document root DOM. We do allow variable customization of some of the CSS properties, all of those variables are prefixed with --readthedocs-....

My guess is there is something else happening here.

[laymonage]

@agjohnson Demo'd the issue in #416.

https://readthedocs-addons--416.org.readthedocs.build/

Screen.Recording.2024-10-30.at.19.28.44.mov

[humitos]

I found a potential similar situation in this site as well https://turtle-confusion-spanish-version.readthedocs.io/en/latest/

If you take a look at the flyout, you will see all its content it centered for some reason. I suspect there is some CSS code of that site that is affected the flyout addons.

(note the pretty old Read the Docs theme used -- nice old good times)

[laymonage]

@humitos FYI web components don't fully guarantee that styles from the parent DOM won't affect the shadow DOM's contents. Inherited styles, e.g. font-weight, opacity, display, text-align etc. can be inherited by the shadow DOM. Ref: https://open-wc.org/guides/knowledge/styling/styles-piercing-shadow-dom/

You should be able to fix this by adding the following to your web components' styles:

:host {
  /* Reset all CSS properties */
  all: initial;
}

This is a separate issue from the one I described here, however. The above reset will not fix the clashing CSS class issue, because CSS classes have higher specificity. The root cause is still the same though (I think), so this can be fixed if you use namespaced CSS classes for your web component. Or, better yet, don't rely on CSS classes being applied on the host element itself.

[agjohnson]

I'll just note here that the CSS classes are not an important part of the implementation, though they seemed like an okay pattern originally. I'm feeling like this is a pattern we should just move away from entirely. Prefixing the CSS classes does work, but prefixing and data attributes are both older patterns for what web components give us.

I'd rather we use element attributes here, as class="toast" does have meaning at the code level, outside CSS styles. I'm describing <readthedocs-notification variant="toast"> or similar which shouldn't greatly affect our style rules.

[laymonage]

Yep, I think that makes sense! I looked into the code a bit and I thought the CSS classes weren't necessary, but I wanted to make sure.

And yes, custom attributes would definitely be a great solution, since it's a custom element. I noticed that the EthicalAds widget is not a custom element yet (still a div), and it also uses the similar raised class like the custom elements, hence I suggested data attributes in case you wanted to keep it consistent among the other addons. If that consistency isn't important, then I think custom attributes are the way to go.

[agjohnson]

That's good insight, that's actually exactly where this pattern originated from. When we started the EA client, we weren't yet invested into web components, and so the patterns there are much different. If I had to restart that library today, it 100% would use a web components and similar approach as to the elements here.

[humitos]

I understand the path forward here is:

1. change class=toast by variant=toast
2. change all CSS queries where .toast by [variant=toast]

@agjohnson what's the correct way to set variant=toast by default when it's not explicitly defined in the HTML code?

[agjohnson]

Element default properties should be set in the constructor. We probably already have a few examples of this.

[laymonage]

Note that properties (the DOM object's property, accessed via .propertyName) and attributes (the HTML attribute in the DOM tree, accessed via .{get,set,has}Attribute()) are different. In Lit's default behaviour, while attributes are observed (i.e. changing an attribute also updates the property), properties are not reflected (i.e. changing a property's value does not update the attribute).

If you intend to use the custom attributes in CSS, you'll probably also want to make the properties reflective. See: https://lit.dev/docs/components/properties/#reflected-attributes

I don't think I found a component property that uses reflect: true in any of the addons.

Ignore me if you already know this 🙂

[agjohnson]

We haven't needed to use reflection yet, but this is indeed a place we'd need to. Any CSS selector that relies on a value we manipulate -- setting a default value or changing the value for some reason -- should be using property reflection.