Problem/Motivation
On Manage display and Manage form display, tabledrag match-parent for field-region writes a wrong value into fields[][region].
When nesting, the rule used source field-name. That copied the parent machine name into region. The value is invalid, so save puts the row in hidden.
When un-nesting to root in the same region (for example Disabled), tabledrag uses the first table row as source. That often copies content into region while the row stays visually disabled. Save then moves the field to content.
Contrib modules such as field_group make this easy to hit. The fault is in core field_ui.
Steps to reproduce
- Nesting: Indent a row under a parent (for example a field_group). Save and reload. The child may be in hidden.
- Un-nesting: Move a group with children to Disabled. Outdent a child while it stays in Disabled. Save and reload. The field may be in content.
Proposed resolution
Remove the field-region match-parent tabledrag rule from EntityDisplayFormBase. Region comes from the table section via field_ui.js, not from parent matching.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 3613588-field-ui-manage-12.patch | 943 bytes | grevil |
| #3 | 3613588-field-ui-manage-3.patch | 663 bytes | grevil |
Issue fork drupal-3613588
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:
Comments
Comment #3
grevil commentedStatic patch for the time being.
Please review!
Comment #4
grevil commentedphpcs and phpstan failures are definitly unrelated, this is a simple string replacement.... unfortunately the tests are skipped now -.-.
Comment #5
grevil commentedAfter rerun everything succeeds and is green now :)
Comment #6
smustgrave commentedEven though it’s a 1 line change definitely feels like something that should have test coverage. Disappointed it didn’t already (haven’t fully digged yet)
Comment #7
thomas.frobieterWorks very well again, I can't reproduce the problem anymore!
So, let's add some tests and finish this off, so that we can finally get rid of this annoying problem! :)
Comment #8
grevil commentedAdded a quick test, that checks, that the correct rule is set in the field-region and field-parent "match parent" rule.
I have not added any rules that simulate the actual dragging in the browser (I wouldn't even know where to start without using the contrib "field_group" module.
I hope that is enough! Please review :)
Comment #9
needs-review-queue-bot commentedThe 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 necessarily 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.
Comment #10
grevil commented@smustgrave actually let us do some further testing with this. The issue and all the parts working with this are quite complex and very hard to trace back to the place where this should get changed. I just found another issue, where when a field-group with a children field is inside "disabled" and the child gets moved out of the group, but stays in disabled, it suddenly is in content again.
This is, because tabledrag’s match-parent finds no parent, so it falls back to the first draggable row in the table (which is content). I'll experiment a bit here with an approach, where tabledrag doesn't handle field-region at all, but only handles weight + parent. Region already comes from which table section the row sits in (field_ui.js onDrop / onChange, and field_group when a group moves), so this might be a more feasible approach.
But I agree, that we definitly needs further proper tests on this. The current test won't do anything relevant really. I'll remove it for now.
Comment #11
grevil commentedComment #12
grevil commentedComment #14
anybodyAs this seems to fix a major issue at field_group which is widely used, I think we should try pushing this forward - if you're sure this is the correct approach @grevil!
Comment #16
charlliequadros commentedHi everyone
Added Functional JavaScript regression coverage for display `tabledrag` region handling.
The first test nests a content field under a display group and verifies that the field remains in the `content` region after saving and reloading the page.
The second test outdents a disabled field while another root-level field remains in the `content` region. It verifies that the disabled field remains in the `hidden` region after saving and reloading.
With the MR applied, both tests pass: **2 tests and 13 assertions**, with no browser console errors.
Temporarily restoring the removed `field-region match-parent` rule causes both tests to fail, confirming the regression:
* the nested field receives an empty region instead of `content`;
* the outdented field receives `content` instead of `hidden`.
`FieldUIIndentationTest` also passes with **1 test and 7 assertions**.
All additional checks also pass successfully: PHP lint, PHPCS, CSpell, PHPStan, ESLint, `commit-code-check`, and `git diff --check`.
Comment #17
charlliequadros commentedComment #18
anybodyThanks all! LGTM and works fine, fixing the issues especially with the widely used field_group module!
@smustgrave what do you think? Are the added test appropriate for you to remove "needs tests"?