Option #description_display for form element fieldset is not changing anything

It is not a minor issue and not major as well so normal and I think it is a novice task.

Had to add descripton_display to template via preprocess to get this working.

Added patch containing tests only. Those are three tests that are WebTestBase. It's purpose it to validate fieldset rendering according to specifications here :
#314385: Make position of #description configurable via the API
- test description 'before' renders description in first of the inner fieldset fieldset-wrapper
- test description 'after' renders description in last of the inner fieldset fieldset-wrapper
- test description 'invisible' renders description in last of the inner fieldset fieldset-wrapper plus add the class "visually-hidden".
According to those tests, the patch #3 fails.
Dom.

Added a patch to fulfill with the tests defined #5. Compared to #3, this new patch introduces :
- add the class "visually-hidden" to hidden description, such as made for elements in general in parent issue.
- change the fieldset generic template (system/templates) but also the fieldset template in 'classy' theme currently used for tests.
- add description for the 'description_display' variable and it's possible value in the templates.
- remove 'aria-describedby' redefinition in template_preprocess_fieldset() since it is already done by heritance of element attributes.
- reorder ligns regarding description in template_preprocess_fieldset() to fit what was defined for elements in parent issue.
- finally thus have the #5 tests validated.

Changin tag to "need review".

Re-Add tests-only patch for simpletesting.

Needs review since tests-only patch #7 cannot validate if solution patch #6 if not also used (obviously). So it would take manual testing here.
Testing procedure :
- apply patch only test and run the WebBaseTes called "ElementsFieldsetTest" : it should fail for invisible and before description cases.
- apply patch with issue solution
- rerun the ElementsFieldsetTest : it should now validate.

Hi,
I did not changed the tests-only patch.
However regarding "standard" patch:
- changed it to reflect #2407725: Remove classes from system templates d*.html.twig otherwise don't apply
- add tests inside, other don't pass validation

Oups, I was not expecting that with previous patch ! Language defines a Tour which rely on the detail description target with the wrong ID (compared to what introduced in this patch to match fieldsets.)
Correction is done in new patch.

Does this still need tests, or can that tag be removed?

What is the best way to test this patch manually?

Tests are included in patch. It tests that the description position can be set using form API. Therefore, there is not much to be done manually but reading through the patch to check how it's done.

Same patch as #16, just making it apply.

Mentoring work on this issue in Barcelona

We're keeping at this. https://www.drupal.org/contributor-tasks/review

@jcnventura: there is no need in custom module, necessary additions are made to a test module

Currently testing this. Added a fieldset and field in a form_alter using the following:

$form['fset'] = [
    '#type' => 'fieldset',
    '#title' => 'Whatever',
    '#description' => 'Is this right?',
    '#description_display' => 'before',
  ];

  $form['fset']['text_element'] = [
    '#type' => 'textfield',
    '#title' => 'Title',
  ];

Without the patch there is no difference in behaviour between "before" and "after".

Patch applies cleanly.

Now testing...

With patch in #22 applied can confirm that "after", "before" and "invisible" all work as expected.

New patch attached which changes the arrays to use the short array syntax as per standards. interdiff included. Hoping to get someone else here at Drupalcon Barcelona to review it shortly!

Patch from #2396145: Option #description_display for form element fieldset is not changing anything solves the issue but it needs some work. Mainly needs to follow better coding standards.

+++ b/core/includes/form.inc
@@ -207,14 +207,18 @@ function template_preprocess_fieldset(&$variables) {
   $variables['legend']['attributes'] = new Attribute();
...
+  $variables['description'] = NULL;

1) If we have to add an empty array shouldn't it be initialized as []? Eg $variables['description'] = []; But why do we need to initialize the empty array? Similar patches on almost the same kind of problem seem to use NULL though.

+++ b/core/modules/language/config/optional/tour.tour.language-edit.yml
@@ -36,7 +36,7 @@ tips:
+      data-id: edit-direction--description

2) Probably this is irrelevant with this issue (tour module) and should be moved to a follow up issue. Or could we put it on the same patch?

+++ b/core/modules/system/src/Tests/Form/ElementsFieldsetTest.php
@@ -0,0 +1,52 @@
\ No newline at end of file

3) We need to add an empty last line on this file. Simple.

4) We need to add the new option on the template_preprocess_fieldset function documentation on core/includes/form.inc file.

5) We need to add a test case with an empty #description_display.

The patch does not apply anymore.
It does not apply because of a change of a comment.

I rerolled it (just an auto merge).

All things that need to be done are listed on #29 (with the new one about the comment).

Go bots!

To address @TheodorosPloumis points in #29 I've rerolled this patch.

I took out the edit-direction--description for the tour module. It was introduced by @Dom in #16. It was added there to make it consistent with the rest. I'm not sure if the tour module is broken without this or not. I've removed it in this patch.

@mgifford, @TheodorosPloumis : You removed "$variables['description'] = NULL;", maybe we need to remove this initialisation in this patch too : https://www.drupal.org/node/2443815#comment-10411487 ?

Adding back in the Tour module changes from #37, the bot wants them.... This makes it more consistent, so should be fine.

Hi Mike, here is a quick review.

1. +++ b/core/includes/form.inc
   @@ -211,14 +211,17 @@ function template_preprocess_fieldset(&$variables) {
   -
   -    // Add the description's id to the fieldset aria attributes.
   -    $variables['attributes']['aria-describedby'] = $description_id;
   

   Out of scope. We should open a follow-up to remove that dead code.

2. +++ b/core/modules/language/config/optional/tour.tour.language-edit.yml
   @@ -36,7 +36,7 @@ tips:
   -      data-id: edit-direction--wrapper--description
   +      data-id: edit-direction--description
   

   Out of scope. If you need to change something here to make your tests pass, you have a problem somewhere else.

And, I think you may also add a test case with an empty #description_display. Doing that we ensure that this default value is not going to be changed by error someday.

I'll re-roll the patch to address 1 & 2 from #47. It will need work though to include the tests.

Thanks @DuaelFr

I want to see what the bots do. Still needs tests.

Bumping and tagging

Hi guys,

reroll is ready

I just rerolled the patch and replicated the changes in the original fieldset.html.twig template to the stable theme one.
Let's see what the testbot thinks.

What is currently holding this one ? How can we help this to land ? Seems almost ok right ?

+++ b/core/includes/form.inc
@@ -214,13 +214,19 @@ function template_preprocess_fieldset(&$variables) {
+   $description_attributes = [
+     'class' => ['description'],
+   ];

We would add then consistency with other form-item description styling.

Uploading link from #64 to run against 8.4.

@Dom. in 67, does this still need to be added or are you commenting that this is what the code already does?

@mgrifford: I am suggesting to add this instead of line
+ $description_attributes = [];
in the patch. So that at least attribute class "description" is added, and so the fieldset description takes the same styling as other form item descriptions.

Ok, I've added that class here.

Rerolled the patch.

Patch in #73 is working for us with 8.4.5.

Cleaning up tags. Will trigger a retest too.

While the patch in #73 is adding elements to a test form, it is not updating the corresponding test to ensure the position of these elements is as expected.

It needs something similar to this from \Drupal\Tests\system\Functional\Form\ElementsLabelsTests:

  /**
   * Tests different display options for form element descriptions.
   */
  public function testFormDescriptions() {
    $this->drupalGet('form_test/form-descriptions');

    // Check #description placement with #description_display='after'.
    $field_id = 'edit-form-textfield-test-description-after';
    $description_id = $field_id . '--description';
    $elements = $this->xpath('//input[@id="' . $field_id . '" and @aria-describedby="' . $description_id . '"]/following-sibling::div[@id="' . $description_id . '"]');
    $this->assertTrue(isset($elements[0]), t('Properly places the #description element after the form item.'));

  ...

This would be helpful at #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc), too.

We already had a test that sadly got lost between #48 and #53. (interdiffs really help avoiding that! :)
I restored it here and converted it to a BrowserTest. But let's see first if it still tests green.

Honest question: why does 'invisible' imply 'after'? Are there not fieldsets that would want to say something via a screen-reader before the elements, too? Shouldn't visible vs. invisible be a separate thing than before vs. after? Or at least, if it's all via #description_display, there should be 4 choices for all combinations? I guess that should have been done at #314385: Make position of #description configurable via the API and this is simply implementing what we already have in other places to work with fieldsets.

Good question, but certainly beyond scope here. D8 accessibility maintainer @mgifford was pretty much involved in that other issue, so I guess the placement might be negligible to screen readers. Otherwise feel free to file a followup issue.

Mostly looks good to me. A few minor nits, then this is RTBC:

1. +++ b/core/modules/system/templates/fieldset.html.twig
   @@ -53,7 +64,7 @@
          <div{{ description.attributes.addClass('description') }}>{{ description.content }}</div>
   

   Isn't this inserting a duplicate 'description' class in this case? We already set that into description.attributions.class in template_preprocess_fieldset(), right?

2. +++ b/core/modules/system/templates/fieldset.html.twig
   @@ -14,6 +14,12 @@
   + *   - after: The description is output after the element. This is the default
   + *     value.
   

   This could be:

    - after: The description is output after the element (the default).
   

   and then it only has to take 1 line.

3. +++ b/core/themes/classy/templates/form/fieldset.html.twig
   @@ -13,6 +13,12 @@
   + *   - after: The description is output after the element. This is the default
   + *     value.
   

   and here...

4. +++ b/core/themes/stable/templates/form/fieldset.html.twig
   @@ -56,7 +67,7 @@
          <div{{ description.attributes.addClass('description') }}>{{ description.content }}</div>
   

   Same concern as point 1 again here.

5. +++ b/core/themes/stable/templates/form/fieldset.html.twig
   @@ -13,6 +13,12 @@
   + *   - after: The description is output after the element. This is the default
   + *     value.
   

   again

I'd re-roll myself, but then I can't RTBC this. @Pancho, are you willing to address these? If so, I'll RTBC as soon as the bot is green. I'll let the committers decide on keeping or ditching our policy of intentionally leaving "stable" templates broken in bug fix cases like this.

Regardless, I think we at least need a CR for the changes to the classy template.

I vote for fixing the stable templates, too (as we already have in here), and warn about it in the same CR.

Thanks!
-Derek

Actually a bug, so against 8.6

re 1/4:

Isn't this inserting a duplicate 'description' class in this case? We already set that into description.attributions.class in template_preprocess_fieldset(), right?

Think so, but this patch doesn't introduce that, and it's not related to this bug, so would be a followup code cleanup task, possibly with different implications to BC, possibly not.

re 2/3/5:

fixed.

Thanks! Almost there. ;)

1. +++ b/core/includes/form.inc
   @@ -214,13 +214,21 @@ function template_preprocess_fieldset(&$variables) {
   +    $description_attributes = [
   +      'class' => ['description'],
   +    ];
   

   This is why the duplication is introduced in this patch, and why I'm asking about it.

2. +++ b/core/modules/system/templates/fieldset.html.twig
   @@ -44,6 +49,11 @@
   +      <div{{ description.attributes }}>
   

   This looks good.

3. +++ b/core/modules/system/templates/fieldset.html.twig
   @@ -56,7 +66,7 @@
          <div{{ description.attributes.addClass('description') }}>{{ description.content }}</div>
   

   This seems wrong, both because we already have it in the template, and because the actual content here should be the same in the before vs. after case - the only thing that should vary is the position, not the content.

4. +++ b/core/themes/classy/templates/form/fieldset.html.twig
   @@ -53,8 +63,10 @@
   +      <div{{ description.attributes.addClass(description_classes) }}>
   

   Even after applying the patch, I don't see where description_classes is getting set. What's up with this? I see it in form-element.html.twig, but nowhere near fieldset. Is this a copy-paste error? Do I misunderstand?

5. +++ b/core/themes/stable/templates/form/fieldset.html.twig
   @@ -53,7 +63,7 @@
          <div{{ description.attributes.addClass('description') }}>{{ description.content }}</div>
   

   Again. This should look exactly like the "before" case, only the position should change.

Meanwhile, I'm still trying to get clarity from the committers about the policy on fixing stable (and apparently, classy). I might open a meta issue for this and some semi-related form element template bugs. I might also open a "policy; no patch" issue to clarify and document the policy somewhere. Stay tuned.

Cheers,
-Derek

p.s. Off topic, but sorry for your frustrations at the file sanitization issue. Please see my latest comment: #2492171-272: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc). *hugs*

p.s. for now, the policy for getting decisions on if/how to fix stable seems to be "ping lauriii about it" via the "Needs frontend framework manager review" tag. ;)

@lauriii via Slack:

we should only make changes to stable and classy in major releases (Drupal 8.0.0, 9.0.0 etc). However, since that is unrealistic we have been granting exceptions for non-disruptive changes that likely have high positive impact. If you want to get feedback prior to RTBC that’s generally the best way to get frontend framework managers attention :)

Probably not worth trying to fix this until we get some clarity at #3031198: [META] Add classy_dev and minor-branch theme snapshots to allow fixing markup bugs within minor releases without front-end disruption.

I don't want to lose #314385: Make position of #description configurable via the API as the parent to this, so marking #3031198 related, instead.

Sorry for the noise, but #3031198: [META] Add classy_dev and minor-branch theme snapshots to allow fixing markup bugs within minor releases without front-end disruption has evolved, and now I see we have #2352949: Deprecate using Classy as the default theme for the 'testing' profile as the immediate solution to our bugfix deadlock.

While this is a bug and needs to be fixed either way, pragmatically we might also want to settle with a consistent naming scheme for the options first, see #3032325: Rename FormAPI #title_display option 'invisible' to what it is: 'visually-hidden' .

fixing accessibility tag

I don't think this is a disruptive change. I think we should be able to make this change in Drupal 8 in a non-BC breaking way. I'll review this in more detail later, but for now, I wanted to let you know that I don't think this should be postponed.

I rerolled the patch based on the latest changes in core.
It didn't apply since 8.8.3.

I think we are effectively now back at #87, right? (I.e. review comments by @dww). Not sure why tests are not queueing...
Edit: no tests were enable, enabled PHP 7.1 + MySQL 5.5

Thanks for moving this forward again!

In addition to the previous review points, here are a few more:

1. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestGroupFieldsetForm.php
   @@ -37,6 +37,55 @@ public function buildForm(array $form, FormStateInterface $form_state, $required
   +      '#title' => 'Fieldset test for description before element',
   +      '#description' => 'Fieldset test for description before element.',
   ...
   +      '#title' => 'Fieldset test for description after element',
   +      '#description' => 'Fieldset test for description after element.',
   ...
   +      '#title' => 'Fieldset test for visually-hidden description',
   +      '#description' => 'Fieldset test for visually-hidden description.',
   

   Doesn't look like the tests care what the content of any of these are, but it seems very fragile to re-use exactly the same string for both the #title and #description. Any test trying to use this form that checks for the existence of a string on the page might hit either the #title or the #description. Since this is all new plumbing, I'd say we should start from a more firm foundation, and make sure we have unique strings in all of these places so we don't get weird false positives or negatives if we're not very specific with our assertions in some future test.

   E.g. "Title: Fieldset test for description before element"
   vs. "Description: Fieldset test for description before element."
   (or whatever).

2. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestGroupFieldsetForm.php
   @@ -37,6 +37,55 @@ public function buildForm(array $form, FormStateInterface $form_state, $required
   +      '#title' => 'Nest in container element',
   ...
   +      '#title' => 'Nest in container element',
   ...
   +      '#title' => 'Nest in container element',
   

   Same with these 3... I'd be more comfortable if they were all unique, too.

   E.g. "Nested text field inside meta_before container"
   ...

3. +++ b/core/modules/system/tests/src/Functional/Form/ElementsFieldsetTest.php
   @@ -0,0 +1,47 @@
   +class ElementsFieldsetTest extends BrowserTestBase {
   

   This is missing $defaultTheme (per the test failures). Should be fine with this:

     /**
      * {@inheritdoc}
      */
     protected $defaultTheme = 'stark';
   

4. +++ b/core/modules/system/tests/src/Functional/Form/ElementsFieldsetTest.php
   @@ -0,0 +1,47 @@
   +  public static $modules = ['form_test'];
   

   Should be protected.

5. +++ b/core/modules/system/tests/src/Functional/Form/ElementsFieldsetTest.php
   @@ -0,0 +1,47 @@
   +    $this->assertTrue(isset($elements[0]), t('Properly places the #description element before the form item.'));
   ...
   +    $this->assertTrue(isset($elements[0]), t('Properly places the #description element after the form item.'));
   ...
   +    $this->assertTrue(isset($elements[0]), t('Properly renders the #description element visually-hidden.'));
   

   These might be more clear as assertCount(1, $elements)

   Also, assertion messages shouldn't be translatable and wrapped in t() like this.

6. 3 code style violations mentioned here: https://www.drupal.org/pift-ci-job/1633540

   /var/lib/drupalci/workspace/jenkins-drupal_patches-40144/source/core/modules/system/tests/src/Functional/Form/ElementsFieldsetTest.php
   line 24	Visibility must be declared on method "testFieldsetDescriptions"
   46	Expected 1 blank line after function; 0 found
   47	The closing brace for the class must have an empty line before it
   

Thanks again!
-Derek

#87.1 / #87.3 Removed the duplicate description class.

#87.4 description_classes was not set anywhere, I updated this line accordingly

#87.5 The content is now identical in the 'before' and 'after' version

#98.1 / #98.2 Added unique titles and description to the test form

#98.3 Added $defaultTheme property to the test file

#98.4 $modules property is now protected

#98.5 Updated the assertions to use assertCount, removed t() calls from the assertion message

#98.6 Fixed drupal coding standards in the patch

Thanks again for moving this forward! Sorry I missed some things on the last reviews. Our policy on markup changes has also evolved, as has my understanding of it. ;)

1. +++ b/core/modules/system/templates/fieldset.html.twig
   @@ -44,6 +49,11 @@
   +    {% if description_display == 'before' and description.content %}
   +      <div{{ description.attributes.addClass('description') }}>{{ description.content }}</div>
   +        {{ description.content }}
   +      </div>
   +    {% endif %}
   

   This is duplicating {{ description.content }}</div> Perhaps that explains the ElementsFieldsetTest::testFieldsetDescriptions failure? Haven't looked too closely.

2. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestGroupFieldsetForm.php
   --- /dev/null
   +++ b/core/modules/system/tests/src/Functional/Form/ElementsFieldsetTest.php
   

   Sorry I didn't notice / suggest this before, but this could be a Kernel test where the tested form is the test class itself. See https://www.previousnext.com.au/blog/drupal-8-ftw-it-test-or-it-form-act... for details or core/modules/system/tests/src/Kernel/Form for some examples in core.

   Doing this would make the test run *much* faster, and avoids having to expand the form_test module's form definition, keeping the form structure/code in the same class as the test that's testing it.

   Note that this approach also allows you to render the form and do assertions on the generated markup. See #3020676: Support the #date_increment property in the 'duration' form element. for a recent example of such a test I wrote for a contrib module.

3. +++ b/core/modules/system/tests/src/Functional/Form/ElementsFieldsetTest.php
   @@ -0,0 +1,53 @@
   +    // Check #description placement with #description_display='before'.
   ...
   +    // Check #description placement with #description_display='after'.
   ...
   +    // for the option with #description_display='invisible' and also check that
   

   Not sure core committers are going to go for the pseudo code in these comments. They'll probably want you to spell them out. More like:

   // Check #description placement with #description_display set to 'before'.

4. +++ b/core/modules/system/tests/src/Functional/Form/ElementsFieldsetTest.php
   @@ -0,0 +1,53 @@
   +    $this->assertCount(1, $elements, 'Properly places the #description element before the form item.');
   ...
   +    $this->assertCount(1, $elements, 'Properly places the #description element after the form item.');
   ...
   +    $this->assertCount(1, $elements, 'Properly renders the #description element visually-hidden.');
   

   We're on a trip to remove custom messages from assertions unless they're inside a loop/helper where they provide crucial context for what's being tested. In all these cases, we should probably remove them, since it would be obvious from the line number what's wrong if any of them fail.

5. +++ b/core/modules/system/tests/src/Functional/Form/ElementsFieldsetTest.php
   index 0d089ed381..85b2a68065 100644
   --- a/core/themes/classy/templates/form/fieldset.html.twig
   
   --- a/core/themes/classy/templates/form/fieldset.html.twig
   +++ b/core/themes/classy/templates/form/fieldset.html.twig
   

   We shouldn't touch classy at all. That's why Drupal\KernelTests\Core\Theme\ConfirmClassyCopiesTest is failing. We should leave it broken.

6. +++ b/core/themes/classy/templates/form/fieldset.html.twig
   index 59200e7793..8151de0ae7 100644
   --- a/core/themes/stable/templates/form/fieldset.html.twig
   
   --- a/core/themes/stable/templates/form/fieldset.html.twig
   +++ b/core/themes/stable/templates/form/fieldset.html.twig
   

   We also shouldn't touch stable. We need to leave these hunks out, too.

   Each core theme needs to be fixed directly (see below)

   However, when we do this, we're going to start getting a test failure at core/tests/Drupal/KernelTests/Core/Theme/StableDecoupledTest.php in the 9.0.x branch. So, we'll also need a fix to that test for that 1 branch. See #3138739: Improve StableDecoupledTest in the 9.0.x branch for more.

7. In the 9.y.x branches, we can completely remove these parts of the patch. The core themes no longer extend from stable (or classy). They should generally be using the templates from system module directly. However, I just looked and each theme is already defining its own copy of this template. Therefore, we need to fix each theme's override.

   So, for example, Seven has: core/themes/seven/templates/classy/form/fieldset.html.twig

   I'm pretty sure that needs to move to: core/themes/seven/templates/form/fieldset.html.twig and be fixed there.

8. Same story with Bartik.
9. Claro has core/themes/claro/templates/fieldset.html.twig which needs to be fixed directly.
10. Umami has core/profiles/demo_umami/themes/umami/templates/classy/form/fieldset.html.twig which needs to move to core/profiles/demo_umami/themes/umami/templates/form/fieldset.html.twig and be fixed.
11. And of course, we still need that Change record so that everyone not using the core themes knows they need to modify their own theme's template to fix this bug. ;)

Thanks/sorry!
-Derek

p.s. Edit: I moved the version back to 8.8.x since I believe that's what we're targeting for backport based on @lauriii in #93.

Pasting (with permission) a Slack exchange with @xjm about the above:

xjm: @dww I didn't read the patch, but the words sound correct
dww: Thanks much!  Don't want to be pumping out BS or confusion. 
xjm: @dww I got validation today that ConfirmClassyCopiesTest is still relevant, but these are already copied so should be OK to do their thing
dww: And moving them out of the 'classy' subdir is correct once they diverge from classy, right?
xjm: "That test should remain throughout 9. It ensures that if an asset copied from Classy is customized for a specific theme, that asset is moved to a new+appropriate directory."
xjm: So sounds like yep

Phew. ;)

Thanks,
-Derek

#102.1 Fixed the test failure

#102.2 Converting the Functional test to a Kernel test resulted in an error 'is_file(): Unable to find the wrapper "vfs" - did you forget to enable it when you configured PHP?' on my local environment. This appears to be related to the Fieldset render element, but I could not find a source for this call. I left the Functional test as is.

#102.3 Updated the comments

#102.4 Removed assertion messages

#102.5 Removed changes to classy

#102.6 Removed changes to stable

#102.7 Moved the template field and added the fix for Seven

#102.8 Moved the template field and added the fix for Bartik

#102.9 Added the fix for Claro

#102.10 Moved the template field and added the fix for Umami

#102.11 Added a draft change record at https://www.drupal.org/node/3143489

The patch was created against 9.1.x HEAD so I suppose this should be the target branch.

Signoff by laurii in #93 so removing 'Needs frontend framework manager review' tag

Thanks, @idebr! Almost there. ;)

Re: #106.#102.2: Hrm, it seems to work fine for me. See attached version as a Kernel test. Not sure what you did differently that resulted in that error. /shrug

I think we should keep the good docs on the description_display variable in all the templates. Since I'm uploading a new patch, I included that fix here. Sadly, the Claro template didn't document any variables, so that's a bigger hunk than the other templates. I hope it's considered in scope to fix that. If we have to back it out and do it in a follow-up, so be it.

Also fixing core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php to resolve the reported failures.

The 9.y.x patch applies to both 9.1.x and 9.0.x branches. Much to my great surprise, core/tests/Drupal/KernelTests/Core/Theme/StableDecoupledTest.php passes on the 9.0.x branch when I run it locally with this patch applied. I can't explain that. I thought that test would fail. Weird. Anyway, I'll queue this patch to test that branch, too, to see what happens on the d.o bot.

I also did an 8.9.x branch backport. This does *not* apply to the 8.8.x branch, but I'm not sure this fix will be backported that far. We can probably wait to do the effort to backport this to 8.8.x once we get confirmation from core committers that it's eligible for that branch.

Thanks for the initial draft of the CR! However, that needs some help. It has to explain to theme builders a) that if they've got a fieldset.html.twig template override in their theme, they need to update it for this fix to work and b) how to do that update if needed. E.g. before/after versions of the template (or at least the relevant parts). Tagging for updating that.

Finally, since the test only tests the raw (system) template, we should probably manually test this in the core themes that we're changing (all of them). I'll bet $20 that @xjm will want to see those screenshots before committing this). 😉

Cheers,
-Derek

Forgot to upload a test-only of the new Kernel test. This will fail and the bot will be confused, but let's see if there are any other fixes needed to the patch before we re-upload #107 as the legit full patch for RTBC.

Fails locally like so:

There was 1 failure:

1) Drupal\Tests\system\Kernel\Form\ElementsFieldsetTest::testFieldsetDescriptions
Failed asserting that actual size 0 matches expected size 1.

...
/.../drupal-9_1/core/modules/system/tests/src/Kernel/Form/ElementsFieldsetTest.php:105

Line 105 is the assertCount() here:

    $field_id = 'edit-fieldset-before';
    $description_id = $field_id . '--description';
    $elements = $this->xpath('//fieldset[@id="' . $field_id . '" and @aria-describedby="' . $description_id . '"]//div[@id="edit-meta-before"]/preceding-sibling::div[@id="' . $description_id . '"]');
    $this->assertCount(1, $elements);

So yay, fails as expected: can't find the description as a preceding-sibling (because even though it's configured to use #description_display => 'before' the unpatched system template doesn't honor that and the description is at the end of the element.

I started skimming the full comment thread. @DuaelFr pointed out in #47:

And, I think you may also add a test case with an empty #description_display. Doing that we ensure that this default value is not going to be changed by error someday.

Seems like a truly excellent idea. ;) Also gives me an excuse to upload test-only and full patches in the right order so the bot isn't confused. Finally, I realized there was some wonkiness with some of the inline comments in the test, which this patch also fixes.

Note: 2396145.107_110.interdiff.txt is the same interdiff between the test-only here and in #108.

New test-only is failing exactly as expected:

There was 1 failure:

1) Drupal\Tests\system\Kernel\Form\ElementsFieldsetTest::testFieldsetDescriptions
Failed asserting that actual size 0 matches expected size 1.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:116
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:54
/var/www/html/vendor/phpunit/phpunit/src/Framework/Assert.php:2887
/var/www/html/vendor/phpunit/phpunit/src/Framework/Assert.php:492
/var/www/html/core/modules/system/tests/src/Kernel/Form/ElementsFieldsetTest.php:128
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

Line 128 is now the assertCount() I mentioned in #108. The new assertion added to test the default case (now lines 117 to 122) all passed (as expected -- the default behavior still works, even without the patch). Huzzah. ;)

I updated the CR:
https://www.drupal.org/node/3143489/revisions/view/11908309/11909678
Removing that tag.

Based on a quick Slack exchange with @xjm, it's not clear what branches this fix is eligible for:

dww: @xjm Would this issue be eligible for the 8.8.x branch? If so, I'll re-roll it and get the backport patch ready. I think it's basically RTBC now, although it's currently my patches, so I'm not qualified to formally say so.
xjm: Nope
xjm: 9.1.x only
dww: Oh, really? Even though it's a BC-friendly bug fix? Laurii had set it to 8.8.x in a previous comment.
dww: (not trying to pick a fight, just wanting to understand)
dww: If people have their own version of this template and don't update their copy, but get the new code from core, nothing bad happens... their template just continues to ignore the #description_display setting (like now).
xjm: Previous disclaimer about not having read the patch applies; one second...
xjm: Note that the chances of anything being backported to 8.8.x... well, four more days for that at best
xjm: Mmmm I dunno, this still looks minor-only to me. I defer to Lauri, obviously.

So, tagging for @lauriii to review this again, now that it's basically ready to go.

Finally, updating the summary to be more complete, added a release note snippet, etc.

Thanks,
-Derek

Fixing headers inside User interface changes and adding correct alt tags.

@lauriii putting this issue against 8.8.x in May of 2019 does not mean that it is eligible for backport to the same branch in May of 2020. So let's assume this is still 9.1.x only. Thanks!

I don't think the change is BC breaking change because it's making the description work consistently across different types of form elements. That said, I think we should make this change only for the next minor release because it does contain a behavior change for some users. People might have accidentally configured the label display to be invisible or before. They wouldn't realize that the lable display is misconfigured because it didn't have any effect on how the label was rendered.

The change looks good so I'm removing the needs FEFM review tag. I think this is ready for RTBC except it would be good if someone could manually test this with Claro and Bartik, and provide screenshots.

Using the following form:

  $form['fieldset'] = [
    '#type' => 'fieldset',
    '#title' => 'Test fieldset',
    '#description' => 'Fieldset description goes here.',
    '#description_display' => 'before',
  ];
  $form['fieldset']['textfield'] = [
    '#type' => 'textfield',
    '#title' => 'Text field',
    '#description' => 'Text field description',
  ];
  $form['fieldset']['checkbox'] = [
    '#type' => 'checkbox',
    '#title' => 'Fix the UI',
    '#description' => 'Check this box if you want a working UI.',
  ];

Here's before/after for all 4 core themes.

It's applying successfully for me and working well on 9.0.2.

I'm not eligible to move this any further forward. I'll see if anyone in #bug-smash is available to RTBC this.

Thanks,
-Derek

Thanks to everyone for all the work. Marking RTBC based on the following:

1) Skimmed the comments from before 2019 and looked more closely at the comments in 2019 & 2020.

2) Reviewed all the screenshots from #117 and they look fine.

3) Patch still applies cleanly to 9.1.x.

4) Lightly reviewed the code since it was already approved in #116. The only thing that caught my eye was the use of === in if ($element['#description_display'] === 'invisible') { vs == in {% if description_display == 'before' and description.content %} but it seems to be consistent with other core code.

5) Read the change record and it seems clear.

6) Tests pass.

7) Test-only code failed as expected.

8) @lauriii already has approved this work per #116.

Looking at this again I'm asking what do people think about fixing this in Stable and Classy too? This feels rather low risk change given that you need to have #description_display configured for this to have any impact.

Moving back to needs review to get some thoughts on #121

I think it would make sense to make sure Stable and Classy are fixed as well.

Asked in #bugsmash for someone to reply to #121.

Setting to NW for fixing this in Stable and Classy too.

Thanks for moving this forward. This patch also fixes stable and classy.

Whoops, thanks bot!

Oh right, ConfirmClassyCopiesTest. How quickly I forget. ;) Thanks again, bot...

Patch #131, fixes the issue for stable and classy too. Adding before and after patch screenshots by using https://www.drupal.org/sandbox/tplcom/2575395 module.

Moving to RTBC.

@Gauravmahlawat, thanks for the screenshots. The issue summary should be updated with those screenshots, it makes it easier for reviewers and committers. Looking at the issue summary, the screenshots are being linked to in the 'other core themes' section in the IS. Those links now point to screenshots from before that latest patch so need to be updated. From you comment I see that you have created the screenshot but there isn't a comment regarding a code review. If you have done that add a comment explaining what you did to review the code changes. There is information available about setting issues to https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-issue/issue-status-field#rtbc.

Adding tag for IS update, just for the screenshot links, and back to NR for a code review. I have updated the remaining tasks.

Issue summary updated.

I reviewed the interdiffs from #128, #129, and #131 and they address stable and classy as noted in #127 but I'm not seeing screenshots for those. @Gauravmahlawat said stable and classy were tested. Do we need screenshots for those to move this forward?

Hi @Kristen Pol, here I have added, classy and stable screenshots. Please review.

Thanks @Gauravmahlawat. Marking RTBC based on:

1. Themes have been manually tested
2. Automated tests are passing
3. Latest code changes address most recent feedback
4. Previous code changes were approved by @lauriii in #116
5. Issue summary was updated with screenshots

@Gauravmahlawat and @Kristen Pol thanks for getting this to RTBC!

@Kristen Pol, both you and benjifisher teach me a lot about writing high quality comments. One thing I like is that you number each point, it makes it really easy to see what has been done.

In reviewing this I had three questions, one of which remains unanswered:

• ✅What happens for existing elements/sites. I found @DuaelFr pointed that out in #47 and that @dww added test-coverage for it in #110. For reference, the default value is added to all elements in \Drupal\Core\Form\FormBuilder::doBuildForm
• ✅Do we need front-end framework manager sign-off for changing classy and stable, but I see @lauriii (FEFM) was the one who suggested that in #121, so I assume that means he's +1 😂
• ❓In #107 re removed fieldset.html.twig from the data-provider providerTestClassyCopies because at that point we weren't duplicating this change into stable and classy. But subsequently in #128 we decided to - but we didn't undo the changes to providerTestClassyCopies made in #107

Moving back to needs-review pending an answer to question three. Other than that, this looks good to me, updating issue credits while I'm here

@larowlan - Excellent point! As much headache as that test has given me, it's saving the day right now. I reverted those changes to the provider and the test was confusingly failing locally. Turns out I botched things from #128 to #131. Now that we *are* fixing classy, we need to keep a bunch of templates in the 'classy' subdir in the child themes. So this patch undoes some of the file renames and leaves things where they should be.

The interdiff is kinda confusing, based on the previous patch trying to rename things it shouldn't have. But once you apply it, and add everything new, you should see this from your git status output:

	modified:   core/includes/form.inc
	modified:   core/modules/system/templates/fieldset.html.twig
	new file:   core/modules/system/tests/src/Kernel/Form/ElementsFieldsetTest.php
	modified:   core/profiles/demo_umami/themes/umami/templates/classy/form/fieldset.html.twig
	modified:   core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
	modified:   core/themes/bartik/templates/classy/form/fieldset.html.twig
	modified:   core/themes/claro/templates/fieldset.html.twig
	modified:   core/themes/classy/templates/form/fieldset.html.twig
	modified:   core/themes/seven/templates/classy/form/fieldset.html.twig
	modified:   core/themes/stable/templates/form/fieldset.html.twig

Both Drupal\KernelTests\Core\Theme\ConfirmClassyCopiesTest and Drupal\Tests\system\Kernel\Form\ElementsFieldsetTest are now passing locally. Let's see if the bot agrees. 🤞

This change looks great. Reviewed it and didn't find anything glaringly obvious. I'm reluctant to rtbc as I just came across it, but I'll add my +1 to rtbc if that's helpful for the next reviewer.

I think this is RTBC. I did a CR in patch #141 and it looks good.
I also tested the patch with all themes available in Drupal Core and all of them are working as supposed to work.

Moving to RTBC and attaching images with the themes that I tested.

Tee hee. While editing https://www.drupal.org/node/3143489 (the draft change record) for this, I realized "fixing classy and stable" should now be "fixing classy, stable, and stable9". 🤦‍♂️ The only diff for fieldset.html.twig between stable vs. stable9 was this fix, so I copied the stable copy to stable9 and added it to the patch.

@paulocs thanks so much! Very thorough and much appreciated. Hate to undo your work to RTBC, but uploading a new patch means I really should reset the status. Maybe when the bot is back green, you'd be willing to re-RTBC?

Thanks again,
-Derek

Needs change record updates : https://www.drupal.org/node/3143489/revisions/view/11909678/12316153

Oh yeah, and upload the new patch, @dww! ;)

Setting to RTBC!

Sorry to do this folks, but we now have one more fieldset.html.twig in core

core/themes/olivero/templates/form/fieldset.html.twig

Other than that, I can't fault this

Good catch! Sorry we missed that.

This is the only fieldset.html.twig with support for prefix and suffix. I figured if description_display is set to before it should come before the prefix. I'll try to get someone Olivero-enabled to confirm. ;)

Meanwhile, I'll update the CR accordingly.

Needs change record updates: https://www.drupal.org/node/3143489/revisions/view/12316153/12316567