#type details: Rename #collapsed to #open

What's the default for #open? I'm assuming TRUE in this patch. The html5 specs only talks about the open attribute so the default state is up to whoever generates the details element.

This patch is based on the patch over at #1852104: #type details: Remove #collapsible property.

I like #open, makes more sense to use than #collapsed :)

CNW for #3, probably needs a reroll by now too.

reroll and added fixed comments from #3

( edit ) Default state is closed. It's pretty interesting to see what it does to the admin UI :)

+++ b/core/modules/system/system.moduleundefined
@@ -554,7 +554,7 @@ function system_element_info() {
   $types['details'] = array(
-    '#collapsed' => FALSE,
+    '#open' => FALSE,

Should be '#open' => TRUE, if we want to keep the same behavior. If not, the attribute can be skipped, as you did throughout the rest of the patch

Edit: nvm, I think this is where the default behavior is described... default should be closed as stated in #3

+++ b/core/modules/system/tests/modules/design_test/form/fieldset.incundefined
@@ -40,7 +40,7 @@ function design_test_form_fieldset($form, &$form_state) {
   $form['collapsed']['#title'] = 'Collapsed details';
-  $form['collapsed']['#collapsed'] = TRUE;
+  $form['collapsed']['#open'] = FALSE;

Can be skipped as done throughout the rest of the patch. Doesn't have to obviously, but we might want to go for consistency?

Besides those small remaks, patch looks sane.

fixed.

reroll

+++ b/core/includes/form.incundefined
@@ -2858,8 +2858,8 @@ function form_get_options($element, $key) {
+ *     Properties used: #attributes, #children, #description, #id, #title,
+ *     #value.
  *
  * @ingroup themeable
  */
@@ -2904,8 +2904,8 @@ function theme_fieldset($variables) {

@@ -2904,8 +2904,8 @@ function theme_fieldset($variables) {
  * @param $variables
  *   An associative array containing:
  *   - element: An associative array containing the properties of the element.
- *     Properties used: #attributes, #children, #collapsed, #description, #id,
- *     #title, #value.
+ *     Properties used: #attributes, #children, #description, #id, #title,

Where's #open? :)

Other than that, I couldn't find anything else to nitpick.

EDIT: nod_ pointed out that fieldsets shouldn't be collapsible anymore, so #open shouldn't exist there. But the second one is for theme_details(), and there #open should exist :)

And now nod_ pointed out that theme_details() uses #attributes['open']… So the doxygen is correct.

Needs a re-roll...

curl https://drupal.org/files/core-details-open-attr-1892182-8.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 41098  100 41098    0     0  81857      0 --:--:-- --:--:-- --:--:--  431k
error: patch failed: core/modules/system/system.admin.inc:1870
error: core/modules/system/system.admin.inc: patch does not apply

Reroll. I left one #open => TRUE for the module page details. If all details are closed, the search doesn't work.

#12: core-details-open-attr-1892182-12.patch queued for re-testing.

Green, back to RTBC.

#12: core-details-open-attr-1892182-12.patch queued for re-testing.

+++ b/core/includes/form.incundefined
@@ -3929,8 +3929,8 @@ function form_pre_render_details($element) {
+    $element['#attributes']['open'] =  'open';

Double spaces?

fixed the double space.

#18: core-details-open-attr-1892182-18.patch queued for re-testing.

Committed c4e52bb and pushed to 8.x. Thanks!

Updated http://drupal.org/node/1852020 with the info.

This change was slightly broken, and was not manually tested.
At least not in the Views UI, one of the largest users of details.
Views UI was broken when details was introduced, and now again.

The default for #collapsed was FALSE.
The default for #open is now FALSE.
Yet they are opposites.

On top of that, anything that was '#collapsed' => TRUE, was deleted.

However, anything that did not specify #collapsed did not have #open => TRUE added.

Either:
All of the old #collapsed => TRUE should have been switched to #open => FALSE and have the default for #open to be TRUE
All of the details elements with no #collapsed should have had #open => TRUE added.

Actually this should probably be straight-up reverted. The issue title implies that one property was being renamed to another to improve DX, and yet it caused sweeping changes throughout the UI.

admin/content, admin/people, Views UI are all different. And those are just the first three I looked at.

For example, these used to be expanded:

git revert c4e52bb

Yeah, this is a pretty big WTF. E.g., the required fields in the installer are now hidden by default.

@alexpott is in line to have his d.o email reset, but he reverted this.

He would have said:
"Reverted due to problems described in #22. Committed 687af78 and pushed to 8.x."

I'll leave the pleasure or rerolling this one to someone else and the UI cleanup of form admin that should have ensued.

whatever. It'll never get done otherwise.

Almost all the #collapsed => TRUE are now #open => TRUE, but they should be #open => FALSE.

oh yeah I got confused when I applied the patch halfway.

@nod Yhea, so @sun is right it should default to closed. The problem is however that its currently implemented in the opposite way, so you'd have to check all forms in core and make sure the details that where open still are and the details that are closed also are. We open some by default, because it is our only grouping pattern beside VT's (so we kinda abuse this pattern). With that in mind I would side with the rest, to leave the default open for now.

++ On the renaming :)

oh yeah I got confused when I applied the patch halfway.

if #collapsed => TRUE remove it.
if #collapsed => FALSE or no #collapse add #open => TRUE.

Thanks a lot for the reroll :)

This one is good, I guess we need tim opinion now.

I'm okay with the name change but I don't see why we're changing the default behaviour...

Having a fieldset open by default seems sensible to me... and the special case is when we want to add a fieldset and it default to the collapsed state.

Why not just...
if #collapsed => TRUE change to #open => FALSE
if #collapsed => FALSE remove it
leave #collapsible well alone
and default the #open to TRUE?

if #collapsed => TRUE change to #open => FALSE
if #collapsed => FALSE remove it

@sun has created the original patch in #1852104: #type details: Remove #collapsible property and I think only @sun can answer this question. Perhaps @nod_ can explain.

leave #collapsible well alone

from #1852104-0

The HTML5 spec says that details are always collapsible.

We don't need it why not remove it.

And the point of the whole details thing was to address the fact that fieldset are overused and often misused — insert semantic argument from previous details issues — and that changing the default to closed would highlight that and there are things details are not really suited for and a uncollapsible fieldset should be used (and we can see from tim's feedback that some places would do better without the ability to be collapsible).

Defaulting to false would make contrib think about that, because it is not possible to have details element not collapsible, making sure they see it's collapse will make sure they don't miss that detail and fill bug reports about chrome not being able to not collapse details elements.

I think that's how the argument should go.

Now that it is not defaulting to open with details. What should we use for the old fieldsets that were set to be always open?

What ever we are supposed to use the documentation should be updated to reflect that.

https://drupal.org/node/1852020

In general, we should either revert patches when there's a significant regression, or open separate followup issues where there's not, to avoid the confusion and staring. :) Too late for that here, so retitling for clarity, and bumping priority appropriately for #36.

Hm? This was reverted, see #25

Thanks @tim.plunkett, missed the post-on-behalf-of-committer. :)

cross-posting #2018871: Make summary and details collapsed by default

I don't disagree with making that the default. But the previous patch did not preserve the (possibly incorrect) behavior of core.

If someone goes through and adds '#open' => TRUE to every place that has no specified #collapsed while doing the conversion, I'm happy.

reroll taking care of #42.

includes/form.inc:2935: * Properties used: #attributes, #children, #collapsed, #description, #id,
This doc block needs update.

/**
 * Returns HTML for a details form element and its children.
 *
 * @param $variables
 *   An associative array containing:
 *   - element: An associative array containing the properties of the element.
 *     Properties used: #attributes, #children, #collapsed, #description, #id,
 *     #title, #value.
 *
 * @ingroup themeable
 */
function theme_details($variables) {

thanks, fixed

reroll.

I'm done with this patch, someone take care of it and get rid of a big Drupal-WTF please.

looks good for me

#48 we at least need a reason provided that this rtbc quality and can disregard @sun's comments in #46.

@sun

3. The previous two points probably cover 80% of the details for which #open was added. The remaining ones can probably be left for a follow-up issue.

#2018871: Make summary and details collapsed by default which Nod was going to mark as a dupe is essentially the follow up. It offers a logical approach to determining what details wrappers in core should have #open and a list of them.

47: core-details-open-attr-1892182-47.patch queued for re-testing.

Applied the patch comment #47, but its not working locally for me.

be more specific please. what and where it's not working?

1. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
   @@ -293,8 +293,7 @@ protected function buildConfigForm(Element $schema, $config_data, $base_config_d
   -            '#collapsed' => $collapsed,
   +            '#open' => !$collapsed,
   

   Should we rename this parameter to $open (including reversing it meaning of course)? Follow-up or in this patch: I found 2 usages, 1 of which was using the default, the other passes in a hardcoded TRUE, so the patch would only get 2 more 1-line changes (function definition line and 1 invocation line).

2. +++ b/core/modules/simpletest/simpletest.theme.inc
   @@ -125,7 +125,7 @@ function theme_simpletest_test_table($variables) {
   -      $rows[] = array('data' => $row, 'class' => array($test_class . '-test', ($collapsed ? 'js-hide' : '')));
   +      $rows[] = array('data' => $row, 'class' => array($test_class . '-test', ($open ? 'js-hide' : '')));
        }
   

   Should this be: $open ? '' : 'js-hide'

Assuming that @sun indeed super-carefully reviewed each change, I did not look for missed #detail elements in the Drupal core code base, I believe @sun.

I like the removal of unnecessary brackets, though 1 case was missed, but that is not a problem at all. If the 2nd point is an error this is NW and point 1 could be taken into account as well for the new patch. If point 2 is not an error, point 1 is not important enough to hold this patch, so then it is RTBC for me.

RTBC for me, but needs manual testing according to one of the tags, which I didn't do.

Needs a re-roll.

Checking patch core/includes/form.inc...
error: while searching for:

// Collapsible details.
$element['#attached']['library'][] = array('system', 'drupal.collapse');
if (empty($element['#collapsed'])) {
$element['#attributes']['open'] = 'open';
}

error: patch failed: core/includes/form.inc:2063
error: core/includes/form.inc: patch does not apply

@sun thanks for the re-roll.

I went through and the diff looks very thorough. Gave it a simple test manual test. There looks like there are regressions for a couple collapsible fieldsets that haven't been converted but not due to this patch and if anything this patch helps that a bit in a couple cases. I think this is ready to go and cleans up the collapsible/collapsed/open intermediate stage greatly.

RTBC IMO.

#63: Perhaps it would be better to list these cases that you found during testing and perhaps even already create a follow-up for it. This might make acceptance of this patch easier.

@fietserwin good call, because I'll likely forget:S

On the install screen during simplytest.me install using 8-alpha8 vs 8.x + patch.
This one stood out:

Great saves opening a duplicate:)

Since the W3C defines this attribute as "open" it makes sense we would call it that in our code, too. Adding the "Approved API change" tag here. It does create a bit of a problem though that #type => fieldset doesn't match, so unfortunately we probably need another major task / API change to switch that one, too. Adding "needs followup" tag for that.

Patch still applies, so getting it in while it's hot.

Committed and pushed to 8.x. Thanks!

LOL oh. Well nevermind then. ;) Thanks.

function hook_field_group_form_process_build_alter(array &$element, FormStateInterface $form_state, &$complete_form) {
      $node = $form_state->getFormObject()->getEntity();
      if ($node->hasField('field_pub_date') && !$node->get('field_pub_date')->isEmpty()) {
        $element['field_pub_date']['#attributes']['open'] = TRUE;
      }
}