Problem/Motivation

After drag and drop using entity_browser_entity_reference field widget, the remove button can have unexpected behavior.

Steps to reproduce:

  • Enable standard profile
  • Enable entity browser example
  • Add 3 articles, "Article 1", "Article 2", "Article 3"
  • Go to /node/add/entity_browser_test
  • Fill in title
  • On "Nodes" field select the three articles
  • Under the first "Files" field, upload three files.
  • Save node
  • Go back and edit node
  • Drag the first article in between the 2nd and 3rd, so it becomes the 2nd.
  • Click the remove button on the item you dragged (the 2nd)
  • Expected: the item is removed
  • Actual, the items from the other field appear, with the second one removed. It's very surprising and wrong behavior!

https://www.drupal.org/files/issues/2019-04-07/wrong_triggering_element.mov

After stepping through the code, it seems the triggering element isn't picked up, so the FAPI grabs the first button, which is wrong, which then executes the code on EntityReferenceBrowserWidget::removeItemSubmit with the wrong triggering element.

Here's another video, where, after updating the file entity browser, it launches a modal when clicking the remove button.

https://www.drupal.org/files/issues/2019-04-07/more_unexpected_behavior.mov

Proposed resolution

Reduce scope of ::getEntitiesByTargetId to only affect when triggering element name starts with 'ief-edit-submit'. See original issue #2764889: Entity Browser widget loses selected images in inline entity form.

::getEntitiesByTargetId is unnecessary outside of the context of inline entity form. There is an odd order of operations where user input is being used to update the form during rebuild. It would be better to figure out how to move this to a submit function, and when the form is rebuilding, it doesn't modify the data. Ultimately, I think ::getEntitiesByTargetId should be replaced.

Remaining tasks

Test coverage, review

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#29 entity_browser-issues-if-ief-module-not-enabled-3046416-29..patch1.43 KBRuslan Piskarov
#22 Screen Shot 2019-04-20 at 11.26.00 AM.png68.65 KBkellyimagined
#22 Screen Shot 2019-04-20 at 11.26.11 AM.png137.92 KBkellyimagined
#18 entity-browser-random-triggering-element-3046416-17.patch14.74 KBoknate
#17 entity-browser-random-triggering-element-3046416-TEST-ONLY.patch12.79 KBoknate
#17 3046416--interdiff-15-17.txt12.79 KBoknate
#15 entity-browser-random-triggering-element-3046416-15.patch1.94 KBoknate
#15 3046416--interdiff-13-15.txt938 bytesoknate
#13 entity-browser-random-triggering-element-3046416-13.patch1.33 KBoknate
#7 more_unexpected_behavior.mov1.09 MBoknate
wrong_triggering_element.mov2.04 MBoknate
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

oknate created an issue. See original summary.

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
FileSize
more_unexpected_behavior.mov1.09 MB
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented

OK, I'm starting to make some progress on this.

It seems to be a bug related to change made for #2764889: Entity Browser widget loses selected images in inline entity form. If I comment out this code, the triggering element is correct. It seems that the form rebuilds but with the updated items, so the buttons no longer match.

    // Find IDs from target_id element (it stores selected entities in form).
    // This was added to help solve a really edge casey bug in IEF.
    if (($target_id_entities = $this->getEntitiesByTargetId($element, $form_state)) !== FALSE) {
      return $target_id_entities;
    }
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Related issues: +#2764889: Entity Browser widget loses selected images in inline entity form
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
FileSize
entity-browser-random-triggering-element-3046416-13.patch1.33 KB

Initial patch

Limiting the scope of #2764889: Entity Browser widget loses selected images in inline entity form so it still works for the initial use case, but doesn't affect the remove button.

Also, adding a second, unrelated change due to sort not working. There are several tickets related to this. I've been working on adding a weight field to EntityReferenceBrowserWidget.

I'm not sure if I'll fix that on this issue or another. But it's another high priority bug we need to fix.

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Status: Active » Needs review
FileSize
3046416--interdiff-13-15.txt938 bytes
entity-browser-random-triggering-element-3046416-15.patch1.94 KB

Fixing new failure with change. It seems that the other behavior was allowing the replace button to work by using the code for the inline entity form hack. This change is better.

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented

Added an updated patch here that adds weight field and does some refactoring in the widget: https://www.drupal.org/project/entity_browser/issues/2973456#comment-130...

I think it's best to keep the weight fix separate from this bug.

This one should go in first. Just needs test coverage.

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
FileSize
3046416--interdiff-15-17.txt12.79 KB
entity-browser-random-triggering-element-3046416-TEST-ONLY.patch12.79 KB

Test only patch, demonstrates the bug.

I got stuck for a while because the drag and drop function used in InlineEntityFormTest wasn't working properly. It drops the item in one step. But the way jquery ui draggable works, it reacts to the trajectory of the drag. So I had to update the function to break up the dragging into multiple steps. I ended up borrowing some logic from jquery.simulate: https://github.com/jquery/jquery-ui/blob/master/external/jquery-simulate...

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Assigned: oknate » Unassigned
FileSize
entity-browser-random-triggering-element-3046416-17.patch14.74 KB

Patch with fix and test coverage (#15 + #17).

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes

The last submitted patch, 17: entity-browser-random-triggering-element-3046416-TEST-ONLY.patch, failed testing. View results

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
kellyimagined’s picture

kellyimagined CreditAttribution: kellyimagined as a volunteer commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screen Shot 2019-04-20 at 11.26.11 AM.png137.92 KB
Screen Shot 2019-04-20 at 11.26.00 AM.png68.65 KB

#18 is a valid solution to the issue. I was able to test before patch, and then again after patch.

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented

I have a follow up fix for #2851580: Re-order + remove broken with the Entity Reference (and File) widget, which will fix a bug introduced by this fix (it doesn't check delta when removing). Although I still think this is should go in ASAP, as it fixes a worse bug.

Ideally, they'll go in together.

  • oknate committed 8e0be71 on 8.x-2.x 
    Issue #3046416 by oknate, kellyimagined: Remove button conflict wrong...

  • oknate committed 345ee61 on 8.x-1.x 
    Issue #3046416 by oknate, kellyimagined: Remove button conflict wrong...
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks again for testing, @kellyimagined.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented
Related issues: +#3104901: Entity Browser used in a entity referenced field of a layout builder custom block is not working

Looks like this caused a regression with entity browsers widgets that are used in layout block entities.

Ruslan Piskarov’s picture

Ruslan Piskarov
he/him
Primary language Ukrainian
Location Kiev, Ukraine
CreditAttribution: Ruslan Piskarov as a volunteer and at KWALL commented
FileSize
entity_browser-issues-if-ief-module-not-enabled-3046416-29..patch1.43 KB

Confirming @bkosborne.
We also encountered another problem that relates to this in the case of the Inline Entity Form module does not exist on the projects.
I have created a quick patch.
I am not sure, should we reopen this issue, maybe better to create another one.

swentel’s picture

swentel CreditAttribution: swentel as a volunteer commented

@Ruslan, maybe try adding on #3104901: Entity Browser used in a entity referenced field of a layout builder custom block is not working - I've tested the patch and it solves a couple of issues when not using IEF, but LB and blocks instead, so definitely worth a try testing it!

Ruslan Piskarov’s picture

Ruslan Piskarov
he/him
Primary language Ukrainian
Location Kiev, Ukraine
CreditAttribution: Ruslan Piskarov as a volunteer and at KWALL commented

Thank you @swentel.

Added https://www.drupal.org/project/entity_browser/issues/3104901#comment-143....