Show 'Edit summary' links on multi-value long text field with summary

Still present in 8.5. Given that text fields are the most basic datatype in any CMS I think this bug deserves a "major" priority.

It's still present in 8.7. I found this for when help text is added but doesn't help with it being a multi-value field:
https://www.drupal.org/project/drupal/issues/2626716

This issue seems isolated to multi-value summary fields only, since the Help text issue linked in the issue summary is now fixed. I expanded the TextareaWithSummaryTest (FunctionalJavascript) to include a test for a multi-value Body field.

With the actual fix, I went with just prepending the Edit Summary button to the wrapper instead of the label, keeping the labels hidden (with: (value 1) etc). To achieve this I simply checked for an existence of a multi-value table container within the parents of the summary field.

There is 1 slight visual/aesthetic drawback of my method, there are situations where a single value text+summary field could be nested within a multi-value field (such as Paragraphs). In this case, the Edit summary link is displayed above the visible label instead of after it. It's not ideal but it works. I thought I'd get a patch in and get the ball rolling at least.

Hi,

I have reviewed the patch #10 and it's working great.

Below is the output screenshot body text before applying the patch.

Below is the output screenshot body text after applying the patch.

Thanks,
ren

Thanks for trying to solve this persistent issue!

This looks promising, but it doesn't apply to 9.1.x, so we need the D9 version of the patch.

Also tagging for review of the JS from the frontend framework manager.

Thanks!

Thanks @baysaa! Tried it out locally and it works for me.

1. +++ b/core/modules/text/text.es6.js
   @@ -20,6 +20,7 @@
   +          const $multiValue = $(this).parents('.field-multiple-table').length;
   

   1. I think we want .closest() here: it returns at most one matching element from its DOM ancestors.
   2. It might be dangerous tying our JS to the presentation class field-multiple-table: could we use a js- class or (perhaps better) a data attribute?

2. +++ b/core/modules/text/text.es6.js
   @@ -20,6 +20,7 @@
              const $widget = $(this).closest('.js-text-format-wrapper');
    
   +          const $multiValue = $(this).parents('.field-multiple-table').length;
   

   Nit: You could store $(this) in a variable.

3. +++ b/core/modules/text/text.es6.js
   @@ -44,7 +45,7 @@
   +                ($multiValue) ? $link.prependTo($widget) : $link.appendTo($fullLabel);
   

   Nit: Just a style thing but personally I'd go for an explicit conditional here: I find that a bit more readable.

Thanks @xjm and @AndyF for the reviews.

Here's an updated D8 patch, taking AndyF's suggestions on board. Using `jQuery.closest()` in combination with `jQuery.find()` I was able to resolve the aesthetic issue mentioned in #10

I'll work on a D9 patch later today.

Tests have not changed.

Added interdiff, but didn't get uploaded with last commend. Here's it.

reroll diff wasn't required.

Here's a D9 reroll against 9.1.x

Worked in collaboration with @AndyF over zoom :)

#18 Looks good to me, thanks! I've tested the 8.9 and 9.1 patches locally, and also checked that the generated JS matches. Tentatively moving back to RTBC as I only helped with the 9.1 reroll rather than the actual update... (:

I've tested this patch and it fixed the problem as described in the issue summary and has automated test coverage. Nice.

There are issues around the default value field whilst creating a field but they exist in HEAD and there's an issue already for it - #3115978: After enabling Require Summary on a field can't save the field

+++ b/core/includes/theme.inc
@@ -1705,6 +1705,8 @@ function template_preprocess_field_multiple_value_form(&$variables) {
       '#attributes' => [
         'id' => $table_id,
         'class' => ['field-multiple-table'],
+        // Allow JS to act differently for multi-value fields, if necessary.
+        'data-field-multiple-table' => TRUE,
       ],

One consideration is whether this is a good idea to add to ALL multiple value fields. I think it is.

I can't remove the "Needs frontend framework manager review" tag as I'm not one but as far as I can see this issue looks good.

Make sure you have the JavaScript tag on javascript issue, otherwise it's very hard to track them :)

I'm not too happy about the whole thing since we're adding the element in different places depending on the cardinality. The styling on seven is not too bad, we go from normal weight to bold and a margin gets changed. But on claro the weight, size, margin and it doesn't look good to be honest.

I have a less invasive take that doesn't conflict with the styling and that will work when the label is hidden outside of a multi-value field. I wanted to remove the part about the placeholder because now we (should) have a label all the time #933004: Test that all form elements have a title for accessibility but that's scope creep.

no interdiff since it's a different approach.

I like the approach in #26 because it makes the styling and markup more consistent. I'm unsure what to think about using visually-hidden as the integration point. Given that the patch has a comment explaining what that extra big of logic is for, it's probably fine. Leaving this for needs review in case others have thoughts on the new approach.

#26 +1, a cleaner and neater solution than mine imho.

I've stared at #26 for a while pondering it. What I've come to wonder if is that adding the edit summary link inside the label is semantically correctly. It's not a label right?

I think it's borderline? The spec says that a button element inside a label is valid under certain conditions. From what I understand we're not fulfilling the conditions… then again, it's not broken?

In any case this is not something we should fix only when the fields are in a tabledrag, so as far as this issue is concerned I'd go with this even if it's borderline to make it in line with the rest. It's been many years and I haven't seen it come up in accessibility reviews before.

@nod_ thanks for pointing me to the standards. Funnily enough I think we're pretty clearly in the bad example area here.

<!-- bad example - link inside label reduces checkbox activation area -->
<label><input type=checkbox name=tac>I agree to <a href="tandc.html">the terms and conditions</a></label>

TIL if you click on the label for a text box the carat is positioned in the text box.

However this of course is not made any worse by the patch. It only make me question whether or not we should review this from an accessibility perspective first and then the fix might be to fix the accessibility issue which has the happen co-incidence of fixing this issue. I'm going to ask in the accessibility channel.

Alex asked about this in the accessiblity slack channel, and I chatted a bit with nod_.

The "edit summary" button is horrible in lots of ways. More on that later.

Re. #30:

The spec says that a button element inside a label is valid under certain conditions. From what I understand we're not fulfilling the conditions… then again, it's not broken?

Where, exactly? I don't think it says that at all. What it does say explicitly, is that a <label> must contain "no descendant labelable elements unless it is the element’s labeled control".

A <button> is a labelable control, so it can only be a descendant of a <label>, if the label is for the button. But a button's accessible name comes from it's content (unless ARIA changes that). The following examples would be permitted (but they are rather perverse):

<label><button>Do Something</button></label> // what's the point of the label?
<label id="lbl1">Do <button id="btn1" aria-labelledby="lbl1 btn1">Something</button></label>

In the case of the long text + summary Drupal widget, we have this markup:

<label for="edit-body-0-value">Body<span class="field-edit-link"> (<button type="button" class="link link-edit-summary">Edit summary</button>)</span></label>

This isn't permitted by the HTML content model, because the <label for> isn't for the <button> inside it.

Re. #31:

<!-- bad example - link inside label reduces checkbox activation area -->
<label><input type=checkbox name=tac>I agree to <a href="tandc.html">the terms and conditions</a></label>

This is fine by the HTML content model!

• The only descendant labelable element is the checkbox input.
• So there is a checkbox input, with a computed accessible name of "I agree to the terms and conditions".
• There is also a link with the computed accessible name of "terms and conditions".

The part about reducing the checkbox activation area isn't very significant, unless the combined width of the checkbox and "I agree to" adds up to less than 44px. The important thing is that the link text can be (a) identified as a link, and (b) distinguished from the rest of the label. (The Seven theme fails badly here - #2958282: Links inside surrounding text fail WCAG Use-of-Color in Seven theme.)

In a single-value, long-text-and-summary widget:

1. The "edit summary" button is inside a label, but the label is for the body textarea. This fails the HTML content model.
2. The computed accessible name of the body textarea is "Body (edit summary)". It's a poor name, because "edit summary" is nothing to do with the body textarea.
3. The "edit summary" button has some unusual behaviour:
   1. It disappears when you activate it!
   2. Consequently, the accessible name of the body textarea has changed. Changing the name of form inputs isn't something that should be done lightly.
   3. It makes something else (the summary textarea) appear. That's a disclosure behaviour, but the summary appears before the button that controls it. That's not the usual order for a disclosure control; a screen reader user may not easily find the summary textarea. It would be more conventional for it to appear after the disclosure button.
4. A new button ("hide summary") appears. The show and hide actions are on separate elements, which is quite cumbersome. Disclosure elements are better done with a single button. The disclosure state can be conveyed with aria-expanded.
5. The "hide summary" button is also inside a <label> for another element, so it fails the HTML content model too.
6. The computed accessible name of the summary textarea is "Summary (Hide summary)". Clumsy, but at least it doesn't refer to a different input.

For a multi-value, long-text-and-summary widget:

1. The entire label for the body textarea gets a .visually-hidden class. That means the "edit summary" button isn't visible. This is the problem this current issue is trying to solve.
   • Sighted users don't know it's there.
   • Pointer users can't operate it.
   • Worse still, keyboard users can operate it, even though they can't see it! That's a failure of WCAG 2.4.7 Focus Visible.
   • Speech control users have very little chance of being able to operate it, though they might discover it accidentally.
   • For (blind) screen reader users, this isn't too bad.
   • For sighted (or partially sighted) screen reader users, there is a mismatch between what can be seen, versus what can be heard. That might confuse people who use a magnifier in combination with a screen reader. It says there's a button, but you can't see it.
2. The body textareas get different names, so they can be distinguished: "Body (value 1)" and "Body (value 2)". But the summary text areas all get called "summary" and you can't distinguish them.
3. Plus all the other problems which the single-value version has.

Patch #26:
This solves the problem of the "edit summary" button being invisible in multi-value scenario.
It doesn't help with any of the other problems though. The behaviour and names are still horrible.

Patch #23:
This is better! It solves problems 1, 2, 3.2, and 3.3 which I described for single-value fields. But... it only fixes them for multi-value fields. The single value fields still have these problems. I think that's what @nod_ was saying in #26? ("I'm not too happy about the whole thing since we're adding the element in different places depending on the cardinality.")

What I'd do:

• Remove the "edit summary" button from the the body label.
• And remove the "hide summary" button from the summary label.
• Replace them with a single button, which stays in one place, and doesn't ever disappear.
• Make the summary appear after the button which discloses it.
• This will mean a visual design change.

Thanks for the thorough review!

You're correct, I did not understand the spec correctly :)

From what I get we could use a detail element to wrap the summary field in, it's already accessible, element stays in one place. Quick and dirty patch to put a details element seems to be working but it'll be a challenge visually.

Not sure how "clean" the solution should be here. I have a quickfix in #26, but fixing it "properly" will take some time and given that this is major I don't know.

all right, opened #3155130: "Edit summary" toggle on text fields has many accessibility issues as a major bug.

I propose we go with #26. The situation will be as bad on multi-value fields as it is on single fields, instead of bad for single fields and unusable for multi-value fields.

Tested via above mentioned steps and the latest patch. Issue is still there. I can't see 'Edit summary' while creating a content

Tested the latest patch on simplytest.me

We are able to see Edit Summary for multivalued field with text + summary

We are seeing a broken UI in bottom of field that needs a fix

Moving back to need fix

I am also experiencing this issue with a muli-value body field - the edit summary link is gone - Drupal 8.9.6.

I tried patch #10 and #26 (random choices) and neither worked for me.

The patch is for 9.2 so not sure how well it'd work in 8.9, should be similar, the js file didn't change in a long time. Were the cache cleared before testing? if you have JS aggregation activated, it won't work until you clear the cache.

The spacing issue in #36 doesn't seem related, it's present with or without this patch. Once a textarea with summary is multi value the CSS leave the existing margins.

The accessibility concerns are not ignored, a proper solution need to be found here #3155130: "Edit summary" toggle on text fields has many accessibility issues. Given that an a11y fix will impact html/css/js it might take a while to finalize. In the meantime we can give back access to the feature (and the data) for users by making it as broken for multi-value field as it is today for single-value fields.

This issue also apparently affects Drupal 7 in #980144: Issues with "required, multiple" fields in forms, so will probably need porting when this finally lands.

The MR fixes the cosmetic issues, for the true accessibility fix see #3155130: "Edit summary" toggle on text fields has many accessibility issues. This one is an intermediate step.

well when the fork was made 9.3.x didn't exist and now we can't create a branch from it easily. Instead of figuring it out here is a good old patch for 9.3

After patch we can access edit summary option

Thanks for the patch for ref sharing screenshot

Thanks

fixing code checks

Patch looks good. Ok to RTBC?

back to rtbc

Committed b57729b and pushed to 9.3.x. Thanks!

This issue also apparently affects Drupal 7 in #980144: Issues with "required, multiple" fields in forms, so will probably need porting when this finally lands.

It sounds like an open and relevant ticket for D7 is #822128: "Textarea + summary" widget broken when field allows multiple values (followup) and associated JavaScript uses fragile selectors, although that is 12 years old... 🤷‍♂️

Closed #2826356: Summary hidden if Text field has Unlimited number of values as a duplicate, adding credit.