Hide empty details containers

added patch.

.

It would be great if someone could post a screenshot of what we are talking here

Another try.

+++ b/core/modules/views/src/Plugin/views/argument/ManyToOne.php
@@ -72,7 +72,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
       '#default_value' => !empty($this->options['break_phrase']),
-      '#fieldset' => 'more',
+      '#group' => 'more',
     );

Its great to see that we finally have an api for that in core. ... so can we remove the preRender function from views after that?

This was quite confusing. I still wonder, why I have to set '#parents' => array('more') on the details element.

@k4v: Awesome idea using #parents, nice one!

Re #11: Yes, in theory we can remove PluginBase::preRenderAddFieldsetMarkup() and all of it's usages now, I think.

oops

Here's a new version with all the fieldset stuff removed.

Really nice cleanup! Sadly still a php error :)

Once again.

lets try this....

@k4v: The patch looks great. Two things are making the tests fail, apparently:
1. FilterPluginBase (line 518) and SortPluginBase (line 205) expect #pre_render to be set, which is no longer the case so that leads to PHP errors.
2. The tests rely on the #parents form structure to set the form values, so they are trying to set form values of the form options[foo][bar] when it should now be just foo[bar] because we have explicitly removed options from the parents. See the test results for exactly where that happens.

I did some research on the subject "fieldset vs details" in the issue queue.

'type' => 'fieldset' has been widely replaced by 'type' => 'details': https://www.drupal.org/node/1168246

Views uses a strange mixture of both at the moment: The '#fieldset' property is a special thing views does to sort the leaf elements into the 'fieldsets', but in our case it has already been replaced with a 'details' container element.

What I did in #9 is not the right thing. The #group property is only valid for elements of type details and fieldset, not for the leaf elements.

Another approach. To remove the #fieldset logic complete should be a new issue.

+++ b/core/lib/Drupal/Core/Render/Element/Details.php
@@ -64,10 +64,12 @@ public static function preRenderDetails($element) {
+      if (empty(Element::getVisibleChildren($element)) && Details::groupHasNoChildren($element)) {

@@ -75,4 +77,20 @@ public static function preRenderDetails($element) {
+  private static function groupHasNoChildren($element) {

note: better use static::groupHasNoChildren and protected. This makes it al little bit easier to override the logic in a subclass.

1. +++ b/core/lib/Drupal/Core/Render/Element/Details.php
   @@ -64,15 +64,33 @@ public static function preRenderDetails($element) {
   +    // TODO: Is there a use case to display empty details? If not we could
   +    // remove the #optional property.
   

   Looks like this needs resolved before this may land.

2. +++ b/core/lib/Drupal/Core/Render/Element/Details.php
   @@ -64,15 +64,33 @@ public static function preRenderDetails($element) {
   +  protected static function groupHasNoChildren($element) {
   +    if (isset($element['#parents'])) {
   +      $group = implode('][', $element['#parents']);
   +      $children = Element::getVisibleChildren($element['#groups'][$group]);
   +      return empty($children);
   +    }
   +    return FALSE;
   

   Is it true if it does not have parents, it does not have children either?

The #group property is only valid for elements of type details and fieldset, not for the leaf elements.

That is not true. See NodeForm for proof. I think #24 was really close and preferable to the current patch.

See: https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...

Searching for '#type'=>'details' I see in almost all of the examples that the children are added to the details directly.

=). I still like #35 better -- why not put the leaf elements right there where they belong?

Re #36.2: The check for the children that are directly contained in the element is done prior to that check: $children = Element::getVisibleChildren($element); if (empty($children) ...). This is not very intuitive (not @k4v's fault, but Form API's), but it's correct. At least I think it makes sense. This needs an issue summary update, I hope I get to that later today. I will also try to polish the inline code-comments a bit and open some follow-ups for this.

Most importantly this needs review from a Views maintainer. I don't know why the fieldset dance was originally introduced into Views, so we should check before altering it. Not sure how old this is in Views (so maybe only @dawehner can help here?) but I will ping someone on the VDC team to have a look at this.

Most importantly this needs review from a Views maintainer. I don't know why the fieldset dance was originally introduced into Views, so we should check before altering it. Not sure how old this is in Views (so maybe only @dawehner can help here?) but I will ping someone on the VDC team to have a look at this.

Well, this dance was introduced in order to decouple the rendered form structure from the form structure/parents structure itself.

@dawehner: So that means you (also) prefer to go back to the patch in #24?

Another simpler variant would be to use the #group property (like in #24) and set it to '#group' => 'options][more'.

This is the most simple thing to do, then we don't need to change the #parent structure, and the logic for #optional shuld work. It's done like this in several places, like CKEditorPlunginManager, Line 167.

Still, the form api documentation says its only for details and fieldsets...

Interesting about CKEditorPlunginManager I wasn't aware of that. Ok, let's got with that then, since we have a precedent?!

@dawehner: So that means you (also) prefer to go back to the patch in #24?

+1

I just re-tested #24 and it does not work, the elements are not added to the group.

Either I did some stupid mistake back then or something changed by now.

Could someone in charge of the form api look into this? I don't really understand how the #group, #parents and #optional are meant to work.

Another try at Drupalcon Barcelona =). After discussing this again with tstoeckler, he proposed this solution.

fix this

go testbot

hmm

+++ b/core/modules/views/src/Plugin/views/ViewsPluginInterface.php
--- a/core/modules/views/src/Plugin/views/argument/ManyToOne.php
+++ b/core/modules/views/src/Plugin/views/argument/ManyToOne.php

+++ b/core/modules/views/src/Plugin/views/argument/ManyToOne.php
@@ -72,7 +72,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
       '#description' => $this->t('If selected, users can enter multiple values in the form of 1+2+3 (for OR) or 1,2,3 (for AND).'),

+++ b/core/modules/views/src/Plugin/views/argument/StringArgument.php
@@ -141,7 +141,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+      '#group' => 'options][more',

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
--- a/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
+++ b/core/modules/views/src/Plugin/views/sort/SortPluginBase.php

Not sure about what is the use of "option][more" means? Please explain its significance

Fixed some coding standard issues

@madhavvyas: we need "#group" => "options][more' because this is the mechanic of the #group property. It has to contain the complete path to the root of the form, with the strange '][' as separators.

I tried to throw out #fieldset entirely, but the tests fail because there are still other places that use #fieldset.

Not sure if we can change them all to use #group.

Ahh @k4v nice catch!!! Very right you are. So let's just remove those hunks that remove the '#fieldset' functionality and just keep everything related to the 'more' stuff here. That should get us to a green patch. Then we can discuss removing the '#fieldset' stuff in a follow-up, but I fear that's D9 material at this point.

Sorry for that, I have reverted 'options][more' change as per patch provided by @k4v in comment #57

Please dis-regards my last patch file it has some extra change due to rebase. Updating original Patch with 'options][more' changes.

@madhayvvas: thanks =).

I put the fieldset-code back in.

even simpler

1. +++ b/core/modules/views/src/Plugin/Block/ViewsBlockBase.php
   @@ -186,8 +186,8 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   -   *   renderable array, containing the optional '#contextual_links' property (if
   ...
   +   *   render-able array, containing the optional '#contextual_links' property
   

2. +++ b/core/modules/views/src/Plugin/views/argument/NumericArgument.php
   @@ -89,6 +89,8 @@ function title() {
   +   *
   +
   

The coding style changes introduced in #62 and following are a bit distracting in general, but some of the instances like the ones shown above a simply incorrect, so I don't think the patch can go in like this. Can we have a patch that's more like #57, but with all the #fieldset stuff reinstanted?

new patch without the code style changes.

Thanks a lot, verified again that this fixes the problem

not sure what happened here, putting it back to RTBC, the test is still green.

As said, we need some tests.

This test should be red here, I include it in the following patch to show its green then.

This should be green. Yay.

Looks good to me.

Looks good to me too!

There is still plenty of use of #fieldset in views...

Use the details elements the right way, possibly remove all the #fieldset logic from the views module.

We either need an issue summary update or more change. Which feels risky given the where we are at in the release cycle and benefits of this change.

Yes, the updated issue summary makes it clear that this is only a bug fix and no API change is involved. We went that route with earlier patches, but no more.

Some before and after screenshots of the UI would be nice.

Here we go.

Committed 458f5be and pushed to 8.0.x. Thanks!

Thanls for the screenshots and issue summary update.

Yeah - finally! Great work @all :-)

yeah!!

Followup to possibly remove #fieldset everywhere: https://www.drupal.org/node/2581255