Problem/Motivation

As a side effect from https://www.drupal.org/node/3473558, multiple admin pages that use vertical tabs have duplicate summaries on their tabs (e.g. Media add/edit forms).

Steps to reproduce

- Install the latest Drupal core 11.x and select Standard installation profile (this should install Claro as the admin theme).
- Go to /block/add and notice how the revision information vertical tab as a duplicate summary: "Revision informationNo revision"
- The same issue occurs when adding/editing media and taxonomy terms.

Proposed resolution

- Since the drupal.collapse library provides the interactivity for elements and it needs "core/misc/vertical-tabs.js" to load first so it can correctly create a summary (by running "core/misc/details-summarized-content.js"), I propose adding the drupal.vertical-tabs library as a dependency of drupal.collapse (see patch on comment #2).

Modify the logic on "core/misc/vertical-tabs.js" to extract only the text from the first child in the summary element instead of getting the concatenated text from all child elements using .textContent. This is because summary elements often have 2 child nodes: a title (either plain text or wrapped in an HTML tag) and a dynamic summary wrapped in . To set the vertical tab title.

Remaining tasks

- Create MR.
- Add test coverage
- MR needs review

User interface changes

- Will fix duplicate summaries on vertical tabs on some content entity forms like Media, Content Block, and Taxonomy Term.
- Note: The proposed solution will also address https://www.drupal.org/project/drupal/issues/3493182 and remove the need for adding "core/drupal.vertical-tabs" as a dependency of "ckeditor5/internal.drupal.ckeditor5.filter.admin".

Add Content Block form before UI fix:
Content block form showing vertical tabs with duplicate summaries.

Add Content Block form after UI fix:
Content block form after fixing UI issue.

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#24 before-MR-3498100.png165.39 KBshivam_tiwari
#24 after-MR-3498100.png147.49 KBshivam_tiwari
#10 3498100-10-core-duplicate-summary-on-vertical-tabs-11.x.patch331 bytesmmenavas
#7 duplicate-summary-after.jpg97.51 KBmmenavas
#7 duplicate-summary-before.jpg90.45 KBmmenavas
#2 3498100-2-core-duplicate-summary-on-vertical-tabs.patch332 bytesmmenavas
media-entity-form.jpg231.55 KBmmenavas

Issue fork drupal-3498100

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mmenavas created an issue. See original summary.

mmenavas’s picture

mmenavas commented
StatusFileSize
new 3498100-2-core-duplicate-summary-on-vertical-tabs.patch332 bytes

Uploading a patch that implements the proposed solution on the issue description (add vertical tabs library as a dependency of collapse).

mmenavas’s picture

mmenavas commented

Updating contribution information.

mmenavas’s picture

mmenavas commented
Issue summary: View changes
mmenavas’s picture

mmenavas commented

Alternatively, to prevent vertical-tabs.js from loading on every page that has Details elements, we can modify the logic on "core/misc/vertical-tabs.js" (line 89 as of v10.4.1) to extract only the text from the first child (text node) in the element as opposed to getting all of the text using the .textContent property which grabs the text from all children (including the element already added by detailsSummarizedContentText()).

quietone’s picture

quietone commented
Version: 10.4.x-dev » 11.x-dev
Assigned: mmenavas » Unassigned
Issue tags: +Usability, +Needs screenshots
Related issues: +#3493182: Block visibility settings have summary duplicated in the title

@mmenavas, thanks for the report and the fix.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies.

In Drupal core we prefer that people add a comment stating they are working on an issue. See Assigning ownership of a Drupal core issue for the details.

This should have before and after screenshots available in the Issue Summary so they can be found by reviewers and committers.

mmenavas’s picture

mmenavas commented
Issue summary: View changes
StatusFileSize
new duplicate-summary-before.jpg90.45 KB
new duplicate-summary-after.jpg97.51 KB
mmenavas’s picture

mmenavas commented
Issue summary: View changes
mmenavas’s picture

mmenavas commented

@quietone: Thank you for the feedback. I added the before and after screenshots. Please let me know how else I can help move this ticket forward. Would it help if I make an MR and write some tests?

mmenavas’s picture

mmenavas commented
StatusFileSize
new 3498100-10-core-duplicate-summary-on-vertical-tabs-11.x.patch331 bytes

Patch for Drupal core 11.x. The patch on #2 is for 10.4.1.

catch’s picture

catch
he/him
Primary language English
commented

An MR with test coverage would be good.

The solution in #5 sounds preferable if that works.

mmenavas’s picture

mmenavas commented

Thank you @catch! I'll create an MR as soon as I have the solution on #5 implemented, unless someone else beats me to it =)

brunoalmeida’s picture

brunoalmeida commented

I can confirm that the patch on #2 fixes the problem for 10.4.x

luke.stewart’s picture

luke.stewart commented
Issue tags: -Needs screenshots +Bug Smash Initiative

Removing needs screenshots tag as they are now included.

mmenavas opened merge request !11249

catch’s picture

catch
he/him
Primary language English
commented
Component: asset library system » javascript
mmenavas’s picture

mmenavas commented

@catch, I created an MR and implemented the changes mentioned in #5. I also added test coverage for vertical tabs with dynamic summaries. Let me know if there's anything else I can help with.

mmenavas’s picture

mmenavas commented
Issue summary: View changes
dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

Thanks for working to smash this bug!

Latest pipeline failed in phpstan. We need to see green testbot results before this is ready for review. In this case, hopefully a failing test-only run, too.

Thanks,
-Derek

mmenavas’s picture

mmenavas commented

Thank you for pointing out the failing PHPStan check, @dww. That should be fixed now. Unfortunately, I don't have permission to run the test-only CI job.

poker10’s picture

poker10 commented
Status: Needs work » Needs review

I run the test-only job. Tests are green and test-only run failed - https://git.drupalcode.org/issue/drupal-3498100/-/jobs/4539840 . Moving back to NR.

mmenavas’s picture

mmenavas commented

Thank you so much, @poker10!

shivam_tiwari’s picture

shivam_tiwari commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new after-MR-3498100.png147.49 KB
new before-MR-3498100.png165.39 KB

I found this issue, checked MR also. It is working fine. I am adding SS also before and after that applying MR. There is no test issue also.
Moving this to RTBC.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

if we had the before/after thing for library this would be a 2 lines diff :)

always useful to harden the code, small code improvement needed

mmenavas’s picture

mmenavas commented

Thank you @_nod! I just rebased my fork branch and pushed the code changes you suggested. I agree the before/after feature is much needed.

Note: I'm currently in the contributing session at DrupalCon Atlanta, and I'd like to thank Igor for being such a great mentor.

mmenavas’s picture

mmenavas commented
Status: Needs work » Needs review
mmenavas’s picture

mmenavas commented
Issue summary: View changes

I just marked the issue as "Needs Review". I'm currently at DrupalCon Atlanta if any core contributors have questions regarding this issue.

igorgoncalves’s picture

igorgoncalves commented
Issue tags: +Atlanta2025

Worked with @mmenavas at #Atlanta2025

Issue still needs review.

brunoalmeida’s picture

brunoalmeida commented

I just updated my fork branch with the latest updates and I can confirm that it is working! +1

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

The test runner seems to have failed on a bunch of kernel tests, but it was not clear if it is related to the changes.

mmenavas’s picture

mmenavas commented

@brunoalmeida: Thank you for reviewing!

@mradcliffe I reran the failing CI check. Checks are green now.

vcizek’s picture

vcizek commented

Confirmed that I'm seeing the corrected version like in the screenshots when adding content blocks, media items and taxonomy terms.

vcizek’s picture

vcizek commented
Status: Needs review » Reviewed & tested by the community

@mmenavas worked with me to understand this issue at Drupalcon.

nod_ closed merge request !11249

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed 0116cbe and pushed to 11.x. Thanks!

  • nod_ committed 1d496dff on 11.x 
    Issue #3498100 by mmenavas, shivam_tiwari, brunoalmeida, catch, dww,...
nod_’s picture

nod_
Primary language French
Location Lille
commented

Crediting igorgoncalves for mentoring

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

idebr’s picture

idebr commented

The regression was introduced in 10.4.x, but the fix is only available in 11.x. Any chance of a backport to 10.x?

andrew.wang’s picture

andrew.wang commented

Echoing the above comment - any chance this will be backported to 10.x given this regression was introduced in 10.4?