String properties should be set to undefined when the associated attribute is removed

[mollykreis]

🐛 Bug Report

When you have a string property that is associated with an attribute, removing the attribute from the element sets the property to null instead of undefined. However, the correct behavior would be setting the property to undefined since that is the default value for a string.

For example, consider an element defined as follows:

class MyElement extends FoundationElement {
    @attr({ attribute: 'my-cool-value' })
    public myCoolValue?: string;
}

If the element is added to the DOM as <my-element></my-element> with no attributes set, interaction with the element would look like:

// myElement.myCoolValue starts as 'undefined'

myElement.setAttribute('my-cool-value', 'hello world');
// myElement.myCoolValue now is 'hello world'

myElement.removeAttribute('my-cool-value');
// myElement.myCoolValue now incorrectly is 'null' rather than returning to the initial state of 'undefined'

This is loosely related to #5320 (comment)

💻 Repro or Code Sample

🤔 Expected Behavior

When an attribute is removed from an element derived from FoundationElement, the property associated with that attribute should be set to undefined.

😯 Current Behavior

When you remove an attribute from an element derived from FoundationElement, the property associated with that attribute gets set to null rather than the default value of undefined. This leads to inconsistent behavior where the property's value starts as undefined but adding and then removing an attribute changes the property's value to null.

💁 Possible Solution

Currently the FAST element sets the property's value to the value passed to attributeChangedCallback. Instead, if the newValue is null, the property should be set to undefined.

🔦 Context

As shown above, I would like to define a property in an element as:

@attr({ attribute: 'my-cool-value' })
public myCoolValue?: string;

However, there are code paths within FAST that could cause myCoolValue to be set to null, which causes problems when trying to write strongly typed TypeScript code.

Alternatively, I could define the property as:

@attr({ attribute: 'my-cool-value' })
public myCoolValue?: string | null;

The above would more accurately describe the possible values of myCoolValue, but I don't want to have to litter my code with | null types. I also don't want the types in the custom elements I define to diverge too much from the types of the properties on FAST controls, the majority of which are typed as string.

🌍 Your Environment

I don't think this is specific to a given environment, but I am using:

Edge Version 109.0.1518.78 (Official build) (64-bit)

[chrisdholt]

Thanks @mollykreis for the issue and your patience. We aren't doing anything to manipulate the values on removal. I can confirm that I repro when an attribute is removed it's value is being set to null; however, I opened the console and grabbed a random native element from the GitHub DOM, removed a custom data-attribute which was set and then logged typeof element.getAttribute(foo) which came back as object. Were the platform reassigning to undefined instead of null, I think I would expect a value of undefined instead of object which signifies a null value.

The above investigation just led me to double check the spec and I can confirm that null is to be expected here rather than undefined. From the spec:

When any of its attributes are changed, appended, removed, or replaced, its attributeChangedCallback is called, given the attribute's local name, old value, new value, and namespace as arguments. (An attribute's old or new value is considered to be null when the attribute is added or removed, respectively.)

[chrisdholt]

@mollykreis - just wanted to follow up and see if you had thoughts on the above or if I'm good to close out given the above line from the spec.

[rajsite]

The concern our team is facing (thanks @mollykreis for creating this issue to track it) is about the JS property representation of HTML attributes not aligning with browser behavior for FAST components. The behavior of getAttribute, setAttribute, and removeAttribute are perfectly reasonable and aligned with DOM. It's the reflection to JS properties that behaves unexpectedly.

Specifically the same attribute state (an absent attribute) has multiple JS property representations (null or undefined). That is not common in DOM APIs.

So for the data- attribute example, the JS property representation behaves like the following:

// No attribute
let a = document.createElement('div');
a.getAttribute('data-test'); // null
a.dataset.test; //undefined <---

// Attribute set
a.setAttribute('data-test', 'foo');
a.getAttribute('data-test'); // 'foo'
a.dataset.test; // 'foo'

// Attribute removed
a.removeAttribute('data-test');
a.getAttribute('data-test'); // null
a.dataset.test; //undefined  <---

Notice how in the above there is not an observable difference between an element with a data- attribute that was unset and an element with the data- attribute removed. The JS property that is synchronized to the value always reports undefined to correspond to an absent attribute.

What Molly's example shows is that FAST's behavior differs from the normal DOM behavior.

To extend the example given:

@customElement('cool-element')
class MyElement extends FoundationElement {
    @attr({ attribute: 'my-cool-value' });
    public myCoolValue?: string;
}

// No attribute
let a = document.createElement('cool-element');
a.getAttribute('my-cool-value'); // null
a.myCoolValue; //undefined  <---

// Attribute set
a.setAttribute('my-cool-value', 'foo');
a.getAttribute('my-cool-value'); // 'foo'
a.myCoolValue; // 'foo'

// Attribute removed
a.removeAttribute('my-cool-value');
a.getAttribute('my-cool-value'); // null
a.myCoolValue; // null  <--- unexpected JS representation of absent attribute, should match the initial representation, i.e. `undefined`

Notice how in FAST there is an observable difference of the JS property between an element with an attribute that was never set and an element with the attribute removed.

This is the unexpected behavior. JS properties synchronized to FAST attributes do not have a consistent mapping. The exact same attribute state (absence of the attribute) has multiple possible property states (can be either null or undefined) in FAST. That is not common in DOM APIs that reflect attributes to properties.

[chrisdholt]

@rajsite thanks - I'm going to tag in @EisenbergEffect here as well given I do not see where we are modifying this in the code. Forgive my confusion, I don't see where we're modifying this in the code. Perhaps you can share the link or perhaps @EisenbergEffect can find it. My point above is that the "Custom Element Reactions" spec specifically calls out this behavior for the attributeChangedCallback, which from what I can tell...we just call in FAST:

When any of its attributes are changed, appended, removed, or replaced, its attributeChangedCallback is called, given the attribute's local name, old value, new value, and namespace as arguments. (An attribute's old or new value is considered to be null when the attribute is added or removed, respectively.)

From what I can tell, any reflection is from the platform here or it could just be that I'm not seeing it. I don't see where we're changing the value on js property reflection in the element-controller code. If this is the platform, I'm not sure we can (or should) do anything about it. As far as I'm aware, the attributeChangedCallback() is specific to custom elements and that could explain the delta between it and data- attributes, which, while they are custom attributes are defined independently in the spec.

[EisenbergEffect]

@chrisdholt is correct here. It's not that we're trying to do something different. The platform itself is telling us that the value is null. So, to "fix" this, we would have to internally map null values to undefined. But I don't think we can make that assumption on behalf of the programmer. It's possible that we could add some new mode to attribute reflection that did this. Have you all looked into whether you could handle this with a custom converter assigned to the attributes? If that works, you could even create your own decorator that wrapped ours and applied this behavior via your converter.