Event propagation fix for Dropdown component (#255)

* Fixed event propagation for onClick handler and provided correct target for option click handler

* Add possibility to update options and markup via [update] method

* Test if this fixes possible memory leak

* Test if this fixes possible memory leak

* Revert "Test if this fixes possible memory leak"

This reverts commit 0265e4e.

* Template renderer as method. Add [update] method testing btn for "Outside controls" story

* Fix documentation for error stated Dropdowns

* Add options to [update] test

Author: sick-sad-world

docs/html/dropdown/dropdown--error.html

@@ -5,9 +5,9 @@
       <option value="Squirtle">Squirtle</option>
       <option value="Squirtle">Charmander</option>
     </select>
+    <label class="ray-dropdown__label">
+      What's your favorite Pokémon?
+    </label>
   </div>
-  <label class="ray-dropdown__label">
-    What's your favorite Pokémon?
-  </label>
-  <span class="ray-dropdown__hint">Is anything wrong?</span>
 </div>
+<span class="ray-dropdown__hint">Is anything wrong?</span>

docs/html/dropdown/rtl-dropdown--error.html

@@ -10,6 +10,6 @@
     <label class="ray-dropdown__label">
       What's your favorite Pokémon?
     </label>
-    <span class="ray-dropdown__hint">Is anything wrong?</span>
   </div>
-</div>
+  <span class="ray-dropdown__hint">Is anything wrong?</span>
+</div>

packages/core/src/components/dropdown/index.js

@@ -136,21 +136,11 @@ class Dropdown {
     this._id = id;
     this._inputElement = el;
 
-    markupTemplates.forEach(template => {
-      const { tpl, elements, position } = template({
-        value: this._selectedOption.innerHTML,
-        id: this._id
-      });
-      insertMarkup(this._inputElement, position, tpl);
-      this._cacheEl(elements);
-    });
+    this._renderMarkup();
     this._processLabel();
     this._setAriaProps();
     this._fillOptionsList();
-
-    if (!this._inputElement.disabled) {
-      this._bindEventListeners();
-    }
+    this._bindEventListeners();
 
     this._value = this._inputElement.value;
 
@@ -170,6 +160,17 @@ class Dropdown {
     });
   }
 
+  _renderMarkup() {
+    markupTemplates.forEach(template => {
+      const { tpl, elements, position } = template({
+        value: this._selectedOption.innerHTML,
+        id: this._id
+      });
+      insertMarkup(this._inputElement, position, tpl);
+      this._cacheEl(elements);
+    });
+  }
+
   _setAriaProps() {
     const listId = this._list.getAttribute('id');
 
@@ -264,12 +265,13 @@ class Dropdown {
   }
 
   _bindEventListeners() {
+    if (this._inputElement.disabled) return;
     window.addEventListener('click', this.onClick);
     this._body.addEventListener('focus', this.onFocus);
     this._body.addEventListener('blur', this.onFocus);
     this._inputElement.addEventListener('change', this.onChange);
     this._getEl('option', true).forEach(el => {
-      el.addEventListener('click', this.onOptionClick);
+      el.addEventListener('click', this.onOptionClick(this));
     });
   }
 
@@ -308,8 +310,16 @@ class Dropdown {
     switchClassName(this._root, ['ACTIVE', 'OPEN'], true);
   };
 
-  update = () => {
+  update = options => {
+    if (options) {
+      this.settings = {
+        ...defaults,
+        ...options
+      };
+    }
+    this._removeEventListeners();
     this._fillOptionsList();
+    this._bindEventListeners();
     this._value = '';
   };
 
@@ -330,25 +340,22 @@ class Dropdown {
   };
 
   onClick = e => {
-    e.stopPropagation();
-    e.preventDefault();
     const isClickInside =
       this._root === e.target || this._root.contains(e.target);
-    switchClassName(
-      this._root,
-      'OPEN',
-      isClickInside && !e.target.dataset.rayIdx
-    );
+    const isOptionClick = this._list.contains(e.target);
+    switchClassName(this._root, 'OPEN', isClickInside && !isOptionClick);
     if (!isClickInside) {
       switchClassName(this._root, 'ACTIVE', false);
       emitEvent(this._root, 'blur');
     }
   };
 
-  onOptionClick = e => {
-    if (e.target.hasAttribute('disabled') || !e.target.dataset.rayIdx) return;
-    this._value = this._options[e.target.dataset.rayIdx].value;
-  };
+  onOptionClick(plugin) {
+    return function onClickListener() {
+      if (this.hasAttribute('disabled') || !this.dataset.rayIdx) return;
+      plugin._value = plugin._options[this.dataset.rayIdx].value; //eslint-disable-line
+    };
+  }
 
   destroy() {
     this._removeEventListeners();

packages/core/stories/dropdown.stories.js

@@ -651,6 +651,30 @@ storiesOf('Dropdown', module)
       const inst = getInst();
       inst.disable();
     }
+    function updateDrop(e) {
+      e.preventDefault();
+      const input = document.getElementById('test-example');
+      const label = document.getElementById('test-example-label');
+      const inst = getInst();
+      label.innerHTML = 'Currency';
+      input.innerHTML = `
+        <option value="USD">USD|United states dollar</option>
+        <option disabled data-ray-separator />
+        <option value="GBP">GBP|Great Britain Pound</option>
+        <option value="EUR">EUR|Euro</option>
+        <option value="JPY">JPY|Japaneese Yen</option>
+      `;
+      inst.update({
+        renderSelected: option => {
+          const [symbol] = option.innerHTML.split('|');
+          return `<b>${symbol}</b>`;
+        },
+        renderOption: option => {
+          const [symbol, name] = option.innerHTML.split('|');
+          return `<b>${symbol}</b><em>${name}</em>`;
+        }
+      });
+    }
     return (
       <>
         <div className="ray-form-item">
@@ -710,6 +734,12 @@ storiesOf('Dropdown', module)
           >
             Disable
           </button>
+          <button
+            className="ray-button ray-button--primary ray-button--compact"
+            onClick={updateDrop}
+          >
+            Update
+          </button>
         </div>
       </>
     );

packages/core/test/components/dropdown.test.js

@@ -128,8 +128,14 @@ describe('Select', () => {
   test('#update should reset value and update option list e.g. adopt chagnges to original select', () => {
     const { select, selectEl } = setupTest();
     selectEl.querySelector('select').innerHTML = newOptionsFixture;
-    select.update();
+    select.update({
+      renderOption: option => {
+        const [symbol, name] = option.innerHTML.split('|');
+        return `<b class="this-is-option">${symbol}</b><em>${name}</em>`;
+      }
+    });
     expect(select.value()).toEqual('');
+    expect(selectEl.querySelector('.this-is-option')).toBeTruthy();
     select.destroy();
   });