[BUGFIX beta] Undefined should not set an attribute.

[rwjblue]

Fixes #10595.

[rwjblue]

@mixonic - r?

[mmun]

Can you add a few assertions that assert that going from not-null/undefined to null/undefined removes the attribute?

[rwjblue]

@mmun - Great point!

I'm also going to add some tests that confirm this does not apply to <div data-foo={{someThingNotThere}}> as I believe that we intend this to work just like standard HTML (and add the data-foo attribute) JSBin here.

[rwjblue]

@mmun - I have changed around the implementation a bit, and added tests as you suggested. As you guessed the attribute is not removed when moving from an existing value to a null value.

Tracking this down, its basically because we are just calling element[attrName] = null (using setPropertyStrict).

@mixonic / @mmun - Thoughts on fixing this?

[mixonic]

placeholder used to be set via an attribute. I'm unsure about a generic way to make property things (value, placeholder) act like attributes.

The logic from AttributeBindingNode here can likely be folded back into AttrNode. Ideally attribute bindings and htmlbars bound attributes should share behavior, since they definitely will share behavior in Glimmer.

[rwjblue]

I originally had it in normal AttrNode, but assumed you didn't want it to apply in non-attributeBinding scenarios. I will move the change in logic back into AttrNode...

The fix here to ensure that placeholder is not set if undefined should likely land before 1.11.0 is released. I still have the general concern (as does @mmun based his comment/question above), but that seems like a larger issue and not isolated to this PR.

I'll update to get this passing shortly.

[rwjblue]

Updated to simplify and just modify AttrNode so the logic is the same. Still does not address @mmun's comment (I do not think it is possible with the current strategy of HTMLBars with regards to props & attrs).

@mmun / @mixonic - Should be ready for another round of review.

[mixonic]

Thanks sir.