Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested components utilizing v-bind() in CSS improperly reference the CSS variable name of the parent node, causing undesired property inheritance in the child #12434

Open
adamdehaven opened this issue Nov 18, 2024 · 5 comments · May be fixed by #12461

Comments

@adamdehaven
Copy link

adamdehaven commented Nov 18, 2024

Vue version

3.5.13

Link to minimal reproduction

Reproduction playground

Steps to reproduce

Block One

  1. In App.vue we set the backgroundColor prop to a value of lightgray.

  2. Inspect the rendered HTML of "Block One" and notice the value of the background-color prop is bound to the root div element via an inline CSS Custom Property, prefixed by a generated hash, e.g. style="--a4f2eed6-backgroundColor: lightgray;"

  3. Notice the hash is also bound to the data-v-a4f2eed6 attribute (for scoped styles) as well.

  4. Inspect the Styles panel in the browser, and notice the .block class for "Block One" refers to the same CSS Custom Property for the background-color property (prefixed in this example with --a4f2eed6-)

    block one background-color styles

Block Two

  1. In App.vue we do not set the backgroundColor prop for Block Two in the component props, meaning it utilizes the prop default value of null (to prevent the property from being bound to the component since it is not desired for this instance)
  2. Notice the hash (for scoped styles) of Block Two is the same as Block One (i.e. data-v-a4f2eed6).
  3. Inspect the Styles panel in the browser, and notice the .block class for "Block Two" refers to the same CSS Custom Property for the background-color property (prefixed in this example with --a4f2eed6-), rather than generating its own hash to attempt to use for the property.

Block Three

  1. Since the CSS custom property is not defined on a parent node of Block Three, there is no issue in its display.

What is expected?

"Block Two" (as referenced in the Steps to reproduce) should generate its own CSS custom property hash prefix (likely corresponding to the scoped styles hash as well) that is then utilized inside its style rule.

<div data-v-a4f2eed6="" id="v-0" class="block" style="--a4f2eed6-backgroundColor: lightgray;">
  <div>Block One</div>
  <div data-v-a4f2eed6="" id="v-1" class="block">
    <div>Block Two</div>
  </div>
</div>

I would expect the generated CSS in the scenario described to evaluate to this:

/** Block One */
.block {
  &[data-v-a4f2eed6] {
    background-color: var(--a4f2eed6-backgroundColor);
  }
}

/** Block Two */
.block {
  &[data-v-a4f2eed6] {
    background-color: var(--DIFFERENT_HASH-backgroundColor);
  }
}

What is actually happening?

Since instances of the same component share the same generated hash for both the scoped styles as well as generated CSS custom properties, the CSS background-color cannot be explicitly "unset" by providing a null prop value for a secondary, nested instance of the same component.

The result of this means that in order for the child component to not reference the CSS Custom Property of its parent, you have to explicitly set the default prop value that is mapped to v-bind to something that essentially "unsets" the value that may be present from the parent.

Using the example from the reproduction, to solve the issue, I would have to change the defineProps implementation to set the backgroundColor prop to initial (i.e. a reset), or alternatively pass a value of something like transparent which isn't ideal.

const { backgroundColor = 'initial' } = defineProps<{ backgroundColor?: string }>()

This will then set the value of the same CSS Custom Property, on the child node, to initial

<div data-v-a4f2eed6="" data-v-7ba5bd90="" id="v-0" class="block" style="--a4f2eed6-backgroundColor: lightgray;">
  <div data-v-7ba5bd90="">Block One</div>
  <div data-v-a4f2eed6="" data-v-7ba5bd90="" id="v-1" class="block" style="--a4f2eed6-backgroundColor: initial;">
    <div data-v-7ba5bd90="">Block Two</div>
  </div>
</div>

System Info

No response

Any additional comments?

Generating a separate hash for each instance of a component (rather than reusing the same one for each one) would obviously lead to a lot of bloat in the injected DOM styles, especially for instances where this behavior doesn't exist.

I'm unsure if having to explicitly provide a "reset" value makes sense either, as it will bloat the inline style rules on every top-level component DOM element to set and/or reset the value.

@adamdehaven adamdehaven changed the title Nested components utilizing v-bind() in CSS improperly reference the CSS variable name of the parent node Nested components utilizing v-bind() in CSS improperly reference the CSS variable name of the parent node, causing undesired property binding in the child Nov 18, 2024
@adamdehaven adamdehaven changed the title Nested components utilizing v-bind() in CSS improperly reference the CSS variable name of the parent node, causing undesired property binding in the child Nested components utilizing v-bind() in CSS improperly reference the CSS variable name of the parent node, causing undesired property inheritance in the child Nov 18, 2024
@Justineo
Copy link
Member

IMO the current repro is working as expected even you remove the default value initial, though there is an unexpected --a4f2eed6-backgroundColor: undefined; on the root element of Block Two, which makes the background-color computes to its default value transparent (background-color is not an inherted property).

The code references the CSS variable value is the stylesheet of the component (not component instance) so if we generate hash per instance, we'll have to generate a separate piece of CSS for each component instance to consume the value, which doesn't feel practical. And even we do this, the outcome is still the same as the repro because then you'll reference an undeclared CSS variable, which works the same way as right now.

@Justineo
Copy link
Member

But after the fix at #12442, the undefined value will no longer be there so the value will then come from the parent 😅
See #12442 (comment)

@adamdehaven
Copy link
Author

adamdehaven commented Nov 20, 2024

IMO the current repro is working as expected even you remove the default value initial, though there is an unexpected --a4f2eed6-backgroundColor: undefined; on the root element of Block Two

But if you pass null as the default in the component, it doesn’t evaluate to undefined and actually inherits the CSS custom property value.

I think a better solution here, since undefined isn’t valid, and we can detect null to unset style properties already, would be to set the CSS custom property on the child node to initial so that it’s not set to a value within the scope of the nested child and does not inherit from the parent. I commented on the open PR with something similar.

If a non-falsy value is provided, then it should be used (e.g. red), otherwise, the CSS custom property should essentially be unset (via initial) so that it doesn’t impact the child.

Note

You cannot set the value to unset as within a child node, this will actually still cause inheritance from the parent.

Typically, you would want user-defined CSS custom properties to fall through (that is desirable) but in this particular case, the property is generated (meaning the child doesn’t know about its existence officially), and it’s improperly causing the child component to inherit the hashed variable value that it really shouldn’t be able to reference since the child component has its own prop binding, and therefore should respect its own default.

@Justineo
Copy link
Member

Justineo commented Nov 20, 2024

After some thoughts and discussion with @adamdehaven, I think I agree with him the binding should be per component instance. Currently the inheritance is introduced by the way we are implementing it (via custom properties) instead of by design. I think I'd propose a practical fix for this: What about we produce --<hash>-<name>:; for those bindings with nullish values (both CSR and SSR)? If I understand correctly, invalid values works the same as the CSS unset value for custom properties (CSS initial and unset keywords won't work for custom properties as they are treated as strings so they essentially work the same way as invalid values). This will stop the unexpected inheritance of non-inherited CSS properties like background-color.

@adamdehaven
Copy link
Author

adamdehaven commented Nov 20, 2024

After some thoughts and discussion with @adamdehaven, I think I agree with him the binding should be per component instance. Currently the inheritance is introduced by the way we are implementing it (via custom properties) instead of by design. I think I'd propose a practical fix for this: What about we produce --<hash>-<name>:; for those bindings with nullish values (both CSR and SSR)?

This is my thinking as well; and seems more ideal (and actually desired) rather than using undefined or another invalid value, taken from here:

The trick is that the value of initial for a custom property will trigger a fallback, while an empty whitespace value will not.
Reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants