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();
       }
     }
   });
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

peterpoe’s picture

Title: Don't append span.form-required to every label within the dependent field » states.js appends span.form-required to every label of a dependent field
Project: Conditional Fields » Drupal core
Version: 7.x-3.x-dev » 8.x-dev
Component: Javascript » javascript
Issue tags: +FAPI #states

This 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.

peterpoe’s picture

Attachments disappeared :0

frazras’s picture

I 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.

mducharme’s picture

Version: 8.x-dev » 7.12

Not sure why this was moved to the 8.x branch since it is still a bug in core 7.x

nod_’s picture

Version: 7.12 » 8.x-dev

Because we fix in the development version first which is 8.x then backport to 7.x

mgifford’s picture

Issue tags: -FAPI #states

Status: Needs review » Needs work
Issue tags: +FAPI #states

The last submitted patch, states-required-label-1239930-1.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript

Tagging.

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.

frazras’s picture

I 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

mgifford’s picture

@frazras - can you roll a new patch against D8? It's a challenge to keep up with Core.

xjm’s picture

Issue tags: +Novice

Tagging 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.

Status: Needs review » Needs work

The last submitted patch, states-required-label-1239930-1.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
965 bytes

Reroll for 8.x.

nod_’s picture

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).

xjm’s picture

Issue tags: +Needs manual testing

Alrighty; time to test manually in all browsers then! IE 6-9, FF, Webkit.

To test this:

  1. Install core.
  2. Install the states exercise module: http://drupal.org/sandbox/rfay/1269964
  3. Test the conditions for the "required" state in several tabs. You should see this:
  4. Apply the patch in #14 and clear the site cache.
  5. Confirm whether the issue is resolved. This is the expected change:
  6. Confirm that there are no other regressions in the behavior of the required state.

We will look at this during office hours as well, hopefully with folks who've used the states exercise module already.

mducharme’s picture

FYI: 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.

xjm’s picture

#16: This might be due to a git access issue a few days back. It works fine for me currently.

droplet’s picture

Status: Needs review » Needs work

How 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.

+++ b/core/misc/states.jsundefined
@@ -503,11 +503,13 @@ $(document).bind('state:disabled', function(e) {
+    var label = 'label' + e.target.id ? '[for=' + e.target.id + ']' : '';

It's always evaluated to be TRUE.

xjm’s picture

Status: Needs work » Needs review

I don't understand #18, but this still needs manual testing.

xjm’s picture

Use these instructions for checking out the correct branch of the module:
http://drupal.org/project/1269964/git-instructions

irunflower’s picture

FileSize
935 bytes

Rerolled.

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.

droplet’s picture

Status: Needs review » Needs work

+ var label = 'label' + e.target.id ? '[for=' + e.target.id + ']' : '';


var label = 'label' + null ? '[for=' + e.target.id + ']' : '';

//

var label = 'label' + anything ? '[for=' + e.target.id + ']' : '';

=

[for=' + e.target.id + ']

@xjm,

steps in #15 cannot reproduce this bug.

xjm’s picture

Status: Needs work » Needs review

@droplet, Yes they do. You need to install the #states exercise module and then trigger the state by setting a matching condition.

droplet’s picture

FileSize
1.02 KB

Conditional 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.)

nod_’s picture

Status: Needs review » Needs work

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.

nod_’s picture

Actually if #21 is rerolled with this change:
'label' + (e.target.id ? '[for=' + e.target.id + ']' : '');

That should do it. Parents were missing there.

irunflower’s picture

#21 rerolled with the change in #26.

irunflower’s picture

Status: Needs work » Needs review
FileSize
937 bytes

Let's try this again...

irunflower’s picture

Rerolled. Removed trailing space in first line and fixed typo.

irunflower’s picture

FileSize
318.51 KB
92.69 KB

For 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.

droplet’s picture

target.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.

irunflower’s picture

I 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.

droplet’s picture

@#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')

irunflower’s picture

The view has changed since we troubleshooted this last. Something has changed, but xjm is going to look at it.

cweagans’s picture

Issue tags: +Needs backport to D7
babruix’s picture

Issue summary: View changes
FileSize
814 bytes

Re-rolled, need manual testing.

cmah’s picture

Novice question - I can't get the patch in #36 to apply. Am I applying it incorrectly?

mymachine: drupal_8_sandbox (8.x) $ git apply -v states-required-label-1239930-36.patch 
Checking patch core/misc/states.js...
error: while searching for:

  $(document).on('state:required', function (e) {
    if (e.trigger) {
      if (e.value) {
        var $label = $(e.target).attr({ 'required': 'required', 'aria-required': 'aria-required' }).closest('.form-item, .form-wrapper').find('label');
        // Avoids duplicate required markers on initialization.
        if (!$label.find('.form-required').length) {
          $label.append(Drupal.theme('requiredMarker'));

error: patch failed: core/misc/states.js:516
error: core/misc/states.js: patch does not apply
droplet’s picture

it may has some new changes druing the time. try apply with ignore spaces.

git apply --ignore-space-change --ignore-whitespace

droplet’s picture

cmah’s picture

That still didn't work for me. It's probably just me. I'll try a different issue later to make sure.

lokapujya’s picture

The 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.

vanessakovalsky’s picture

Status: Needs review » Needs work

The last submitted patch, 36: states-required-label-1239930-36.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
804 bytes

Rerolled.

dcam’s picture

Issue tags: -Novice

Updating 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.

jhedstrom’s picture

Patch 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.

Status: Needs review » Needs work

The last submitted patch, 44: states-required-1239930-44.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
800 bytes

Reroll.. I think the spaces were removed between the brackets..

ohthehugemanatee’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
800 bytes
114.8 KB

Patch 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.
required asterisks not appearing on every checkbox

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/misc/states.js
@@ -523,8 +523,9 @@
+      var label = 'label' + (e.target.id ? '[for=' + e.target.id + ']' : '');
       if (e.value) {
-        var $label = $(e.target).attr({'required': 'required', 'aria-required': 'aria-required'}).closest('.form-item, .form-wrapper').find('label');
+        var $label = $(e.target).attr({'required': 'required', 'aria-required': 'aria-required'}).closest('.form-item, .form-wrapper').find(label);

I think the line starting with var label = can go after the if (e.value) {.

lokapujya’s picture

.

mgifford’s picture

Status: Needs work » Needs review
FileSize
800 bytes

a 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.

droplet’s picture

From 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..

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Just 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

Jelle_S’s picture

Patch looks solid to me. RTBC++

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: states-required-1239930-53.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
832 bytes
attiks’s picture

Status: Needs review » Reviewed & tested by the community

@mgifford thanks for the reroll, marking as RTBC assuming bot will be happy

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed c7de07a and pushed to 8.0.x. Thanks!

  • alexpott committed c7de07a on 8.0.x
    Issue #1239930 by irunflower, mgifford, babruix, peterpoe,...

  • alexpott committed c7de07a on 8.1.x
    Issue #1239930 by irunflower, mgifford, babruix, peterpoe,...

  • alexpott committed c7de07a on 8.3.x
    Issue #1239930 by irunflower, mgifford, babruix, peterpoe,...

  • alexpott committed c7de07a on 8.3.x
    Issue #1239930 by irunflower, mgifford, babruix, peterpoe,...

  • alexpott committed c7de07a on 8.4.x
    Issue #1239930 by irunflower, mgifford, babruix, peterpoe,...

  • alexpott committed c7de07a on 8.4.x
    Issue #1239930 by irunflower, mgifford, babruix, peterpoe,...
cslevy’s picture

In 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>

cslevy’s picture

Never mind. The #states was attached in the wrong place

sjerdo’s picture

Status: Patch (to be ported) » Needs review
FileSize
700 bytes

Backport of patch #58 to D7.

Status: Needs review » Needs work

The last submitted patch, 69: states-required-1239930-69.patch, failed testing. View results

sjerdo’s picture

Status: Needs work » Needs review

Reran tests. All tests passed, so I will update the status to Needs review.

sjerdo’s picture

Status: Fixed » Closed (fixed)

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