Permalink
Browse files

Issue #1431: writeAttribute('checked') fails to write the 'checked' a…

…ttribute, writes an 'undefined' attribute instead
  • Loading branch information...
sdumitriu committed Oct 26, 2012
1 parent d017062 commit 4a22a52e08fce7dfe95bf54b8f52314614ee24b3
Showing with 22 additions and 4 deletions.
  1. +1 −1 src/prototype/dom/dom.js
  2. +21 −3 test/unit/dom_test.js
View
@@ -2353,7 +2353,7 @@
name = table.names[attr] || attr;
value = attributes[attr];
if (table.values[attr])
- name = table.values[attr](element, value);
+ name = table.values[attr](element, value) || name;

This comment has been minimized.

Show comment Hide comment
@sdumitriu

sdumitriu Aug 27, 2014

Contributor

The problem is that the function in ATTRIBUTE_TRANSLATIONS.values correct attribute values, not the names, so the returned value should be assigned to value, not name.

@sdumitriu

sdumitriu Aug 27, 2014

Contributor

The problem is that the function in ATTRIBUTE_TRANSLATIONS.values correct attribute values, not the names, so the returned value should be assigned to value, not name.

if (value === false || value === null)
element.removeAttribute(name);
else if (value === true)
View
@@ -1090,18 +1090,36 @@ new Test.Unit.Runner({
testElementWriteAttributeWithBooleans: function() {
var input = $('write_attribute_input'),
- select = $('write_attribute_select'),
- checkbox = $('write_attribute_checkbox'),
- checkedCheckbox = $('write_attribute_checked_checkbox');
+ select = $('write_attribute_select');
this.assert( input. writeAttribute('readonly'). hasAttribute('readonly'));
this.assert(!input. writeAttribute('readonly', false). hasAttribute('readonly'));
this.assert( input. writeAttribute('readonly', true). hasAttribute('readonly'));
this.assert(!input. writeAttribute('readonly', null). hasAttribute('readonly'));
this.assert( input. writeAttribute('readonly', 'readonly').hasAttribute('readonly'));
this.assert( select. writeAttribute('multiple'). hasAttribute('multiple'));
this.assert( input. writeAttribute('disabled'). hasAttribute('disabled'));
+ },
+ testElementWriteAttributeForCheckbox: function() {
+ var checkbox = $('write_attribute_checkbox'),
+ checkedCheckbox = $('write_attribute_checked_checkbox');
this.assert( checkbox. writeAttribute('checked'). checked);
+ this.assert( checkbox. writeAttribute('checked'). hasAttribute('checked'));
+ this.assertEqual('checked', checkbox.writeAttribute('checked').getAttribute('checked'));
+ this.assert(!checkbox. writeAttribute('checked'). hasAttribute('undefined'));
+ this.assert( checkbox. writeAttribute('checked', true). checked);
+ this.assert( checkbox. writeAttribute('checked', true). hasAttribute('checked'));
+ this.assert( checkbox. writeAttribute('checked', 'checked'). checked);
+ this.assert( checkbox. writeAttribute('checked', 'checked'). hasAttribute('checked'));
+ this.assert(!checkbox. writeAttribute('checked', null). checked);
+ this.assert(!checkbox. writeAttribute('checked', null). hasAttribute('checked'));
+ this.assert(!checkbox. writeAttribute('checked', true). hasAttribute('undefined'));
this.assert(!checkedCheckbox.writeAttribute('checked', false). checked);
+ this.assert(!checkbox. writeAttribute('checked', false). hasAttribute('checked'));
+ },
+ testElementWriteAttributeForStyle: function() {
+ var element = Element.extend(document.body.appendChild(document.createElement('p')));
+ this.assert( element. writeAttribute('style', 'color: red'). hasAttribute('style'));
+ this.assert(!element. writeAttribute('style', 'color: red'). hasAttribute('undefined'));
},
testElementWriteAttributeWithIssues: function() {

10 comments on commit 4a22a52

@Deses

This comment has been minimized.

Show comment Hide comment
@Deses

Deses Aug 27, 2014

With this change, this code now always sets the checkbox to checked no matter what on IE8 and Chrome, in IE10 it's working fine though.

new Element('input', {
    type:'checkbox', 
    id: 'name',
    name: 'name',
    checked: rowObj.checked,
    disabled: !rowObj.disabled
});

rowObj.checked and rowObj.disabled are booleans and both are false.

With this change, this code now always sets the checkbox to checked no matter what on IE8 and Chrome, in IE10 it's working fine though.

new Element('input', {
    type:'checkbox', 
    id: 'name',
    name: 'name',
    checked: rowObj.checked,
    disabled: !rowObj.disabled
});

rowObj.checked and rowObj.disabled are booleans and both are false.

@sdumitriu

This comment has been minimized.

Show comment Hide comment
@sdumitriu

sdumitriu Aug 27, 2014

Contributor

Oh, this code is so WRONG! What was I thinking?

Contributor

sdumitriu replied Aug 27, 2014

Oh, this code is so WRONG! What was I thinking?

@Deses

This comment has been minimized.

Show comment Hide comment
@Deses

Deses Aug 27, 2014

Hmm... so, how can I hot fix this?

I've been looking to the code and I don't have that ATTRIBUTE_TRANSLATIONS.values function! Mysterious!

Hmm... so, how can I hot fix this?

I've been looking to the code and I don't have that ATTRIBUTE_TRANSLATIONS.values function! Mysterious!

@sdumitriu

This comment has been minimized.

Show comment Hide comment
@sdumitriu

sdumitriu Aug 27, 2014

Contributor

I think this should work, but I didn't test it yet:

- name = table.values[attr](element, value) || name;
+ value = table.values[attr](element, value);
Contributor

sdumitriu replied Aug 27, 2014

I think this should work, but I didn't test it yet:

- name = table.values[attr](element, value) || name;
+ value = table.values[attr](element, value);
@Deses

This comment has been minimized.

Show comment Hide comment
@Deses

Deses Aug 27, 2014

That should do it, it was like that in 1.7.1 and it worked fine then, but I wasn't sure that it would break another thing.

Thank you, this is pretty critical for our application. :)

That should do it, it was like that in 1.7.1 and it worked fine then, but I wasn't sure that it would break another thing.

Thank you, this is pretty critical for our application. :)

@savetheclocktower

This comment has been minimized.

Show comment Hide comment
@savetheclocktower

savetheclocktower Aug 27, 2014

Collaborator

So was this a matter of insufficient test coverage, or did we not run the tests in enough browsers? I confess I do not remember how thoroughly I checked this before I merged it, but I swear I ran the tests in every browser known to man before the 1.7.2 release.

Collaborator

savetheclocktower replied Aug 27, 2014

So was this a matter of insufficient test coverage, or did we not run the tests in enough browsers? I confess I do not remember how thoroughly I checked this before I merged it, but I swear I ran the tests in every browser known to man before the 1.7.2 release.

@sdumitriu

This comment has been minimized.

Show comment Hide comment
@sdumitriu

sdumitriu Aug 27, 2014

Contributor

@Deses see #262 for the full fix.

@savetheclocktower Looks like insufficient browsers tested, it worked with FF and modern IE; not sure why it fails in Chrome, as @Deses claims.

Contributor

sdumitriu replied Aug 27, 2014

@Deses see #262 for the full fix.

@savetheclocktower Looks like insufficient browsers tested, it worked with FF and modern IE; not sure why it fails in Chrome, as @Deses claims.

@sdumitriu

This comment has been minimized.

Show comment Hide comment
@sdumitriu

sdumitriu Aug 27, 2014

Contributor

Speaking of tests, I get a lot of test failures with the current master. Is it just me?

Contributor

sdumitriu replied Aug 27, 2014

Speaking of tests, I get a lot of test failures with the current master. Is it just me?

@savetheclocktower

This comment has been minimized.

Show comment Hide comment
@savetheclocktower

savetheclocktower Aug 27, 2014

Collaborator

The problem is that all the DOM tests pass in 1.7.2 in Chrome. And if I try to extract @Deses's example code into a unit test…

var newUncheckedElement = new Element('input', {
  type: 'checkbox', 
  name: 'name',
  checked: false,
  disabled: false
});

var newCheckedElement = new Element('input', {
  type: 'checkbox',
  name: 'name',
  checked: true,
  disabled: false
});

this.assert(!newUncheckedElement.checked);
this.assert(newCheckedElement.checked);

…then everything still passes (on both Chrome 36 and Chrome 37). @Deses, you're running 1.7.2, right? What version of Chrome are you on?

Collaborator

savetheclocktower replied Aug 27, 2014

The problem is that all the DOM tests pass in 1.7.2 in Chrome. And if I try to extract @Deses's example code into a unit test…

var newUncheckedElement = new Element('input', {
  type: 'checkbox', 
  name: 'name',
  checked: false,
  disabled: false
});

var newCheckedElement = new Element('input', {
  type: 'checkbox',
  name: 'name',
  checked: true,
  disabled: false
});

this.assert(!newUncheckedElement.checked);
this.assert(newCheckedElement.checked);

…then everything still passes (on both Chrome 36 and Chrome 37). @Deses, you're running 1.7.2, right? What version of Chrome are you on?

@Deses

This comment has been minimized.

Show comment Hide comment
@Deses

Deses Aug 28, 2014

I was working with Chrome 36, and IE10 using "Standards" document mode and "IE8 Standards" document mode.

I'm really not sure what was the correct behavior (probably Chrome was the one that worked fine), but I wasn't getting consistent results on the two (three?) browsers.

The issue with our app is that we are upgrading from IE8 to IE10, but IE10 has this document mode stuff that renders pages depending on how badly is the code, but we force it to use the "Standard" mode using <meta http-equiv="X-UA-Compatible" content="IE=edge">. But somehow on IE8 all the check boxes were always checked and on IE10 all the check boxes were unchecked. And Chrome, IIRC, showed some check boxes checked and some weren't.

I know I'm not being very clear but I was frustrated and pretty confused, so I don't remember all the details, sorry. :(

Hope this helps!

I was working with Chrome 36, and IE10 using "Standards" document mode and "IE8 Standards" document mode.

I'm really not sure what was the correct behavior (probably Chrome was the one that worked fine), but I wasn't getting consistent results on the two (three?) browsers.

The issue with our app is that we are upgrading from IE8 to IE10, but IE10 has this document mode stuff that renders pages depending on how badly is the code, but we force it to use the "Standard" mode using <meta http-equiv="X-UA-Compatible" content="IE=edge">. But somehow on IE8 all the check boxes were always checked and on IE10 all the check boxes were unchecked. And Chrome, IIRC, showed some check boxes checked and some weren't.

I know I'm not being very clear but I was frustrated and pretty confused, so I don't remember all the details, sorry. :(

Hope this helps!

Please sign in to comment.