Problem/Motivation
When using a CTools modals to create a custom form, the CTools call to Drupal.attachBehaviors() results in #states (Form API) being applied twice.
Specifically, the original poster was using a form in D7 that asked the user whether or not she had a loyalty card and, if so, created a CTools modal that required the user to enter a loyalty card number. Since #states was being applied twice, the required field was being marked with two asterisks instead of the expected single asterisk.
The original discussion identifies three issues:
- Main issue: states.js allows #states to be applied multiple times. This is AJAX related since testing shows that if you are on the node/edit page, there is no issue.
- Secondary issues
- CTools does not pass an argument to Drupal.attachBehaviors() - #5
- Code in states.js function stateEventHandler() is self-referencing - tests for $.inArray(state, dependeeStates) but since state is by definition an element of dependeeStates, this is always true. Harmless code that doesn't need to be fixed to fix this issue. (#11)
Proposed resolution
Initially proposed that states.js code should use the .once method, but sun notes (#16)
it is perfectly legit to bind multiple different #states behaviors to a single element... This behavior currently works in HEAD and must be retained.
jgtrescazes suggests a fix that could address both the issue of allowing multiple state behaviors on an element and dealing with states applied to elements that have been removed via Ajax.
Unanswered: should there "be a call to Drupal.detachBehaviors() before reattach" to avoid the problem.
Remaining tasks
Solution in msg #17 needs to be rolled into a patch along with fixes suggested in #22. The solution has supposedly been tested D7 (#19), but no patch submitted. Tests cases are provided by fietserwin in #15, but it probably also needs more test cases to address the concerns raised by sun (#16).
User interface changes
None
API changes
None
Original report by jgtrescazes
In the project I'm working on, we implemented a custom form that use ctools modals (for help, terms of uses...). The form also use #states (FAPI) to change fields requirement.
CTool modals call function Drupal.attachBehaviors() that causes issues with #states as behaviors are applied twice after ctools modal call. ('*' is displayed twice next to field label)
To avoid this kind of problem with Ctools and other contributed modules that call Drupal.attachBehaviors(), and after reading this : http://drupal.org/node/114774#javascript-behavior, I wrote a patch for states.js that simply add a states-processed class while applying behavior and test that class is not present before attach behaviors.
Comment | File | Size | Author |
---|---|---|---|
#62 | 1592688-62.patch | 781 bytes | eric.chenchao |
#55 | 1592688-55.patch | 801 bytes | eric.chenchao |
#48 | interdiff.txt | 854 bytes | fietserwin |
#48 | 7545251-48.patch | 792 bytes | fietserwin |
#44 | 1592688-44.patch | 590 bytes | bogdan khrupa |
Comments
Comment #1
nod_Can you post the code showing the bug please? So that it can be reproduced.
Also, should be fixed on D8 first. #states js is the same for D8 and D7 so reproducing in either works.
Comment #3
jgtrescazes CreditAttribution: jgtrescazes commentedHere is a piece of the form's code (i stripped irrelevant code)
Here is the ctools modal_display code
The #state behavior is right on page loading but after click on ctools modal button it's not anymore when switching radio button there are two '*' displayed instead of one.
Comment #4
tstoecklerI'm not really a JS guy, but couldn't this use .once()?
Comment #5
nod_you're right this should be using .once.
There is two issues really, ctool should be passing an argument to Drupal.attachBehaviors() and this code doesn't use once.
Can you update the patch please? I can't rtbc if i'm involved :)
Comment #6
jgtrescazes CreditAttribution: jgtrescazes commentedI wasn't aware of that clever function.
Here's the patch using .once()
Comment #8
jgtrescazes CreditAttribution: jgtrescazes commentedIt seems that states.js has been moved on drupal 8.
Here's the same patch adapted for D8 filesystem
Comment #9
jgtrescazes CreditAttribution: jgtrescazes commentedComment #10
fietserwinI did some further research into this issue and #1655114: FAPI #states: required span added multiple times. They are closely related, so I will close the other one as a duplicate, hoping that this issue will solve both cases.
Test cases
Manual test case for D7 where #8 works. Add the following lines to image.field.inc, function function image_field_formatter_settings_form(), $element['image_link'], line 517:
Manual test case for D7 where #8 does not work. Add the following lines to image.field.inc, function function image_field_formatter_settings_form(), $element['image_link'], line 517:
How to test?
Make sure you have a content type with an image field. Go to "manage display" for that content type and open the settings for the image field. The logic behind the sates is something like "if you select thumbnail or medium as image style you must specify a link, otherwise it will not be linked". So play a bit with the settings and see what happens.
The code I abused for the test cases seems to be pretty much the same in D8. So I guess the test cases can be used in D8 as well.
Findings
The solution thus looks to be something like "the once behavior should be on the "element/attribute combination", not on the element alone." Can we pass in the "attribute" to once: once('states-required'), once('states-enabled'),etc. Do we have this info in the attach behavior?
Comment #11
fietserwinOne more "needs work":
Looking at the code in initializeDependee (in states.js):
$.inArray(state, dependeeStates) will always equals i!
So it seems that this code tries to achieve something similar asw eare trying to achieve here, but in an incorrect way. BTW: this code was introduced by #735528: FAPI #states: Fix conditionals to allow OR and XOR constructions.
Comment #12
jgtrescazes CreditAttribution: jgtrescazes commentedHi
I understand your point and yes we do have this info.
Here's the same patch including with once parameter set as 'states-' + state (variable containing current state) instead of simply 'states'.
It works in the test case you provide, I tried also using plural trigering elements and it looks right too.
Comment #13
jgtrescazes CreditAttribution: jgtrescazes commentedFor your comment #10, I agree that this "if" statement has no sense.
It seems that script looks for state in the bad array.
There is an other array that seems to be used to cache state's information
It might be that one that script use as haystack in inArray statement but it implyed changing also the condition...
May be people who wrote this code can provide more information about it...
I saw your post on the original thread and suscribe waiting someone answer to your question
Comment #15
fietserwinReplying to #11:
Yes, that was simple and works, but still has a problem:
If you also set the #required option at the server side, and you should as you may never rely on client-side (javascript based) validation only, you will get 2 markers if the settings you start with lead to the #required set to be true.
Changed the test case to show this:
Test: set style to thumbnail or medium, save and reopen the settings form again.
Solution: always remove any current required marker first: states.js, line 492 (in D7). (Note that this always works for every situation, the other solutions can be seen as just a performance optimization to prevent to many events being fired and handled.)
The patch also does not solve/remove the mentioned portion of bogus code. But as that code in essence seems harmless, we may defer that to a follow-up issue.
A last possible problem I see is that states.js allows to use aliases like visible versus invisible and required versus optional. Are these handled correctly? Thus if I have both an required and an optional state on a certain element, won't I get 2 markers if the required state evaluates to true and the optional to false? (Not with the solution introduced above in this comment ... but what if we solve that differently?)
Comment #16
sunThe location where .once() is invoked does not seem right to me.
With this code, a new Dependent is actually instantiated on subsequent calls, but it gets no element (an empty jQuery object) assigned. That seems bogus to me.
Additionally, it is perfectly legit to bind multiple different #states behaviors to a single element. For example, consider two other elements that both depend on the checked state of a single checkbox, and similar scenarios. This behavior currently works in HEAD and must be retained.
Furthermore, we also need to carefully consider the expectations of combined #states with #ajax here:
In that case, it is possible that an Ajax response contains new #states settings, which may need to override existing #states for a particular element; i.e., changing its behavior, since the element was replaced or whatever through the Ajax behavior.
Comment #17
jgtrescazes CreditAttribution: jgtrescazes commentedOk, you're right.
To avoid Dependant instancied on empty jquery elements, may be we could add a test like this :
And precising 'states-' + state like fietserwin purposed will allow multiple states behaviors on the target element.
This fix is not a problem when it applies on a form's part that is reloaded as it lose its old behaviors (for browser it is a new dom elements) and drupal attach behaviors that are set in code so if they change during ajax call, the new behaviors are applyed.
A problem can occur when the behavior that have been changed during ajax call is applied on an element that isn't part of the reloaded section. With this fix instead of cumulate old and new behaviors (like it's the case in HEAD) the behavior will not be updated as it has the processed class (added by .once()).
May be a call to Drupal.detachBehaviors() before reattach them would solve this problem ?
Comment #18
cweagansFixing tags per http://drupal.org/node/1517250
Comment #19
seanrI applied the suggested change in #17 and can confirm it does work. We were actually seeing a lot more than two required asterisks (it added three or four every time), and the fix above (applied to D7) resolved the issue for me.
Comment #20
BrockBoland CreditAttribution: BrockBoland commentedNeeds summary
Comment #21
Atomox CreditAttribution: Atomox commentedAny progress on a patch for D7 here? We're waiting on this issue for launch.
Comment #22
seutje CreditAttribution: seutje commented@#17:
if (element.size() > 0) {
could be replaced with
if (element.length) {
$.fn.size
is just a silly wrapper around grabbing the length property: https://github.com/jquery/jquery/blob/master/src/core.js#L184Comment #23
onelittleant CreditAttribution: onelittleant commentedFor those looking for a quick solution before this gets committed / backported to D7, try this CSS:
Comment #24
onelittleant CreditAttribution: onelittleant commentedFor those looking for a quick solution before this gets committed / backported to D7, try this CSS:
Comment #24.0
ergophobe CreditAttribution: ergophobe commentedIssue summary by ergophobe for status as of Jan 29, 2013
Comment #24.1
ergophobe CreditAttribution: ergophobe commentedformatting changes
Comment #25
ergophobe CreditAttribution: ergophobe commentedremoving Issue summary initiative tag
Comment #26
Dmitriy.trt CreditAttribution: Dmitriy.trt commentedWe've experienced similar issues with states system and here is a patch for D7 states to better handle AJAX requests and multiple calls to behaviors attaching on the same content in general. Major differences:
states.Dependent
instances and use instance ID for the dependee flag that lets us know if state event handler have been attached to this dependee or not. Full flag key is built from instance ID + state name + dependee selector.states.Dependent
instance to the element data and try to use existing instance. Instance key contains dependent selector + state.Solution is still far from perfect, because it doesn't handle changed settings passed with AJAX requests, it just re-attaches event handlers to new dependees properly.
Comment #27
nod_handling of #states has been changed, can you please retest to see if the problem still exists?
Comment #28
stefan.r CreditAttribution: stefan.r commentednod_: assuming this refers to D8?
I can find manual test instructions for D7 but not for D8, if anyone can provide these i am willing to test.
Comment #28.0
stefan.r CreditAttribution: stefan.r commentedformatting
Comment #29
stefan.r CreditAttribution: stefan.r commentedPosting #17 as a patch
Comment #30
stefan.r CreditAttribution: stefan.r commentedSetting back issue to previous status
Comment #31
lachezar.valchev CreditAttribution: lachezar.valchev commentedHi,
I did a small change to the patch in #29 as it was preventing the application of more then one state.
I particularly had a case where I was using visible and required states on one element and the second one was not applied.
Comment #32
tstoecklerI can verify that this solves the problem in Drupal 7. Attached patch is for jQuery Update module for anyone using that. Will try to reproduce this in D8 later.
Comment #33
hass CreditAttribution: hass commentedThis file will be removed asap from jquery update. See #1448490: Remove the states.js overwrite as it was fixed upstream
Comment #34
tstoecklerThe following test patch for Drupal 8 adds some form elements to the user login form. Interestingly I've found that #states are apparently not run at all on the part of the form that is being inserted via Ajax. I didn't debug that very much though, so there might be something wrong with the example code as well.
Comment #35
mgiffordCan we just get this issue fixed in D7 if we don't know we can reproduce it even in D8 (and it is done in a completely different way).
Comment #38
stefan.r CreditAttribution: stefan.r commentedComment #39
tvn CreditAttribution: tvn commentedComment #40
YesCT CreditAttribution: YesCT commentedLet's try the patch from #29 here on #2130501: Cosmetic issues on the node/add/project-module page (could also try it on 7.x in general). making that one active/needs review again.
Comment #41
danylevskyi#29 worked great in #2130501: Cosmetic issues on the node/add/project-module page.
Comment #42
Heine CreditAttribution: Heine commentedHere's a reroll of 31 . Also attached is a Drupal 7 test module where you can test several scenarios on scratch/states_issue . The modification in #31 of #29 is necessary to allow both visible and required states on the form element.
To test with the module, check "Fetch the first name textfield via AJAX", then play with the hide and require checkboxes. The disable checkbox on top is to demonstrate another issue, for which I will reopen #2226405: FAPI #states: dependent element added via AJAX initializes incorrectly if dependee has been initialized earlier , because this is quite a confusing issue atm.
Comment #43
Heine CreditAttribution: Heine commentedApologies, here's the test module (see 42).
Comment #44
bogdan khrupa CreditAttribution: bogdan khrupa commentedAdded flag to each element then States init.
So 'new states.Dependent()' never twice executed. This fixed problem with CTools modal forms.
Comment #45
mgiffordPatch still applies to D7. Seems like a simple fix. Would be great to get this RTBC & into Core.
Comment #46
fietserwinIt's a long time I had a look at thi sproblem, but my intuition (on quickly comparing #42 and #44) says that #42 is correct and that #44 is incorrect.
- What's wrong with once()? In Drupal I see that pattern more often than the "data pattern".
- The once is per selector per state, whereas the data is per selector. The latter seems wrong to me.
So I would like to go for the patch from #42 but with the following remark:
once() accepts a function as 2nd parameter to execute on the elements that "passed" the once. So we can combine this into something like:
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commentedI was using #42 with jQuery 1.8 (from https://drupal.org/project/jquery_update) and it breaks the WYSIWYG module. There's a JavaScript error on the page: "Syntax error, unrecognized expression: .states-!enabled-processed". After downgrading to jQuery 1.7 it works fine though.
Also are we sure this issue isn't still valid for Drupal 8? Doesn't look like it's been tested recently.
Comment #48
fietserwinI tried the tests from #10 and #34 and could NOT reproduce this in D8, while #10 still fails in d7. So, I think it is safe to continue solving it for D7 here.
Regarding the !enabled: this seems to be allowed, so we should cater for that. Luckily for us, there's already a function that handles this for us:
states.State.sanitize(): returns a states.State object whose property name will be sanitized (will not contain !). So attached patch should solve #46 and #47 (and is thus based on #42).
Comment #49
stefan.r CreditAttribution: stefan.r commented#48 still works well, can anyone else review? Would be good if we could get this to RTBC!
Comment #50
heddnApplied and tested #48, which resolves the issue more elegantly than #2125563: 'Required' state doesn't work correctly for a form element, which is now closed duplicate to this.
Marking RTBC.
Comment #51
tstoecklerAfter having bitten by this issue quite a few times and having this patch applied on a couple of live sites I'm no longer sure whether this patch is a good idea. The problem is that with this #states are no longer re-applied after Ajax requests. So when using #states in ajaxified forms this can lead to unwanted results. If an Ajax requests results in new form elements, for example, these will never participate in the #states system at all because all those behavior have already run (once). That has bitten me in combination with the Addressfield module for example.
I think in this case it really makes sense to fix the actual "symptom" of this issue only, i.e. the double required asterisks that appear after form elements when making form elements required via #states. Before adding such an asterisk we should check whether such an asterisk already exists and if so, simply bail.
Don't want to block this, though, just wanted to give my perspective because I used to very much endorse this issue but no longer do.
Comment #52
hass CreditAttribution: hass commentedComment #53
tstoecklerAlso I'm not entirely sure why this issue targets 7.x. In #34 I investigated whether this issue still exists in D8 and that was never conclusively disproven. I suspect it does, to be honest. If that were to be true this must be fixed in Drupal 8 first per our process.
Comment #54
fietserwinDifferences in D7 and D8 states.js are quite minimal (ignoring white space), so I guess #53 is correct.
#51 is more difficult:
(not sure if the case distinguishing below is correct, as context will differ, but settings might also differ)
- If dependee and all dependents are refreshed: no problem, once is no longer set and the events are gone, so code will run.
- If nor the dependee, neither any of the dependents are refreshed: no problem
- If dependee is refreshed, but a dependent not: the once is gone, so events will be refreshed. Some events will remain, but those address
a no longer existing target dependee and thus can be ignored.
- if dependee is not refreshed, but dependents are: the once remains but the events are gone: incorrect.
Perhaps we should do an unbind and then execute the current code as normal.
Comment #55
eric.chenchao CreditAttribution: eric.chenchao commentedThis issue has already been fixed in Drupal 8.0.x
Here is the code in D8
Regarding states working with Ajax, here is some thoughts:
- When I use form #ajax to implement field change event, the return element is this field element. However in the
Drupal.behaviors.states
the context is actually the form element instead. So we cannot use context to restrict which element needs to be applied with state.- use jQuery.once() to restrict that apply state once will make state broken after form AJAX request.
Solution:
1. A decent solution is to avoid state apply more than once. In order to work with AJAX, we have to detach state change and restore the form element status to the initial status and then apply state again after AJAX. But this is way too complex to implement. For example we have to store initial state and compare.
2. A simple way (preferred)
Do not add restriction and apply state again after AJAX request. This should not add too much overhead and works well for all bind events except for 'required'. So we just need to check if '*' has already been added before appending a new one.
When I check Drupal 8 and I have found this has already been implemented in D8 as the same way.
I have attached a patch for D7.
A test use case is:
I have a gift subscription form to let users bug gift and send to their friends. This form allows them to notify gift to their friend either by email or by mail.
So if user chooses by email, an email field shows up as required field. Or user have to enter address fields with all required fields appear.
In the address fields user can select country and state list will show up depending on what country is via AJAX.
Comment #56
eric.chenchao CreditAttribution: eric.chenchao commentedComment #57
fietserwinThis (#55) may explain why we were not able to show this error still exists in D8. BTW: According to this thread on Stack overflow, the check is superfluous, jquery already checks this. For D7 it would be needed as we are adding a DOM element, not a class.
If we are going for symptom solving, then be aware that all these bound events add up quickly and eventually will harm client side performance, even if the events all do the same and do not have visual side effects.
Comment #58
rahul.shinde#55 works for me.
Comment #59
Georgique CreditAttribution: Georgique commentedConfirming #55 works.
Comment #60
fietserwin- OK, let's go for symptom solving as per #51 and because solving it neatly would be extremely difficult as per #54.
- Tested as per #58 and #59
- Reviewed by me.
=> RTBC
Comment #61
David_Rothstein CreditAttribution: David_Rothstein commentedWhy aren't we doing this the same way Drupal 8 does?
The Drupal 8 code caches the selector in a variable so we don't needlessly run it twice, and also has a code comment explaining what's going on:
Especially given that we're solving the symptom for now rather than a direct fix, I think we should make sure we don't waste extra cycles running the same selector twice.
Comment #62
eric.chenchao CreditAttribution: eric.chenchao commentedUpdate patch with object cache suggested by @David_Rothstein
Comment #63
fietserwinThis patch addresses the concerns of #61.
BTW: the D8 code that #61 refers to is a bad example: the comment is superfluous as it is done purely with classes, not by adding a child element (twice) and addClass() already checks if the class is there so the hasClass() is superfluous as well (see e.g. http://stackoverflow.com/questions/13358887/should-i-use-hasclass-before...). But that should be a different issue.
Comment #64
seanrVery much like #62 - elegant.
Comment #65
pgandul CreditAttribution: pgandul commentedHi guys, patch #62 worked for me.
Comment #66
geertvd CreditAttribution: geertvd at XIO commented+1 for the fix in #62
Comment #67
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!
Nice and simple fix in the end, although since it didn't quite solve the full breadth of the problem implied by the original issue title, I'm retitling it now. I guess we can create a new issue if we want to keep looking for a more comprehensive fix.
Comment #70
quicksketchAdding related issue #1091852: Display Bug when using #states (Forms API) with Ajax Request, which addresses the original problem this issue did not: the double-binding of the #states behavior.