states.js appends span.form-required to every label of a dependent field

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.

Attachments disappeared :0

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.

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

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

#2: states-required-label-1239930-1.patch queued for re-testing.

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.

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

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

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.

Reroll for 8.x.

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

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.

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.

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

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.

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

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

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.

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

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

Conditional Fields replaced the JS file, disable and can reproduce the bug.

Here's my version:

• fixed above bug
• optimized for performance
  • use prev() instead of find form attr and closest
  • use css instead of dom (http://jsperf.com/append-remove-using-dom-vs-css
    )

(optimization assumed 50% users may change the states one or two times, otherwise we can get rid of if-condition.)

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.

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

That should do it. Parents were missing there.

#21 rerolled with the change in #26.

Let's try this again...

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

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.

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.

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.

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

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

Updating tags per http://drupal.org/node/1517250

Re-rolled, need manual testing.

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

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

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

36: states-required-label-1239930-36.patch queued for re-testing.

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

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.

36: states-required-label-1239930-36.patch queued for re-testing.

Rerolled.

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.

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.

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

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.

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

.

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.

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

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

Patch looks solid to me. RTBC++

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

Committed c7de07a and pushed to 8.0.x. Thanks!

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>

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

Backport of patch #58 to D7.

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

Moved backport of this patch to #2791899: [D7] #states api "required" state incorrectly adds required-asterisk to all individual radio inputs for a "radios" element