Bug with select field in forms #158

New Issue

2018-05-19T05:39:23-05:00

zmitic commented

2018-05-19 05:39:23 -05:00

(Migrated from github.com)

Hi,
for a select field in my GET form, I have default choice with no value like in this screenshot:

But when I submit it, that text goes into URL like http://127.0.0.1:8000/admin/products?manufacturer=--%20Select%20manufacturer%20. My backend framework (Symfony) complains it is not a valid select option (and it isn't) which makes form to fail.

It was an easy fix. Instead of this code:

for (var i = 0; i < element.options.length; i++) {
    opt = element.options[i]
    if (opt.selected) {
        values.push(opt.value || opt.text)
    }
}

I just replaced it with

for (var i = 0; i < element.options.length; i++) {
    opt = element.options[i]
    if (opt.selected) { 
        values.push(opt.value)       // <--- this changed
    }
}

and it works. Now my URL doesn't have incorrect values any more.

Can you merge that?

Hi, for a select field in my GET form, I have default choice with no value like in this screenshot: ![image](https://user-images.githubusercontent.com/1964158/40267638-43b8a43c-5b60-11e8-8b7c-be9031f67b9a.png) But when I submit it, that text goes into URL like ``http://127.0.0.1:8000/admin/products?manufacturer=--%20Select%20manufacturer%20``. My backend framework (Symfony) complains it is not a valid select option (and it isn't) which makes form to fail. It was an easy fix. Instead of this code: ```js for (var i = 0; i < element.options.length; i++) { opt = element.options[i] if (opt.selected) { values.push(opt.value || opt.text) } } ``` I just replaced it with ```js for (var i = 0; i < element.options.length; i++) { opt = element.options[i] if (opt.selected) { values.push(opt.value) // <--- this changed } } ``` and it works. Now my URL doesn't have incorrect values any more. Can you merge that?

BehindTheMath commented

2018-05-27 21:16:13 -05:00

(Migrated from github.com)

I believe this is the expected behavior.

From MDN:

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.

What we can do, is prevent the option from being sent if the disabled or hidden attributes are set.

@robinnorth What do you think?

I believe this is the expected behavior. From [MDN](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/option#attr-value): >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. What we can do, is prevent the option from being sent if the `disabled` or `hidden` attributes are set. @robinnorth What do you think?

BehindTheMath commented

2018-05-27 21:35:10 -05:00

(Migrated from github.com)

I did some quick testing. Looks like the standard behavior is that the field does not get sent when the disabled attribute is set.

I did some quick testing. Looks like the standard behavior is that the field does not get sent when the `disabled` attribute is set.

zmitic commented

2018-05-28 01:50:05 -05:00

(Migrated from github.com)

You are right; after reading MDN specs, I retested that page and Chrome was lying to me. In reality, generated HTML is value="" but developer tools didn't display the value, only the attribute.

So the value isn't missing from html, it is an empty string which is totally valid.

You are right; after reading MDN specs, I retested that page and Chrome was lying to me. In reality, generated HTML is ``value=""`` but developer tools didn't display the value, only the attribute. So the value isn't missing from html, it is an empty string which is totally valid.

BehindTheMath commented

2018-05-28 12:31:27 -05:00

(Migrated from github.com)

You are right; after reading MDN specs, I retested that page and Chrome was lying to me. In reality, generated HTML is value="" but developer tools didn't display the value, only the attribute.

So the value isn't missing from html, it is an empty string which is totally valid.

Correct. I think the best thing is to only add the option to the request if the disabled attribute is not set.

>You are right; after reading MDN specs, I retested that page and Chrome was lying to me. In reality, generated HTML is value="" but developer tools didn't display the value, only the attribute. >So the value isn't missing from html, it is an empty string which is totally valid. Correct. I think the best thing is to only add the option to the request if the `disabled` attribute is not set.

robinnorth commented

2018-05-28 14:25:26 -05:00

(Migrated from github.com)

I think the best thing is to only add the option to the request if the disabled attribute is not set.

Yes, that should keep us in line with default browser behaviour.

> I think the best thing is to only add the option to the request if the disabled attribute is not set. Yes, that should keep us in line with default browser behaviour.

zmitic commented

2018-05-28 14:44:45 -05:00

(Migrated from github.com)

But pls, add it even if it is an empty string.

BehindTheMath commented

2018-05-28 14:46:32 -05:00

(Migrated from github.com)

opt.value || opt.text will return opt.text even if the value is an empty string.

`opt.value || opt.text` will return `opt.text` even if the value is an empty string.

zmitic commented

2018-05-28 14:53:04 -05:00

(Migrated from github.com)

Yes, that is why I created this issue. This code already exists and if value is empty string, pjax will send text value instead.

At that point, backend framework complains avoid invalid value; it didn't render option with such value i.e. --Select manufacturer-- is just something for user (which can be translated), not for backend.

That is correct behaviour of Symfony, browser must send one of rendered values of it will consider someone is hacking.

I hope you understand my point. Having url like ?manufacturer_id=&other_id=13 is perfectly valid. If you try same form outside this package, you will see blank value in url as well.

Example, take a look at url in dev bar:

Yes, that is why I created this issue. This code already exists and if value is empty string, pjax will send text value instead. At that point, backend framework complains avoid invalid value; it didn't render option with such value i.e. ``--Select manufacturer--`` is just something for user (which can be translated), not for backend. That is correct behaviour of Symfony, browser **must** send one of rendered values of it will consider someone is hacking. I hope you understand my point. Having url like ``?manufacturer_id=&other_id=13`` is perfectly valid. If you try same form outside this package, you will see blank value in url as well. Example, take a look at url in dev bar: ![image](https://user-images.githubusercontent.com/1964158/40627963-51f1db72-62c2-11e8-8ef0-45e69a01d2bb.png)

BehindTheMath commented

2018-05-28 15:02:57 -05:00

(Migrated from github.com)

Ok, so this should be better:

if (opt.selected && !opt.disabled) {
    values.push(opt.attributes.getNamedItem("value") ? opt.value : opt.text)
}

This will send opt.value if the value attribute, no matter what value it has. If it doesn't exist, it will send opt.text. I think this matches the spec.

Ok, so this should be better: ``` if (opt.selected && !opt.disabled) { values.push(opt.attributes.getNamedItem("value") ? opt.value : opt.text) } ``` This will send `opt.value` if the `value` attribute, no matter what value it has. If it doesn't exist, it will send `opt.text`. I think this matches the spec.

zmitic commented

2018-05-28 15:05:49 -05:00

(Migrated from github.com)

Well... I am not so good in JS but I think it will do the job. Maybe someone more skilled should give an opinion.

But to be honest, I don't care about text value at all as long as my empty string is working 😄

Well... I am not so good in JS but I think it will do the job. Maybe someone more skilled should give an opinion. But to be honest, I don't care about text value at all as long as my empty string is working :smile:

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#158