In the case where a country field is selected, the addresses module loads addresses.js in order to replace the province textfield with a select field using an ajax call to function _addresses_province_ajax().

The problem that I am seeing is that currently this process does not maintain any attributes that are set for the textfield across the switch between text/selection field.

For instance, if I set an attribute of "tabindex" using a form_alter, the selection field that replaces the province selection field will not have an attribute of "tabindex".

Currently the only way of overriding this behavior is either by hacking the address module directly, or by overriding the 'addresses/province_ajax' menu and altering the callback function.

This problem can be fixed by sending along additional attributes in the json call that can be added to the return element, and/or by wrapping the return element in a theme function that can be more easily overridden by themers or developers.

CommentFileSizeAuthor
#3 fixes-addresses-loss-attributes-at-ajax-1216746-2.patch2.44 KBdkinzer
#1 fixes-addresses-loss-attributes-at-ajax-1216746.patch2.44 KBdkinzer

Comments

dkinzer’s picture

dkinzer commented
Status: Active » Needs review
StatusFileSize
new fixes-addresses-loss-attributes-at-ajax-1216746.patch2.44 KB
AlexisWilke’s picture

AlexisWilke commented
Status: Needs review » Needs work

Hi dkinzer,

Misspelled variable: $field_attribuets

Why do you use the quotes around attributes when all the other fields don't use such?

+ language:Drupal.settings.addresses.language,
+ 'attributes' : JSON.stringify(attributes)

Thank you for taking the time and providing a patch!
Alexis

dkinzer’s picture

dkinzer commented
Status: Needs work » Needs review
StatusFileSize
new fixes-addresses-loss-attributes-at-ajax-1216746-2.patch2.44 KB

Thanks. I've made the correction.

If I don't use the quotes around attributes, then I'm worried that jquery will get confused because

var attributes

is already defined as an object.

AlexisWilke’s picture

AlexisWilke commented

dkinzer,

Okay, cool. I have one more question. In the following unnamed function you have a case where you return true and the other case splits through (i.e. returns 'undefined'.)

      $.each(provinceElement[0].attributes, function(index, attr) {
        if (attr.name in {'type':1, 'value':1, 'size':1, 'id':1, 'name':1, 'class': 1}) {
          return true;
        }
        attributes[attr.name] = attr.value;
      });

Should there be a return after the assignment?

Thank you.
Alexis

dkinzer’s picture

dkinzer commented

Hi Alexis,

In this case, returning true inside the `$.each(...)` construct is like `continue` in a PHP `foreach` statement. Also, if I were to return `false`, it would be like using a `break` in PHP.

So there is no need to return anything here. That `if` statement is simply a catch for attributes that we don't want to include in the object. And, returning `true` is the only way to `continue` to the next attribute.

(Admittedly, it would be nice if we could `continue` or `break` here :)

Cheers,

D

AlexisWilke’s picture

AlexisWilke commented

I suppose they accept undefined as a continue as well because the end of the function exists without a return. That means 'undefined' is returned... (see comment below)

      $.each(provinceElement[0].attributes, function(index, attr) {
        if (attr.name in {'type':1, 'value':1, 'size':1, 'id':1, 'name':1, 'class': 1}) {
          return true;
        }
        attributes[attr.name] = attr.value;
        // return undefined; !
      });

Another way would be to use this instead:

      $.each(provinceElement[0].attributes, function(index, attr) {
        if (!(attr.name in {'type':1, 'value':1, 'size':1, 'id':1, 'name':1, 'class': 1})) {
          attributes[attr.name] = attr.value;
        }
        return true;
      });

Anyway... if you tested and it worked as intended I can keep your first version.

Thank you.
Alexis

dkinzer’s picture

dkinzer commented

Yes, what you did will work too. Though, it's not the standard way of using the $.each() function @see http://api.jquery.com/jQuery.each/

We can break the $.each() loop at a particular iteration by making the callback function return false. Returning non-false is the same as a continue statement in a for loop; it will skip immediately to the next iteration.

Thus, returning `undefined` is correct here because it's a non-false.

Though looking at your suggestion I guess the following would work too and is more concise:

      $.each(provinceElement[0].attributes, function(index, attr) {
        if (!(attr.name in {'type':1, 'value':1, 'size':1, 'id':1, 'name':1, 'class': 1})) {
          attributes[attr.name] = attr.value;
        }
      });
AlexisWilke’s picture

AlexisWilke commented
Status: Needs review » Fixed

Okay, that sounds good. I have checked it in.

Thank you.
Alexis Wilke

dkinzer’s picture

dkinzer commented

Cool. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

AlexisWilke’s picture

AlexisWilke commented
Status: Closed (fixed) » Active

dkinzer,

Some people are saying that they have problems because JSON is undefined. See #1312492: JSON in Addresses breaks Drupal fieldsets in IE

Any idea what they're missing in that regard?

Thank you.
Alexis Wilke

dkinzer’s picture

dkinzer commented

Yes. It looks like the JSON object is not defined for those systems. The fix should be that we check for it and then revert to the original way of doing things.

I think it's a straight forward fix, but I probably wont have time to submit a patch until this weekend.

I Hope that's okay. Best regards,

D

dkinzer’s picture

dkinzer commented

Or better yet, if JSON does not exist we can load an object that will do the same for us:

http://stackoverflow.com/questions/1480393/alternatives-of-json-stringif...

dkinzer’s picture

dkinzer commented
Status: Active » Closed (fixed)

Hi Alexis,

I added a patch to fix #1312492: JSON in Addresses breaks Drupal fieldsets in IE so I'm closing this here as to not duplicate efforts.

Best regard,

David

AlexisWilke’s picture

AlexisWilke commented

David,

No problem. I re-activated this issue so you'd know about the problem. 8-)

Thank you for the fix!
Alexis Wilke