Entity Browser widget loses selected images in inline entity form

+++ b/src/Plugin/Field/FieldWidget/EntityReference.php
@@ -323,7 +323,7 @@ class EntityReference extends WidgetBase implements ContainerFactoryPluginInterf
-    elseif ($form_state->has(['entity_browser_widget', $this->getFormStateKey($items)])) {
+    elseif ($form_state->has(['entity_browser_widget', $this->getFormStateKey($items)]) && !empty($form_state->get(['entity_browser_widget', $this->getFormStateKey($items)]))) {

Could we do just !empty()?

Can you explain a bit why this patch fixes it? It is not obvious and I'd like to fully understand what is going on before committing.

I'd say we simplify the condition a bit. Should be equivalent.

I tested few pretty edge-casey scenarios and it seems that widget still works OK in other cases too.

We should definitely have tests for this. @Denchev is working on basic Javascript test. When that lands it will be easier to extend it with coverage for cases like this one.

I suggest to commit this and add tests later.

This problem is not just with adding new entities, it's also with editing of selection. If you already have created media with few entities selected and you want to edit selection and remove entity. When you click update media and edit again - old selection will be displayed.

I have attached module with configuration, that can be used to reproduce it.
Modules needed: Entity Browser + EB Examples + EB IEF, Inline Entity Form, Entity, Media Entity
+ Test module I have attached (test_ief_remove).

Steps to reproduce:
1. Create new "Test IEF Remove" content
2. Set title -> Click "Add new Test File Media" -> set name for media
3. Click "Select entities" -> upload few files or use existing
4. Click "Create Test File Media" -> "Save and Publish"

5. Make edit of that saved content
6. Click "Edit" to edit Test File Media
7. Remove any entity for existing list
8. Click "Update Test File Media"
9. Click again "Edit" to edit Test File Media
=> Old preselection will be populated.

If content would be "Saved and Published" -> selection would properly saved.

I have investigated a bit and looks that problem is with not correct saved state in $form_state after Remove action.
If someone have idea where to look further, I would appreciate that?

I have investigated further what makes inconsistent form state -> and I have ended in IEF module.

So what is going on (in "short"): for example when remove button is pressed without IEF

1. in processing of action (FormBuilder::processForm()), rebuild is triggered -> FormBuilder::rebuildForm() and then FormBuilder::retrieveForm() -> that will execute EntityReferenceBrowserWidget::formElement()
2. setting of new state is done in EntityReferenceBrowserWidget::formElement()
3. and after FormBuilder::retrieveForm() in FormBuilder::rebuildForm() form state will be saved (cached)
4. so we end with correct form state (when I say form state I mean form state relevant for Entity Browser)

Other case: for example when remove button is pressed within IEF

1. in processing of action (FormBuilder::processForm()), rebuild is triggered -> FormBuilder::rebuildForm() and then FormBuilder::retrieveForm() -> that will execute creating of IEF form and -> it will generate #process callback for inner form
2. and after FormBuilder::retrieveForm() in FormBuilder::rebuildForm() form state will be saved (cached)
3. after saving of form state inside FormBuilder::rebuildForm() -> FormBuilder::doBuildForm() will be triggered. That will pick up all defined #process callbacks and execute them
4. execution of defined #process callback will execute EntityReferenceBrowserWidget::formElement() and new form state will be set
5. But!!! Problem is that form state is already saved (cached) and form state set during execution of #process callback will not be saved and actions after that will have wrong form state

I have changed that IEF doesn't return #process callback but actually just to execute it and provide result directly. In that case form state will be properly saved (cached). Also build flow is more similar to normal build flow.

I have provided patch for Inline Entity Form module, maybe it's easier for someone to see code instead of "short" explanation. I'm not sure is that right way to go? And what are other options to solve issue?

Just a reroll of #5 so I can test it.

Note: Turns out that the paragraph problems that I had in alpha7 are already fixed in 8.x-1.x-dev, which explains why I couldn't reproduce with that version.

Given that, not sure if this is needed and how to reproduce this problem exactly.

@mtodor: If you think there is a bug in IEF, I suggest you open a new issue there and reference this.

Doesn't sound logical that #process would cause any $form_state issues. If there's an IEF bug somewhere, it's not due to #process.

Added test to represent problem. It still doesn't cover cases when Entity Browser with preselection is used (multi step).

Tests that should be added:

1. case when elements are only reordered
2. case when elements are added
3. case when elements are added/removed/reordered inside Entity Browser (multi step display with edit mode)

Added test dependency ^.

Reroll.

Here is solution proposal with using of "target_id" input element as source of selection if it's available.

I have also added additional tests (test class is also checked with Selenium):

1. case when elements are only reordered
2. case when elements are added inside Entity Browser (multi step display with edit mode)
3. case when elements are reordered inside Entity Browser (multi step display with edit mode)
4. case when elements are removed inside Entity Browser (multi step display with edit mode)

Additional info for new test module: I have added jQuery library for drag/drop simulation, because we need it for testing of reordering. Library is taken from: https://github.com/jquery/jquery-simulate

What would be nice to do is:

1. to split methods of getting selected entities in EntityReferenceBrowserWidget::formElementEntities into 3 separate methods: based on trigger element, from form state and from items.

Adjusted test to work properly with faster execution.

An alternative to adding the jquery.simulate library would be to just simulate the event we're hoping to trigger. In #2421427: Improve the UX of Quick Editing single-valued image fields I do this with the following code:

+    // Trigger the upload logic with a mock "drop" event.
+    $script = 'var e = jQuery.Event("drop");'
+      . 'e.originalEvent = {dataTransfer: {files: jQuery("#quickedit-image-test-input").get(0).files}};'
+      . 'e.preventDefault = e.stopPropagation = function () {};'
+      . 'jQuery(".quickedit-image-dropzone").trigger(e);';
+    $this->getSession()->executeScript($script);

This specific test was mocking a "dropped file" event, which is likely more complicated than mocking a DOM element being dragged somewhere.

@samuel.mortenson: I'll take a look at your solution. I'm always up to get rid of additional dependencies. I guess that for this test tricky part will be to find correct drop event coordinates (x, y).

1. +++ b/src/Plugin/Field/FieldWidget/EntityReferenceBrowserWidget.php
   @@ -672,4 +677,42 @@ class EntityReferenceBrowserWidget extends WidgetBase implements ContainerFactor
   +    $targetIdElementPath = array_merge(
   ...
   +    $currValue = $form_state->getUserInput();
   

   Use snake case. From coding standards.

   Variables should be named using lowercase, and words should be separated either with uppercase characters (example: $lowerCamelCase) or with an underscore (example: $snake_case). Be consistent; do not mix camelCase and snake_case variable naming inside a file.

   snake_case is used in this file.

   Also getUserInput() returns raw and potentially unsecure values. getValues() should be used instead.

2. +++ b/tests/modules/entity_browser_ief_test/js/jquery.simulate.js
   @@ -0,0 +1,331 @@
   + /*!
   + * jQuery Simulate v@VERSION - simulate browser mouse and keyboard events
   + * https://github.com/jquery/jquery-simulate
   + *
   + * Copyright jQuery Foundation and other contributors
   + * Released under the MIT license.
   + * http://jquery.org/license
   + *
   + * Date: @DATE
   + */
   

   I am not sure if we're allowed to add this into the repository since it is not GPL. We should check and be very careful with that.

   Another reason to try to avoid it.

Working on addressing the feedback. I'm quite sure that you can simulate dragging much nicer.

Mh, I tried to use the following code, but sadly this somehow looses content. If you are honest, this bugfix for itself, doesn't need to test the drag&drop functionality, right?

I believe that JS testing is required to reproduce the bug (as reported, at least), but you should be able to submit the form with arbitrary weights without drag and drop. *Fingers crossed*

I'll take look to simulate DD.

I think that problem with native driver dragTo method is that you can't specify position and that's what we need in this case, because drop element should be container of draggable elements.

And tests for reorder are here, because better test coverage for entity browser and that's what we should improve.

And tests for reorder are here, because better test coverage for entity browser and that's what we should improve.

No question this totally makes sense, but this issue is about a different bugfix, to be honest. Creating an issue is easy and we do it not often enough.

Unfortunately solution with getValues() doesn't work, it doesn't contain values required to determine selected elements, so I have used getUserInput().

And I have added solution for drag->drop, it's not nice, but it should work fine (also tested with Selenium and PhantomJS).

Nice work!

Reroll.

A bit of a clean-up/code optimization. I am not super happy with the solution as it looks a bit hacky. Also, the fact that we need to use raw user input probably shows that we're doing something wrong.

However, I don't have any better suggestions right now. Patch clearly fixes the problem and adds a bunch of new tests. This should allow us to refactor in the future. I opened #2825890: Refactor and clean EntityReferenceBrowserWidget::formElementEntities() for that.

Great work @mtodor!!

Nice, thank you @slashrsm!

I'm going to revisit this issue, as it looks to be causing a regression in the EntityReferenceBrowserWidget when used without inline entity form:

#3046416: Remove button conflict wrong triggering element

It seems that updating the form on rebuild causes the remove button to not be recognized, so Drupal FAPI grabs a random button leading to some surprising and hilarious consequences.