Attributes not removed when set to null on updates

[steadicat]

The behavior of attributes with a value of null or undefined is inconsistent. On first render they are not present on the element at all. On subsequent renders, the attribute sticks around with an empty value.

For example, <a href={null} /> renders what you expect (<a />), but if the value of href changes to null after the first render, you get <a href />. In the case of href (and likely others) an empty attribute has unintended side effects.

See this fiddle: http://jsfiddle.net/pGG2A/1/

[syranide]

cc @sebmarkbage @benjamn @zpao

href and most "attributes" are handled as properties by React, and not attributes, because it's (supposedly) significantly faster. Properties do not understand null or undefined and thus their empty state becomes "". So by default, a property is "" and does not show in the DOM, but by setting it to "" it will show up in the DOM (yay HTML).

http://jsperf.com/propattr/2

Some benchmarking shows that it's not as clear in practice, what's clear though is that all the slow browsers are enormously faster at dealing with properties, Properties being roughly 3 times faster than attributes with the mixed test roughly in the middle. But we're only really interested in removing attributes, so that leaves us with the current implementation being roughly 50-100% faster than removeAttribute, although IE8 being the slowest does not appear to fare as badly, which is good.

Considering even poor IE8 can churn through 300.000 property assignments and attribute removes per second (or 5000 per frame at 60 FPS, IE9 can do 15000 per frame). Couple that with toggling between values and explicitly null/undefined being a rather uncommon case (on huge amounts of nodes and only on properties), I would be surprised to see any measurable real life impact of switching to removeAttribute.

So it seems to me that, unless my tests are seriously flawed, that switching to removeAttribute could actually be viable. It would keep the DOM clean (and would let us do away with the ugly defaultValue probing ⭐ ), and apart from IE11, it is actually faster in my edge browsers (although IE11 is sadly by far the slowest browser in this test (if you can call it that)).

Incidentally, this will also solve #1431 (in a preferable way even, perhaps).

---

Percentages are relative to current implementation, 2nd vs 1st and 3rd vs 1st respectively. The op/s per second is total assignments + removes per second for the 1st column (2nd test). So unless we want to go all-out attributes, only the first column is of interest.

VMIE8: 17% / 40% slower (~300k/s)
VMIE9: 39% / 69% slower (~900k/s)
VMIE10: 45% / 70% slower (~950k/s)
IE edge: 51% / 75% slower (~1000k/s)
Chrome edge: +44% / +19% *faster* (~4000k/s)
FF edge: +27% / +1% *faster* (~7800k/s)

[syranide]

Proper implementation #1510

[sebmarkbage]

What is the consequence of leaving the DOM attribute there? Is it messing with your CSS rules? What is the end problem caused by leaving them in?

[syranide]

@sebmarkbage <progress /> is different from <progress value="" />, but that's probably the only exception (I know of at least). CSS-rules is an obvious one I guess, although extremely rarely used it seems, but there are some legit uses.

As I mention in my PR, FF and Chrome are actually faster with the attribute methods. Although older browsers suffer slightly (still blazingly fast though).

[sebmarkbage]

I think it might make sense to have several different code paths based on browser and/or the attribute used. Before committing to this as a permanent supported solution, I'd like to clarify that this is a common enough problem.

There is another reason I'd like to treat React props as properties rather than attributes in general. HTML/SVG and Web Components doesn't allow complex types as attributes. To support complex types like style, class lists, custom user data, we prefer to model these as properties rather than attributes.

If a property is inherently broken in the DOM (missing functionality) we can fix those on a case-by-case basis. Ideally the behavior of React props should correspond to DOM properties instead of DOM attributes. But in the end, we'll do whatever makes sense.

[syranide]

@sebmarkbage My proposed solution only uses removeAttribute to clear (non-boolean) properties (it does not use setAttribute), so unless it interacts poorly with the techs your mentioned, it doesn't seem to me like the use of removeAttribute on properties itself would be an issue.

Just bringing in the reason it's a problem from my closed duplicate - when a link has an empty href it still takes focus. I can kind of sort out the style problems with extra css but it'll still get focus as you tab the focus through the controls on the page.

[jaredatron]

@sebmarkbage <a /> is different from <a href="" />

[ariabuckles]

I'm also running into the <a href /> thing (demo http://jsfiddle.net/uLk47d8y/1/ ).

In practice, this means that our markdown editor isn't making things appear as non-links when they become invalid.

I can't comment on the implementation or html properties vs attributes (yuck :( ), but this is surprising as a user. I'll probably just leave it and let it be confusing, but if this behaviour wasn't acceptable, working around it would be a bit painful (key the <a> tag so it becomes remounted?), so here's my small vote for fixing this.

[jaredatron]

I decided to swap out the a element for a span because just toggling the presence of the href attribute isn't possible atm.

[syranide]

I have PR #1510 for this which seems rather safe (perhaps need to add a test for "property-only" properties), nag on @sebmarkbage a bit more and he might consider it. ;)