Fix attribute removal in props updates

[chromakode]

While developing a cyclejs application, I noticed that sometimes spurious class attributes would remain after diffing. After digging a bit, it appears that the test for this functionality wasn't working: it was not checking the right element instance for the attribute, so the test was naively succeeding even though the attribute was not removed.

This patch fixes the test and implements working attribute removal. The removeAttribute method is now used for this, which requires us to translate JS property names to DOM attribute names ("className" -> "class"). I added a dependency on html-attributes for this.

Initially, I tried setting the attribute properties to null or undefined, because I didn't want to have to translate the prop names to attribute names. Unfortunately, some HTML attributes like "href" are tricky, and assigning null to them actually stringifies the value to 'null'. So it appears that this added complexity is necessary.

[TylorS]

I must admit that I'm not really in favor of adding a dependency
Is it really needed?

That's my 2 cents anyways

[chromakode]

Yeah, I didn't want to add a dep either. Looks like we could take a page out of React's book and special-case the 4 attributes that aren't simply a lowercase transformation. Will try this out and update.

[chromakode]

Alright! That worked. Updated as discussed and expanded the test to cover all of the cases.

[chromakode]

@paldepind apologies if this is disruptive, but wanted to bump this PR in case it slipped through the cracks. I think this is a pretty serious bug: snabbdom is currently not removing attributes in DOM updates.

I've rewritten the PR to no longer add any dependencies. This is a pretty simple fix and includes complete tests. Please let me know if there's anything else I can do to help finish this up. I'd love to get it out, since it fixes broken styling in a cyclejs project I'm publishing soon. Thanks!

[amirouche]

This might fix #167

[AlexGalays]

Would it fix your issue if snabbdom did set the class attribute rather than the className property inside createElm ? Or are you using the className property yourself ?

Also, I don't think all properties mirror to an attribute. e.g input.value. While removeAttribute might be a no-op in theses cases, it seems wasteful.

[mightyiam]

The props module and the attributes module are two separate modules. They currently do not intend to overlap, even though in HTML and the DOM some of them do. This keeps the library simple, at the possible cost of inconvenience for the user in those cases. To determine this inconvenience, I would like to see a use case with some code, please. I know it has been a while, but still. If someone bothered making a PR for this, it may be worth addressing, either as a change or as documentation.

Is there an issue open for this?

[mightyiam]

Just marking this as "changes requested" for tracking purposes.