Required summary element hidden when other text-with-summary fields do not require summary

Queueing for tests.
Meanwhile, let me try to replicate the scenario.

That's a good one, thanks. And you're correct about the fix.

I would check for the required attribute instead of the class.

+++ b/core/modules/text/text.js
@@ -8,7 +8,7 @@
+      $(context).find('.js-text-summary').not('.required').once('text-summary').each(function () {

I would use the required attribute and let the browser deal with evaluating the selector: .find('.js-text-summary:not([required])')

Also you edited the text.js file directly, have a look at the top of the file to see that we now use *.es6.js files for source: #2815083: Drupal core now using ES6 for JavaScript development

no worries. Was almost there :) you had the change in the right file and you needed to run yarn run build:js to update the text.js file. for interdiffs, it's better to name them .txt to make sure the testbot doesn't run them.

Provided the update text.js file.

But here we need tests to make sure we don't run into this later.

First pass at making a test

Tested the patch from #9 on 9.1.x. Patch worked for me as the form exposes the summary form element for the first text field, and there is no "Hide Summary" link for the Text1(marked required summary), as expected behavior.

Please refer to the attached screenshots with detailed insights. RTBC+1

I think it would be easier if we change the field widget to not output the js-text-summary class when the field is required.

I tested the patch from #8 first but it has a small problem, if a theme overrides text.js (like Bootstrap does), the fix in core's text.js is not carried over without an additional patch for that theme.

Needs to be confirmed btut we might still need to make the JS change to handle required status changes through JS #states API. (a non required issue summary becomes required after a change in another field).

Fixed MARKED SNIFF VIOLATIONS of patch #16, Attached interdiff for the same.

Patch doesn't apply anymore on 9.3.18, here's a re-roll.

Tested the patch #21 patch applied successfully in drupal-9.5.x-dev.

Applied patch #21 on 9.5.x and the required summary form element is exposed after this patch.

Before patch;

After patch:

Thank you for a super clear description and testing steps. Following those I was able to confirm the bug and with the patch confirm it's fixed.

This patch is the same as #21 but also uploading a tests-only patch for the person who will commit this.

Looks like a good test

The issue summary does not have the standard template. Meaning there is no proposed resolution so reviewers can't confirm that the agreed to fix has been made. Adding tag.

Skimming the comments I see a question in #17 has not been answered. Setting to NW.

I also don't see a code review, my few comments below are not a full review.

1. +++ b/core/modules/text/tests/src/FunctionalJavascript/TextareaWithSummaryTest.php
   @@ -100,4 +100,45 @@ public function testTextSummaryBehavior() {
   +    $page = $this->getSession()->getPage();
   +    $summary_field = $page->findField('edit-' . $field_name . '-0-summary');
   

   Looks like we can skip creating $page.

2. +++ b/core/modules/text/tests/src/FunctionalJavascript/TextareaWithSummaryTest.php
   @@ -100,4 +100,45 @@ public function testTextSummaryBehavior() {
   +    $this->assertEquals(TRUE, $summary_field->isVisible(), 'Required summary is always shown.');
   

   Messages are no longer included is assertions.

Addressed #28

Took a look at #17 by
Hiding a text summary field until the title is filled in
When the field appeared it's summary was required
The default body field was not requiring summary.

So think we are good there.

Also updated issue summary so removing that tag.

Patch #30 failed to apply.
Re-roll the patch.

Passes 10.1 too

Applied patch #33 works fine with expected results on Drupal 10.1.x-dev
Refer to the screenshot

#30 faild to apply, because of #3181778: [w/c September 17th] Replace t() with $this->t() in all plugins

interdiff-30-33.txt in #33 is empty. The following is supposed to be its content.

$ diff 3145850-30.patch 3145850-33.patch
2c2
< index 1dcb00b96b..1986fa120f 100644
---
> index 60b51650c8..3b1f541d7d 100644
6c6
<        '#title' => t('Summary'),
---
>        '#title' => $this->t('Summary'),

• #36 tested it manually,
• the test-only patch in #25 is still valid,
• #28 was addressed.

Thanks!

Hi I have created an MR . Please Merge. thanks

@kunal_sahu thanks for the interest but the patch #33 was already reviewed. So the MR is just reuploading the work that was already done. So removing credit for that unless there was something you were fixing.

Thanks

Updated attributions

+++ b/core/modules/text/tests/src/FunctionalJavascript/TextareaWithSummaryTest.php
@@ -94,10 +94,50 @@ public function testTextSummaryBehavior() {
+    $this->assertEquals(TRUE, $summary_field->isVisible(), 'Required summary is always shown.');

No need for the message here, as @quietone pointed out above in #28.

On second thoughts, I can fix that on commit.

Updating credits

Fixed on commit

diff --git a/core/modules/text/tests/src/FunctionalJavascript/TextareaWithSummaryTest.php b/core/modules/text/tests/src/FunctionalJavascript/TextareaWithSummaryTest.php
index 75f6768dea9..0867f962c95 100644
--- a/core/modules/text/tests/src/FunctionalJavascript/TextareaWithSummaryTest.php
+++ b/core/modules/text/tests/src/FunctionalJavascript/TextareaWithSummaryTest.php
@@ -137,7 +137,7 @@ public function testTextSummaryRequiredBehavior() {
     $page = $this->getSession()->getPage();
     $summary_field = $page->findField('edit-' . $field_name . '-0-summary');
 
-    $this->assertEquals(TRUE, $summary_field->isVisible(), 'Required summary is always shown.');
+    $this->assertEquals(TRUE, $summary_field->isVisible());
   }
 
 }

Committed to 10.1.x and backported to 10.0.x and 9.5.x

Manually tested with a multi-cardinality field as there's some JS regarding that, but it need not run if the summary is optional - so all good.