"Textarea + summary" widget broken when field allows multiple values (followup) and associated JavaScript uses fragile selectors

It's true:

nice :-/

Was playing around with this some more - here is some more info for anyone trying to re-create:

1. Problem occurs as soon as you change cardinality to more than 1 (not just unlimited).

2. Problem occurs as soon as you enable the input of an explicit summary.

3. The way it break is not (unfortunately) consistent. It does not add the same multiple number of summary links based on cardinality each time...

4. I came across another bug but will file that separately.

I added a condition to check if there’s a label per textarea, and if not – cardinality is unlimited or > 1 – it creates one.
Also, I check if the summary is already processed, to prevent multiple processing.

Instead of doing the check with the CSS class, I think you should use jquery.once().
Also some whitespace in the patch.

Ooops! Fixed.

Thks roborm. This looks good to me, I'll let tstoeckler feedback or set to RTBC is this is good for him.

hah, crosspost with the bot :-p

Code looks good, but I haven't tested it. Also, I don't get the bot failure...

#6: textarea-summary-822128-6.patch queued for re-testing.

Indeed, the failure looks hardly related. Re-test.

Patch works, and pass testing. Code looks clean.

Ha! Nice bug. :) And fix!

Committed to HEAD! Welcome to the core team, roborn!

+++ modules/field/modules/text/text.js	9 Jun 2010 16:42:46 -0000
@@ -9,30 +9,40 @@ Drupal.behaviors.textSummary = {
     $('.text-summary', context).once('text-summary', function () {
       var $widget = $(this).closest('div.field-type-text-with-summary');
...
+      var $summaries = $widget.find('div.text-summary-wrapper');
...
+      $summaries.once('text-summary-wrapper').each(function(index) {

1) Missing space after anonymous "function".

+++ modules/field/modules/text/text.js	9 Jun 2010 16:42:46 -0000
@@ -9,30 +9,40 @@ Drupal.behaviors.textSummary = {
+        var $full = $widget.find('.text-full').eq(index).closest('.form-item');

I'm concerned that this index-based selection heavily depends on the form elements and markup/output of the default text field widget. I'd highly recommend to rework this code, so as to be not index-based. This may mean that we need to change and improve the HTML markup of the text field widget, e.g., to wrap each widget in a new container so as to be able to properly target it via JS.

+++ modules/field/modules/text/text.js	9 Jun 2010 16:42:46 -0000
@@ -9,30 +9,40 @@ Drupal.behaviors.textSummary = {
+        // Create a placeholder label when the field cardinality is
+        // unlimited or greater than 1.

Is "unlimited" and "greater than 1" not simply the same as "multiple" here? Inline comments should explain more "why" than "what" is being done - I don't really get what happens here and why.

+++ modules/field/modules/text/text.js	9 Jun 2010 16:42:46 -0000
@@ -9,30 +9,40 @@ Drupal.behaviors.textSummary = {
+        return;

Stale return? (was there before already, it seems)

Powered by Dreditor.

Followup not critical.

Clean-up, turns out there was an unnecessary loop and hard to read selectors, fixed that. removed the return, doesn't do anything. Changed return false; to e.preventDefault().

It's not using indexes to do it's thing now.

Re-rolled and tested #17. No problems.

BTW, it was #1419968: Replace $('selector', domelement) with $(domelement).find('selector') that broke the patch in #17.
#18 patch just changes
$('.text-summary', context).once('text-summary', function () {
to
$(context).find('.text-summary').once('text-summary', function () {

stray console.log() ;-)

Thanks :-). Looks fine.

Comment cleanup.

Thanks! Committed/pushed to 8.x. This looks like it needs a 7.x backport.

Needed a reroll due to #1419968: Replace $('selector', domelement) with $(domelement).find('selector') not being in D7 yet.

Test ok for the backport. Seems OK. Anyone makes another test for RTBTC ?

tested in #27

As JavaScript changes go, this is a little bigger than I feel comfortable committing right before the Drupal 7.15 release :) So, let's wait until after 7.15 is released and try again then. Sorry...

Also, as far as I understand, there is no longer any direct bug here anymore, right? (This is just followup at this point.)

I also found this a little odd:

-        var $link = $('<span class="field-edit-link">(<a class="link-edit-summary" href="#">' + Drupal.t('Hide summary') + '</a>)</span>').toggle(
.....
+      var $link = $('<span class="field-edit-link">(<a class="link-edit-summary" href="#nogo">' + Drupal.t('Hide summary') + '</a>)</span>');

Is that "nogo" stuff something we're doing elsewhere?

I'm also curious about the 'nogo', this is the first occurrence of it in core, and I just saw it again in a comment here #1538462-26: Cannot log in with OpenID due to "required" attribute.

I understand the idea behind it (clicking a link with just # will jump to the top whereas #nogo won't move at all) but maybe we should document that somewhere globally, or add inline comments when using it. (That depends on how widespread it should become).

Yeah. Let's remove that #nogo part from this patch and discuss the idea/approach in an own issue first. It looks like a smart hack with good intentions, but it is confusing, could very easily hit a false-positive, and in this case, it looks unnecessary.

It's more like, why do we create links that don't go anywhere in JS to begin with?

It got in in D8. I'll open an issue to replace all that with button like it's supposed to be done. #1719640: Use 'button' element instead of empty links

Changing the title because my understanding is that the original bug is fixed.

However, I just came across #1983516: "Text area with summary" fields are missing the "Edit summary" functionality and realized that the patch here fixes that issue as a side effect. (This is because it gets rid of the problematic var $widget = $(this).closest('div.field-type-text-with-summary') line of code.)

Hi all,

I'm re-rolling this patch for Drupal 7. I know D7 is very close to "end of life" but this issue has been languishing for a long time and I'm hoping to get it fixed.

Re-running tests on the re-rolled patch in comment #39 and added this issue to #3216978: [meta] Priorities for 2021-12-01 release of Drupal 7 in anticipation that issue will be rolled over into a new issue of priorities for the next release of D7 core.

Correct me if I'm wrong, but the "#nogo" is removed from the patch, so the status can be updated.

I've tested patch #39, therefore I'm updating the status to RTBC.

Do note that this patch actually changes the behavior of the summary field. It'll now render expanded by default instead of collapsed.
Not sure if we want this, since it differs from D9.

@Fonteijne Good observation.

this patch actually changes the behavior of the summary field. It'll now render expanded by default instead of collapsed. Not sure if we want this, since it differs from D9.

Agree this is not desirable and it is not possible to change the configuration of a site to hide summary by default.

Changing status to "Needs work".

Here is a related issue for Drupal 9, #2826356: Summary hidden if Text field has Unlimited number of values.

Here's the commit diff that might be of help,

https://git.drupalcode.org/project/drupal/-/merge_requests/1684/diffs

Can an new issue be opened for that please? reopening 9 years after the last comment and changing the version is not ideal.

Actually, almost half of the comments are related to the backport to D7, comments from #25 on. Maybe the comments from #39 onwards can be copied to a new issue for the D7 back port.