Navigation Menu

Skip to content

Commit

Permalink
Make sure BUTTON elements are included in form serialization
Browse files Browse the repository at this point in the history
  • Loading branch information
Juriy Zaytsev committed May 30, 2009
1 parent 6c35231 commit 2c1923c
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 41 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG
View file
@@ -1,3 +1,5 @@
* Make sure BUTTON elements are included in form serialization. (Luis Gomez, freshteapot, Samuel Lebeau, kangax)

* Make sure (defficient) APPLET, OBJECT and EMBED elements are extended with simulated methods in IE8. Return early if _extendedByPrototype is present on an element. (Tobie Langel, kangax)

* Add missing semicolons. This allows to strip newlines from distribution file. (kangax)
Expand Down
3 changes: 2 additions & 1 deletion src/dom/dom.js
View file
Expand Up @@ -1710,7 +1710,8 @@ Element.addMethods = function(methods) {
"FORM": Object.clone(Form.Methods),
"INPUT": Object.clone(Form.Element.Methods),
"SELECT": Object.clone(Form.Element.Methods),
"TEXTAREA": Object.clone(Form.Element.Methods)
"TEXTAREA": Object.clone(Form.Element.Methods),
"BUTTON": Object.clone(Form.Element.Methods)
});
}

Expand Down
82 changes: 45 additions & 37 deletions src/dom/form.js
View file
Expand Up @@ -374,68 +374,76 @@ var $F = Form.Element.Methods.getValue;

/*--------------------------------------------------------------------------*/

Form.Element.Serializers = {
input: function(element, value) {
Form.Element.Serializers = (function() {
function input(element, value) {
switch (element.type.toLowerCase()) {
case 'checkbox':
case 'radio':
return Form.Element.Serializers.inputSelector(element, value);
return inputSelector(element, value);
default:
return Form.Element.Serializers.textarea(element, value);
return valueSelector(element, value);
}
},

inputSelector: function(element, value) {
if (Object.isUndefined(value)) return element.checked ? element.value : null;
}

function inputSelector(element, value) {
if (Object.isUndefined(value))
return element.checked ? element.value : null;
else element.checked = !!value;
},

textarea: function(element, value) {
}
function valueSelector(element, value) {
if (Object.isUndefined(value)) return element.value;
else element.value = value;
},
}

select: function(element, value) {
function select(element, value) {
if (Object.isUndefined(value))
return this[element.type == 'select-one' ?
'selectOne' : 'selectMany'](element);
else {
var opt, currentValue, single = !Object.isArray(value);
for (var i = 0, length = element.length; i < length; i++) {
opt = element.options[i];
currentValue = this.optionValue(opt);
if (single) {
if (currentValue == value) {
opt.selected = true;
return;
}
return (element.type == 'select-one' ? selectOne : selectMany)(element);
var opt, currentValue, single = !Object.isArray(value);
for (var i = 0, length = element.length; i < length; i++) {
opt = element.options[i];
currentValue = optionValue(opt);
if (single) {
if (currentValue == value) {
opt.selected = true;
return;
}
else opt.selected = value.include(currentValue);
}
else opt.selected = value.include(currentValue);
}
},
}

selectOne: function(element) {
function selectOne(element) {
var index = element.selectedIndex;
return index >= 0 ? this.optionValue(element.options[index]) : null;
},
return index >= 0 ? optionValue(element.options[index]) : null;
}

selectMany: function(element) {
function selectMany(element) {
var values, length = element.length;
if (!length) return null;

for (var i = 0, values = []; i < length; i++) {
var opt = element.options[i];
if (opt.selected) values.push(this.optionValue(opt));
if (opt.selected) values.push(optionValue(opt));
}
return values;
},
}

optionValue: function(opt) {
// extend element because hasAttribute may not be native
return Element.extend(opt).hasAttribute('value') ? opt.value : opt.text;
function optionValue(opt) {
return Element.hasAttribute(opt, 'value') ? opt.value : opt.text;
}
};

return {
input: input,
inputSelector: inputSelector,
textarea: valueSelector,
select: select,
selectOne: selectOne,
selectMany: selectMany,
optionValue: optionValue,
button: valueSelector
};
})();

/*--------------------------------------------------------------------------*/

Expand Down
1 change: 1 addition & 0 deletions test/unit/fixtures/form.html
View file
Expand Up @@ -72,6 +72,7 @@
<input type="radio" name="tf_radio" value="on" />
<input type="hidden" name="tf_hidden" />
<input type="password" name="tf_password" />
<button name="tf_button"></button>
</div>
</form>

Expand Down
7 changes: 4 additions & 3 deletions test/unit/form_test.js
View file
Expand Up @@ -204,7 +204,7 @@ new Test.Unit.Runner({

testFormGetElements: function() {
var elements = Form.getElements('various'),
names = $w('tf_selectOne tf_textarea tf_checkbox tf_selectMany tf_text tf_radio tf_hidden tf_password');
names = $w('tf_selectOne tf_textarea tf_checkbox tf_selectMany tf_text tf_radio tf_hidden tf_password tf_button');
this.assertEnumEqual(names, elements.pluck('name'))
},

Expand All @@ -226,7 +226,7 @@ new Test.Unit.Runner({
testFormSerialize: function() {
// form is initially empty
var form = $('bigform');
var expected = { tf_selectOne:'', tf_textarea:'', tf_text:'', tf_hidden:'', tf_password:'' };
var expected = { tf_selectOne:'', tf_textarea:'', tf_text:'', tf_hidden:'', tf_password:'', tf_button: '' };
this.assertHashEqual(expected, Form.serialize('various', true));

// set up some stuff
Expand All @@ -235,10 +235,11 @@ new Test.Unit.Runner({
form['tf_text'].value = "123öäü";
form['tf_hidden'].value = "moo%hoo&test";
form['tf_password'].value = 'sekrit code';
form['tf_button'].value = "foo bar";
form['tf_checkbox'].checked = true;
form['tf_radio'].checked = true;
var expected = { tf_selectOne:1, tf_textarea:"boo hoo!", tf_text:"123öäü",
tf_hidden:"moo%hoo&test", tf_password:'sekrit code', tf_checkbox:'on', tf_radio:'on' }
tf_hidden:"moo%hoo&test", tf_password:'sekrit code', tf_button: 'foo bar', tf_checkbox:'on', tf_radio:'on' }

// return params
this.assertHashEqual(expected, Form.serialize('various', true));
Expand Down

8 comments on commit 2c1923c

@tobie
Copy link

@tobie tobie commented on 2c1923c May 30, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity. How do you handle the IE6 discrepancies between innerHTML and value. I recall that was the issue which prevented us from fixing this.

@kangax
Copy link
Owner

@kangax kangax commented on 2c1923c Jun 1, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually passes all form tests in IE6. Wouldn't it makes sense to simply document IE6 inability to set button's value from "value" attribute set in HTML? Programatically set values are all serialized properly, afaik.

@tobie
Copy link

@tobie tobie commented on 2c1923c Jun 2, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about that?

@kangax
Copy link
Owner

@kangax kangax commented on 2c1923c Jun 2, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About programatically set values being serialized? Yep. That's what test does form['tf_button'].value = "foo bar"; and then var expected = { /*... */ tf_button: 'foo bar', tf_checkbox:'on', tf_radio:'on' }

@samleb
Copy link

@samleb samleb commented on 2c1923c Jun 2, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked and it seems that, as Tobie pointed out, assigning value with form['tf_button'].value = "foo bar"; actually changes button's innerHTML, and reading value "consistently" returns its innerHTML.

A simple workaround is to add a function named button that handles buttons cases:

function button(element, value) {
  if (Object.isUndefined(value)) {
    var attr = element.attributes['value'];
    return attr ? attr.value : '';
  } else {
    var attr = document.createAttribute('value');
    attr.nodeValue = value;
    element.setAttributeNode(attr);
  }
}

However, this line in testFormSerialize has to be rewritten to call Form.Element.setValue instead of assigning with value =.

I'll test this against all supported browsers and submit a patch if it's working as expected

@kangax
Copy link
Owner

@kangax kangax commented on 2c1923c Jun 3, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly are we working around here? If a user sets button's value, he would expect that value to be visually changed as well, right? Wouldn't the change you're talking about only affect property of a value attribute, leaving button displayed with an old value?

@samleb
Copy link

@samleb samleb commented on 2c1923c Jun 3, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, actually <button> tags appearance should be determined by their innerHTML and not by their value attribute.

@samleb
Copy link

@samleb samleb commented on 2c1923c Jun 3, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workaround worked well on all browsers, I attached a patch on Lighthouse.

Please sign in to comment.