Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Node edit page "Menu Settings" hidden (as with other closed details elements) because it has classes added ClaroPreRender::verticalTabs
reaches too many "groups" that are not "vertical tabs" when an a vertical tab is added to the form. AKA "overreaching code"
Steps to reproduce
- Using Claro admin theme
- Add an
image_widget_crop
widget to a node (or any thing that adds vertical tab) - Edit node and add a photo (to add a vertical tab to the page) and save
- Visit node edit form again and notice all the collapsed details like Menu Settings are missing because they have vertical tab classes attached.
Proposed resolution
Only target groups that the element belongs to instead of all #groups.
Remaining tasks
User interface changes
Unbroken details elements
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#23 | Screenshot from 2021-06-09 14-07-33.png | 73.12 KB | Rinku Jacob 13 |
#19 | 3177415-18-vertical-tabs-applied-to-more-detail-groups.patch | 3.14 KB | joelpittet |
#19 | 3177415-18-vertical-tabs-applied-to-more-detail-groups-test-only.patch | 2.26 KB | joelpittet |
#19 | interdiff.txt | 2.02 KB | joelpittet |
#17 | interdiff-14-16.txt | 320 bytes | kishor_kolekar |
Comments
Comment #2
jplana CreditAttribution: jplana commentedHere is a simple patch for this issue.
Comment #3
joelpittetComment #4
joelpittetComment #5
joelpittetComment #7
joelpittetI think this is a great fix because it prevents the classes from being applied to all other details elements in every group and only targets the group that the vertical tab element belongs to.
It could use a comment describing that, but should fix a major UI bug. Moving to Major because it hides all the details summaries in the node form side bar ("advanced") group
Comment #8
joelpittetre-title
Comment #9
lauriiiTested manually for regression and everything seemed to work as expected. However, I think we should add test coverage for the bug just to make sure that we don't break this again in future.
Comment #10
jplana CreditAttribution: jplana commentedI had a second look at the original patch code posted and worked a little on refactoring it. The idea behind the refactor is to include all parents, and avoid the overreach of all the details elements.
I'm posting here my work in progress as a patch.
Comment #11
ramil g CreditAttribution: ramil g at The University of British Columbia commentedUpdated the comments.
Comment #12
joelpittetWhile writing unit tests, jplana, @ramil g, and I found that in Unit tests the classes from core themes weren't loading. @larowlan and @berdir helped pinpoint the issue in Drupal Slack and created an issue #3209048: Core themes are not added to the test autoloader
Comment #13
jplana CreditAttribution: jplana commentedI'm posting here a potential test, in a work in progress state. I ran out of time today, please feel free to jump and fix/complete it :)
core/tests/Drupal/KernelTests/Core/Theme/ClaroVerticalTabsTest.php
Comment #14
ramil g CreditAttribution: ramil g at The University of British Columbia commentedThis is an iteration on jplana's code (#13).
I've added the assertions which tries to find the classes in the elements.
The output markup is missing the bug, but the structure of the form element has the incorrect class.
Comment #15
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedComment #17
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedadded patch as#14 custom command failed.
please review the patch.
Comment #19
joelpittetFinally tricked it out, the order matters of the rendering for this bug to show... apparently.
One test should fail (tests only) and the other should pass!
Comment #20
joelpittetComment #22
joelpittetI should mention I RTBCd the patch because though I had a hand in it, it was only a small tweak to the order of elements that made the patch test act accordingly. Feel free to knock it back if you need another set of 👀
Comment #23
Rinku Jacob 13 CreditAttribution: Rinku Jacob 13 at Zyxware Technologies commentedverified and tested patch #19 and applied successfully thanks @joelpittet.
Comment #24
larowlanHi @Rinku Jacob 13 thanks for uploading a screenshot, however a screenshot showing a patch applies is not something we give issue credit for.
We have automation to tell if a patch applies or not.
Comment #26
lauriiiI was concerned that the #parents couldn't be relied for this but it seems like vertical tabs is documented to only work in forms and
Drupal\Core\Render\Element\VerticalTabs::preRenderVerticalTabs
already depends on it.Committed c5820b9 and pushed to 9.3.x. Thanks!
Since this is fixing a major bug, I'm moving this to 9.2.x for a potential backport.
Comment #28
lauriiiBackported this to 9.2.x because @catch +1'd on Slack.