Add a "changes not applied until saved" warning when changing widget/formatter settings

Suppose we need js.behavior tableDragSetCaption/Message

A definite UX issue. Took me ages to figure out I needed to save my changes to the display format on 'manage display'.

Subscribe: we have the same issue on manage fields screen with field_group, but I guess the patch will fix it on a broader level ?

#1025976: Updating a field formatter at Manage display fields does not save the change, and the user is not warned of this marked duplicate, bumping and tagging.

Was there already a starting patch for this somewhere?

subscribing

Patch works on display (field_group and fields). As field_group did some altering to allow the configuration of field_groups in the fields screen, this is not implemented in the patch. So this does not work yet.
I am not sure about the approach either. This patch works on a fix rather than a improvement of core imho.

Subscribe & tagging. For Drupal 8 we of course save everything automagically and let people undo as they like it :)
But yes, please do add this warning. (Doesn't views disable the submit button untill you update settings? Can we borrow? Just thinking out loud…)

Leaving as 'major', but moving to a task, this is more of an enhancement than a straight bug.

+1, this UX issue confused me for quite awhile.

+1

In my opinion, the settings shouldn't be saved automagically if it'll introduce problems. This could be a feature for D8, though, as when you click the "update" button of the specific field settings, you really expect the setting to be permanently saved.

Is there a chance that this will get into the December 5 (7.10) release?

...coming from the now closed as duplicate #1428532: After editing a widget's properties, there should be a hint that informs that form needs to be saved too (a-la Views)

#11 -manually applied- fixes the problem partially (using latest devs of core and field_group). Tested the "Manage fields" tab & the "Manage display" tab (default only - not the teaser, but I guess it works the same for that too).

"Manage display" tab

Behaves (mostly) as expected with the patch applied when a widget's settings are updated (tested changing the label of a vtab within a vtab group):

[v] no confusing "Your settings have been saved" message on widget settings update.
[x] no dirty flag set in the widget row that has been updated to indicate that though.
[v] "* Changes made in this table will not be saved until the form is submitted." message to indicate that the form needs to be saved too in order for the settings to be finally saved.
[v] "Your settings have been saved" message only after submitting the whole form ("Save" button).

Minor WTF that happens only on successive updates of widgets: Changing the widget and hitting the "Update" button does bring the "* Changes made in this table will not be saved until the form is submitted." message, but any "Your settings have been saved" message from a successful previous save doesn't go away till the page gets refreshed. So you end up having both messages in this case. So, we need to make sure that we remove any previous messages when we display the "* Changes made in this table will not be saved until the form is submitted." message.

"Manage fields" tab

Doesn't behave as expected at all:

[v] no confusing "Your settings have been saved" message on widget settings update.
[x] still, no dirty flag set in the widget row that has been updated to indicate that.
[x] still, no warning message or indication whatsoever that the form needs to be saved too.
[v] "Your settings have been saved" message only after submitting the whole form as expected.

The annoying thing now is that the message disappears on ajax response.
See #1664836: Fields tabledrag form jumping up and down due to message.

What we need is:
- Show the message as soon as someone changes a form value (esp, if someone rearranges the rows). Esp, let it survive ajax response.
- Do not hide the message until the form is submitted (that is, don't hide it at all)
- Never show more than one message.
- Optionally, hide the message if the form returns to its original state (probably we cannot reliably detect this, so forget about it)

Now, we probably all know this already, and this comment does not help at all. Ok...

Marked a previous commit as a duplicate and moving to this thread.

http://drupal.org/node/1536252

This is my patch.

Should I test this against D8 or D7? ...asking because as you can see from #19 my issue was with Field group before this shifted to a core issue.

#1770720: [META] Gradual changes to Field UI says that this should use #1642062: Add TempStore for persistent, limited-term storage of non-cache data
Which would be pretty cool!

New patch against 8.x after all the changes. 2 things still todo

- also act on label change
- figure out the position of the message - with tabledrag it's after 'Show row weights', after an ajax request, it's before, ugh

Also - only test this against core, we'll see for D7 backport if we need to help out fieldgroup/ds or not a little further or not.

@tim yes, we really hope to use tempstore, but fore now see if we can get this one in for backport to D7 :)

Moving back to task, implementing this as temp store is going to be insane hard. Patch goes a little further that it also triggers it on a label change, but still needs work, however, moving to features now first :)

Bringing this on the radar

Removing focus, added on #1963340: Change field UI so that adding a field is a separate task too

This issue is listed in #1770720: [META] Gradual changes to Field UI and #1642062: Add TempStore for persistent, limited-term storage of non-cache data (that is now in) is supposed to "kill" this issue here.

Just closed a duplicate: #2172325: Manage display screen should warn you to save

So, with #1642062: Add TempStore for persistent, limited-term storage of non-cache data fixed, are we done here?

The patch in #25 no longer applies (not too surprising).

The whole directory structure has been changed. I need some help getting #25 to re-roll. :(

It sounds from recent comments like step #0 might be verifying the problem still exists.

Removing the tag because Drupal is very different now, we should probably approach this with fresh new eyes :)

Bug still exists

1. visit admin/structure/types/manage/article/display
2.1 drag rows - no message
2.2 change formatter settings - no message

Actually when used at admin/config/people/accounts/display page

I can see the message only when something is moved in visible section

But if move something in Hidden - state is cleared and table flickering

This appears to need an issue summary update.

This was a spinoff task so what is being addressed here?
What is the proposed solution?
Remaining tasks?
Etc.

Thanks.

Running into some inconsistency between the tests an local that I haven't fully figured out yet. In some cases changing formatter settings and clicking the "update" button saves those changes, in other cases it doesn't.

Scenario: Article content type, update the Trimmed limit of the Body field with the summary or trimmed formatter in the default view mode

Formatter setting changes save after clicking Update, no need to save the manage display form as well

• Umami install profile

Formatter setting changes do not save after clicking Update. The manage display form must be saved for the changes to stick

• Automated tests
• Standard Profile
• A body field in a new content type created in Umami profile

Since this worked in Umami and not in Standard, I tried starting with a Standard profile install and enabling modules one by one until it matched Umami, and Standard continued to not save the settings while Umami did.

Because a Body field in a new content type in Umami doesn't save the settings, while Article does, I compared the field config and there's no differences aside from the names.

There's probably other config in Umami that could be impacting this. Trying to narrow down what that could be.

Talked to @effulgentsia about this and it sounds like the not-Umami (the majority of the scenarios) is the expected behavior, so that is a step towards understanding this. It would be good to understand why, though, as there may be a broader need to distinguish saving-changes from non-saving ones so the warning only appears for the latter.

The inconsistency between Standard and Umami install profiles is created by Layout Builder module. Specifically, Layout Builder overrides enabled in Umami is causing this.

This is because we have some code in \Drupal\field_ui\Form\EntityDisplayFormBase::buildFieldRow to ensure that any fields that have no applicable widgets, are disabled. Something about this logic leads into the entity display being re-saved every time the page is loaded, when Layout Builder overrides are enabled. 🤯

No functional test coverage was written when the following check was introduced in #2552799: FieldType with no available widget causes Fatal error, so we'll need to write some:

    // Disable fields without any applicable plugins.
    if (empty($this->getApplicablePluginOptions($field_definition))) {
      $this->entity->removeComponent($field_name)->save();
      $display_options = $this->entity->getComponent($field_name);
    }

I believe the fix is a straightforward one, but I'm going to work on tests first.

Code is NR, still needs IS update

I think I pushed too quickly, hopefully this patch will fail correctly

Tested this manually and it seems to work pretty well. Something that is a bit off is that every time something is changed on the form, the message above the table flashes. I'm wondering if that's something we would be able to address?

Fixed #65 with this commit.

Applied MR 3562
Went to the form display.
Changed a formatter option and confirmed the warning.
Changed a formatter setting and saved. Confirmed the warning.

Not noticing the flicker mentioned in #65.

Looks good

There are issues when dragging rows from the active region to the disabled region in the field UI.

To reproduce:

1. go on a content type's manage display page: /admin/structure/types/manage/page/display/teaser (i'm using umami)
2. The active region has "Body" and "Links" fields.
3. Drag the Moderation control row from the disabled region, and add it below "Links"
4. The "Links" row is pushed down to the disabled region

I was able to remove all the rows from the active region by dragging things around a bit switching rows from active to disabled region. so definitely a timing issue somewhere.

The warning is added when reordering rows but it gets hidden is a row is moved from active to disabled, even though nothing is saved, the warning should be kept.

Additionally the use of the ajaxStart/Success event is a bit problematic since we have our own command processing on top of the jQuery ajax calls this adds a dependency on a workaround in core, which isn't ideal. That might be one of the thing that makes the UI unreliable (haven't looked closely). See #3324062: [Regression] Changes to Drupal.ajax in 9.5.x have caused regressions in paragraphs_features module.

Umm now the row disapearing/moving to the disabled region doesn't happen. would be good for someone else to try and move things around and see if some unexpected rows gets moved to the disabled region.

The changed warning removed when dragging from active to disabled region is still an issue though (because things are not actually saved).

Nice, this solves the issues when dragging rows.

There a minor inconsistency from the removed ->save(), the changed marker (*) gets removed from all rows when dragging a row across the active/disabled region, ideally it shouldn't be removed but the main warning is still displayed. Information that a save is needed is still there it's just not clear what changed anymore.

couple of details in the MR comments

Good feedback from @nod_

Open threads still it seems.

Seems to have build errors

Running PHPStan on *all* files.
------ ------------------------------------------------------------------------
Line core/modules/field_ui/src/Form/EntityDisplayFormBase.php
------ ------------------------------------------------------------------------
708 Ignored error pattern #^Variable \$updated_rows might not be
defined\.$# in path
/var/www/html/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
is expected to occur 1 time, but occurred 2 times.
720 Variable $updated_rows might not be defined.
------ -------------------------------------------------------

Will need to look into the test failures.

Like the commit message "do not double dip change handlers"

Retested following

Went to the form display.
Changed a formatter option and confirmed the warning.
Changed a formatter setting and saved. Confirmed the warning.

Posted review on the MR

Confirmed that all feedback was addressed and saw that all worked as expected when testing manually

Committed 1996c9d and pushed to 10.1.x. Thanks!

We should write a CR for the new ajax command. Moving to NW for that.

Change record added.

I'm seeing a possibly unexpected side effect from this in our behat tests, it looks like it's no longer possible to change the region select and edit settings at the same time:

   Scenario: Create a redirect when a taxonomy term has a representative node
     Then I am at "admin/structure/types/manage/article/display"
     And I select "Content" from "fields[field_tags][region]"
+    And I press "Save"
     And I click the element "input[name='field_tags_settings_edit']"
     And I check the box "Link label to the referenced entity"

The settings_edit click no longer works, I need to save first. Not an issue when moving it up with drag and drop, but I can reproduce manually when I switch to displaying row weights.

It could be another issue, but this seems very related.

Another side effect: if we have a custom field settings form element with an ajax callback, and we click any input after that callback run, the entire form refreshes and we loose our input values.

This breaks for example Display Suite (but not exclusively, I also encountered the same in one of our projects in custom code):
- Go to manage display
- for any field, edit settings and change "Field Template" from "Master" to "Minimal"
- click for example the "Show label colon" or any other field
-> the entire form refreshes and we loose all values we entered.

Specifically, the changes in Drupal.fieldUIDisplayOverview.field seems to cause that.
Either ds_field_ui_display_overview_multistep_js() needs to be updated to add the ajax-new-content class (similar to how EntityDisplayFormBase::multistepAjax does) or is this be considered as a core bug/regression?

Edit: DS issue created #3402902: Field formatter settings form and AJAX issue (regression on 10.1.x after #857312)