MoOx / pjax

Easily enable fast Ajax navigation on any website (using pushState + xhr)
MIT License
1.46k stars 125 forks source link

Bug with select field in forms #158

Closed zmitic closed 6 years ago

zmitic commented 6 years ago

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

image

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?

BehindTheMath commented 6 years ago

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?

BehindTheMath commented 6 years ago

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 6 years ago

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 6 years ago

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 6 years ago

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 6 years ago

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

BehindTheMath commented 6 years ago

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

zmitic commented 6 years ago

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

BehindTheMath commented 6 years ago

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 6 years ago

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: