Problem/Motivation
Dependent fields can be marked as required if the dependee is in a certain state. If the dependent field contains many labels, as in the case of a list of checkboxes, the present behavior is to add the .form-required * to all of them. This makes it look like the user needs to check every checkbox for the dependent field.
Proposed resolution
If the target/dependent has an id, only look for labels declaring that id in their "for" attribute.
diff -ur /tmp/conditional_fields/js/states.js conditional_fields/js/states.js
--- /tmp/conditional_fields/js/states.js 2011-07-28 14:46:07.000000000 -0400
+++ conditional_fields/js/states.js 2011-08-04 17:56:24.881436765 -0400
@@ -507,11 +507,12 @@
$(document).bind('state:required', function(e) {
if (e.trigger) {
+ var label = $(e.target).closest('.form-item, .form-wrapper').find('label' + (e.target.id ? '[for='+e.target.id+']' : ''));
if (e.value) {
- $(e.target).closest('.form-item, .form-wrapper').find('label').append('<span class="form-required">*</span>');
+ label.append('<span class="form-required">*</span>');
}
else {
- $(e.target).closest('.form-item, .form-wrapper').find('label .form-required').remove();
+ label.find('.form-required').remove();
}
}
});
Comment | File | Size | Author |
---|---|---|---|
#58 | states-required-1239930-58.patch | 832 bytes | mgifford |
#53 | states-required-1239930-53.patch | 800 bytes | mgifford |
#50 | Screenshot 2015-05-16 13.21.14.png | 114.8 KB | ohthehugemanatee |
#50 | states-required-1239930-50.patch | 800 bytes | ohthehugemanatee |
#49 | states-required-1239930-49.patch | 800 bytes | mgifford |
Comments
Comment #1
peterpoe CreditAttribution: peterpoe commentedThis is actually a core bug (Conditional Field's states.js is just a clone of core's with modifications).
(Just fixed in Conditional Fields though).
Attaching a screenshot showing the error with checkboxes and radio buttons, and a patch against core with modified version of the solution shown above.
Comment #2
peterpoe CreditAttribution: peterpoe commentedAttachments disappeared :0
Comment #3
frazras CreditAttribution: frazras commentedI manually applied this patch because it wasn't applying to my version on states.js however the addressed issue seemed to have been fixed from my tests. see before and after files attached.
Comment #4
mducharme CreditAttribution: mducharme commentedNot sure why this was moved to the 8.x branch since it is still a bug in core 7.x
Comment #5
nod_Because we fix in the development version first which is 8.x then backport to 7.x
Comment #6
mgifford#2: states-required-label-1239930-1.patch queued for re-testing.
Comment #8
mgiffordTagging.
Want to try to bring this back into D7, but first gotta get it into D8.
@frazras I'm not clear if this patch from August fixed your problem or not.
To me the BEFORE screenshot looks better than the AFTER as far as required messages go. I think I'm missing something though.Comment #9
frazras CreditAttribution: frazras commentedI manually applied the patch in #2 to a drupal 8 install, for some reason the line numbers had changed.
The screenshots showed that instead of putting the REQUIRED asterisk beside every element in a multi-option field it only put the required asterisk on the label
Comment #10
mgifford@frazras - can you roll a new patch against D8? It's a challenge to keep up with Core.
Comment #11
xjmTagging novice for a reroll against current 8.x. We'll also take a look during next week's core office hours if it isn't picked up before then.
Comment #13
star-szrReroll for 8.x.
Comment #14
nod_reroll with extra ";" removed and direct access to "e.target.id" (that kind of change went in earlier in an issue for autosubmit and in a followup clean up issue).
Comment #15
xjmAlrighty; time to test manually in all browsers then! IE 6-9, FF, Webkit.
To test this:
We will look at this during office hours as well, hopefully with folks who've used the states exercise module already.
Comment #16
mducharme CreditAttribution: mducharme commentedFYI: Trying to checkout the http://drupal.org/sandbox/rfay/1269964 results in:
warning: remote HEAD refers to nonexistent ref, unable to checkout.
I'm just going to write a test myself and dump screenshots for these browsers.
Comment #17
xjm#16: This might be due to a git access issue a few days back. It works fine for me currently.
Comment #18
droplet CreditAttribution: droplet commentedHow to reproduce this ??
I believe xjm post a wrong ref in #15 or forgot commit the changes to repo.
#16, that repo missing mater branch, you have to checkout yourself.
It's always evaluated to be TRUE.
Comment #19
xjmI don't understand #18, but this still needs manual testing.
Comment #20
xjmUse these instructions for checking out the correct branch of the module:
http://drupal.org/project/1269964/git-instructions
Comment #21
irunflower CreditAttribution: irunflower commentedRerolled.
Tried to apply this patch today in office hours. Came back with:
patching file core/misc/states.js
patch unexpectedly ends in middle of line
Hunk #1 succeeded at 503 with fuzz 1.
Comment #22
droplet CreditAttribution: droplet commented=
[for=' + e.target.id + ']
@xjm,
steps in #15 cannot reproduce this bug.
Comment #23
xjm@droplet, Yes they do. You need to install the #states exercise module and then trigger the state by setting a matching condition.
Comment #24
droplet CreditAttribution: droplet commentedConditional Fields replaced the JS file, disable and can reproduce the bug.
Here's my version:
)
(optimization assumed 50% users may change the states one or two times, otherwise we can get rid of if-condition.)
Comment #25
nod_umm i'm not totally sure about the .prev thing. What if a theme messes up the markup? using the for attribute would be more reliable.i'd say just keep using the .closest().find() thing from before, it's less likely to break.and I'd just append the string directly,
var formRequired
is not very useful here since the selector is very simple there isn't too much going on on the same line like before.Comment #26
nod_Actually if #21 is rerolled with this change:
'label' + (e.target.id ? '[for=' + e.target.id + ']' : '');
That should do it. Parents were missing there.
Comment #27
irunflower CreditAttribution: irunflower commented#21 rerolled with the change in #26.
Comment #28
irunflower CreditAttribution: irunflower commentedLet's try this again...
Comment #29
irunflower CreditAttribution: irunflower commentedRerolled. Removed trailing space in first line and fixed typo.
Comment #30
irunflower CreditAttribution: irunflower commentedFor some reason the fields don't expand or collapse when triggered.
Is this because the field is empty?This is before a patch has been applied. I have tested with a patch applied and it still doesn't change the collapse/expand fields (look at the arrow icons, they don't change).As for the pictures - the picture on the left is before the trigger, picture on right is after trigger. Firefox is the only browser that greys out a field (compared to Safari, Chrome, Opera). Haven't tested on IE yet.
EDIT: I should also note that even though Firefox greys out the "disabled field" and other browsers don't, that doesn't mean it's not disabled. If you try to click on a disabled field in Chrome/Safari/Opera, it won't let you.
Comment #31
droplet CreditAttribution: droplet commentedtarget.id is better in some case but by defaults markups I don't see any good reason to use it. use CSS to hide element is much better.
Comment #32
irunflower CreditAttribution: irunflower commentedI installed 7-dev, installed _states_exercise and found two things:
1) I still can't seem to get back to the "screenshot view" (#15). It looks like my screen shots (#30)
2) Collapsed/Expanded fields work in D7, but not D8. You can see the icons change in D7.
Comment #33
droplet CreditAttribution: droplet commented@#32,
1, Yes, me too. but xjm at #23 said she can. I guess she forgot to push updates to remote repo :)
2, see #1419968: Replace $('selector', domelement) with $(domelement).find('selector')
Comment #34
irunflower CreditAttribution: irunflower commentedThe view has changed since we troubleshooted this last. Something has changed, but xjm is going to look at it.
Comment #35
cweagansUpdating tags per http://drupal.org/node/1517250
Comment #36
babruix CreditAttribution: babruix commentedRe-rolled, need manual testing.
Comment #37
cmah CreditAttribution: cmah commentedNovice question - I can't get the patch in #36 to apply. Am I applying it incorrectly?
Comment #38
droplet CreditAttribution: droplet commentedit may has some new changes druing the time. try apply with ignore spaces.
git apply --ignore-space-change --ignore-whitespace
Comment #39
droplet CreditAttribution: droplet commented36: states-required-label-1239930-36.patch queued for re-testing.
Comment #40
cmah CreditAttribution: cmah commentedThat still didn't work for me. It's probably just me. I'll try a different issue later to make sure.
Comment #41
lokapujyaThe patch applied ok for me. We'll need advice on how to test this now in D8 since the "states exercise" module is out of date.
Comment #42
vanessakovalsky CreditAttribution: vanessakovalsky commented36: states-required-label-1239930-36.patch queued for re-testing.
Comment #44
babruix CreditAttribution: babruix commentedRerolled.
Comment #45
dcam CreditAttribution: dcam commentedUpdating in preparation for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....
This was tagged as a Novice issue in #11 because it needed a reroll at that time. The last patch in #44 applies to HEAD, so I'm removing the tag.
Comment #46
jhedstromPatch still applies. I tried to reproduce the issue by editing parts of the views ui to make various checkboxes that have dependent checkboxes required (eg, in
FieldPluginBase::buildOptionsForm()
), but was unable to.Comment #49
mgiffordReroll.. I think the spaces were removed between the brackets..
Comment #50
ohthehugemanatee CreditAttribution: ohthehugemanatee as a volunteer commentedPatch applied, though had to find an offset. Attaching a re-roll.
I wrote a simple form testing module for this: https://github.com/ohthehugemanatee/form-test . Enable the module and visit /test-form to see the checkboxes and radios.
Works for me.
Comment #51
alexpottI think the line starting with
var label =
can go after theif (e.value) {
.Comment #52
lokapujya.
Comment #53
mgifforda re-roll as per #51.
Agreed that there is no need to have this outside of the if statement as it isn't used elsewhere.
Comment #54
droplet CreditAttribution: droplet commentedFrom code review, it's improved but I can't reproduce this error myself. Demo module in #50 isn't an Ajax request and even I modify few code myself I still didn't hit the bug..
Comment #55
attiks CreditAttribution: attiks at Attiks commentedJust run into this problem when fixing #2481635: sizes is now mandatory in the spec, patch looks good so the sooner it gets committed the better
Comment #56
Jelle_SPatch looks solid to me. RTBC++
Comment #58
mgiffordComment #59
attiks CreditAttribution: attiks at Attiks commented@mgifford thanks for the reroll, marking as RTBC assuming bot will be happy
Comment #60
alexpottCommitted c7de07a and pushed to 8.0.x. Thanks!
Comment #67
cslevy CreditAttribution: cslevy commentedIn drupal 8 this patch breaks the functionality on node add/edit form. The * won't appear.
var label = 'label' + (e.target.id ? '[for=' + e.target.id + ']' : '');
Simple implementation:
1 checkbox, 1 textfield. When the checkbox is checked the field should be required.
My textfield machine_name is field_text.
The label generated with
var label = 'label' + (e.target.id ? '[for=' + e.target.id + ']' : '');
is
label[for=edit-field-text-wrapper]
but the actual label should be:
<label for="edit-field-text-0-value" class="js-form-required form-required">Text</label>
Comment #68
cslevy CreditAttribution: cslevy commentedNever mind. The #states was attached in the wrong place
Comment #69
sjerdoBackport of patch #58 to D7.
Comment #71
sjerdoReran tests. All tests passed, so I will update the status to Needs review.
Comment #72
sjerdoMoved backport of this patch to #2791899: [D7] #states api "required" state incorrectly adds required-asterisk to all individual radio inputs for a "radios" element