Create a details/summary FieldFormatter plugin for text fields.

@andrewmacpherson demoed this to the UX team this past Tuesday, June 18th, and it went over incredibly well. This patch would allow us to fix A-level WCAG violations that we currently ship in the Standard profile's included media types. So, I'm tagging this as accessibility-related and bumping its priority.

Nice work! I like how simple this is. I'm tagging it for tests as well.

1. +++ b/core/modules/text/config/schema/text.schema.yml
   @@ -96,6 +96,20 @@ field.formatter.settings.text_trimmed:
   +    details_open:
   +      type: boolean
   +      label: 'Open details group by default.'
   

   I think we should rename this parameter to 'open'.

2. +++ b/core/modules/text/config/schema/text.schema.yml
   @@ -96,6 +96,20 @@ field.formatter.settings.text_trimmed:
   +    summary_source:
   +      type: string
   +      label: 'Summary text type'
   +    summary_custom_text:
   +      type: string
   +      label: 'Custom summary text'
   

   We probably don't need these to be two distinct settings. Maybe we could just default summary_custom_text to the field label, and let site builders change it if they want.

3. +++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
   @@ -0,0 +1,136 @@
   +      $summary[] = t('Open by default.');
   

   We should be using $this->t() throughout the class.

4. +++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
   @@ -0,0 +1,136 @@
   +        $summary[] = XSS::filterAdmin(t('Summary text: ' . $this->getSetting('summary_custom_text')));
   

   Nit: Should be 'Xss::filterAdmin'.

5. +++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
   @@ -0,0 +1,136 @@
   +          $token_service = \Drupal::token();
   +          $entity = $items->getEntity();
   +          $summary = $token_service->replace(
   +            $this->getSetting('summary_custom_text'),
   +            [$entity->getEntityTypeId() => $entity],
   +            ['langcode' => $item->getLangcode()],
   +            $bubbleable_metadata
   +          );
   +          break;
   

   I think we might want to do the token stuff in a follow-up issue, because we'll want to add some inline help to the configuration form explaining what tokens are available. Removing the tokens, for now, would make this patch easier to commit (with less test coverage needed).

   If we continue to do it here, then we'll certainly want to inject the token service into this class.

6. +++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
   @@ -0,0 +1,136 @@
   +        '#open' => $this->getSetting('details_open') ? TRUE : NULL,
   

   This should always be boolean, for example:

   '#open' => (bool) $this->getSetting('details_open');

Thanks, @andrewmacpherson!

I tried this but ran into a snag. Removing the summary_source radio options was easy enough. The problem was in providing useful default value for the summary text. FormatterBase::defaultSettings() is a static method, so there isn't any object context to get the field definition out of $this.

I don't know how we can get around this, so I have left the summary_source radio buttons in place for now.

In defaultSettings(), we can just have it be an empty string or NULL, I think. Then, in the non-static settingsForm() method, we should have access to $this->fieldDefinition->getLabel(), which would be a good default value.

I'm kicking this back for tests, but this is getting close to ready, IMHO.

Maybe we can check for an empty value in viewElements(), and fill in the field label as a fallback when we render the field. The settingsSummary() is another place where the empty value can be addressed in a similar way. Is this a sane approach though? It feels very hacky pretending there is a stored setting when there isn't.

I think that is a perfectly sane approach. We don't have to "pretend" there's a stored value; we simply use the stored value if one exists, or the field label if not. I think that is easy to understand for a developer, and acceptable from a site builder/user perspective (it "just works" even if you don't bother doing any configuring). But if you really don't like it, perhaps we could set the #placeholder text in the settings form, and make it non-required, so that it's clearer that we have a fallback value.

It would be good to use it in combination with a #description which explains that it defaults to the field label. Don't rely on placeholder to provide instructions.

Agreed!

Wrote a nice kernel test of the formatter settings, and verified that they affect the output as we expect, and did a touch of refactoring to prevent repetition of logic.

First, this is great, thanks for working on it. I think this is getting really close and is simple and well contained and could be really useful as a transcription field formatter indeed for audio or as a long description field for figures, etc.

+++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
@@ -0,0 +1,121 @@
+ *     "text_with_summary",

I was wondering about this bit. How does this operate with a text field that in itself has a summary part? You have a "Summary" setting on the field settings but then you have a summary on the field value itself too. That is not really handled in the code, and I don't know what would users expect either.

Adding NorthAmerica2021 tag for visbility.

DrupalCon NA is April 12-16 with a focus on EOOTB on Wednesday, April 14.
Thanks

D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.

Also if someone could address the follow-up ticket please.

Looks real interesting too!

10.1.x Reroll for #27

@ayush.khare thank you for the patch unfortunately during the reroll you left out a change for one of the files.

Also The last patch doesn't pass commit checks, could you make sure to run ./core/scripts/dev/commit-code-check.sh before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel...

Also removing quickedit as that's removed in D10

Applied patch #41 successfully works fine.

I started a review by reading the issue summary. First off, I think this is a very useful feature to add. The proposed resolution lists 3 items. The second one is still a point of discussion. If that has been resolved, add it to the Issue Summary. I am adding the tag for an IS update.

This issue has user interface changes and there are no screenshots linked to in the IS. This needs up to date screenshots.

The 'User interface changes' sections says that there are UI changes but there are no up to date screenshots. The section also states that this is using an existing template that is styled for Bartik/Seven. Surely that is out of date. I then searched the issue to see if this has had a usability review. I do not see that so I am adding the tag.

I did not read the comments, instead I then applied the patch and tested it. I added some text fields to the article content type. It appears to work just fine but the text does need to be reviewed. I noticed the following;

• The changes to the help page do not match the existing style of help text.
• On 'Manage Display'
  • I noticed that the text 'Open by default.' has a full stop where similar text for other fields does not have a full stop. Also, the text is usually of the form "Category: type", where the new text includes 'Open by default.'
  • The format is 'Details/Summary' which contains a '/' unlike other fields and is not very helpful. Will an admin know what this means?
  • On clicking to get to the settings, I see 'details' used again in the checkbox text 'Open details by default'. I still wonder if an admin will know what this is. It would help if the help text made this clearer.

I skimmed the patch and this jumped out at me.

+++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php
@@ -0,0 +1,149 @@
+// cspell:ignore Yorick

Although a nice reference, let's use another test string so we can avoid this.

Updated IS

Before adding screenshots want to make sure we get the language correct.

"Open by default" is no longer a full stop
Changed "Details/Summary" to "Details with Summary tag" - thoughts?
Removed the cspell:ignore Yorick

If language looks good we can add screenshots.

I applied the patch and looked at the help page. Then looked at the patch, making the following points.

1. +++ b/core/modules/text/text.module
   @@ -32,6 +32,8 @@ function text_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<dt>' . t('Displaying the text inside a details group.') . '</dt>';
   

   No period at the end, like the others on the page.

2. +++ b/core/modules/text/text.module
   @@ -32,6 +32,8 @@ function text_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<dd>' . t('Text fields can be displayed in a compact way using the <em>Details with Summary tag</em> format on the <em>Manage display</em> page. The field name will be used for the summary element by default, but this can be customized.') . '</dd>';
   

   This will need work. I don't think we can expect an admin to know what an 'element' is. I suspect that 'but this can be customized' needs to be changed to actual instructions. But this is not for me to decide this is for the UX folks.

I used the new formatter and the supporting text is better, but this needs a Usability review to get all the strings to conform to Drupal standards. That is not something I can do.

Finally, I skimmed the issue summary and see that the remaining tasks is out of date. It states that the formatter needs to be implemented and so do tests. Can that be updated to show the work still to do?

I hope to start a code review tomorrow.

Tagging for WCAG SC 1.3.1

This time I looked at the code in conjunction with testing the patch and running the tests.

1. +++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
   @@ -0,0 +1,118 @@
   +use Drupal\Component\Utility\Xss;
   

   This should be sorted to ease readability.

2. +++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
   @@ -0,0 +1,118 @@
   + *   label = @Translation("Details with Summary tag"),
   

   The ux folks will give feedback but this should be sentence case.

3. +++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
   @@ -0,0 +1,118 @@
   +    if ($this->getSetting('open')) {
   

   Why is this shown only when open? As a user I need to click to find the state. Would it not be better to inform the user of the state?

4. +++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
   @@ -0,0 +1,118 @@
   +    $bubbleable_metadata->applyTo($elements);
   

   In \Drupal\text\Plugin\Field\FieldFormatter\TextDefaultFormatter::viewElements there is a comment that the ProcessedText elements handles the cache. So, I think this is not necessary.

5. +++ b/core/modules/text/config/schema/text.schema.yml
   @@ -116,6 +116,17 @@ field.formatter.settings.text_trimmed:
   +      label: 'Open details group by default.'
   

   Labels do not have full stops.

6. +++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
   @@ -0,0 +1,118 @@
   +      // Compute the text to use for the <summary> element.
   

   Let's make this something helpful. Something along the lines of 'Use the user text for the summary. If not supplied then use the field label." This block is in two methods, so both should be changed.

7. +++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php
   @@ -0,0 +1,118 @@
   +      // Use the field label as a fallback.
   ...
   +        // Use the field label as a fallback.
   

   Can be removed when the comment is changed.

8. +++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php
   @@ -0,0 +1,147 @@
   +use Drupal\field\Entity\FieldConfig;
   

   Sort the use statements.

9. +++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php
   @@ -0,0 +1,147 @@
   + * Tests the text formatters functionality.
   

   As I read this, this tests is testing multiple text formatters. Let's update the comment to reflect that this is testing a specific text formatter.

10. +++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php
    @@ -0,0 +1,147 @@
    +class TextDetailsFormatterTest extends EntityKernelTestBase {
    

    This test has the same setup as TextFormatterTest, does it make sense to extend from that?

11. +++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php
    @@ -0,0 +1,147 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    

    This can be inheritdoc.

12. +++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php
    @@ -0,0 +1,147 @@
    +   *   Sets of arguments to pass to the formatter.
    

    Let's be more descriptive. Use an item list to describe the array values.

13. +++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php
    @@ -0,0 +1,147 @@
    +    return [
    

    Why is there no case with 'open' set to TRUE?

14. +++ b/core/modules/text/text.module
    @@ -32,6 +32,8 @@ function text_help($route_name, RouteMatchInterface $route_match) {
    +      $output .= '<dt>' . t('Displaying the text inside a details group.') . '</dt>';
    

    This is a heading and does not need a full stop.

Finally, I think that this new formatter should be added to the list being tested in \Drupal\Tests\text\Kernel\TextFormatterTest.

I want to take another look at TextDetailsFormatterTest.

I would also consider allowing the end user to input the summary text, and even supporting the existing "Text with summary" field type when the summary is required (in this case, check current bug at #3115978: After enabling Require Summary on a field can't save the field).

We can take inspiration from the details_summary_field_formatter module.