Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verifies destroy methods remove all attributes and listeners #31

Merged
merged 9 commits into from
Mar 29, 2020
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion docs/_data/assets.yaml
Original file line number Diff line number Diff line change
@@ -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' }
2 changes: 2 additions & 0 deletions docs/build/js/global.0bfa4ac419357bb76f4b.bundle.min.js

Large diffs are not rendered by default.

Large diffs are not rendered by default.

2 changes: 0 additions & 2 deletions docs/build/js/global.14fb354bfee817c4342c.bundle.min.js

This file was deleted.

This file was deleted.

9 changes: 7 additions & 2 deletions src/Dialog/Dialog.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ const {
keydownEsc,
} = events;

// Set up our document body
document.body.innerHTML = `
const dialogMarkup = `
<main>
<article>
<h1>The Article Title</h1>
Expand All @@ -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');
Expand Down Expand Up @@ -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);
});
});
});
5 changes: 5 additions & 0 deletions src/Dialog/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ export default class Dialog extends AriaComponent {

/**
* The Popup instance controlling the Dialog.
*
* @type {Popup}
*/
this.popup = new Popup({
Expand Down Expand Up @@ -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);
Expand Down
13 changes: 11 additions & 2 deletions src/Disclosure/Disclosure.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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');

Expand Down Expand Up @@ -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();
Expand All @@ -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);
});
});

Expand Down Expand Up @@ -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);
});
});
15 changes: 15 additions & 0 deletions src/Disclosure/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
45 changes: 30 additions & 15 deletions src/Listbox/Listbox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ const {
keydownEnd,
} = events;

// Set up our document body
document.body.innerHTML = `
const listboxMarkup = `
<button>Choose</button>
<ul>
<li>Anchorage</li>
Expand All @@ -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);
Expand All @@ -41,7 +43,7 @@ const onInit = jest.fn();
const onDestroy = jest.fn();

describe('Listbox with default configuration', () => {
beforeEach(() => {
beforeAll(() => {
listbox = new Listbox({
controller,
target,
Expand Down Expand Up @@ -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);
});
});
});
7 changes: 7 additions & 0 deletions src/Listbox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
Expand Down
27 changes: 18 additions & 9 deletions src/Menu/Menu.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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>
Expand All @@ -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'),
Expand Down Expand Up @@ -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;
25 changes: 14 additions & 11 deletions src/MenuBar/MenuBar.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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>
Expand All @@ -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'),
Expand Down Expand Up @@ -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();
Expand All @@ -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);
});
});
8 changes: 5 additions & 3 deletions src/MenuBar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Loading