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

Issue fork drupal-3613588

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:

Comments

grevil created an issue. See original summary.

grevil’s picture

Status: Active » Needs review
StatusFileSize
new663 bytes

Static patch for the time being.

Please review!

grevil’s picture

phpcs and phpstan failures are definitly unrelated, this is a simple string replacement.... unfortunately the tests are skipped now -.-.

grevil’s picture

After rerun everything succeeds and is green now :)

smustgrave’s picture

Status: Needs review » Needs work

Even 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)

thomas.frobieter’s picture

Works 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! :)

grevil’s picture

Status: Needs work » Needs review

Added 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 :)

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new1.18 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 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.

grevil’s picture

Issue tags: +Needs tests

@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.

grevil’s picture

Issue summary: View changes
grevil’s picture

StatusFileSize
new943 bytes

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

anybody’s picture

As 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!

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

charlliequadros’s picture

Hi 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`.

charlliequadros’s picture

Status: Needs work » Needs review
anybody’s picture

Thanks 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"?