After enabling Require Summary on a field can't save the field

#68 was failing to apply on 9.5.3 so i rerolled it to work with 9.5.3

Luckily it passes for 10.1.x as well ;-)

Since this about required field think we need a follow up for #70.5 then this may be ready for review

and @sascha_meissner please include interdiffs with your patches even if they're just rerolls too.

Sorry, i focused more on the reroll than on the actual issue.

So i spent some hours into re-evealute the issue in 10.x / 9.5.3

To keep the vanilla-state short: The initial errors might have gone but still you can create odd scenarious mentioned above because the form is not dynamic.

To really mitigate all of those symptoms the form needs two "simple" steps in updating itself before you submit it.

1. Insert/Enable require summary only if Summary input is checked
2. (the missing step) If require summary is checked the Summary Default value input must be inserted/enabled without having to submit the form first.

So i tried to code a solution but am really struggling to utilize ajaxcommands to rebuild the whole form, especially the default_value_widget from the element context..

Still convinced 70.5 can be a separate issue. Having the summary appear when checked could be a nice novice follow up.

This got me again today where I needed to edit a setting of a required field and couldn't get past the field.

Fixed up the patch with latest dev branch.

#70.5 is a false positive, the 'edit summary' link is shown in the screenshot, that is the expected behaviour. The field is only shown expanded by default if 'require summary' is checked.

1. +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextareaWithSummaryWidget.php
   @@ -73,7 +73,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $required = $display_summary && !$form_state->has('default_value_widget') && $this->getFieldSetting('required_summary');
   
   @@ -88,7 +88,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    if (!$this->getSetting('show_summary') && !$required && !str_starts_with($route_name, 'entity.field_config.')) {
   

   doesn't the $form_state->has('default_value_widget') make the route name check redundant?

   this also prevents other modules (e.g. field_union) from reusing this form

2. +++ b/core/modules/text/tests/src/Kernel/TextSummaryTest.php
   @@ -251,8 +253,49 @@ public function assertTextSummary(string $text, string $expected, ?string $forma
   +  public function testRequiredSummary($display_summary, $require_summary, $expected_entity_form, $expected_field_config_form) {
   

   In my last review I asked for type hints here, still needs to be actioned

Addressed the points in #80

this also prevents other modules (e.g. field_union) from reusing this form

Not sure I follow or know how to fix that? But if they're using this form wouldn't the default_value_widget check we are doing still apply?

I think using default_value_widget is fine - other modules using the widget need to pass that too. The issue I have is with checking the route name

This issue is being reviewed by the kind folks in Slack → , #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

1. The issue summary refers to "Outstanding questions", which is also listed as point 3. in the remaining tasks section. I don't see that these questions have been answered and even then, this should be completed/removed. Also, the URLs for the block UI in the steps to reproduce changed in 10.1 and should be updated. Tagging for IS update.

2. #70.5 is a false positive, the 'edit summary' link is shown in the screenshot, that is the expected behaviour. The field is only shown expanded by default if 'require summary' is checked.

3. I think #70.5 and #77.2 refer to the same thing, i.e. not being able to set a default value for the summary directly after checking the "Enable summary" checkbox. I agree that this could be a follow-up issue, if needed. Not tagging, because I'm not sure that it's really such an issue that you have to save the field first before you can enter a default value.
4. Reviewing the code in #82 I have only one nit: The data sets in the data provider could be named (e.g. have keys) to make it more clear what each data set is testing and allow for easier filtering without having to "count" the data sets. This isn't a requirement though and with the most important two values of the data set right at the top it is not such a big problem, so I wouldn't requite that for RTBC.
5. I tested the patch both with the body field for articles and for blocks. Unfortunately, I think there is something wrong still.
   1. Go to /admin/structure/block-content/manage/basic/fields/block_content.basic.body
   2. Check "Summary input" and "Require summary"
   3. Click "Save settings"
   4. Go to /admin/structure/block-content/manage/basic/fields/block_content.basic.body again
   5. Uncheck "Summary input"
   6. Click "Save settings"
   7. Verify that the field is saved successfully with message "Saved Body configuration."
   8. Go to /admin/structure/block-content/manage/basic/fields/block_content.basic.body again.
   9. Verify that both "Summary input" and "Require summary" are unchecked.

   In step 7 I would have expected that the field validation would fail and I get an error message that I need to uncheck "Require summary" before I can uncheck "Summary input", not that "Require summary" is silently removed. Are we sure that this is really the expected behavior?

Moving to Needs work for the IS update and my testing results, since I'm not sure that this is the desired behavior.

Hiding the patches as switching to an MR.

Cleaned out some crust from the issue summary to make it easier to read. Addressed the outstanding questions. Think 2/3 are follow ups. Tried to make it more clear too, but how I read this issue, was that there is a bug where you can't edit a text field after making the summary required.

I've manually tested the latest patch in #86 🐛 After enabling Require Summary on a field can't save the field Needs review on a fresh install of drupal 11.x-dev. functionally everything behaves as expected and i haven't noticed anything striking. looks good!

the only detail i wonder about is why to grey out the require summary checkbox as long as the summary input checkbox is unchecked. disabled interface components are always sort of confusing and make me as a user think for a second why the checkbox is greyed out and how i am able to reenable it. and from an accessibility perspective the disabled checkboxes bordercolor (#bababf) has a color contrast of 1.9:1 against the background color white which isn't meeting WCAG SC 1.4.11 which requires at least a contrast of 3:1. Why not apply progressive disclosure and instead of greying out the checkbox hide the checkbox and it's label as long as the summary input checkbox is unchecked?

overall i would simply trying to avoid using a disabled interface components pattern. might be suitable for a follow up issue or should it be already handled within this issue?

Updated state to hide required field when summary unchecked.

Opened follow ups.

thank you for the fast update! i've quickly applied the updated patch.

1. that is odd, try the following:

- summary input checked & require summary unchecked.
- now uncheck summary input
- now recheck summary input

=> summary input and require summary are both checked now. i would have expected that only summary input is checked.

2. just check require summary and save the settings (tried it on a block type's body field)

=> when going into the field settings again after saving both summary input and require summary are both checked in contrast to the previous saved settings?

So this may be a bug with the states API. I left the state for unchecking but seems doesn't matter when it's hidden. So think we should revert back to disabled vs hiding.

Actually may be a problem with the unchecked state.

        'unchecked' => [
          ':input[name="settings[display_summary]"]' => ['checked' => FALSE],
        ],

Seems it does the opposite when that condition isn't met.

Unless there is something I'm missing the current setup may be the best we can get.

I think #87 is covered by 🐛 Claro: Form labels that are disabled have too low color contrast Needs work , which mentions border color of disabled elements in the IS, so we don't need a follow-up for that.

Form API states has various issues already filed against it, but doing a quick search, I didn't find any obvious candidates for #92. The tests only cover usage with TRUE, not with FALSE. I think we should file an issue for this, if it doesn't exist already, but I don't think we need to postpone on this. But I also think we shouldn't commit it like this, so let's go back to the previous behavior. We could then file another follow-up to use the current behavior using Form API states in this case to improve usability and post-pone that PP2 on this issue and the new follow-up for Form API states.

Tagging Needs Followup again for that.

Opened up 🐛 States API visible stops others states Active and reverted states to us enabled/unchecked

Was looking in dept at the code, had a few 🤔 moments when looking at the tests and added comments, but figured it out when looking at the files as a whole and not only the code changes, so I then deleted my comments 😅.

Added one nit but that should not stop if from getting RTBC I think.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Just needed a rebase.

Updating issue credits

larowlan → changed the visibility of the branch 3115978-after-enabling-require to hidden.

As someone who worked on the original issue that introduced this bug at least 10 years ago - I really wanted to commit this.

But learning the mistakes from the past, I thought I would give it one more manual test.

I installed demo umami and checked the configuration of the body field on the article node type.

$ drush cget field.field.node.article.body
uuid: 5c641576-be7f-4129-b953-b314defaa178
langcode: en
status: true
dependencies:
  config:
    - field.storage.node.body
    - node.type.article
  module:
    - text
_core:
  default_config_hash: 4Wxs1LseZ8JsNwD9uJVIbzZqG35yQXN8Eo1Hub6NMuI
id: node.article.body
field_name: body
entity_type: node
bundle: article
label: Body
description: ''
required: false
translatable: true
default_value: {  }
default_value_callback: ''
settings:
  display_summary: true
  required_summary: false
  allowed_formats: {  }
field_type: text_with_summary

As you can see there

required:summary: false

BUT if I visit /en/admin/structure/types/manage/article/fields/node.article.body without making any changes, the checkbox is ticked

I suspected this might be an issue with the #states added to that form, so I disabled Javascript and reloaded the page.

And sure enough, the checkbox showed the correct default value.

So unfortunately, this is still needs work folks 😭😭😭😭

Adding 'needs manual testing' for the next MR

I added a new condition, based on an example in 2477053 → where the Require Summary field is set to unchecked if it's not checked. It feels a bit dirty, but I think a better fix could be out of scope for this issue.
I re-added the `visible` condition, see #88 🐛 After enabling Require Summary on a field can't save the field Needs review .
I kept the `enabled` condition because otherwise it attempts to validate Require summary instead of just unchecking it when Summary Input is unchecked and then the form is saved without the user explicitly unchecking Require summary also.

I manually tested by doing lots of checking and unchecking the fields, and it behaved as I expected.

I need to fix my duplicated "unchecked."

Updated the unchecked condition to use the default OR statement.

Tested new changes by creating a text formatted field.
Before when I added a default and made summary required. When I went to edit the field I couldn't save.
That issue appears to be addressed still.

The same issue still exists.

Steps to reproduce

* Install Umami
* Inspect node article field with drush cget field.field.node.article.body and take note that summary isn't required
* Visit /en/admin/structure/types/manage/article/fields/node.article.body and see the checkbox is checked when it shouldn't be

Removed 1 of the checks. As discussed in the 80s-90s comments appears there may be a bug in states API, followups were opened

Checked and the bug seen in 105 is gone.

Code wise this also looks good! Previous remarks are implemented and all quality gates are green.

Added some review questions to the MR. Also what is interesting and we should work out why - node fields are not affected by the same bug. If you set them to have a required summary but do not display the summary nothing in broken. I suspect that with this change there is a workaround we can remove.

Please also note that currently the Summary is required for as many default values as the field storage cardinality!