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

Missing ref values for select / options #2629

Closed
youbastard opened this Issue Oct 16, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@youbastard

youbastard commented Oct 16, 2018

  1. Describe your issue:

When creating a select input with options, if the value attribute is not present, riot will use the text content of the option as the value. However, when generating options using a Key/Value pair (or something similar), Riot will not create the "value" attribute of an option with a "falsey" value.

In the plnkr link below, the generated DOM looks like

<select ref="selected">
    <option value="H">Home</option>
    <option value="R">Road</option>
    <option>All Locations</option>
</select>

But the expected behavior (in my opinion) should be more similar to this:

<select ref="selected">
    <option value="H">Home</option>
    <option value="R">Road</option>
    <option value="">All Locations</option>
</select>

If you are using this value to look up an object in your array,

  1. Can you reproduce the issue?

https://plnkr.co/edit/7luU84Q0yEvBd89ELzFT?p=preview

Expected Behavior:
http://plnkr.co/edit/EB7urw7KSmLjMFXw9KQb?p=preview

  1. On which browser/OS does the issue appear?

Chrome, Safari, Firefox

  1. Which version of Riot does it affect?

Latest

  1. How would you tag this issue?
  • Question
  • Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance
@sourcegr

This comment has been minimized.

Contributor

sourcegr commented Oct 17, 2018

Ha! Had run into it also. Never took the time to send a pr though...

You are a victim of #1642, or should I say the implementation of the "solution". On creation, riot does add the value="'' attribute to the looped <option>. You can even see it if you right click->inspect element on the select->options... But on update... boy you are in trouble!

It's an easy fix.

Somewhere in the riot code in updateExpr there is a line that removes the attribute...
it checks for falsy values (which '' is).

Change that line so it will skip the tagName ==='OPTION'.

That's all to it. Tests pass, even the stupid fails to update value of input with same internal value or something...

@sourcegr

This comment has been minimized.

Contributor

sourcegr commented Oct 17, 2018

@GianlucaGuarini just thought of a caveat in this pr.

Please keep in mind that this pr only checks for <option>s. I am afraid that it should also take into account the input[type="checkbox"]s and <radio>s tags

Its up to you to create a list of exceptions where ever you see fit in the riot code, in order to exclude these as well.

@GianlucaGuarini

This comment has been minimized.

Member

GianlucaGuarini commented Oct 17, 2018

I don't see any bug here Riot behaves exactly as vanilla javascript <select> http://jsfiddle.net/gianlucaguarini/vo5kuqLt/

The option specs are really clear:

The value attribute provides a value for element. The value of an option element is the value of the value content attribute, if there is one, or, if there is not, the value of the element's text IDL attribute.

MDN Doc

The content of this attribute represents the value to be submitted with the form, should this option be selected. If this attribute is omitted, the value is taken from the text content of the option element.

@sourcegr

This comment has been minimized.

Contributor

sourcegr commented Oct 17, 2018

@GianlucaGuarini you are wrong.

is the value of the value content attribute, if there is one

(if there is one attribute)

If this attribute is omitted, the value is taken from the text content

(not if the value of the attribute is emtpy string!)

If there is an empty string in the attribute its a perfectly fine value. This is why if you inspect the element you can see <option value></option>

The empty string IS a value.

Its is, indeed, pretty clear.

@GianlucaGuarini

This comment has been minimized.

Member

GianlucaGuarini commented Oct 18, 2018

screen shot 2018-10-18 at 08 05 54

This is the markup rendered on Firefox in the first "buggy" example provided, the empty option value is set properly like expected using a riot loop. I don't see where is the bug here am I missing something else?

Basically the "expected" and the "buggy" examples render exactly the same markup.

@sourcegr

This comment has been minimized.

Contributor

sourcegr commented Oct 18, 2018

Please read what I write in my first comment

On creation, riot does add the value="'' attribute to the looped . You can even see it if you right click->inspect element on the select->options... But on update... boy you are in trouble!

@sourcegr

This comment has been minimized.

Contributor

sourcegr commented Oct 18, 2018

so try to change the option value to something else and inspect the element once again

@GianlucaGuarini

This comment has been minimized.

Member

GianlucaGuarini commented Oct 18, 2018

@sourcegr ah I see, this problem was not mentioned in the initial issue. The fix should be simple I will check it ASAP. Thanks for making that clear

sourcegr added a commit to sourcegr/riot that referenced this issue Oct 18, 2018

sourcegr added a commit to sourcegr/riot that referenced this issue Oct 18, 2018

GianlucaGuarini added a commit that referenced this issue Oct 20, 2018

@GianlucaGuarini

This comment has been minimized.

Member

GianlucaGuarini commented Oct 20, 2018

this problem was solved in riot@3.13.0 please use

riot.settings.keepValueAttributes = true

To enable this feature

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