Created progressValue to be used with <progress /> tags

[danielschonfeld]

progressValue seems to be a safer way to go about changing the attribute 'value' for the tag when wanting to distinguish between intermediate and determined statuses.

Where progressValue 0 is a determined state and null is the intermediate state.

This pull request is a product of issue #1431

[syranide]

As I mentioned in chat, it seems bad to introduce a new attribute to fix a local issue (especially when value would still keep working but being broken), when a big part of the win of the virtual DOM is that we can fix the broken things in HTML.

document.createElement('progress').value = 0 is the root of the issue, is there a good reason MUST_USE_PROPERTY is used today? MUST_USE_ATTRIBUTE would solve this issue at least, but I'm unsure if it would break something else. EDIT: Nope it wouldn't because then 0 would also be removed, does it require a custom callback perhaps?

@danielschonfeld If it absolutely has to be MUST_USE_PROPERTY then creating a special wrapper ReactDOMProgress that solves it "locally" would seem like a better solution to me, but I doubt we would need to go there.

[syranide]

cc @spicyj @zpao

@danielschonfeld dug around a bit and it seems that it's there from the very early github commits, do you guys have any idea if it's "truly intentional"? Also I saw @spicyj I had a commit for fixing attributes not being removed when properties where nulled, but that's obviously not happening now?

[sophiebits]

We should be able to make value work always without making a new progressValue prop.

The two important things is that we don't set value to the current value if it matches (that causes weird things with the cursor sometimes) and that null turns into an empty input box, not the string "null" (which I think it does in old IE if you do .value = null). If just doing MUST_USE_ATTRIBUTE doesn't work cross-browser, then we can probably make a custom mutation method -- perhaps checking node.tagName but perhaps we could do something craftier like checking if defaultValue is present (and using the property accessor if it is, the attribute otherwise).

[zpao]

Thanks for taking the time to start looking into this. I definitely don't want to do what you did here. @syranide is right - introducing a new property just to fit into our existing set of constraints is bad. We can change our implementation as we find new constraints though.

In this case it might be as simple as saying value should be an attribute now. However, I think there must be a reason we didn't do that earlier. @yungsters - do you remember?

We may also be able to special case properties when they are on certain nodes, maybe for the default values or for the way we handle the setting (attribute vs property).

[danielschonfeld]

@zpao: I'm redoing this using @syranide and @spicyj idea of giving it a mutation method. I should have it up in a little bit

[zpao]

While we're here, 2 spaces for indentation, not tabs. (we have an .editorconfig file with that, though I know not many actually use those... yet)

[steveluscher]

Nice find. It's a shame that the progress element doesn't unset its value property when found in an indeterminate "barbershop pole" state.

I was just about to whip up a ReactDOMProgress wrapper when I read that @danielschonfeld is considering a mutation method. Is a mutation method the right way to go? That would introduce the overhead of an extra function call for a frequently-manipulated property (value) just for the purpose of fixing the behavior of an infrequently-used-but-arguably-broken HTML element (<progress>). Maybe a wrapper is the way to go?

[steveluscher]

In case you decide that a wrapper solution is preferable to a mutation method, there's #1443.

[danielschonfeld]

@steveluscher solution eats the cake. I'm closing this PR.