In the same spirite than #1480568: use $.attr with false instead of empty string I would like to fix states.js to make it compatible with jQuery we have in core *and* more recent version.

I'm explaining the problem with new jquery version in #1498858: [meta] attr and prop.

The corresponding issue on jQuery Update #1448490: Remove the states.js overwrite as it was fixed upstream.

From my first reading of the code I think the problem is in

states.Trigger.states = {
  // 'empty' describes the state to be monitored
  empty: {
    // 'keyup' is the (native DOM) event that we watch for.
    'keyup': function () {
      // The function associated to that trigger returns the new value for the
      // state.
      return this.val() == '';
    }
  },

  checked: {
    'change': function () {
      return this.attr('checked');
    }
  },

checked.change can certainly return undefined. So we need to return false if we have anythings but true.

So somethings like that (untested)

 checked: {
    'change': function () {
      if(this.attr('checked') === true){
       return true;
      }else {
       return false;
     }
    }
  },
CommentFileSizeAuthor
#8 states.patch294 bytesdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gagarine’s picture

Assigned: Unassigned » gagarine
marcingy’s picture

Status: Active » Closed (won't fix)

Drupal 7 does not support versions of jquery higher than 1.4.

gagarine’s picture

Status: Closed (won't fix) » Active

The script was working on 1.4 but it was already not the recommended way to use the API. So I think it will not hurt to fix it and as a side effect it will make it works on jQuery 1.6.1+ and save countless hours to a lot of peoples.

Renee S’s picture

Given the number of installs of jQuery Update for people who want to used advanced jQuery, and given that this fix preserves backwards compatibility, why not?

marcingy’s picture

So jquery update ships with replace file for states in 1.7+, so really the issue is one for jquery update not core.

nod_’s picture

Version: 7.x-dev » 7.9

It's ok we can update core to make it easier on contrib, we just have to not break stuff.

The only way this moves along is if a patch is posted :)

nod_’s picture

Version: 7.9 » 7.x-dev

also, proper version.

droplet’s picture

Version: 7.9 » 7.x-dev
Status: Active » Needs review
FileSize
294 bytes

maybe this way or access DOM directly. I think it will be accepted

goron’s picture

Patch in #8 worked for me

Renee S’s picture

Status: Needs review » Reviewed & tested by the community
gagarine’s picture

Patch #8 look good. It's one of the recommended way to solve this problem http://api.jquery.com/attr/

nod_’s picture

+1 on RTBC too

Renee S’s picture

#8 is the way to go. Works beautifully.

hass’s picture

That's really all? Just wondering why jquery_update has a states.js with tons of changes... Why are there so many differences?

oriol_e9g’s picture

Tested! This works, makes the live easier to developers and don't break anything so... why not? RTBC+

hass’s picture

Issue tags: +7.24 release blocker

Just to make sure this is not getting missed by the maintainer of D7.

gagarine’s picture

@hass I never get why they "didn't want" to fix drupal core when is possible.... for a long time I think nobody really dig into this issue.

gagarine’s picture

Issue summary: View changes

typo

presleyd’s picture

I've tested this with and without jquery_update now for a couple of days and I can't find any problems. It causes jquery_update to not need to keep their own copy of states.js and fixes conditional_fields for jquery 1.7.

David_Rothstein’s picture

Issue summary: View changes
Issue tags: -7.24 release blocker
David_Rothstein’s picture

7.24 was a security release (that's why I removed the tag). Seems like this should definitely go into the next bugfix release, but no reason for it to be a release blocker either.

sbrattla’s picture

Any chance this will go into any upcoming bug fix release of Drupal?

oriol_e9g’s picture

Issue tags: +7.25 release blocker
oriol_e9g’s picture

Issue tags: -7.25 release blocker
sbrattla’s picture

This question is probably mostly due to my uncomplete knowledge of the process ahead, but what will make this patch go into a next release?

Is it the fact that it's tagged with a certain tag, or the fact that it is set as "Reviewed & Tested By The Community" - or does the patch have to be applied by a maintainer?

droplet’s picture

2 months. it seems the maintainer skipped this patch for some reason ? Dear maintainers, can you share your thought?

System Lord’s picture

I'm not afraid to say it...I'm confused as all heck! I need jquery_update to be 1.7. My CF 7.x-3.0-alpha1 doesn't like it. So, #8 patch is all I need?

I think I'm confused because on another similar thread there is a #8 patch that I "thought" fixed this problem, but the patch is completely different.

Would anyone mind telling me which patch I need to make CF play well with jquery_update 1.7?

Renee S’s picture

System Lord: Likely. You can replace states.js with the jQuery Update version, but this patch should fix most wayward checkbox behaviour, so you might not need it.

System Lord’s picture

Would /project/jqeries work for this issue?

System Lord’s picture

Sorry, I'm still confused. Going back to my #26 do I need the #8 patch from this thread or the #8 patch from https://drupal.org/node/1448490, which is where gagarine brought this discussion from?

System Lord’s picture

Also, should I be using the DEV at this point?

droplet’s picture

@System Lord,

Please report your problems with details in new thread. May be report it to "CF 7.x-3.0-alpha1" issue thread ?

System Lord’s picture

I'm currently using the alpha1, but it's giving me the same problems described in this thread (or at least where this thread originated from). I'm posting here because it sounds like patch #8 is a solution if I use the DEV. This is what I'm trying to find out. Let me phrase my question differently...

If I use the DEV and install patch #8 above it will solve the jquery issue?

Additionally, look at gagarines third paragraph at the top of this thread. In the thread he references there: #1448490: fix states.js upstream and remove the overwrite in jQuery Update, there seems to be another solution patch #8. Hence my confusion.

SocialNicheGuru’s picture

this fix went into core:
https://drupal.org/node/735528

gagarine’s picture

8: states.patch queued for re-testing.

gagarine’s picture

@SocialNicheGuru I don't get it? Do you mean #735528: FAPI #states: Fix conditionals to allow OR and XOR constructions also fix this one?

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
System Lord’s picture

+1 #35

SocialNicheGuru’s picture

#33, yes

gagarine’s picture

Yes! Just before new year!

System Lord’s picture

Ok, what's the bottom line here? lol What fixed what, where?

Status: Fixed » Closed (fixed)

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

DrCord’s picture

The states.js file still uses .attr() inappropriately.

line 484 - disabled
line 486 - disabled
line 512 - checked

From the jQuery 1.9 change guide [http://jquery.com/upgrade-guide/1.9/#attr-versus-prop-]

jQuery 1.6 introduced the .prop() method for setting or getting properties on nodes and deprecated the use of .attr() to set properties. However, versions up to 1.9 continued to support using .attr() for specific situations. This behavior in the name of backwards compatibility causes confusion when selectors are used that distinguish between attributes and properties.

For example, boolean attributes such as checked and disabled on a checkbox are affected by this change.