Issues with "required, multiple" fields in forms

Wow, took one year less a week for us to stumble across this bug. I'll look at this patch shortly. It is interesting looking at the process of actually registering users in live sites in D7. It is a piece which probably is far enough into the process that it's easy to miss.

How do we make sure that changes like this are caught earlier for D8?

Thanks @yched!

+++ includes/form.inc
@@ -3759,7 +3759,7 @@ function theme_form_element($variables) {
   // If #title is not set, we don't display any label or required marker.

Comment needs to be updated and should elaborate a bit on the reasoning for the empty string check.

That said, assigning a #title but setting it to an empty string, sounds like a bogus idea to me in the first place.

Powered by Dreditor.

@sun is there anything other than this comment that you'd change in the patch? Other than getting it through the simpletests?

It certainly seems to be an important issue to bring into core.

I'm not sure whether we shouldn't fix Field module instead. Specifying an empty string as #title is perfectly valid from a Form API perspective, and getting an empty title potentially followed by a required marker is what I'd expect when doing that.

+++ modules/field/field.form.inc
@@ -172,7 +172,8 @@ function field_multiple_value_form($field, $instance, $langcode, $items, &$form,
         // For multiple fields, title and description are handled by the wrapping table.
-        '#title' => $multiple ? '' : $title,
+        '#title' => $multiple ? t('@label (value @delta)', array('@label' => $title, '@delta' => $delta)) : $title,
+        '#title_display' => $multiple ? 'invisible' : 'before',
         '#description' => $multiple ? '' : $description,

We should move those conditional assignments into a real if/else condition. Like for #title, an empty #description should also output a totally needless empty DIV.description. Thus, a proper

if ($multiple) {
}
else {
}

separation would make much more sense here.

Not sure whether we need the @delta in the #title though. I think that aforementioned add-labels-for-accessibility patch contained similar constructs, so would be good to make this consistent.

Powered by Dreditor.

@yched - I was certainly very supportive of that patch. Like many patches, I've had to roll, re-roll, re-re-roll them to keep up with core and to bring in legitimate concerns from folks like @sun. I think at the end of the day I really don't like macro patches like this. It's just suich a difficult thing to test for.

I suspect that what happened is that the "required, multiple" use case just got overlooked. Considering that it took a year to find it, it's clear it's an edge use case. I don't know how many rare instances like this there are, but it would be good to document them so we know.

Looking back at the early patches it's clear that Everett's initial goal was to use attributes for inserting titles rather than invisible labels. Unfortunately in D7 we weren't able to bring attributes into the Forms API.

I think it was added in comment 140 after discussions with @chx and @sun. Owen & Brandon did most of the heavy lifting on this patch, and Owen's comments were "we can use #title being unset as an implicit 'none', which would work the same way in terms of output - suppressing the label completely - but would simplify the API a bit for most use cases. I am not sure which one I prefer - it doesn't seem too critical either way."

Seems we found a use case where it was critical. I would think that getting the Field API to display it as invisible would be preferred but can see if we can get some perspectives.

I haven't read through all of the comments here. However, there is, IMO, no use-case for a null string to be used as a form field #title.

All fields need titles, whether they are displayed as a visible label, an invisible label or the @title attribute of the field itself.

The question is, are there times when the form field needs a visible required marker, and an invisible title. If there is really a use-case for this, then this is what needs to be addressed.

#14: form_required_empty_label-980144-14.patch queued for re-testing.

Hi,

I ran into this problem as well. Tested the patch at #14 and it works on my local machine. I would change how @delta is added to #title, though. For me, the most common case of failed validation is when a user doesn't enter anything in the field. In this case, the only existing @delta is 0, and I think it can be removed without impacting screen readers.

So I would change

if ($multiple) {
        // Provide an accurate, hidden title for screen-readers and in case of
        // failed #required validation.
        $element['#title'] = t('@label (value @delta)', array('@label' => $title, '@delta' => $delta));
        $element['#title_display'] = 'invisible';
        $element['#description'] = '';
      }

to

      // For multiple fields, title and description are handled by the wrapping table.
      if ($multiple) {
        // Provide an accurate, hidden title for screen-readers and in case of
        // failed #required validation.
        if ($max == 0) {
          $element['#title'] = t('@label', array('@label' => $title));
        } else {
          $element['#title'] = t('@label (value @delta)', array('@label' => $title, '@delta' => $delta));
        }
        $element['#title_display'] = 'invisible';
        $element['#description'] = '';
      }

Is there anything I can do to help move this forward?

#20: multiple-required-field.patch queued for re-testing.

I have this problem as well. I also have the module media installed and the previous patches didn't work.
Here it is what is worked for me.

I think this might need to go to D8 and then get brought back to D7 but want the testing done now against D7.

Also setting to Needs review.

Fixed for testing

*Moved the status for needs review

Applies well on 7.22 !
Why hasn't it be commited yet ?

If you want to understand one of the usage of this patch :

1. install drupal
2. create a content type "test"
3. add to "test" a required and multiple text field "multiple text"
4. add to "test" a required and single text field "single text"
5. on the node creation form, let your field empty
6. look at the error messages

7.x version is working very well but I know someone will ask for the 8.x version so here it is.

This patch is part of the #1day1patch initiative.

Followed the process in #31. Looks good to me now.

You definitely shouldn't be sorry @yched.. Definitely I should have caught that error..

It's confusing that single form elements seem to get piped through formMultipleElements()

Anyways, this inserts the labels back in.

It did make me wonder about adding fieldsets here around the table too. Not sure that is necessary though.

#35: drupal_core-8.x-fix_multivalue_validation-980144-35b.patch queued for re-testing.

Random failure

Should this have test coverage?

#35: drupal_core-8.x-fix_multivalue_validation-980144-35b.patch queued for re-testing.

Marked #2235319: Unlimited value required autocomplete entity_reference fields show bogus error message on validation as a dupe.

Here's a D7 backport for those in need. Still needs tests.

Added a test to catch this.

Stupid mistakes in 46.

+++ b/modules/field/field.form.inc
@@ -213,14 +213,19 @@ function field_multiple_value_form($field, $instance, $langcode, $items, &$form,
+        $element['##description'] = '';

A little typo in #45.

Attaching updated patch for D7.

Rerolled for PSR4

A lot of things changed in form structure so it's not just a basic reroll that's needed.
I'm on it.

Per patch in #58, placement of "required" asterisk is correct, both visually and in the source, for the textfield.

+++ b/core/modules/field/src/Tests/FormTest.php
@@ -320,24 +320,24 @@ function testFieldFormUnlimited() {
+    entity_create('field_storage_config', $field_storage)->save();
+    entity_create('field_config', $this->field)->save();

Could we use the entityManager instead of entity_create()?

FieldStorageConfig::create($field_storage)->save();
FieldConfig::create($this->field)->save();

@anavarre @tim.plunkett I think we should open an other issue for that change because it would make this test inconsistent with the other ones of the same class.

Using deprecated functions in new code is not a good idea. Consistency within a single method wouldn't even convince me, but within a same class? That's not a reason to write worse code.

Let's work on a new patch then :)

Fair enough :)

1. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -176,10 +176,21 @@ protected function formMultipleElements(FieldItemListInterface $items, array &$f
   +        $element = array(
   ...
   +        $element = array(
   

   Might as well use [] on new code.

2. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -176,10 +176,21 @@ protected function formMultipleElements(FieldItemListInterface $items, array &$f
   +          '#title' => $title . ' ' . t('(value @number)', array('@number' => $delta + 1)),
   

   $this->t() please

3. +++ b/core/modules/field/src/Tests/FormTest.php
   @@ -317,6 +319,31 @@ function testFieldFormUnlimited() {
   +  function testFieldFormUnlimitedRequired() {
   

   public funtion

4. +++ b/core/modules/field/src/Tests/FormTest.php
   @@ -317,6 +319,31 @@ function testFieldFormUnlimited() {
   +    $field_storage = $this->fieldStorageUnlimited;
   

   No real reason to create a new variable for this, we don't bother for $this->field

Here is a new patch. There is a lot of existing code that migh be rewriten according to these rules. Should we open an issue somewhere?

Looks like this wasn't rebased properly (from 3kb to 40kb!)
You can open up a follow-up to modernize other code, though it might wait for 8.1.x.
I'm just against adding new code that follows obsolete standards.

Oops sorry! That's what happens when working late :/

+++ b/core/lib/Drupal/Core/Field/WidgetBase.php
@@ -176,10 +176,21 @@ protected function formMultipleElements(FieldItemListInterface $items, array &$f
+          '#title' => $title . ' ' . t('(value @number)', array('@number' => $delta + 1)),

$this->t(), array() to []

Then it's RTBC!

I should configure my IDE to warn me for all these things T_T

:)
Thanks! RTBC

Committed 2a4ea49 and pushed to 8.0.x. Thanks!

Thanks for the screenshots and beta evaluation in the issue summary.

Whoops! Uploaded the same file twice ;)

Looks like a solid fix! Thank you everyone who worked on this issue.

This doesn't necessarily matter too much for the backport, but just for the record in tests XPaths are definitely preferred over assertRaw for asserting markup with certain attributes. Using XPaths in general makes the tests much less brittle when it comes to changing markup. Refactored the added test to use XPath over here: #2473951-15: Prefix form-required classes with js-

@Cottser @alexpott @tim.plunkett Do you think we should rewrite the D8 test to use xpath() instead of assertRaw() right now?

Would be great to have this backported! Mostly I'm removing the a11y tag as we're trying not to use that.

@DuaelFr sorry for the delayed response yes the D7 test should use XPath as well I'd say.

@mgifford The patch in #77 still applies on 7.x for me.

@Cottser The D8 patch have been commited without using xpath. I suggest to stay consistent with that patch to ease future modifications. As changin that in D8 is probably going to be postponed to 8.1 I think we should not invest more time in that issue. The tests are working well as-is.

@DuaelFr - sorry.. Realized I was just applying this patch to D8..

I added a multiple text field to user/1/edit pre & post patch. It's looking good!

So any concerns with marking this RTBC?

No problem Mike.
If we agree about keeping the tests as-is I think we can RTBC that issue :)

Ok, I just reapplied the patch to see that it all works still and marking it RTBC.

I tested this patch. When I tried to submit the form without filling out the required multivalued field, I got this error message:

My Field Name (value 1) field is required.

That's pretty confusing (although probably still better than the current behavior). Why isn't it just "My Field Name field is required"?

I guess we want the parenthetical part in there for the field label that screen readers use, but it doesn't seem like it should be part of the error message for non-screen-readers.

Something like #742344: Allow forms to set custom validation error messages on required fields might be a real solution, but possible independent solutions could include:

• Leave the "value 1" part off the first label (only add it to the 2nd and beyond). This would solve the issue since only the first label is used in the error message. Is that OK for accessibility?
• Make the titles look like My Field Name <span class="element-invisible">(value X)</span>. Kind of ugly (since the <label> has an element-invisible too so there will be a nested element-invisible) but maybe it makes sense - both regular users and screen reader users will get an error message that matches the field labels that are shown to them.

+        $element['#title'] = $title . ' ' . t('(value @number)', array('@number' => $delta + 1));

This doesn't look properly translatable. Shouldn't it be t(!title (value @number)')?

+    $this->assertRaw(format_string('<label>@label !required</label>', array('@label' => $this->instance['label'], '!required' => $required)),
+      'Required symbol added field label.');

Should be "Required symbol added to field label", I think?

+    $this->assertRaw(format_string('<label class="element-invisible" for="edit-@fieldname-und-0-value">@label (value 1) !required</label>', array('@fieldname' => strtr($this->field_name, '_', '-'),'@label' => $this->instance['label'], '!required' => $required)),
+      'Required symbol not added for field input.');

The assertion message doesn't match the code - it's actually asserting that a required symbol is present. We probably want to say something more like "Required symbol and field label are visually hidden".

Also per the above comments we should really backport the tests as they currently exist in Drupal 8 (with xpath). Doesn't really matter if that got originally committed to Drupal 8 in this issue or another; it's still the right thing to backport :)

Here's what's currently in Drupal 8:

    // Check that the Required symbol is present for the multifield label.
    $element = $this->xpath('//h4[contains(@class, "label") and contains(@class, "js-form-required") and contains(text(), :value)]', array(':value' => $this->field['label']));
    $this->assertTrue(isset($element[0]), 'Required symbol added field label.');
    // Check that the label of the field input is visually hidden and contains
    // the field title and an indication of the delta for a11y.
    $element = $this->xpath('//label[@for=:for and contains(@class, "js-form-required") and contains(text(), :value)]', array(':for' => 'edit-field-unlimited-0-value', ':value' => $this->field['label'] . ' (value 1)'));
    $this->assertTrue(isset($element[0]), 'Required symbol not added for field input.');

Should be a good guide for backporting, although obviously the HTML that's being asserted is slightly different.

Is this the right place to watch for a fix to this to hit 7.x? Seeing a bunch of 8.x stuff at the end, I'm confused.

An attempt to update the patch as per comments in #91.

@aerozeppelin - what about the issues in #90?

@mgifford Thank you for pointing it out. I've made updates to accommodate changes from #90.

Something like #742344: Allow forms to set custom validation error messages on required fields might be a real solution, but possible independent solutions could include:

- Leave the "value 1" part off the first label (only add it to the 2nd and beyond). This would solve the issue since only the first label is used in the error message. Is that OK for accessibility?
- Make the titles look like My Field Name (value X). Kind of ugly (since the has an element-invisible too so there will be a nested element-invisible) but maybe it makes sense - both regular users and screen reader users will get an error message that matches the field labels that are shown to them.

I wasn't sure what option to choose in this case. I chose option one for no specific reason. Any thoughts ?

The code looks good.
I didn't test it manually though.

Thanks @aerozeppelin

I'm not actually sure about the accessibility. I'd need to look at it more or see an example.

If a required field is required, it should indicate that. Perhaps not visually (if it is clear in other ways), but semantically.

I did test it manually and patch #98 works good!

@mgifford can you be more precise about the changes you recommend? I didn't fully understood what you're asking for.
Anyway, let me ask: Was your request fulfilled before and it stopped being fulfilled after applying this patch? Could that improvement be done in a new issue?

I would mark this as RTBC now. We should apply the patch and close this major bug ASAP, we have already a long story here and if one patch fixes the bug, please let's get on with it.

@dxvargas - This is an improvement over D7 Core. I agree with the RTBC. Without the patch the "*" is repeated visually. With the patch it looks like this:

@mgifford I see your point.
For me the "*" next to the field is not important, because we have that signal next to the label, as usual.

The weird thing is that currently only one "*" is shown next to one field and the others don't count. Any field once filled should be enough. Even if one of the text fields is filled but it is missing the "*" field, then we have the error saying the field is required.

So, again in my opinion, this should be fully thought and worked in a new issue.

@dxvargas I agree that the issue with error messages for multiple fields needing its own issue. The problem you described is also an issue in 8.3.x. Not that it gives you the same error (there's an HTML5 required element that pops up), but that error only happens on the first field.

We should discuss if it is:

• Any field once filled should be enough
• All fields are filled in
• All open fields are filled in (unlimited fields have to be negotiated)
• Only the first one needs to be filled in.

But this isn't the place for this. It also doesn't seem like it would be an accessibility issue, but rather general behavior.

Can you open up a new issue for D8.3 and mark this current one RTBC for D7?

I've tested it and patch #98 works fine for me too, marking it as RTBC as requested.

+++ b/modules/field/field.form.inc
@@ -213,14 +213,25 @@ function field_multiple_value_form($field, $instance, $langcode, $items, &$form,
+          $element['#title'] = t('!title', array('!title' => $title));

This t() is not really doing anything here

We need a new patch then.

IMHO reopening an issue that was created in 2010 and it has a patch that has already been merged, only because there is a t() that is doing nothing, is not the best option.

@dxvargas I don't know what has changed.. Can't we just re-roll the patch and take out the t()?

The issue has not been reopened, it is a 7.x issue and there is not 7.x patch that has been committed so far as I can tell.

Edit: Sorry rereading the thread I see that you probably mean it doesn't make sense to remove RTBC for this issue.

Hi,
To solve the problem of required multiple fields not showing the field name on validation, I preferred using a custom module and a HOOK to avoid problems when updating :

/**
 * Implements HOOK_field_widget_form_ALTER() 
 * from drupal_alter(array('field_widget_form', 'field_widget_' . $instance['widget']['type'] . '_form'), $element, $form_state, $context);
 * defined in /modules/field/field.form.php : function field_multiple_value_form()
 */
function HOOK_field_widget_form_alter(&$element, $form_state, $context) {
	if( isset($element['target_id']['#title']) && empty($element['target_id']['#title']) ){
		$element['target_id']['#title'] = $context['instance']['label'];
	}
}

It did the job for me. Hope it helps.

Updates to #108. Removed t().

Thank you aerozeppelin - my colleague and I have also found Patch #98 to work for our entity reference field set to mandatory.

By works I mean that "field is required" error message (i.e. without the actual field name shown/missing) now has the field name in the error message shown after attempting to save the content with the field empty.

We are now doing more testing to ensure hopefully no side effects in applying the patch (regression testing).

Thanks again, given that this has been available for a few months, hopefully this can be included in a forthcoming update to core soon.

For now, the patch is very welcome thank you. I'll let you know if we have any findings from our regression testing.

I've tested this as well and it works perfect for entityreference with multiple values.

Before it would not have a title and after it would, no duplication on the form.

Same on my site. Fixes issues with multiple values in entityreference fields.

RTBC++ The patch takes unlimited value entity reference fields from being completely unable to delete entries to being usable again. I hope it can be committed soon so I can stop tracking that this patch is required for all my sites.

Any chance that this can get committed? It seems like the community reviewed the patch and confirmed it is working...

I queued a re-test, the patch is a year old. It's been at RTBC for 6 moths though, so worth bumping.

Patch applies cleanly, RTBC+.

RTBC+

Bumping to 7.61, this didn't make it into 7.60

Fixing tags

Perhaps we should have a standard by which we reference the comment when we thumbs up a patch?

Is https://www.drupal.org/project/drupal/issues/980144#comment-11954051 the latest patch everyone is using? I can't tell from comments if they are referencing the updated version or the original at https://www.drupal.org/project/drupal/issues/980144#comment-11695545

Any update on getting this into the next release, please, please?

#114 applies cleanly to 7.65

RTBC+

Patch for review is #114 which passes all tests.

D8/9 commit for comparison was https://www.drupal.org/commitlog/commit/2/2a4ea49e9c4eb88bfbc30a18588d4f...

I think this looks good, other than we don't need t() around the messages in the assertions:

+    $this->assertEqual($result[0], '*', t('Required symbol added to field label.'));
+    // Check that the label of the field input is visually hidden and contains 
+    // the field title and an indication of the delta for a11y.                   
+    $result = $this->xpath("//label[contains(@class, 'element-invisible') and contains(text(), :label)]/span/text()", array(':label' => $this->instance['label']));
+    $this->assertEqual($result[0], '*', t('Required symbol and field label are visually hidden.'));

That can be fixed on commit if it's the only problem.

RTBC + 1, except for the t() I agree with that change, we usually use format_string().

This should also improve things for screenreaders unless I miss something. (Yeah, just saw the Accessibiltiy tag)

+1000

RTBC +1 #113 for a while too.

Removed the t() functions in the test's assertions on commit.

Thanks everyone!

This seems to have broken the "Edit summary" behaviour on Long Text with Summary fields with multiple cardinality.

https://stm60fecf90ca939-kv5vfpdqprkhiplfwfkqskyhd8w9ukjg.tugboat.qa/nod... (7.80):

https://stm60fecfa319da7-kx6gxlnflq7ymq2kz4ibtmcw5imklyjw.tugboat.qa/nod... (7.82):

The same bug exists on D8/D9 (#2817081: Show 'Edit summary' links on multi-value long text field with summary) presumably because of that same commit.

NB: The tugboat instance might expire after a couple of days.
Login credentials are u:admin p:admin.