fix: boolean attributes not overriding default property values in fast-element

[nicholasrice]

🐛 Bug Report

Min Repro: https://stackblitz.com/edit/typescript-bxsxvb?file=index.ts

💻 Repro or Code Sample

1. Define an custom-element with some attribute with mode="boolean", with the default value assigned true
2. Define another custom-element that renders the above element into it's shadow DOM. Use boolean template binding syntax to assign the attribute a false value.

🤔 Expected Behavior

The attribute should not exist on the element and the underlying property value should be false.

😯 Current Behavior

The attribute exists on the element and the underlying property value is true.

[nicholasrice]

This behavior is a byproduct of some DOM changes being routed through DOM.queueUpdate while others are not. At a high level, what is happening in the above min-repro is:

1. The element constructor sets the trueBoolAttr property value to true, which kicks off a DOM.queueUpdate() from AttributeDefinition.tryReflectToAttribute to set the attribute value. At this point the property value is true and no attribute exists on the element.
2. Templating engine evaluates the template binding for the element which evaluates false. The BindingBehavior for the binding then goes through DOM.setBooleanAttribute to remove the attribute because the behavior evaluates false. This call to DOM.setBooleanAttribute is not routed through DOM.queueUpdate so the code-path synchronously attempts to remove the attribute (at this point there still is no attribute to remove, so nothing happens)
3. The DOM.queueUpdate() callback from step 1 gets invoked, the trueBoolAttr property value is still true so the attribute is added.

@EisenbergEffect scheduling DOM updates in updateAttributeTarget() (not related to this issue but is a related implementation) and updateBooleanAttributeTarget() with DOM.queueUpdate() seems to fix the issue, does that seem like the right approach to you?

[EisenbergEffect]

Hmm. I think this is the correct fix, but I'm not 100% confident. I think we may need to spend some time reviewing this part of the system. I wonder if the internals of the AttributeDefinition should handle dom reflection different or if the Binding should queue but only in particular cases, or if this is an issue that's present only during initial bind, etc. @nicholasrice Do you have any time next week where you could explore a bit more just to make double sure this is the path we want to go?

[nicholasrice]

Yea I'll poke around more. This seems to be be primarily an issue during initial binding and construction, but I suppose it could manifest anytime the property value is set simultaneously as the template compiler sets the attribute, where both values are different.

[prudepixie]

I don't know if this has already been explored, but omitting the boolean expression - the ? - from the template runs as expected. @customElement({ name: "my-outer", template: html '<my-inner true-bool-attr="${(x) => false}"></my-inner> ' }) I'm guessing this is because the attribute type has already been set from initialization of Inner. Thus this ends up triggering updateAttributeTarget() instead of updateBooleanAttributeTarget(). At this point of time, it seems updateAttributeTarget() has been correctly scheduled on DOM.queueUpdate(), according to @nicholasrice comment above, and updateBooleanAttributeTarget() has not yet been correctly set.

This makes me think that if an attribute type has already been defined, is there a need to explicitly add the boolean - the ?- expression in the template again. And if a user does add it, perhaps we can just leave it to the template engine to process it naturally.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[chrisdholt]

@nicholasrice @EisenbergEffect Do we know if this is still a valid issue?