set value property explicitly for "option" element even if value is empty

[everdimension]

Fixes #6219

• if option element has empty string as value, it should be used
• if option has no value attribute, the contents of the option tag should be used

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[chicoxyzzy]

We definitely need unit test here to avoid future regressions

[syranide]

#6219 (comment) for more information on this bug.

I'm not sure how the team feels about it, but an acceptable short-term workaround if we want to get it out quickly could be (haven't tested it):

      } else if (propertyInfo.mustUseProperty) {
        var propName = propertyInfo.propertyName;
        // Must explicitly cast values for HAS_SIDE_EFFECTS-properties to the
        // property type before comparing; only `value` does and is string.
        if (!propertyInfo.hasSideEffects ||
            ('' + node[propName]) !== ('' + value) ||
            !node.hasAttribute(propertyInfo.attributeName)) { // <-------- BUG FIX -------------
          // Contrary to `setAttribute`, object properties are properly
          // `toString`ed by IE8/9.
          node[propName] = value;
        }

Basically, we forcibly set the property whenever value === '' but the associated attribute is missing (only value has HAS_SIDE_EFFECTS, so it only applies to that in practice).

[everdimension]

@syranide
But that would set an empty value attribute to input[type=text] as well, which has no practical reason and I thought was best to avoid.

[syranide]

@everdimension Answering in original issue instead.

[facebook-github-bot]

@everdimension updated the pull request.

[everdimension]

Updated PR to always set value property if it is not null. Added a test case, updated another corresponding test.

[zpao]

You call out value as being special here but this will apply to other things too (eg selected which is also MUST_USE_PROPERTY). Is that your intention?

[syranide]

Not saying it shouldn't be changed, but it only applies to HAS_SIDE_EFFECTS which is only used by value.

[everdimension]

@zpao It's a question of being either clear or general. In any case, I'm not aware of other attributes which may need to be handled similarly. And currently it is the only one. The previous comment line mentions this, so I believe writing value makes it more clear for others reading this code.

[zpao]

Going to take this for 15 to fix the regression. There has been some discussion about doing something more involved with properties and attributes but this is the safest fix for the time being. Thank for doing this!