New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"value" attribute is not removed if previously set and then set again to falsy value #2427

Closed
bminer opened this Issue Aug 22, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@bminer
Contributor

bminer commented Aug 22, 2017

  1. Describe your issue:

The "value" attribute, specifically for <input> tags, is not removed properly if the value is changed to a falsy value. This problem only occurs if the "value" attribute was already set to a value and then changed to a falsy value some time later.

To be clear, the dom.value is changed properly, but the dom.getAttribute("value") is not updated.

  1. Can you reproduce the issue?

http://jsfiddle.net/c7psjcqd/3/

  1. On which browser/OS does the issue appear?
    Chromium 60.0.3112.78
    Linux Mint 18.1

  2. Which version of Riot does it affect?
    3.6.1 and 3.6.2

  3. How would you tag this issue?

  • Question
  • Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance
@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Aug 22, 2017

Member

this is not an issue it's how DOM + js works https://jsfiddle.net/catezfp0/

Member

GianlucaGuarini commented Aug 22, 2017

this is not an issue it's how DOM + js works https://jsfiddle.net/catezfp0/

@bminer

This comment has been minimized.

Show comment
Hide comment
@bminer

bminer Aug 22, 2017

Contributor

I understand the difference between the "value" attribute and the dom.value property, but I'm still confused. Why bother setting the "value" attribute in the first place then? Shouldn't it also get removed when set to a falsy value?

Contributor

bminer commented Aug 22, 2017

I understand the difference between the "value" attribute and the dom.value property, but I'm still confused. Why bother setting the "value" attribute in the first place then? Shouldn't it also get removed when set to a falsy value?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Aug 22, 2017

Member

@bminer why should we bother adding more logic (more code) here for handling something useless? The value gets properly updated and there are not unexpected behaviors here since vanilla js works the same way

Member

GianlucaGuarini commented Aug 22, 2017

@bminer why should we bother adding more logic (more code) here for handling something useless? The value gets properly updated and there are not unexpected behaviors here since vanilla js works the same way

@bminer

This comment has been minimized.

Show comment
Hide comment
@bminer

bminer Aug 22, 2017

Contributor

@GianlucaGuarini - my point is that Riot should be consistent. If it sets the "value" attribute, it should also clear it as necessary.

Right here, the value attribute is set. Maybe line 213 should be prepended with an "else" to read:

else if (hasValue && value !== false) {

You are right that right here on line 210, doing dom.value = value is good enough. So why bother setting the "value" attribute at all?

Contributor

bminer commented Aug 22, 2017

@GianlucaGuarini - my point is that Riot should be consistent. If it sets the "value" attribute, it should also clear it as necessary.

Right here, the value attribute is set. Maybe line 213 should be prepended with an "else" to read:

else if (hasValue && value !== false) {

You are right that right here on line 210, doing dom.value = value is good enough. So why bother setting the "value" attribute at all?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Aug 22, 2017

Member

@bminer because of this case

Member

GianlucaGuarini commented Aug 22, 2017

@bminer because of this case

@bminer

This comment has been minimized.

Show comment
Hide comment
@bminer

bminer Aug 22, 2017

Contributor

What do you mean? If you omit i.setAttribute('value', 'bar'), the result of console.log(i.value) is the same.

Contributor

bminer commented Aug 22, 2017

What do you mean? If you omit i.setAttribute('value', 'bar'), the result of console.log(i.value) is the same.

@bminer

This comment has been minimized.

Show comment
Hide comment
@bminer

bminer Aug 22, 2017

Contributor

What I am saying is that Riot changes the "value" attribute using setAttr only when the value is "truthy". It should not do this. It should only do dom.value = value. What do you think?

Contributor

bminer commented Aug 22, 2017

What I am saying is that Riot changes the "value" attribute using setAttr only when the value is "truthy". It should not do this. It should only do dom.value = value. What do you think?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Aug 22, 2017

Member

input.value has always the precedence over input.setAttribute for HTMLInput elements. This means that riot should treat the value updates differently from all the other DOM attributes updates, that's because a simple setAttribute call wouldn't work for cases such as the one demoed above. What you are asking is to add an additional useless code to riot in order to set the value attribute even when it's not necessary since it has no effect on your components updates. I would like to avoid redundant code riot especially if this code has any effect at all

Member

GianlucaGuarini commented Aug 22, 2017

input.value has always the precedence over input.setAttribute for HTMLInput elements. This means that riot should treat the value updates differently from all the other DOM attributes updates, that's because a simple setAttribute call wouldn't work for cases such as the one demoed above. What you are asking is to add an additional useless code to riot in order to set the value attribute even when it's not necessary since it has no effect on your components updates. I would like to avoid redundant code riot especially if this code has any effect at all

@bminer

This comment has been minimized.

Show comment
Hide comment
@bminer

bminer Aug 22, 2017

Contributor

@GianlucaGuarini - Sorry for the confusion. That's not what I'm saying.

I'm saying that Riot currently sets the "value" attribute even when it's not necessary. It set the value attribute and the value property of the element. This should be changed so that the "value" attribute is not set.

So, basically, I agree with you. :)

My suggestion was changing this code...

Current Riot (snippet from here):

    if (attrName === 'value' && dom.value !== value) {
      dom.value = value
    }


    if (hasValue && value !== false) {
      setAttr(dom, attrName, value)
    }

Suggested change:

    if (attrName === 'value' && dom.value !== value) {
      dom.value = value
    } else if (hasValue && value !== false) {
      setAttr(dom, attrName, value)
    }

What do you think?

Contributor

bminer commented Aug 22, 2017

@GianlucaGuarini - Sorry for the confusion. That's not what I'm saying.

I'm saying that Riot currently sets the "value" attribute even when it's not necessary. It set the value attribute and the value property of the element. This should be changed so that the "value" attribute is not set.

So, basically, I agree with you. :)

My suggestion was changing this code...

Current Riot (snippet from here):

    if (attrName === 'value' && dom.value !== value) {
      dom.value = value
    }


    if (hasValue && value !== false) {
      setAttr(dom, attrName, value)
    }

Suggested change:

    if (attrName === 'value' && dom.value !== value) {
      dom.value = value
    } else if (hasValue && value !== false) {
      setAttr(dom, attrName, value)
    }

What do you think?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Aug 22, 2017

Member

@bminer please make a pull request.. If it passes all our tests we can discuss about it further. At moment I don't see any improvement but just more code that doesn't improve anything concretely

Member

GianlucaGuarini commented Aug 22, 2017

@bminer please make a pull request.. If it passes all our tests we can discuss about it further. At moment I don't see any improvement but just more code that doesn't improve anything concretely

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Sep 2, 2017

Member

fixed in riot@3.7.0 thank you for your patch

Member

GianlucaGuarini commented Sep 2, 2017

fixed in riot@3.7.0 thank you for your patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment