Problem/Motivation

Using of Entity Browser is not so smooth. Any action in browser makes full reload of entity browser and that makes reset of form. For example: when view with filtering and paging of entities is used, after adding one entity in list of selected entities, previous defined filter is reset and paging too. Additionally when view with checkboxes (or some other way of selection) is used, there is one additional not needed step in selection process (click checkbox -> click select entities -> click use selected entities) - and first click can be automated to automatically add entity in list of selected entities.

Proposed resolution

Adding/removing of entities in selection display for multi step display, should be made over Ajax requests, where only relevant part of entity browser form will be loaded or removed (without full reload, without loosing filtering, paging, etc.)

Remaining tasks

- provide add functionality for multistep display that will load entities over Ajax request
- provide remove functionality for multistep display that will remove element from page and update selection in form

*Edit* Usability example

Example

Files: 
CommentFileSizeAuthor
#55 interdiff_2822009_55_53.txt671 bytesmtodor
#55 2822009_55.patch87.41 KBmtodor
#53 2822009_53.patch87.37 KBslashrsm
#48 interdiff_2822009_48_46.txt8.29 KBmtodor
#48 2822009_48.patch87.42 KBmtodor
#46 2822009_46.patch87.41 KBmtodor
#46 interdiff_2822009_46_45.txt3.69 KBmtodor
#45 2822009_45.patch87.52 KBmtodor
#45 interdiff_2822009_45_43.txt7.93 KBmtodor
#43 interdiff_2822009_43_39.txt5.29 KBmtodor
#43 2822009_43.patch88.99 KBmtodor
#39 2822009_39.patch88.72 KBchr.fritsch
#38 2822009_without_improvements.gif1.68 MBmtodor
#38 2822009_with_improvements.gif1.19 MBmtodor
#36 2822009_interdiff_36_31.txt46.59 KBmtodor
#36 2822009_36.patch88.23 KBmtodor
#31 2822009_31.patch41.64 KBmtodor
#31 2822009_interdiff_31_21.txt4.31 KBmtodor
#23 auto-select-problem-example.gif371.49 KBsamuel.mortenson
#21 2822009_21.patch40.22 KBmtodor
#20 2822009_20.patch40.68 KBsamuel.mortenson
#20 field_widget_selection.png107.78 KBsamuel.mortenson
#20 auto-select-buttons-bug.gif373.62 KBsamuel.mortenson
#9 2822009_9.patch54.71 KBmtodor
#4 2822009_4.patch45.91 KBmtodor
#2 prototype-ajax-entity_browser.gif2.31 MBmtodor

Comments

mtodor created an issue. See original summary.

mtodor’s picture

mtodor’s picture

FileSize
34.81 KB

Here is initial work. It's still work in progress, but it would be nice if someone can take a look and give some feedback.
I have also added example in example module, so that's easier to check it.

Note
In order for functionality to work one core patch is required: #2699489: FormBuilder $ajax_form_request check does not check which AJAX form is being requested

Next things on my list are:

  1. add handling of remove functionality over ajax
  2. testing possible combinations of configuration
  3. adding tests - but for that I would like to have test functionalities provided in: #2764889: Entity Browser widget loses selected images in inline entity form
mtodor’s picture

FileSize
45.91 KB

Here is new patch. It contains further work:

  1. some refactoring with support for adding multiple entities with one action
  2. added support for removing of entity from list over ajax request
  3. added additional validation of combined configuration (fe. using of widget with auto_select, but not selection display that supports it)
  4. also providing configuration option for auto_select, only for widgets that supports it

I have also added ticket for Upload Widget (in this case DropZoneJS widget), to support "auto_select" functionality. More or less, just drop files and they will be added to multistep selection display (rejected files ignored, valid automatically uploaded, entities created and added in list of selected entities for multistep selection display) - ticket: #2823670: Improved MultiStep selection display (DropZone Widget).

mtodor’s picture

I have created pull request on github - because I think it's easier to review it there and collaborate when it comes to review, feedback, comments etc.

Pull request is here: https://github.com/drupal-media/entity_browser/pull/153

slashrsm’s picture

Status: Active » Needs review

Checked the pull request. Approach generally looks OK to me. Left one comment, but nothing major.

Still needs a proper review and testing (let's continue doing that on the pull request from #5).

Status: Needs review » Needs work

The last submitted patch, 4: 2822009_4.patch, failed testing.

The last submitted patch, 4: 2822009_4.patch, failed testing.

mtodor’s picture

Status: Needs work » Needs review
FileSize
54.71 KB

Made adjustments from comments provided on github pull request. (except moving functionality provided in view widget example into view widget).

Pushing patch here for checking are tests running and easier usage in packaging tools.

Status: Needs review » Needs work

The last submitted patch, 9: 2822009_9.patch, failed testing.

The last submitted patch, 9: 2822009_9.patch, failed testing.

mtodor’s picture

Issue with displayed action buttons for multistep display should be solved (now they appear only when there is some selection in multistep display).

I have moved functionality provided in example into view widget, now view widget can work out of the box. (as @samuel.mortenson has suggested)

mtodor’s picture

Status: Needs work » Needs review
mtodor’s picture

Assigned: mtodor » Unassigned
mtodor’s picture

There was issue with Hide/Show button -> because click behavior was registered on every ajax request, now it should work properly.

esolitos’s picture

I love the idea! Haven't got the chance to test the patch so i will not put in RTBC, but the code overall seems ok.

chr.fritsch’s picture

I rebased the pull-request and re-rolled the patch to make them work with current HEAD again

slashrsm’s picture

Finally managed to look at this patch. I was able to do general approach review and it looks great.

We would need someone to do a proper frontend review (I don't feel competent enough in this area). I will try to find some time in next few days to properly test this and make an in-depth review.

Great work @mtodor!!

mtodor’s picture

Issue summary: View changes
Issue tags: +Needs usability review
samuel.mortenson’s picture

Using this feels pretty good!

Some front-end notes:

  1. When editing an existing selection using one of the field widgets and the iFrame , it feels weird to see the same selection in two places. See: https://www.drupal.org/files/issues/field_widget_selection_0.png
  2. New items in the selection display are wrapped in a div tag, so the inline block style applied by the ".entities-list .item-container" selector wasn't working. I've uploaded a patch to fix this.
  3. If you add an item using auto_open, remove it, then add it again, you get two sets of the "Use/Hide selected" buttons. See: https://www.drupal.org/files/issues/auto-select-buttons-bug_0.gif

The patch required a small re-roll so I'm not uploading the (broken) interdiff.

mtodor’s picture

@samuel.mortenson thank you for feedback.

I have checked with example provided in example module and I'm not getting problems you have listed here. It would be nice, if you can provide information about environment you have used or create module with only configuration, so that I can reproduce it.

  1. This problem is not related with auto selection, it's more related with displaying preselection for multistep display. If we want to improve it, I would make new ticket for that.
  2. I have checked this and selected elements are always generated properly with correct css class "item-container js-form-wrapper form-wrapper". And selector should work properly there too. I don't see need for new css selector there. Maybe you can provide screenshot from your development console, too see how DOM looks like for you?
  3. Can you check do you have some JS errors in console in this case? Since this works for me, and only possible problem that comes on my mind is that removing of buttons fails (that css selectors are failing).

I have rebased pull request and here is new patch based on that.

mtodor’s picture

Status: Needs work » Needs review
samuel.mortenson’s picture

@mtodor #21.1 Agreed - the issue's title threw me off but if the scope is just auto selection, then we can work on this issue elsewhere.

For #21.2 and #21.3, here are full replication steps:

  1. Install the Standard profile and the "entity_browser_example" module.
  2. Visit /node/add/article and create a new Article with an uploaded Image, so we have a File Entity to select.
  3. Visit /node/add/entity_browser_test and expand "Files (With Auto Loading)", then click "Select entities"
  4. Click on (auto select) the File twice - notice that it's stacking vertically and not horizontally? That's due to the CSS problem outlined in #20.2
  5. Remove both instances of the auto-selected file
  6. Click on the File again - you should see duplicated "Use/Hide Selected" buttons. That's the problem outlined in #20.3

I see no JS errors, but if it helps I'm on Mac OSX running Chrome 54.0.2840.98 (64-bit). Here's another GIF of what I'm seeing.

A few other notes when reviewing the patch again:

  1. diff --git a/js/entity_browser.view.js b/js/entity_browser.view.js
    index a4f33ad..45c68ec 100644
    +          views_instance.$view.find('tr')
    +            .once('register-row-click')
    

    This code won't work for Views that aren't using the "Table" display, like File Browser.

  2. diff --git a/js/entity_browser.view.js b/js/entity_browser.view.js
    index a4f33ad..45c68ec 100644
    +              var $row = $(this);
    +              var $input = $row.find('input');
    

    Can we make the "input" selector more specific here?

  3. +              $row.parents('form')
    +                .find('.entities-list')
    +                .trigger('add-entities', [[$input.val()]]);
    +            });
    +
    +          // Hide column with checkboxes.
    +          views_instance.$view.find('.views-field-entity-browser-select')
    +            .hide();
    

    Can we indicate that a View result has been selected somehow, maybe by adding a class to the View row? The checkboxes are hidden now, so as a user there is no longer a way to visually indicate that a row has been selected. It's worth noting that File Browser and the browser provided by Lightning Media both highlight Views rows on click, or based on the checkbox state.

samuel.mortenson’s picture

Status: Needs review » Needs work
mtodor’s picture

@samuel.mortenson I can't reproduce it. What Drupal version are you using? Do you have some additional modules installed? I'm using Drupal 8.2.3 and only EB module and examples.

I'll take a look what can be done regarding #23.1 and #23.2.
When it comes to #23.3, since it's possible to select same entity multiple times, marking entities looses purpose, because you can't demark it (any new click will just add entity in selection).

In Thunder we have mix, when we use "multi_step_display" we are not marking selected entities in view (because they are displayed in selection display anyway). When we use "no_display", then we are marking selected entities in view.

samuel.mortenson’s picture

@mtodor I'm using 8.2.x HEAD, no additional modules beyond what the Standard profile provides.

samuel.mortenson’s picture

When it comes to #23.3, since it's possible to select same entity multiple times, marking entities looses purpose, because you can't demark it (any new click will just add entity in selection).

That makes sense, I take that point back then. If I want specific styling in File Browser I can implement it myself :-)

mtodor’s picture

@samuel.mortenson I wasn't able to reproduce with Drupal 8.2.x HEAD. But problem occurred for chr.fritsch and looks like it's caused by Twig debug mode. Twig adds additional wrapping div with HTML comments and that's why after element is removed, wrapping div with comments stays. Can you check is that problem in your case too? Because you mentioned in #20.2 that element is additionally wrapped with div, and that should not happen.

samuel.mortenson’s picture

@mtodor I can confirm that I have Twig debugging on, but I think the CSS rules should work even in that case. I have Twig debugging on for most of my local development and don't want to have to turn it off to use Entity Browser.

I've actually looked into this before, you can see my the issue where they added the wrapping div and my comment here: https://www.drupal.org/node/736066#comment-11747543 In my opinion wrapping AJAX responses in a div when there is only one Node.ELEMENT_NODE element is a bug.

mtodor’s picture

Assigned: Unassigned » mtodor
Issue tags: +dcmuc16
mtodor’s picture

Assigned: mtodor » Unassigned
Status: Needs work » Needs review
FileSize
4.31 KB
41.64 KB

I have adjusted Ajax response. HTML comments are removed from single tag HTML (only base comments, inner comments will stay). Other solution is to make custom JS commands, but that complicates code and functionality, only to support twig debugging mode.

Css selectors in JS are adjusted so that it can work with all view types (table, html, grid and unformatted). I have pushed commit to PR on github and here is patch + interdiff.

pixelmord’s picture

samuel.mortenson’s picture

Issue tags: +Needs tests

@mtodor Awesome! I just looked over my review in #20 and #23 and I think that everything I brought up has been addressed. trimSingleHtmlTag() seems a bit complicated but I understand the motivation behind writing it.

Looking over the patch again, I don't see test coverage for the code being added here. It appears that the only change to tests is to disable auto_select so that existing tests continue to work. We'll definitely need Javascript test coverage given the complexity of this feature.

mtodor’s picture

@samuel.mortenson I wanted to get functionality first with feedback and review before adding tests. To keep smaller code base. But it looks like, we have quite stable feature here and functionality is satisfying. So, next logical step will be adding tests (for that I have also waited for #2764889: Entity Browser widget loses selected images in inline entity form with all tests provided there). We already have tests in Thunder for this functionality and it should not be difficult to port it here.

mtodor’s picture

Assigned: Unassigned » mtodor
mtodor’s picture

Assigned: mtodor » Unassigned
FileSize
88.23 KB
46.59 KB

Here are tests!

Ajax commands test are one method, but there are 4 parts of it. What is covered here is following:

  1. HTML view is used. Check of proper action buttons adding/removing. In short: check there are no buttons, add entity, check there are buttons, remove entity -> check there are no action buttons (= list is empty)
  2. Table view is used. Checking of quick clicking. In short: 5 entities are quickly added, check there are 5 entities in selection, quickly removed all 5 entities -> check there are no action buttons (= list is empty)
  3. Grid view is used. Checking of preselection adding. In short: Add one entity, close browser, open browser, add one more entity, close browser -> check selection is two entities.
  4. Unformatted view is used. Check for preselection removing. In short: Add 3 entities, close browser, open browser, remove one entity, close browser -> check selection is two entities.

It's also tested with Selenium and PhantomJS.

Bojhan’s picture

What feedback do you want in terms of usability review?

mtodor’s picture

@Bojhan Someone on IRC (I think it was @slashrsm) asked to make UX review. I'm not sure what are exactly key points for review. I have made 2 gifs (without improvements and with improvements added by this ticket).

Few improvements introduced with this issue:

  1. Currently in view for selection of entities, there are checkboxes for every entity and then after checking entities you have to click "Select Entities" button and that action will add all selected entities in current selection list. We wanted to get rid of that step, so that entities are automatically added in selection list.
  2. With removing of "Select Entities" button, checkboxes loose purpose and action for selecting entity is triggered by just clicking on Entity (on row for entity in View)
  3. Next thing that we have noticed is, that every action (fe. adding entity in selection list, removing entity from selection list), would actually reload View (it actually reloads whole form) and with that View state is cleared (paging, filtering, checked entities, etc.)
  4. With improvements here, request for adding/removing entities are triggered over Ajax, so there is no full form reload and View state stays unchanged.

Without patch:
Without Patch

With patch:
With Patch

chr.fritsch’s picture

I rerolled the patch, based on the latest EntityBrowser changes. Couldn't provide an interdiff, because it was broken.

slashrsm’s picture

Issue tags: -Needs tests

Thanks! This looks great. We are almost there. Few nitpiks and a bit of clean-up in the area of the "auto_select" configuration variable on widgets:

  1. +++ b/js/entity_browser.command_queue.js
    @@ -0,0 +1,108 @@
    +}(jQuery, Drupal));
    \ No newline at end of file
    

    Missing newline.

  2. +++ b/src/Form/EntityBrowserForm.php
    @@ -145,6 +148,25 @@ class EntityBrowserForm extends FormBase implements EntityBrowserFormInterface {
    +  protected function isFunctionalForm(FormStateInterface $form_state) {
    +    /** @var \Drupal\entity_browser\WidgetInterface $widget */
    +    $widget = $this->entityBrowser->getWidgets()
    +      ->get($this->getCurrentWidget($form_state));
    +
    +    /** @var \Drupal\entity_browser\SelectionDisplayInterface $selectionDisplay */
    +    $selectionDisplay = $this->entityBrowser->getSelectionDisplay();
    +
    +    if ($widget->requiresJsCommands() && !$selectionDisplay->supportsJsCommands()) {
    +      throw new ConfigException('Used entity browser selection display cannot work in combination with settings defined for used selection widget.');
    +    }
    

    Would be nice to check all widgets and not just the current one.

    Would also be nice if we would validate this on config form too. Can be a follow-up (let's create an issue).

  3. +++ b/src/Plugin/EntityBrowser/Widget/Upload.php
    @@ -188,6 +189,14 @@ class Upload extends WidgetBase {
    +    // Allow "auto_select" setting when autoSelect is supported by widget.
    +    $form['auto_select'] = [
    +      '#type' => 'checkbox',
    +      '#title' => $this->t('Automatically submit selection'),
    +      '#default_value' => $this->configuration['auto_select'],
    +      '#disabled' => !$this->getPluginDefinition()['autoSelect'],
    +    ];
    +
    

    This is already added on the base class. Why add it here too?

  4. +++ b/src/WidgetBase.php
    @@ -120,31 +122,34 @@ abstract class WidgetBase extends PluginBase implements WidgetInterface, Contain
       public function defaultConfiguration() {
         return [
           'submit_text' => $this->t('Select entities'),
    +      'auto_select' => FALSE,
         ];
    
    @@ -197,6 +202,14 @@ abstract class WidgetBase extends PluginBase implements WidgetInterface, Contain
    +    // Allow "auto_select" setting when autoSelect is supported by widget.
    +    $form['auto_select'] = [
    +      '#type' => 'checkbox',
    +      '#title' => $this->t('Automatically submit selection'),
    +      '#default_value' => $this->configuration['auto_select'],
    +      '#disabled' => !$this->getPluginDefinition()['autoSelect'],
    +    ];
    +
    
    @@ -323,4 +336,11 @@ abstract class WidgetBase extends PluginBase implements WidgetInterface, Contain
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function requiresJsCommands() {
    +    return $this->getConfiguration()['settings']['auto_select'];
    +  }
    +
    

    This should not be disabled if widget doesn't support it. Is shouldn't be added at all. One of the consequences of this is that we need to add schema definition for this value even to widgets that don't need it at all. This is not desired.

  5. +++ b/tests/src/FunctionalJavascript/MultiStepSelectionDisplayTest.php
    @@ -0,0 +1,253 @@
    +  /**
    +   * Open iframe entity browser and change scope to iframe.
    +   */
    +  protected function openEntityBrowser() {
    +    $this->getSession()->getPage()->clickLink('Select entities');
    +    $this->getSession()
    +      ->switchToIFrame('entity_browser_iframe_test_entity_browser_file');
    +    $this->waitForAjaxToFinish();
    +  }
    

    Could this go on the base class? Might be useful for other tests too.

  6. Please check the patch with code sniffer/eslint. I think that there are some things that are reported there.
slashrsm’s picture

Status: Needs review » Needs work
naveenvalecha’s picture

  1. +++ b/config/schema/entity_browser.schema.yml
    @@ -91,6 +91,9 @@ entity_browser.browser.widget.upload:
    +    auto_select:
    

    We are adding the new key to the schema? Do we need update hook/default values would work? if default values will work could you add a test for that?

  2. +++ b/config/schema/entity_browser.schema.yml
    @@ -108,6 +111,9 @@ entity_browser.browser.widget.view:
    +    auto_select:
    

    similarly as above new key in schema.

  3. +++ b/js/entity_browser.command_queue.js
    @@ -0,0 +1,108 @@
    \ No newline at end of file
    

    Nitpick: :)

mtodor’s picture

Status: Needs work » Needs review
FileSize
88.99 KB
5.29 KB

I have made few modifications listed in previous comments:

Comment #40:

  1. adjusted
  2. adjusted
  3. unfortunately Update widget doesn't take form build by parent. It builds form from scratch. :(
  4. problem here is that "auto_select" configuration option is used in WidgetBase. Submit button is removed based on that configuration in WidgetBase::getForm(). If we move option from base into widget that supports auto submit functionality, then it will not be clean from code where it's used and where it's set. I can do that, but it will be more messy. And also I would like to see Update widget to use auto select functionality too.
  5. that's true, it is possible to use this functionality in other tests, but I would do it in other ticket (move method in Base, generalize method that iframe name can be passed and change tests to use it)
  6. adjusted for introduced code

Comment #42:

  1. what should exactly be done here? for all existing widgets of entity browsers set configuration for new introduced configuration to default value (FALSE)?
  2. same as #42.1
  3. same as #40.1

So I need feedback regarding #40.4 and #42.1.

slashrsm’s picture

Status: Needs review » Needs work

#40-3: That is a mistake. Feel free to add the parent call and remove submit_text from Upload.
#40-4: I am not saying that config should be moved away from WidgetBase. I am just saying it should not be added for the widgets that do not support it (condition in defaultConfiguration() and on other relevant places).

#42.1: We don't need to do any updates in my opinion. defaultConfiguration() should handle that.

mtodor’s picture

Status: Needs work » Needs review
FileSize
7.93 KB
87.52 KB

Here are changes related to #40.3 and #40.4.

mtodor’s picture

Adjusted code for #40.4 -> now everything stays in base widget class, but with additional checking if widget supports auto select.

mtodor’s picture

Renamed annotation names -> from camelCase to snake_case.

mtodor’s picture

Patch at #47 is not complete -> here is new upload.

The last submitted patch, 45: 2822009_45.patch, failed testing.

The last submitted patch, 46: 2822009_46.patch, failed testing.

The last submitted patch, 47: 2822009_47.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 48: 2822009_48.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
87.37 KB

Reroll of #48.

Status: Needs review » Needs work

The last submitted patch, 53: 2822009_53.patch, failed testing.

mtodor’s picture

Status: Needs work » Needs review
FileSize
87.41 KB
671 bytes

Here is adjustment for tests.

  • slashrsm committed ca1f2aa on 8.x-1.x authored by mtodor
    Issue #2822009 by mtodor, chr.fritsch, samuel.mortenson, slashrsm,...
slashrsm’s picture

Status: Needs review » Fixed

Committed! Thank you all. It is a great improvement to the module!

samuel.mortenson’s picture

Great job everyone, I'm glad this got in even with all the core Media work being done. I'll be updating File Browser to add full support for this soon. :-D

Status: Fixed » Closed (fixed)

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