Problem/Motivation

Following #description_display work defined for elements in issue : #314385: Make position of #description configurable via the API and what has been done more specifically for fieldsets here #2396145: Option #description_display for form element fieldset is not changing anything, this issue defines the same work to be done for details type element. Two issue found :

  1. The description id is wrong, currently details-description instead of $field_id . '--description as other elements.
  2. The description_display position is not taken into consideration.

Proposed resolution

Change template_preprocess_details() in form.inc to include accounting of #description_display element key. Also update the details.html.twig templates to use description_display setting.
NOTE: contrarily to fieldset, the description for details is before by default.

Remaining tasks

- Create a WebBaseTest test to check the rendering of the description position in details.
- Create a patch to solve the two points of this issue.
- Review patch and fix.

User interface changes

Description of details elements will now be positionned before or after details content depending of description_display.

API changes

None: #description_display should already be part of formAPI regarding #314385: Make position of #description configurable via the API.

Files: 

Comments

Dom.’s picture

Add a WebTestBase test for details group rendering. This test will check:
- default description of details is before content, contrarily to all other elements.
- description is before, after or invisible depending on description_display for details element
- description class is generated with the same convention as per other elements.

Changing task to be done in issue description and adding linked issues.

NOTE: these tests will fail obvioulsy since they need the correction patch to run properly.

Dom.’s picture

Issue summary: View changes
FileSize
5.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,140 pass(es), 25 fail(s), and 68,832 exception(s). View

Add corrective patch to list for evaluation : need review and comments.
Thanks

Status: Needs review » Needs work
Dom.’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +accessibility, +Usability, +d8ux
Parent issue: » #314385: Make position of #description configurable via the API
Related issues: -#314385: Make position of #description configurable via the API
FileSize
5.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch description_display_broken_for_details_elements-2443815-5.patch. Unable to apply patch. See the log in the details link for more information. View

Corrected missing property check in previous patch.
Also add issue tags Novice

Cottser’s picture

Issue tags: +Twig

Don't mind me :)

Status: Needs review » Needs work
rpayanm’s picture

Status: Needs work » Needs review
Issue tags: +Rer
FileSize
5.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,252 pass(es). View

Rerolled

rpayanm’s picture

Issue tags: -Rer

Ups wrong tag.

vanilla-bear’s picture

Assigned: Unassigned » vanilla-bear
Issue tags: +drupaldevdays

i will work on this issue.

vanilla-bear’s picture

Assigned: vanilla-bear » Unassigned
Issue tags: -drupaldevdays
mgifford’s picture

You can test this by going to /admin/config/people/accounts and looking down at the first fieldset which has a description.

Without the patch:

With the patch

The results look good to me. I haven't tested it with the description in another position. Looks good to me though.

spiritcapsule’s picture

i'm at drupalcon la sprints and will take a look at this issue

spiritcapsule’s picture

I attempted applying this patch and testing per mgifford's suggestion above. The description text disappeared entirely.

spiritcapsule’s picture

Status: Needs review » Needs work
FileSize
219.07 KB
Dom.’s picture

Status: Needs work » Needs review
FileSize
11.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,832 pass(es). View

Also tested patch #9 as per @mgifford : no issue for me contrarily to #15. Everything fine. It also validates the --tests-only patch at #1.

Thus, here is a patch for final review, it is a simple concatenation of #1 tests + #9 fix.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me Dom. I think this should be good to go.

Edit: Forgot to say that I tested the site in SimplyTest.me and got the results I'd seen earlier with the new patch. I also reviewed the code and it looks fine.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -708,11 +708,12 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
    +      '#description_display' => (isset($element['#type']) && $element['#type'] == 'details') ? 'before' : 'after',
    

    I think the override for details can go in Drupal\Core\Render\Element\Details

  2. +++ b/core/modules/system/templates/details.html.twig
    @@ -19,15 +25,22 @@
    +      <div{{ description.attributes }}>
    +        {{ description.content }}
    +      </div>
    ...
    +      <div{{ description.attributes.addClass(description_classes) }}>
    +        {{ description.content }}
    +      </div>
    

    It's weird that we only add the classes when the description is set to after - how come? And yes this is the way we do it in HEAD - so maybe this should be a followup.

Dom.’s picture

Status: Needs work » Needs review
FileSize
11.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View
565 bytes

Regarding @alexpott comment #19:
1/ Sorry I cannot make it:

  • if adding $element['#description_display'] = 'before'; to Details.php, I override the DX value (the one from formAPI).
  • if adding $element['#description_display'] = isset($element['#description_display']) ? $element['#description_display'] : 'before'; to Details.php, then default placement is 'after' for it is what FormBuilder.php does.

Thus the easy way I found to make the change is what I did on patch #17 in FormBuilder.php.

2/ I have added here in this new patch the classes in description regardless of position. To be validated if we keep it.

Status: Needs review » Needs work
prashantgoel’s picture

Assigned: Unassigned » prashantgoel

working on this.

cilefen’s picture

Issue tags: +Needs reroll
prashantgoel’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
9.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,330 pass(es), 1 fail(s), and 0 exception(s). View

attached updated patch

Status: Needs review » Needs work

The last submitted patch, 24: description_display-2443815-24.patch, failed testing.

prashantgoel’s picture

FileSize
9.83 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,153 pass(es). View

I have updated the test scenario which was failing, although I am not 100% sure if I selected the correct selector. Please review the first test case with more attention as that is the first test case written by me. :)

Attaching updated patch file.

Thank You

prashantgoel’s picture

Status: Needs work » Needs review

Oops missed to change the state.

joshi.rohit100’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestGroupDetailsForm.php
@@ -41,6 +42,53 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $form['details_before'] = array(
+      '#type' => 'details',
+      '#title' => 'Details test for description before element',
+      '#description' => 'Details test for description before element.',
+      '#description_display' => 'before',
+    );
+    $form['meta_before'] = array(
+      '#type' => 'container',
+      '#title' => 'Group element',
+      '#group' => 'details_before',
+    );
+    $form['meta_before']['element'] = array(
+      '#type' => 'textfield',
+      '#title' => 'Nest in container element',
+    );
+
+    $form['details_after'] = array(
+      '#type' => 'details',
+      '#title' => 'Details test for description after element',
+      '#description' => 'Details test for description after element.',
+      '#description_display' => 'after',
+    );
+    $form['meta_after'] = array(
+      '#type' => 'container',
+      '#title' => 'Group element',
+      '#group' => 'details_after',
+    );
+    $form['meta_after']['element'] = array(
+      '#type' => 'textfield',
+      '#title' => 'Nest in container element',
+    );
+
+    $form['details_invisible'] = array(
+      '#type' => 'details',
+      '#title' => 'Details test for visually-hidden description',
+      '#description' => 'Details test for visually-hidden description.',
+      '#description_display' => 'invisible',
+    );
+    $form['meta_invisible'] = array(
+      '#type' => 'container',
+      '#title' => 'Group element',
+      '#group' => 'details_invisible',
+    );
+    $form['meta_invisible']['element'] = array(
+      '#type' => 'textfield',
+      '#title' => 'Nest in container element',
+    );

All the #titles should be translatatble, i.e. $this->t().

Also I think, short array syntax would be good (AFAIK patch policy).

prashantgoel’s picture

Changing this to needs review again as I think for tests its really not required to use t().

I am taking into consideration the following test case files in core/modules/system/tests/modules/form_test/src/Form

├── FormTestAlterForm.php
├── FormTestButtonClassForm.php
├── FormTestCheckboxForm.php
├── FormTestCheckboxTypeJugglingForm.php
├── FormTestCheckboxesRadiosForm.php
├── FormTestCheckboxesZeroForm.php
├── FormTestClickedButtonForm.php
├── FormTestColorForm.php
├── FormTestDescriptionForm.php
├── FormTestDisabledElementsForm.php
├── FormTestEmailForm.php
├── FormTestEmptySelectForm.php
├── FormTestFormStateValuesCleanAdvancedForm.php
├── FormTestFormStateValuesCleanForm.php
├── FormTestGroupContainerForm.php
├── FormTestGroupDetailsForm.php
├── FormTestGroupFieldsetForm.php
├── FormTestGroupVerticalTabsForm.php
├── FormTestInputForgeryForm.php
├── FormTestLabelForm.php
├── FormTestLanguageSelectForm.php
├── FormTestLimitValidationErrorsForm.php
├── FormTestNumberForm.php
├── FormTestPatternForm.php
├── FormTestPlaceholderForm.php
├── FormTestProgrammaticForm.php
├── FormTestRangeForm.php
├── FormTestRangeInvalidForm.php
├── FormTestRebuildPreserveValuesForm.php
├── FormTestRedirectForm.php
├── FormTestRequiredAttributeForm.php
├── FormTestResponseForm.php
├── FormTestSelectForm.php
├── FormTestStatePersistForm.php
├── FormTestStorageForm.php
├── FormTestStoragePageCacheForm.php
├── FormTestTableSelectColspanForm.php
├── FormTestTableSelectEmptyForm.php
├── FormTestTableSelectFormBase.php
├── FormTestTableSelectJsSelectForm.php
├── FormTestTableSelectMultipleFalseForm.php
├── FormTestTableSelectMultipleTrueForm.php
├── FormTestUrlForm.php
├── FormTestValidateForm.php
├── FormTestValidateRequiredForm.php
├── FormTestValidateRequiredNoTitleForm.php
├── FormTestVerticalTabsForm.php
└── RedirectBlockForm.php

Do we need to take short array syntax for this??

Thanks

prashantgoel’s picture

Status: Needs work » Needs review
joshi.rohit100’s picture

It is not a requirement to use $this->t() over plain t() but as we are doing things in OO way, It is better to try to reduce procedural code (if something is available) and do in OO way.

For all examples mentioned in #29, see RedirectBlockForm.php.

Someplaces in core are still not consistent (t or $this->t) . But if it uses OO, we should do it in OO way (my suggestion).

Do we need to take short array syntax for this??

As D8 has PHP 5.4 or greater requirements and you can patches are already adapting this, so I think yes we should do this.

I am not changing the status for this. But I think we should go with as #28

prashantgoel’s picture

Assigned: prashantgoel » Unassigned

borisson_’s picture

FileSize
3.09 KB
9.79 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,569 pass(es). View

#28: this is only a test so I doubt that it's needed to translate this.
#29: Using the short array syntax is only allowed in lines / hunks that are already modified for other reasons.

Attached is a patch that changes usages of the old array syntax to the new array syntax in already changed lines of code.

valthebald’s picture

This patch should fail

valthebald’s picture

FileSize
2.43 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,563 pass(es). View

valthebald’s picture

Status: Needs review » Needs work

Added test is not failing without the patch (and it should)

hussainweb’s picture

Status: Needs work » Needs review
FileSize
5.32 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,550 pass(es), 3 fail(s), and 0 exception(s). View

I think you missed the actual test in testonly patch. Here is an updated patch.

Status: Needs review » Needs work

The last submitted patch, 39: 2443815-39-testonly.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review

I think these are the expected failures. This mean the patch in #34 is fine. Back to needs review.

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

@hussainweb: you're right!
So, we have a test that is failing without a patch, and passes with a patch. Marking RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/includes/form.inc
    @@ -236,9 +236,22 @@ function template_preprocess_details(&$variables) {
    +  $variables['description'] = NULL;
    +  if (!empty($element['#description'])) {
    +    $variables['description_display'] = isset($element['#description_display']) ? $element['#description_display'] : 'before';
    +    $description_attributes = [];
    +    if ($variables['description_display'] === 'invisible') {
    +      $description_attributes['class'][] = 'visually-hidden';
    +    }
    +    if (!empty($element['#id'])) {
    +      $description_attributes['id'] = $element['#id'] . '--description';
    +    }
    +    $variables['description']['attributes'] = new Attribute($description_attributes);
    +    $variables['description']['content'] = $element['#description'];
    +  }
    

    Can't all of this logic go in the twig templates?

  2. +++ b/core/modules/system/templates/details.html.twig
    @@ -20,7 +26,12 @@
    +  <div{{ description.attributes }}>
    +    {{ description.content }}
    +  </div>
       {{ children }}
       {{ value }}
    +  <div{{ description.attributes.addClass(description_classes) }}>
    +    {{ description.content }}
    +  </div>
    

    This looks wrong - I think we need the before and after check here and what is description_classes? Looks NULL in this template.

  3. +++ b/core/themes/classy/templates/form/details.html.twig
    @@ -18,14 +18,21 @@
    +      <div{{ description.attributes.addClass(description_classes) }}>
    

    What's description_classes?

nova7249’s picture

Issue summary: View changes
GaëlG’s picture

We'll try to work on this with @cwoky as part of DC Barcelona mentored sprint.
The contributor task is: https://www.drupal.org/contributor-tasks/create-patch

valthebald’s picture

Issue tags: +Barcelona2015

tagging

GaëlG’s picture

FileSize
10.63 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,990 pass(es). View

We rerolled the #34 patch to apply on current head (even if we know it was wrong). Credit goes to @cwoky, @jeremypinto and me.

mgifford’s picture

Status: Needs work » Needs review
DuaelFr’s picture

I helped a bit on this as a mentor at DrupalCon Barcelona :)

cwoky’s picture

FileSize
12.13 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,258 pass(es), 6 fail(s), and 43,278 exception(s). View

This new patch applies the 3 recommandations of #43, with the following details:
1. We passed the logic of setting the 'visually-hidden' class to Twig. We removed the default value handling because it's already done in FormBuilder.php (according to #20 this cannot be in Details.php). For the id, we keeped the code in the preprocess because it's how it's done elsewhere: template_preprocess_form_element().

Credit goes to @gaëlg, @jeremypinto and me.

jeremypinto’s picture

I participate with the patch #50

Status: Needs review » Needs work
cwoky’s picture

Status: Needs work » Needs review
FileSize
12.1 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,978 pass(es). View

Actually we were wrong about the fact that FormBuilder.php was setting the default value.
This new patch will fix that warning.

Status: Needs review » Needs work

Status: Needs work » Needs review
TheodorosPloumis’s picture

+++ b/core/modules/system/templates/details.html.twig
@@ -8,6 +8,12 @@
+ * - description_display: (optional) Description display setting. It can have these values:
+ *   - before: The description is output before the element. This is the default
+ *     value.
+ *   - after: The description is output after the element.
+ *   - invisible: The description is output after the element, hidden visually

Is this the default value for sure? I think that the "after" value if the default one.

TheodorosPloumis’s picture

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -859,11 +859,12 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
+    // NOTE: description_display is below by default, excepted for details.

Just saw this comment. So the default value for the Details element is "before" right?

DuaelFr’s picture

@TheodorosPloumis yes it is. See #19 :)

GoZ’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/templates/details.html.twig
    @@ -8,6 +8,12 @@
    + * - description_display: (optional) Description display setting. It can have these values:
    

    Should be on 2 lines to respect line break.

  2. +++ b/core/themes/classy/templates/form/details.html.twig
    @@ -8,12 +8,24 @@
    + * - description_display: (optional) Description display setting. It can have these values:
    

    Should be on 2 lines to respect line break.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
11.33 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,959 pass(es). View

Fixed #60.

GoZ’s picture

Testing patch :

  1. The description id is now $field_id . '--description' for details and all elements.
  2. The description_display position is ok for details and all elements depending of configuration like expected.

Special default position for details is noticed in comments.
May be we should had 2 more tests for default position for details and common fields ?

DuaelFr’s picture

Status: Needs review » Needs work

I agree.
Can you extend the code coverage a bit to be sure that the default value won't unexpectedly change in the future?
Thank you :)

Status: Needs work » Needs review
mgifford’s picture

Issue tags: +Needs reroll

Do we need $variables['description'] = NULL; coming from #2396145-29: Option #description_display for form element fieldset is not changing anything with suggestion from @cwoky a bit later in that thread.

Also, patch no longer seems to apply.

sdstyles’s picture

Rerolled patch and deleted $variables['description'] = NULL;

mgifford’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
304.33 KB

@GoZ said "May be we should had 2 more tests for default position for details and common fields ?" in #62

@DuaelFr said "Can you extend the code coverage a bit to be sure that the default value won't unexpectedly change in the future?"

Do we need this before it can be marked RTBC?

This is a screenshot with the latest patch from here /admin/config/people/accounts

accounts screenshot

GoZ’s picture

Test for details default description position already exists, so i only had test for common element.

DuaelFr’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Go testbot! Go!

mgifford’s picture

Issue summary: View changes
mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Retested the new patch. Looks good. With the additional tests I think it's now RTBC.

I also updated the Beta, to the RC phase evaluation table.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -903,11 +903,13 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
    +    // NOTE: description_display is below by default, excepted for details.
    

    Can this be rewritten to be a normal English sentence? We don't use NOTE: like that.

  2. +++ b/core/modules/system/src/Tests/Form/ElementsDetailsTest.php
    @@ -0,0 +1,58 @@
    + * Definition of Drupal\system\Tests\Form\ElementsDetailsTest.
    

    Contains \Drupal\....

  3. +++ b/core/modules/system/src/Tests/Form/ElementsDetailsTest.php
    @@ -0,0 +1,58 @@
    +  function testFieldsetDescriptions() {
    

    public function.

  4. +++ b/core/modules/system/src/Tests/Form/ElementsDetailsTest.php
    @@ -0,0 +1,58 @@
    +    $this->assertTrue(isset($elements[0]), t('Properly places by default the #description element before the form item within details group.'));
    

    Please don't include messages, and definitely don't use t()

  5. +++ b/core/modules/system/src/Tests/Form/ElementsDetailsTest.php
    @@ -0,0 +1,58 @@
    +  }
    +}
    

    Missing blank line

GoZ’s picture

Status: Needs work » Needs review
FileSize
6.71 KB
16.16 KB

4. Please don't include messages, and definitely don't use t()

@tim.plunkett I don't understand why you don't want to add a message to this asserts.

mgifford’s picture

Wondering if:
// Note that #description_display is below by default, excepted for details.

Should actually be:
// Note that #description_display is below by default, except for details.

That's pretty minor but think it makes more sense.

From the interdiff though it looks like you hit all of @tim.plunkett's point.

Without t() follows the example https://www.drupal.org/node/811254

mgifford’s picture

Status: Needs review » Needs work

The last submitted patch, 75: description_display-2443815-75.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
685 bytes
14.84 KB

Remade the #75 patch with an interdiff as it had a strange syntax.
I hope it's going to help for the tests.

Status: Needs review » Needs work

The last submitted patch, 77: description_display-2443815-77.patch, failed testing.

DuaelFr’s picture

I finally found out what was happening. The stable theme, used by the tests, was overriding the details.html.twig template. It was treating the description as a simple text so when it received an array it tried to render it.
This patch also contains a bit of cleanup and consistency (with fieldsets) improvements.

The last submitted patch, 79: description_display-2443815-79-tests-only.patch, failed testing.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
522.84 KB

Looks good. Tim's concerns from 72 are addressed, bots like it, here's a screenshot of the latest code, and the comments look good too.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/includes/form.inc
    @@ -233,6 +233,8 @@ function template_preprocess_fieldset(&$variables) {
    +  Element::setAttributes($element, array('id'));
    +  Element\RenderElement::setAttributes($element);
    

    Let's actually add use Drupal\Core\Render\Element\RenderElement so that this can be RenderElement::setAttributes($element);. Also I guess we can use the new [] array syntax here - the magic in Element::setAttributes() to convert ['id'] to ['#id' => 'id'] seems unnecessary (but that is not the fault of this patch).

  2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -903,11 +903,12 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
    +    // Note that #description_display is below by default, except for details.
    

    Shouldn't "below" be changed to "after"? But also I'm not sure that this comment actually adds anything that can not be seen by just looking at the code.

  3. +++ b/core/modules/system/src/Tests/Form/ElementsDetailsTest.php
    @@ -0,0 +1,59 @@
    + * Contains \Drupal\system\Tests\Form\ElementsDetailsTest.
    

    Is there any reason why this test can't be part of the ElementsLabelsTest::testFormDescriptions() test?

  4. +++ b/core/modules/system/src/Tests/Form/ElementsLabelsTest.php
    @@ -101,17 +101,24 @@ function testFormLabels() {
    -    $this->assertTrue(isset($elements[0]), t('Properly places the #description element after the form item.'));
    +    $this->assertTrue(isset($elements[0]), 'Properly places the #description element after the form item.');
    ...
    -    $this->assertTrue(isset($elements[0]), t('Properly places the #description element before the form item.'));
    +    $this->assertTrue(isset($elements[0]), 'Properly places the #description element before the form item.');
    
    @@ -119,7 +126,7 @@ function testFormDescriptions() {
    -    $this->assertTrue(isset($elements[0]), t('Properly renders the #description element visually-hidden.'));
    +    $this->assertTrue(isset($elements[0]), 'Properly renders the #description element visually-hidden.');
    

    Strictly speaking this did not need to change in this patch. Not changing it mkaes less code to review and think about.

  5. +++ b/core/themes/classy/templates/form/details.html.twig
    index f4c99e1..76e8c0f 100644
    --- a/core/themes/stable/templates/form/details.html.twig
    
    --- a/core/themes/stable/templates/form/details.html.twig
    +++ b/core/themes/stable/templates/form/details.html.twig
    
    +++ b/core/themes/stable/templates/form/details.html.twig
    +++ b/core/themes/stable/templates/form/details.html.twig
    @@ -25,7 +25,7 @@
    
    @@ -25,7 +25,7 @@
         </div>
       {% endif %}
     
    -  {{ description }}
    +  {{ description.content }}
       {{ children }}
       {{ value }}
     </details>
    

    Shouldn't stable support before and after?

snehi’s picture

Assigned: Unassigned » snehi
snehi’s picture

Assigned: snehi » Unassigned
claudiu.cristea’s picture

Issue tags: -Barcelona2015 +#SprintLUX2016, +SprintWeekend2016
claudiu.cristea’s picture

Issue tags: -#SprintLUX2016, -SprintWeekend2016
tstoeckler’s picture

Issue tags: +DrupalBCDays
diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php
index bb7034f..c901c9c 100644
--- a/core/lib/Drupal/Core/Form/FormBuilder.php
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -903,11 +903,12 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
       $element['#defaults_loaded'] = TRUE;
     }
     // Assign basic defaults common for all form elements.
+    // Note that #description_display is below by default, except for details.
     $element += array(
       '#required' => FALSE,
       '#attributes' => array(),
       '#title_display' => 'before',
-      '#description_display' => 'after',
+      '#description_display' => (isset($element['#type']) && $element['#type'] == 'details') ? 'before' : 'after',
       '#errors' => NULL,
     );
 

Couldn't we change this default in \Drupal\Core\Render\Element\Details::getInfo() instead?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DuaelFr’s picture

Simple reroll against 8.4 to serve as a fresh start to work on #82 and #87

nesta_’s picture

Status: Needs work » Needs review

Change Status :)

The last submitted patch, 91: description_display-2443815-91--tests-only.patch, failed testing.

mgifford’s picture

Status: Needs review » Needs work

We just need a new patch to address issues raised in #82 and #87.

Glad the bots like it.

DuaelFr’s picture

Assigned: Unassigned » DuaelFr

I'm on it

DuaelFr’s picture

All the fixes from #82 and #87 are in this new patch.
I ran the edited tests locally and they are passing. I hope the testbot is gonna be happy :)

The last submitted patch, 96: description_display-2443815-96-tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 96: description_display-2443815-96.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.16 KB
15.79 KB
9.85 KB

I'm really sorry I branched from the wrong place so my last patches were full of crap.
Here are updated ones. I double checked them so it should be better.

GoZ’s picture

Status: Reviewed & tested by the community » Needs review

The last submitted patch, 99: description_display-2443815-99.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 99: description_display-2443815-99-tests-only.patch, failed testing.

DuaelFr’s picture

Status: Needs review » Needs work

The last submitted patch, 103: description_display-2443815-103-tests-only.patch, failed testing.

GoZ’s picture

Status: Needs work » Needs review
GoZ’s picture

Status: Needs review » Needs work

Can you convert ElementsDetailsTest WTB to BrowserTestBase please ? There is no tricky wtb tests in there.

DuaelFr’s picture

The last submitted patch, 107: description_display-2443815-107-tests-only.patch, failed testing.

GoZ’s picture

Status: Needs review » Reviewed & tested by the community

Looks good and tests are BrowserTestBase now :D Thanks for that @DuaelFr

Go to next step

The last submitted patch, 107: description_display-2443815-107.patch, failed testing.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/form.inc
@@ -251,7 +253,18 @@ function template_preprocess_details(&$variables) {
-  $variables['description'] = (!empty($element['#description'])) ? $element['#description'] : '';
...
+    $variables['description']['attributes'] = new Attribute($description_attributes);
+    $variables['description']['content'] = $element['#description'];

Changing the description to an array is a BC breaking change and will break any custom implementations of the template. Stable and Classy should remain intact.