Improved MultiStep selection display

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

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.

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

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

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

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

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

What feedback do you want in terms of usability review?

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

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.

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

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

Reroll of #48.

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

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