Merge pull request #31 from goodguyry/fix/destroy-audit

Verifies destroy methods remove all attributes and listeners

CHANGELOG.md

@@ -15,7 +15,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 **Fixed**
 
 - Updates NPM dependencies (#25)
-- Corrects issues with `Menu.destroy()` (#26)
+- Corrects issues with the reliability of `destroy` methods (#26 & #31)
 
 ## 0.2.0
 

docs/_data/assets.yaml

@@ -1 +1 @@
-global: { css: 'build/css/global.f0739c4bd2ecae532939.min.css', js: 'build/js/global.14fb354bfee817c4342c.bundle.min.js' }
+global: { css: 'build/css/global.f0739c4bd2ecae532939.min.css', js: 'build/js/global.0bfa4ac419357bb76f4b.bundle.min.js' }

src/Dialog/Dialog.test.js

@@ -8,8 +8,7 @@ const {
   keydownEsc,
 } = events;
 
-// Set up our document body
-document.body.innerHTML = `
+const dialogMarkup = `
   <main>
     <article>
       <h1>The Article Title</h1>
@@ -34,6 +33,9 @@ document.body.innerHTML = `
   </div>
 `;
 
+// Set up our document body
+document.body.innerHTML = dialogMarkup;
+
 const controller = document.querySelector('.link');
 const target = document.getElementById('dialog');
 const close = target.querySelector('button');
@@ -156,6 +158,9 @@ describe('Dialog with default configuration', () => {
       expect(target.getAttribute('hidden')).toBeNull();
 
       expect(onDestroy).toHaveBeenCalled();
+
+      // Quick and dirty verification that the original markup is restored.
+      expect(document.body.innerHTML).toEqual(dialogMarkup);
     });
   });
 });

src/Dialog/index.js

@@ -138,6 +138,7 @@ export default class Dialog extends AriaComponent {
 
     /**
      * The Popup instance controlling the Dialog.
+     *
      * @type {Popup}
      */
     this.popup = new Popup({
@@ -285,9 +286,13 @@ export default class Dialog extends AriaComponent {
     // Destroy the Dialog Popup.
     this.popup.destroy();
 
+    // Remove the `aria-hidden` attribute from the content wrapper.
+    this.content.removeAttribute('aria-hidden');
+
     // Remove event listeners.
     this.close.removeEventListener('click', this.hide);
     this.target.removeEventListener('keydown', this.handleTargetKeydown);
+    document.body.removeEventListener('keydown', this.handleKeydownEsc);
 
     /* Run {destroyCallback} */
     this.onDestroy.call(this);

src/Disclosure/Disclosure.test.js

@@ -8,8 +8,7 @@ const {
   keydownSpace,
 } = events;
 
-// Set up our document body
-document.body.innerHTML = `
+const disclosureMarkup = `
   <dl>
     <dt class="question">
       <button class="button">What is Lorem Ipsum?</button>
@@ -20,6 +19,9 @@ document.body.innerHTML = `
   </dl>
 `;
 
+// Set up our document body
+document.body.innerHTML = disclosureMarkup;
+
 const controller = document.querySelector('.button');
 const target = document.querySelector('.answer');
 
@@ -104,6 +106,7 @@ describe('Disclosure with default configuration', () => {
   it('Should remove all DOM attributes when destroyed', () => {
     disclosure.destroy();
 
+    expect(controller.getAttribute('role')).toBeNull();
     expect(controller.getAttribute('aria-expanded')).toBeNull();
     expect(controller.getAttribute('aria-controls')).toBeNull();
     expect(controller.getAttribute('tabindex')).toBeNull();
@@ -115,6 +118,9 @@ describe('Disclosure with default configuration', () => {
 
     expect(disclosure.controller.disclosure).toBeUndefined();
     expect(disclosure.target.disclosure).toBeUndefined();
+
+    // Quick and dirty verification that the original markup is restored.
+    expect(document.body.innerHTML).toEqual(disclosureMarkup);
   });
 });
 
@@ -163,5 +169,8 @@ describe('Disclosure with non-default configuration', () => {
     expect(disclosure.controller.disclosure).toBeUndefined();
     expect(disclosure.target.disclosure).toBeUndefined();
     expect(onDestroy).toHaveBeenCalled();
+
+    // Quick and dirty verification that the original markup is restored.
+    expect(document.body.innerHTML).toEqual(disclosureMarkup);
   });
 });

src/Disclosure/index.js

@@ -279,18 +279,33 @@ export default class Disclosure extends AriaComponent {
     // Remove the references to the class instance.
     this.deleteSelfReferences();
 
+    // Remove IDs set by this class.
+    [this.controller, this.target].forEach((element) => {
+      if (element.getAttribute('id').includes('id_')) {
+        element.removeAttribute('id');
+      }
+    });
+
     // Remove controller attributes.
     this.controller.removeAttribute('aria-expanded');
     this.controller.removeAttribute('aria-controls');
     this.controller.removeAttribute('aria-owns');
     this.controller.removeAttribute('tabindex');
 
+    if ('BUTTON' !== this.controller.nodeName) {
+      this.controller.removeAttribute('role');
+    }
+
     // Remove target attributes.
     this.target.removeAttribute('aria-hidden');
     this.target.removeAttribute('hidden');
 
+    // Remove tabindex attributes.
+    tabIndexAllow(this.interactiveChildElements);
+
     // Remove event listeners.
     this.controller.removeEventListener('click', this.toggleExpandedState);
+    this.controller.removeEventListener('keydown', this.handleControllerKeydown);
     document.body.removeEventListener('click', this.closeOnOutsideClick);
 
     // Reset initial state.

src/Listbox/Listbox.test.js

@@ -13,8 +13,7 @@ const {
   keydownEnd,
 } = events;
 
-// Set up our document body
-document.body.innerHTML = `
+const listboxMarkup = `
   <button>Choose</button>
   <ul>
     <li>Anchorage</li>
@@ -29,6 +28,9 @@ document.body.innerHTML = `
   </ul>
 `;
 
+// Set up our document body
+document.body.innerHTML = listboxMarkup;
+
 const controller = document.querySelector('button');
 const target = document.querySelector('ul');
 const listItems = Array.from(target.children);
@@ -41,7 +43,7 @@ const onInit = jest.fn();
 const onDestroy = jest.fn();
 
 describe('Listbox with default configuration', () => {
-  beforeEach(() => {
+  beforeAll(() => {
     listbox = new Listbox({
       controller,
       target,
@@ -211,21 +213,34 @@ describe('Listbox with default configuration', () => {
     });
   });
 
-  it('Should destroy the Listbox as expected', () => {
-    listbox.destroy();
+  describe('Listbox destroy', () => {
+    it('Should destroy the Listbox as expected', () => {
+      listbox.destroy();
+
+      expect(controller.getAttribute('aria-haspopup')).toBeNull();
+      expect(controller.getAttribute('aria-expanded')).toBeNull();
+      expect(controller.getAttribute('aria-controls')).toBeNull();
+      expect(target.getAttribute('aria-activedescendant')).toBeNull();
+      expect(target.getAttribute('aria-hidden')).toBeNull();
+      expect(target.getAttribute('hidden')).toBeNull();
+
+      listItems.forEach((item) => {
+        expect(item.getAttribute('role')).toBeNull();
+        expect(item.getAttribute('aria-selected')).toBeNull();
+      })
 
-    expect(controller.getAttribute('aria-haspopup')).toBeNull();
-    expect(controller.getAttribute('aria-expanded')).toBeNull();
-    expect(controller.getAttribute('aria-controls')).toBeNull();
-    expect(target.getAttribute('aria-hidden')).toBeNull();
-    expect(target.getAttribute('hidden')).toBeNull();
+      expect(controller.listbox).toBeUndefined();
+      expect(target.listbox).toBeUndefined();
 
-    expect(controller.listbox).toBeUndefined();
-    expect(target.listbox).toBeUndefined();
+      controller.dispatchEvent(click);
+      expect(listbox.getState().expanded).toBeFalsy();
 
-    controller.dispatchEvent(click);
-    expect(listbox.getState().expanded).toBeFalsy();
+      expect(onDestroy).toHaveBeenCalled();
 
-    expect(onDestroy).toHaveBeenCalled();
+      // Quick and dirty verification that the original markup is restored.
+      // But first, restore the button's original text label.
+      controller.textContent = 'Choose';
+      expect(document.body.innerHTML).toEqual(listboxMarkup);
+    });
   });
 });

src/Listbox/index.js

@@ -439,6 +439,12 @@ export default class ListBox extends AriaComponent {
     // Remove the role attribute from each of the options.
     this.options.forEach((listItem) => {
       listItem.removeAttribute('role');
+      listItem.removeAttribute('aria-selected');
+
+      // Remove IDs set by this class.
+      if (listItem.getAttribute('id').includes('id_')) {
+        listItem.removeAttribute('id');
+      }
     });
 
     // Destroy the Popup.
@@ -447,6 +453,7 @@ export default class ListBox extends AriaComponent {
     // Remove the listbox role.
     this.target.removeAttribute('role');
     this.target.removeAttribute('tabindex');
+    this.target.removeAttribute('aria-activedescendant');
 
     // Remove event listeners.
     this.controller.removeEventListener('keyup', this.handleControllerKeyup);

src/Menu/Menu.test.js

@@ -2,11 +2,6 @@
 import { Menu, Disclosure } from 'root';
 import { events, typeCharacter } from '../lib/events';
 
-// Create the help text elements.
-const ariaDescribedbyTestMarkup = Menu.getHelpIds().reduce((acc, id) => (
-  `${acc}<div id="${id.replace('#', '')}"></div>`
-), '');
-
 const {
   keydownDown,
   keydownUp,
@@ -16,8 +11,7 @@ const {
   keydownHome,
 } = events;
 
-// Set up our document body
-document.body.innerHTML = `
+const menuMarkup = `
   <nav class="nav" aria-label="Menu Class Example">
     <ul class="menu">
       <li>
@@ -43,9 +37,15 @@ document.body.innerHTML = `
     </ul>
   </nav>
 
-  ${ariaDescribedbyTestMarkup}
+  <div id="ac-describe-submenu-help"></div>
+  <div id="ac-describe-esc-help"></div>
+  <div id="ac-describe-submenu-explore"></div>
+  <div id="ac-describe-submenu-back"></div>
 `;
 
+// Set up our document body
+document.body.innerHTML = menuMarkup;
+
 // Collect references to DOM elements.
 const domElements = {
   list: document.querySelector('.menu'),
@@ -258,17 +258,26 @@ describe('Menu instatiates submenus as Disclosures', () => {
   it('Should remove all Menu Disclosure DOM attributes when destroyed', () => {
     menu.destroy();
 
+    expect(list.getAttribute('role')).toBeNull();
+
     expect(domElements.listFirstItem.getAttribute('aria-expanded')).toBeNull();
     expect(domElements.listFirstItem.getAttribute('aria-controls')).toBeNull();
     expect(domElements.listFirstItem.getAttribute('tabindex')).toBeNull();
     // The test markup isn't detatched, so this doesn't apply.
     expect(domElements.listFirstItem.getAttribute('aria-owns')).toBeNull();
+    expect(domElements.listFirstItem.getAttribute('role')).toBeNull();
+    expect(domElements.listFirstItem.parentElement.getAttribute('role')).toBeNull();
+    expect(domElements.listFirstItem.getAttribute('aria-describedby')).toBeNull();
+    expect(domElements.listFirstItem.getAttribute('aria-setsize')).toBeNull();
+    expect(domElements.listFirstItem.getAttribute('aria-posinset')).toBeNull();
 
     expect(domElements.sublistOne.getAttribute('aria-hidden')).toBeNull();
     expect(domElements.sublistOne.getAttribute('hidden')).toBeNull();
 
     expect(domElements.listFirstItem.disclosure).toBeUndefined();
     expect(domElements.sublistOne.disclosure).toBeUndefined();
+
+    // Quick and dirty verification that the original markup is restored.
+    expect(document.body.innerHTML).toEqual(menuMarkup);
   });
 });
-export default ariaDescribedbyTestMarkup;

src/MenuBar/MenuBar.test.js

@@ -2,14 +2,6 @@
 import { MenuBar, Popup, Menu } from 'root';
 import { events } from '../lib/events';
 
-// Create the help text elements for both MenuBar and Menu.
-const ariaDescribedbyTestMarkup = Array.from(new Set([
-  ...MenuBar.getHelpIds(),
-  ...Menu.getHelpIds(),
-])).reduce((acc, id) => (
-  `${acc}<div id="${id.replace('#', '')}"></div>`
-), '');
-
 const {
   keydownDown,
   keydownRight,
@@ -20,8 +12,7 @@ const {
   keydownReturn,
 } = events;
 
-// Set up our document body
-document.body.innerHTML = `
+const menubarMarkup = `
   <nav class="nav" aria-label="Menu Class Example">
     <ul class="menubar">
       <li>
@@ -47,9 +38,16 @@ document.body.innerHTML = `
     </ul>
   </nav>
 
-  ${ariaDescribedbyTestMarkup}
+  <div id="ac-describe-submenu-help"></div>
+  <div id="ac-describe-esc-help"></div>
+  <div id="ac-describe-submenu-explore"></div>
+  <div id="ac-describe-submenu-back"></div>
+  <div id="ac-describe-top-level-help"></div>
 `;
 
+// Set up our document body
+document.body.innerHTML = menubarMarkup;
+
 // Collect references to DOM elements.
 const domElements = {
   list: document.querySelector('.menubar'),
@@ -223,6 +221,7 @@ describe('Menu should destroy properly', () => {
     expect(domElements.list.getAttribute('role')).toBeNull();
 
     expect(domElements.listFirstItem.getAttribute('aria-setsize')).toBeNull();
+    expect(domElements.listFirstItem.getAttribute('tabindex')).toBeNull();
     expect(domElements.listSecondItem.getAttribute('aria-describedby')).toBeNull();
     expect(domElements.listThirdItem.getAttribute('aria-posinset')).toBeNull();
     expect(domElements.listThirdItem.getAttribute('role')).toBeNull();
@@ -231,8 +230,12 @@ describe('Menu should destroy properly', () => {
 
     expect(domElements.sublistTwoSecondItem.getAttribute('aria-setsize')).toBeNull();
     expect(domElements.sublistTwoLastItem.getAttribute('aria-posinset')).toBeNull();
+    expect(domElements.sublistTwoLastItem.getAttribute('tabindex')).toBeNull();
 
     expect(domElements.list.menubar).toBeUndefined();
     expect(onDestroy).toHaveBeenCalled();
+
+    // Quick and dirty verification that the original markup is restored.
+    expect(document.body.innerHTML).toEqual(menubarMarkup);
   });
 });

src/MenuBar/index.js

@@ -482,17 +482,19 @@ export default class MenuBar extends AriaComponent {
       link.removeEventListener('click', this.handleMenuBarClick);
     });
 
+    // Remove tabindex attribute.
+    tabIndexAllow(this.menuBarItems);
+
     // Destroy nested components.
     this.popups.forEach((popup) => {
       if (isInstanceOf(popup.target.menu, Menu)) {
         popup.target.menu.destroy();
       }
+      popup.target.removeEventListener('keydown', this.handleMenuItemKeydown);
+
       popup.destroy();
     });
 
-    // Revert tabindex attributes.
-    tabIndexAllow(this.menuBarItems);
-
     // Run {destroyCallback}
     this.onDestroy.call(this);
   }