Required alt tag missing on image alt tag input

I am afraid this cannot be fixed easily. Moreover I think this is by the design and not really a bug.

The problem is that on hitting the upload button it gets validated. So there is a custom validation callback for the alt and title attributes that take into account if the file has been initially uploaded or the form is actually being submitted as a whole. In case you set the field to be #required you will get error messages just after hitting the Upload button, which is not correct as from the user point of view the error has not occurred. You can mess around with #limit_validation_errors for upload and remove buttons, but it will not work properly for remove action in case you remove item that is not the last one. Attaching patch with which I could come up... maybe someone else will be able to handle it.

Another solution would be to simply display Alt/Title fields implicitly for new items.

sorry, includes some phpunit stuff, so fixed patch

try following:

- add multiple images
- hit any Remove action but the last one

result will be that all the items that followed get removed as well.

So if this can be fixed then we are fine. But I could not come up with reasonable solution, it looks like the problem is that $form_state gets rebuild in a different way then the submit_validation_errors arrays are built and so the remaining items get removed from $form_state['values'].

So another step in evolution :)

This patch work just fine. However, introduces one small bug - when I upload an image, fill out alt or title, then upload another one without saving the whole form the values I previously entered for title and alt are gone. So if this gets solved, I think we have what we wanted.

Engaging the bot...

With this patch it should all work nicely. Please try out.

another try

#11: drupal-alt_title_required-1906264-5.patch queued for re-testing.

#11: drupal-alt_title_required-1906264-5.patch queued for re-testing.

Thanks @mgifford and @blueminds!

This looks like an accessibility bug to me. Also, I think we probably need to add an automated test for this?

Also, a few minor suggestions for the comments:

1. +++ b/core/modules/image/image.field.incundefined
   @@ -358,9 +362,45 @@ function image_field_widget_process($element, &$form_state, $form) {
   +  // The following code dealing with limit validation errors is the logic that
   +  // avoids triggering error messages on elements that are accompanying the
   +  // field element. These are alt and title elements. The idea is to dynamically
   +  // build the definition of which fields should not be validated.
   

   This comment could be simplified and clarified a lot. For example, the whole first line is superfluous.

2. +++ b/core/modules/image/image.field.incundefined
   @@ -358,9 +362,45 @@ function image_field_widget_process($element, &$form_state, $form) {
   +    // In case the triggering element is not set - form is being rendered - or in
   +    // case the rendering element is "Upload" - user uploaded new file - build
   +    // the limit validation error array.
   

   This could also be shortened and clarified. Also, the first line is over 80 characters.

3. +++ b/core/modules/image/image.field.incundefined
   @@ -358,9 +362,45 @@ function image_field_widget_process($element, &$form_state, $form) {
   +      // For files that have just been uploaded validate only the fid.
   

   There should be a comment after "uploaded," and let's say "file ID" instead of "fid."

4. +++ b/core/modules/image/image.field.incundefined
   @@ -358,9 +362,45 @@ function image_field_widget_process($element, &$form_state, $form) {
   +        // Here we need to set the alt and title values from POST as the limit
   +        // validation logic will remove them from form_state.
   

   The words "Here we need to" can be removed.

5. +++ b/core/modules/image/image.field.incundefined
   @@ -358,9 +362,45 @@ function image_field_widget_process($element, &$form_state, $form) {
   +      // Previously uploaded files - validate whole field.
   

   I'd change this to "Validate the whole field for previously uploaded files."

6. +++ b/core/modules/image/image.field.incundefined
   @@ -358,9 +362,45 @@ function image_field_widget_process($element, &$form_state, $form) {
   +    // Set the built limit validation error definition array for upload as well as
   +    // for the remove button.
   

   This can also be simplified. I'd suggest "Limit validation errors for the upload and remove buttons."

@mgifford Can you provide an interdiff? Thanks!

Not really the place to talk about all this on issue, but ask in IRC if you're having any difficulty with git and folks will help.

There was also stuff missing like, how do you delete a branch:
git branch -D old_patch

I'm not sure how that can be "missing" from a doc that has a very specific purpose. That's elsewhere in the git documentation.

It's also particularly frustrating to try to follow these instructions already having 2 patches built, but not being able to apply the second on because the assumption is that you haven't already built the second patch...

Create two branches. Apply your old patch to the first and your new patch to the second. Diff the second branch against the first. There's your interdiff. You can diff anything against anything in git; the instructions are just for a common usecase.

I just went to a Linux box though and installed interdiff sudo apt-get install interdiff and then am doing it with:
interdiff old.patch new.patch > interdiff.txt

IIRC that's also suggested in a lot of the documentation.

#24: drupal-alt_title_required-1906264-23.patch queued for re-testing.

I'm reviewing and testing.

+++ b/core/modules/image/image.field.inc
@@ -360,9 +364,41 @@ function image_field_widget_process($element, &$form_state, $form) {
+          $item['alt'] = $_POST[$instance['field_name']][$element['#language']][$element['#delta']]['alt'];

Is this a potential XSS vulnerability? Should it be sanitized here or is it already sanitized or sanitized after?

+++ b/core/modules/image/image.field.inc
@@ -360,9 +364,41 @@ function image_field_widget_process($element, &$form_state, $form) {
+          $item['title'] = $_POST[$instance['field_name']][$element['#language']][$element['#delta']]['title'];

Is this a potential XSS vulnerability? Should it be sanitized here or is it already sanitized or sanitized after?

I did not fix the issue, but here is a unit test, which obviously fails, because the bug is still unfixed. That's why I added -do-not-test to the patch name.

Let's see how fails. Can you repost without suffix, so that we can see the test running and failures?

As requested in #39, the unit test without do-not-test. Which will fail.

I tried the patch from #40 as well with simpletest.me. I configured the story content type to make the alt text of the image required. When I then want to add a new story, I get notices 'Undefined variable: settings in image_field_widget_process()'. (see attached screenshot)

And when I select an image, the required indicator for the alt text is not shown. (see other attached screenshot)

Oops, I forgot to change the status to 'Needs review'. Sorry.

OK. Cancelled the test from #43.

I wrote new patch instead of reroll, because i think it more simple.

Reroll

+++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -216,6 +217,7 @@ public static function process($element, FormStateInterface $form_state, $form)
@@ -239,13 +241,12 @@ public static function validateRequiredFields($element, FormStateInterface $form

@@ -239,13 +241,12 @@ public static function validateRequiredFields($element, FormStateInterface $form
       // We check for the array key, so that it can be NULL (like if the user
       // submits the form without using the "upload" button).
       if (!array_key_exists($field, $image_field)) {
+        $form_state['limit_validation_errors'] = array();
         return;
       }
-      // Check if field is left empty.
-      elseif (empty($image_field[$field])) {
-        \Drupal::formBuilder()->setError($element, $form_state, t('The field !title is required', array('!title' => $element['#title'])));
-        return;
-      }
+    }
+    else {
+      $form_state['limit_validation_errors'] = array();
     }
   }

It seems to me that only the return statement should be within the if clause.

Thanks, @barraponto.
In new path removed $form_state['limit_validation_errors'] = array();

Reroll. Changed randomName -> randomMachineName.

Removed because we have now required attribute on alt and title and not need custom validation.

Without patch message
$form_state->setError($element, t('The field !title is required', array('!title' => $element['#title'])));
uppear when #title_field_required = 1 and submit form not image process.

+++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -241,11 +243,8 @@ public static function validateRequiredFields($element, FormStateInterface $form
+    } else {

Only a small coding standards fix, please. Should be

}
else {

Please review revised patch with a fix mentioned in #62.

+++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
@@ -59,13 +59,50 @@ function testRequiredAttributes() {
+    $this->assertText(t('Alternate text field is required.'), 'Node save failed when alt text required was set and alt text was left empty.');
+    $this->assertText(t('Title field is required.'), 'Node save failed when title text required was set and title text was left empty.');
...
+    $this->assertNoText(t('Alternate text field is required.'), 'Node save not failed when alt text not required was set and alt text was left empty.');
+    $this->assertNoText(t('Title field is required.'), 'Node save not failed when title text not required was set and title text was left empty.');
...
+    $this->assertNoText(t('Alternate text field is required.'), 'Node save not failed when alt text not required was set and alt text was left empty.');
+    $this->assertNoText(t('Title field is required.'), 'Node save not failed when title text not required was set and title text was left empty.');

It would be better if there were no messages here since since the assert's default message will indicate exactly what is the problem.

What I mean is this:

+++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
@@ -59,13 +59,50 @@ function testRequiredAttributes() {
+    $this->assertText(t('Alternate text field is required.'));
+    $this->assertText(t('Title field is required.'));
...
+    $this->assertNoText(t('Alternate text field is required.'));
+    $this->assertNoText(t('Title field is required.'));
...
+    $this->assertNoText(t('Alternate text field is required.'));
+    $this->assertNoText(t('Title field is required.'));

+++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
@@ -59,13 +59,50 @@ function testRequiredAttributes() {
+    // look for form-required for the alt text

Nitpick. Capitalize first letter, end with a dot.

RTBC if turns green.

Whoa, reroll.

rtbc

interdiff against #66.

+++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -206,6 +206,7 @@ public static function process($element, FormStateInterface $form_state, $form)
+      '#required' => $element['#alt_field_required'],
       '#element_validate' => $element['#alt_field_required'] == 1 ? array(array(get_called_class(), 'validateRequiredFields')) : array(),

@@ -216,6 +217,7 @@ public static function process($element, FormStateInterface $form_state, $form)
+      '#required' => $element['#title_field_required'],
       '#element_validate' => $element['#title_field_required'] == 1 ? array(array(get_called_class(), 'validateRequiredFields')) : array(),

I think we can remove the #element_validate now and remove ImageWidget::validateRequiredFields()

Now that #2303765: Make the default 'alt' attribute for Image fields required is green - we really need to get this one resolved.

Looks like there's something wrong with the patch - there's a bunch of stuff added at the bottom, that looks like it shouldn't be there.

Re-roll of #79.

Rerolled patch #84 to prevent git line offset warning.

@alexpott, unfortunately #element_validate (and ::validateRequiredFields()) cannot be removed. In fact there we are doing the "required" validation. Why? Because when the form is displayed for the first time the 2 textfields are not displayed and are empty. When an image is picked up the form is submitted and validated and that is causing a validation error. ::validateRequiredFields() is the workaround. See also ::validateRequiredFields() docblock. We are using #required only to get the red * on the form.

Interdiff against #84

Setting back to needs work.

Just did manual test with simplytest.me

Uploaded an image. No indication that alt text is required, also let me save without alt text. So, I'm not getting the error, but the point here was to improve our base accessibility by requiring alt text, and informing editors that they should insert an empty tag if the image is purely decorative.

It has inserted an empty alt tag though.

Hmmm.

Been thinking about this. Maybe this is actually better? Quietly just insert the empty alt tag. If most images are purely decorative... maybe this is the way to go? It's less jarring to the user than getting an error. Setting back to needs review for someone else's opinion.

The use of alt text around images isn't always straightforward. And because a field image is often used in multiple contexts (on it's node, but also in lists generated by views, possibly as links from a list view to a node...) it can get quite tricky, and the way forward seems subjective.

The content editor needs to make a concious determination whether an image is purely decorative. So defaulting to adding an empty alt attribute is contrary to providing accessibility.

@kattekrab, @Charles Belov,

Yes, I get the point but in this ticket we are only showing the "required" asterisk to alt & title form elements if the site administrator decided that those fields are required. Setting the alt tag required by default is out of scope. Please fill a follow-up explaining why we need that. I would be happy to provide a patch.

In the meantime if you feel that the patch from #88 has fixed the original requirement, please RTBC.

Oops - of course @claudiu.cristea - Sorry about that!

I tested again, edited the article content type to make the image field required this time, and voila - the asterisk is there now!

You are absolutely right that making it required by default is out of scope. And there is already another ticket for that. See #2303765: Make the default 'alt' attribute for Image fields required which is still at "need work".

Screenshot attached showing "Alternative Text" field now has red asterisk indicating it is a required field. RTBC from manual test. Could probably do with a quick actual code review from someone who reads code.

Thanks @claudiu.cristea!! This is great. I'm updating the issue summary too.

Looks like Alex has been following this one.

Committed cd57d59 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.