Add test to ensure fieldset allows any non-empty-string #title

The type-casts are unnecessary and should be removed. But aside from that, the proposed change makes sense to me.

This patch will need a re-roll after #2152209: Convert theme_fieldset() to Twig has landed.

Sorry for the late reply.

The comparison operator has to be strict (!==), because otherwise a title of "0" would not be appear.

I agree with Liam here. If someone set the title to FALSE, it would not get in. What needs to be identified is what would it do ideally if someone set title to FALSE, should it get in or not, I think it should.

Patch no longer applies.

The fix in #19 makes sense, and I agree leaving the type casting in place makes sense.

@Liam would you feel comfortable adding a test for this so it doesn't regress in the future? I think this would go in Drupal\form_test\Form\FormTestGroupFieldsetForm, and then ensure the expected markup in Drupal\system\Tests\Form\ElementTest.

According to the Twig documentation an empty should evaluate to false, see http://twig.sensiolabs.org/doc/tags/if.html

Or does Drupal implement a different version of Twig if that I am not aware of?

The rules to determine if an expression is true or false are the same as in PHP; here are the edge cases rules:

Value	Boolean evaluation
empty string	false
numeric zero	false
whitespace-only string	true
empty array	false
null	false
non-empty array	true
object	true

Hello!

Thank you for working on this issue!

We should all try and use the same sprint tag. According to https://groups.drupal.org/node/447258 it should be SprintWeekend2015 with no #.

Patch applies.

+++ b/core/modules/system/src/Tests/Form/ElementTest.php
@@ -129,6 +129,19 @@ function testGroupElements() {
+    ¶
...
+    ¶

Trailing whitespace issues.

When fixing those, could the test also be attached separately to illustrate the current failing behavior?

Patch in #32 makes sense to me. Can somebody address the concerns in #24?

Triaging as part of the Bug Smash Initiative.

Looks to me like template_preprocess_fieldset doesn't use empty() any more, and the lack of activity here for many years would indicate that it's no longer an issue.

But just in case, instead of closing this I'm tagging it for issue summary update and steps to reproduce and marking it PMNMI. If there is no further activity after 3 months this issue may be closed.

Good point!

Having trouble locally, let's see what the testbot makes of this.

Oops, missed actually adding the test elements.

That's weird... A simple (and hacky) manual test shows that the title is actually visible, what does not appear is the description (which makes me suspect that maybe the test isn't right).

Looking deeper I noticed that the problem is just the page where the asserts are happening. They should be moved up a a few lines so they are executed after the $this->drupalGet('form-test/group-fieldset'); like this.

public function testGroupElements() {
  $this->drupalGet('form-test/group-details');
  $this->assertSession()->elementsCount('xpath', '//div[@class="details-wrapper"]//div[@class="details-wrapper"]//label', 1);
  $this->drupalGet('form-test/group-container');
  $this->assertSession()->elementsCount('xpath', '//div[@id="edit-container"]//div[@class="details-wrapper"]//label', 1);
  $this->drupalGet('form-test/group-fieldset');
  $this->assertSession()->elementsCount('xpath', '//fieldset[@id="edit-fieldset"]//div[@id="edit-meta"]//label', 1);

  $this->assertSession()->elementsCount('xpath', '//fieldset[@id="edit-fieldset-zero"]//legend', 1);
  //$this->assertNotEmpty($this->cssSelect('#edit-fieldset-zero>legend>span')[0]);
  $this->assertSession()->elementsCount('xpath', '//fieldset[@id="edit-fieldset-zero"]//div[@id="edit-fieldset-zero--description"]', 1);
  $this->assertSession()->elementsCount('xpath', '//fieldset[@id="edit-fieldset-false"]//legend', 1);
  //$this->assertEmpty($this->cssSelect('#edit-fieldset-false>legend>span')[0]);
  $this->assertSession()->elementsCount('xpath', '//fieldset[@id="edit-fieldset-false"]//div[@id="edit-fieldset-false--description"]', 0);

  $this->drupalGet('form-test/group-vertical-tabs');
  $this->assertSession()->elementsCount('xpath', '//div[@data-vertical-tabs-panes]//details[@id="edit-meta"]//label', 1);
  $this->assertSession()->elementsCount('xpath', '//div[@data-vertical-tabs-panes]//details[@id="edit-meta-2"]//label', 1);
}

Another problem I noticed is that the html actually creates a legend element, even when there's no title inside, so instead of counting for 0 elements we should check if the content is empty (that's what I tried to do on the above commented out lines of code, but I had no success).

I'm attaching a file of how the html from form-test/group-fieldset looks like.

Somehow, all my attempts to check the text inside span (either using xpath() or cssSelector()) failed. These functions return an array of \Behat\Mink\Element\NodeElement[]. If I try to access the first element and use the getValue() method from NodeElement (like $this->cssSelect('#edit-fieldset-false>legend>zero')[0]->getValue() for example) it always return NULL for me.

@Liam Morland

It shouldn't generate an empty legend

This is weird. I got the code at html_test.png (#55) by using a var_dump() on the test and it generated the empty legend tags. Should we report this on a new issue?

Okay, so what I understood so far is:
1. We shouldn't rely on checking the contents of legend as a way to prove that the title isn't showing because this tag shouldn't appear at all when there's no title. This means we either keep the $this->assertSession()->elementsCount('xpath', '//fieldset[@id="edit-fieldset-false"]//legend', 0); and postpone this issue here until the legend problem is fixed, or we find another way to test that doesn't rely on the legend tag.
2. We should probably update the issue summary to focus on fixing the description field instead of the title field (as the title is apparently working as per #53)

Is it right?

So here is a patch with a very simple test that shows the title is correctly displayed when the #title is equal to 0
I also created a new issue to deal with the <legend> element appearing even when the title is empty: #3283883: Fieldset should not create title element when title is empty

Radom unrelated fail, restoring status

Fixing the title.

This was fixed in #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes.

Committed and pushed 8915191bae to 10.0.x and 42bbd1ad1d to 9.5.x and 4465497ee8 to 9.4.x. Thanks!

Backported this to 9.4.x as it is a test-only patch.