Problem/Motivation

In the refactoring issue, https://www.drupal.org/project/drupal/issues/3239134, the [0] hatch was added to misc/vertical-tabs.js in line 98: https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/misc/vertica...

If there are multiple forms on a page that uses vertical tabs, this will produce a javascript error because of the hard-coded [0].

The following javascript error occurs

Uncaught TypeError: Cannot set properties of undefined (setting 'value')
    at Drupal.verticalTab.focus (vertical-tabs.js?v=9.4.0:98:78)
    at vertical-tabs.js?v=9.4.0:65:40
    at Array.forEach (<anonymous>)
    at Object.attach (vertical-tabs.js?v=9.4.0:25:68)
    at drupal.js?v=9.4.0:27:24
    at Array.forEach (<anonymous>)
    at Drupal.attachBehaviors (drupal.js?v=9.4.0:24:34)
    at HTMLDivElement.open (nd_visualshortcodes.js?v=1:588:16)
    at t.<computed>.<computed>._trigger (widget.js:706:13)
    at t.<computed>.<computed>.open (dialog.js:300:8)

See screenshot:

Steps to reproduce

1. Create a page that has multiple forms using vertical tabs.
2. If you click one of the tabs, you get the javascript error described above.

Proposed resolution

Remove the [0] hatch from misc/vertical-tabs.js like such.

From:

        focus() {
      this.details
        .siblings('.vertical-tabs__pane')
        .each(function () {
          const tab = $(this).data('verticalTab');
          tab.details.hide();
          tab.details.removeAttr('open');
          tab.item.removeClass('is-selected');
        })
        .end()
        .show()
        .siblings(':hidden.vertical-tabs__active-tab')[0].value =
        this.details.attr('id');

to

        focus() {
      this.details
        .siblings('.vertical-tabs__pane')
        .each(function () {
          const tab = $(this).data('verticalTab');
          tab.details.hide();
          tab.details.removeAttr('open');
          tab.item.removeClass('is-selected');
        })
        .end()
        .show()
        .siblings(':hidden.vertical-tabs__active-tab')
        .get()
        .forEach((hidden) => {
          hidden.value = this.details.attr('id');
        });

Using [0] directly assumes that there is exactly one hidden element matching `:hidden.vertical-tabs__active-tab`.But with the `.siblings(':hidden.vertical-tabs__active-tab')` selects all hidden sibling elements with the class 'vertical-tabs__active-tab'. and .get() method iterates through all the elements matching the condition.
With the latest changes

If there are multiple forms on a page that uses vertical tabs, this will produce a javascript error because of the hard-coded [0].

will be solved.

CommentFileSizeAuthor
#15 interdiff_10-15.txt710 byteskostyashupenko
#15 3291764-15.patch881 byteskostyashupenko
#13 3291764-nr-bot.txt1.23 KBneeds-review-queue-bot
#10 multiple-form-on-vertical-tabs-3291764-10.patch1.04 KBfarnoosh
#8 3291764-8.patch483 byteseugene.brit
#6 multiple_forms_on_vertical_tabs-3291764-5.patch2.02 KBrhiss
#4 interdiff_2-4.txt663 bytesranjith_kumar_k_u
#4 3291764-4.patch1.1 KBranjith_kumar_k_u
Edit_custom_block_Homepage_Tabs___SOFIA_Science_Center.png947.3 KBbaldwinlouie

Issue fork drupal-3291764

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

baldwinlouie created an issue. See original summary.

baldwinlouie opened merge request !2420

baldwinlouie’s picture

baldwinlouie commented
Status: Active » Needs review

attaching patch with proposed fix.

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new 3291764-4.patch1.1 KB
new interdiff_2-4.txt663 bytes
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Postponed (maintainer needs more info)

The fix can not work, this is setting the value property on a jQuery object, not on all the matching DOM objects. I didn't see an issue with the replaced code at the time, would be great to have the steps to reproduce (or better add some test code that can reproduce the issue).

to add some details, the fix probably removes the error but it most likely breaks one of the vertical tabs functionality

rhiss’s picture

rhiss commented
StatusFileSize
new multiple_forms_on_vertical_tabs-3291764-5.patch2.02 KB

Hello, i had this problem too while woking with the visual shortcode module. The patch was missing one line, after adding it woked out fine for me. I created a patch for core 9.4.x.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

eugene.brit’s picture

eugene.brit
Primary language Ukrainian
Location Kyiv 🇺🇦
commented
StatusFileSize
new 3291764-8.patch483 bytes

Re-roll for 10.0.x

kevin.pfeifer’s picture

kevin.pfeifer commented

I also have a project with the drupal/shortcode module and the patch from #5 fixes the problem!

farnoosh’s picture

farnoosh commented
StatusFileSize
new multiple-form-on-vertical-tabs-3291764-10.patch1.04 KB

A quick solution to remove the error for now on drupal 10.1.0

mkdok’s picture

mkdok commented

Patch #8 from @eugene.brit works perfectly on D10.

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Version: 9.5.x-dev » 10.0.x-dev
Status: Postponed (maintainer needs more info) » Reviewed & tested by the community

Because of #11

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3291764-nr-bot.txt1.23 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

saranyamariappan’s picture

saranyamariappan commented

I am working on this.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Needs work » Needs review
StatusFileSize
new 3291764-15.patch881 bytes
new interdiff_10-15.txt710 bytes

Fixed linting errors

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Can we add a test case to cover this bug.

saganakat’s picture

saganakat commented

Hello,

Thank you for the contribution.

I've encountered a similar issue with the nd_visualshortcodes module.

Applying the patch from the merge request using Composer:

"patches": {
    "drupal/core": {
        "[3291764] The [0] hatch in misc/vertical-tabs.js causes issues if there are multiple forms with vertical tabs.": "https://git.drupalcode.org/project/drupal/-/merge_requests/2420.patch"
    }
}

It worked for me. Thank you very much!

Tested on Drupal 9.5.11

candelas’s picture

candelas commented

Thanks @kostyashupenko : patch #15 works in Drupal 10.2.2

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue tags: -Needs tests +Needs issue summary update

The patch solution is a bit more complex than it needs to be. Looping through the results called by get() should be sufficient as it will simply not loop through anything if there are no results

      .siblings(':hidden.vertical-tabs__active-tab')[0].value =
        this.details.attr('id');

can be replaced with
.siblings(':hidden.vertical-tabs__active-tab').get().forEach((hidden) => hidden.value = this.details.attr('id');)

The issue summary currently suggests a solution that 1) would not work as jQuery does not have a .value property 2) suggests reverting back to jquery value settings, which our linter would not allow.

Update the suggestion to something that reflects what is actually happening.

As a committer / FEFM I'm fine with no tests on this as long as the scope of changes is just ensuring that value is not called on a result that might be empty.

Gauravvvv changed the visibility of the branch 3291764-the-0-hatch to hidden.

Gauravvvv opened merge request !8366

gauravvvv’s picture

gauravvvv commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Updated proposed solution in the issue summary

Utkarsh_33 made their first commit to this issue’s fork.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Adding back the issue summary tag from #20.

Can it be updated to include the solution and why it's needed. Currently it's showing the diff of the MR essentially

Thanks!

utkarsh_33’s picture

utkarsh_33 commented
Issue summary: View changes
utkarsh_33’s picture

utkarsh_33 commented
Status: Needs work » Needs review

Updated the Proposed resolution section explaining the solution.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update +Needs Review Queue Initiative

Thank you @Utkarsh_33 for providing the additional detail.

Believe all feedback has been addressed

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: The [0] hatch in misc/vertical-tabs.js causes issues if there are multiple forms with vertical tabs. » The [0] hatch in misc/vertical-tabs.js causes issues if there are multiple forms with vertical tabs
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 8c9ff22228 to 11.x and 035142460f to 11.0.x and 10691987c8 to 10.4.x and c55ad7b289 to 10.3.x. Thanks!

nod_ closed merge request !8366

  • nod_ committed d4483f5c on 11.x 
    Issue #3291764 by Gauravvvv, baldwinlouie, Utkarsh_33, kostyashupenko,...

  • nod_ committed d850c199 on 10.3.x 
    Issue #3291764 by Gauravvvv, baldwinlouie, Utkarsh_33, kostyashupenko,...

  • nod_ committed cee1be2b on 10.4.x 
    Issue #3291764 by Gauravvvv, baldwinlouie, Utkarsh_33, kostyashupenko,...

  • nod_ committed 605e9bfe on 11.0.x 
    Issue #3291764 by Gauravvvv, baldwinlouie, Utkarsh_33, kostyashupenko,...

Status: Fixed » Closed (fixed)

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