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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 Update event listeners for mega menu #25091

Merged
merged 4 commits into from
Oct 23, 2019
Merged

馃悰 Update event listeners for mega menu #25091

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
44d8ce6
remove event listeners on unlayout and update tests
leafsy Oct 17, 2019
04c2f63
add manual test file
leafsy Oct 17, 2019
6f72435
simplify event listener code:q
leafsy Oct 21, 2019
ffc5f14
update logic for registering menu items
leafsy Oct 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
187 changes: 134 additions & 53 deletions extensions/amp-mega-menu/0.1/amp-mega-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ import {Keys} from '../../../src/utils/key-codes';
import {Layout} from '../../../src/layout';
import {Services} from '../../../src/services';
import {
closest,
closestAncestorElementBySelector,
isConnectedNode,
isRTL,
scopedQuerySelector,
scopedQuerySelectorAll,
Expand Down Expand Up @@ -48,8 +50,8 @@ export class AmpMegaMenu extends AMP.BaseElement {
constructor(element) {
super(element);

/** @private {?Array<!Element>} */
this.items_ = null;
/** @private {!Array<!Element>} */
this.items_ = [];

/** @private {number} */
this.itemCount_ = 0;
Expand All @@ -71,6 +73,15 @@ export class AmpMegaMenu extends AMP.BaseElement {

/** @private {number|string} */
this.prefix_ = element.id || Math.floor(Math.random() * 100);

/** @private {function(!Event)} */
this.domUpdateHandler_ = this.registerMenuItems_.bind(this);

/** @private {function(!Event)} */
this.rootClickHandler_ = this.handleRootClick_.bind(this);

/** @private {function(!Event)} */
this.rootKeyDownHandler_ = this.handleRootKeyDown_.bind(this);
}

/** @override */
Expand All @@ -94,79 +105,92 @@ export class AmpMegaMenu extends AMP.BaseElement {
this.registerMenuItems_();
// items may not be present after build if dynamically rendered via amp-list,
// in which case register them after DOM update instead.
this.element.addEventListener(AmpEvents.DOM_UPDATE, () => {
this.registerMenuItems_();
});

const mask = this.document_.createElement('div');
mask.classList.add('i-amphtml-mega-menu-mask');
mask.setAttribute('aria-hidden', 'true');
// append mask to header so that all children of header appear above the mask
const maskParent =
closestAncestorElementBySelector(this.element, 'header') || this.element;
maskParent.classList.add('i-amphtml-mega-menu-mask-parent');
maskParent.appendChild(mask);
this.maskElement_ = mask;
this.element.addEventListener(AmpEvents.DOM_UPDATE, this.domUpdateHandler_);

this.documentElement_.addEventListener('click', () => this.collapse_());
this.documentElement_.addEventListener('keydown', event => {
// Collapse mega menu on ESC.
if (event.key === Keys.ESCAPE && this.collapse_()) {
event.preventDefault();
}
});
if (!this.maskElement_) {
this.maskElement_ = this.createMaskElement_();
}

return Promise.resolve();
}

/** @override */
unlayoutCallback() {
// TODO(#25047): unregister event listeners.
this.element.removeEventListener(
AmpEvents.DOM_UPDATE,
this.domUpdateHandler_
);
// Ensure that menu is closed when hidden via media query.
this.collapse_();
return false;
return true;
}

/**
* Create a new mask element and append it to the header if present,
* otherwise to the component itself.
* @return {!Element} the mask that was created.
* @private
*/
createMaskElement_() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice commenting!

const mask = this.document_.createElement('div');
mask.classList.add('i-amphtml-mega-menu-mask');
mask.setAttribute('aria-hidden', 'true');
// append mask to header so that all children of header appear above the mask
const maskParent =
closestAncestorElementBySelector(this.element, 'header') || this.element;
maskParent.classList.add('i-amphtml-mega-menu-mask-parent');
maskParent.appendChild(mask);
return mask;
}

/**
* Find all expandable menu items under this mega menu and add appropriate
* classes, attributes and event listeners to its children.
* Find all menu items under this mega menu and add appropriate classes,
* attributes and event listeners to its children.
* @private
*/
registerMenuItems_() {
this.items_ = toArray(scopedQuerySelectorAll(this.element, 'nav > * > li'));
this.items_.forEach(item => {
// skip if the item has already been registered.
this.items_ = [];
const items = toArray(scopedQuerySelectorAll(this.element, 'nav > * > li'));
items.forEach(item => {
// check if the item has already been registered.
if (item.classList.contains('i-amphtml-mega-menu-item')) {
this.items_.push(item);
return;
}
const heading =
scopedQuerySelector(item, '> button') ||
scopedQuerySelector(item, '> [role=button]');
const content = scopedQuerySelector(item, '> [role=dialog]');
// do not register item if either its heading or content element is missing,
// or if heading has on tap action.
if (
!heading ||
!content ||
this.action_.hasAction(heading, 'tap', item)
) {
return;
// register item only if heading element is present.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think when @kristoferbaxter suggested use of a filter function, he meant use the filter function to filter down to only the items in the array you want to operate on. For example, you could also write this as:

this.items_ = toArray(scopedQuerySelectorAll(this.element, 'nav > * > li'));
this.items_.filter(item => !item.classList.contains('i-amphtml-mega-menu-item'))
.foreach(item => {
  const heading = ...
  ...
  this.registerMenuItem_(item, heading, content);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm the thing is I only want this.items_ to include items with heading elements; so i can filter out the elements without headings first, but then in the foreach I'll need to get a reference to heading again to pass to registerMenuItem_.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made some changes that should allow me to use the filter and foreach pattern now. For any <li> that conforms to the specs laid out in #25210, we'll register it as a menu item and include it in this.items_. The only case not covered by validation but should be disallowed is when there're two elements under an item and neither is a button/role=button, in which case I've added a userAssert to warn against this. Hopefully the documentation will make it clear.

if (heading) {
this.registerMenuItem_(item, heading, content);
leafsy marked this conversation as resolved.
Show resolved Hide resolved
this.items_.push(item);
}
this.registerMenuItem_(item, heading, content);
});
}

/**
* Register the given menu item, along with its heading and content elements.
* @param {!Element} item
* @param {!Element} heading
* @param {!Element} content
* @param {?Element} content
* @private
*/
registerMenuItem_(item, heading, content) {
item.classList.add('i-amphtml-mega-menu-item');
this.itemCount_++;

heading.classList.add('i-amphtml-mega-menu-heading');
if (!heading.hasAttribute('tabindex')) {
heading.setAttribute('tabindex', 0);
}
heading.addEventListener('click', e => this.handleHeadingClick_(e));
heading.addEventListener('keydown', e => this.handleHeadingKeyDown_(e));

// Skip if item does not have a submenu or its heading already has tap action.
if (!content || this.action_.hasAction(heading, 'tap', item)) {
return;
}
content.classList.add('i-amphtml-mega-menu-content');
content.setAttribute('aria-modal', 'false');
let contentId = content.getAttribute('id');
Expand All @@ -182,22 +206,41 @@ export class AmpMegaMenu extends AMP.BaseElement {
content.firstChild
);
content.appendChild(this.createScreenReaderCloseButton_());
// prevent click event listener on document from closing the menu
content.addEventListener('click', e => e.stopPropagation());

heading.classList.add('i-amphtml-mega-menu-heading');
// haspopup value not set to menu since content can contain more than links.
heading.setAttribute('aria-haspopup', 'dialog');
heading.setAttribute('aria-controls', contentId);
if (!heading.hasAttribute('tabindex')) {
heading.setAttribute('tabindex', 0);
}
heading.setAttribute('aria-expanded', 'false');
heading.addEventListener('click', e => this.handleHeadingClick_(e));
// prevent focus on mousedown so that there's no sudden shift in focus
// when the content container opens.
heading.addEventListener('mousedown', e => e.preventDefault());
heading.addEventListener('keydown', e => this.handleHeadingKeyDown_(e));
}

/**
* Handle click event on document element to collapse mega menu if opened;
* do nothing if click is inside the expanded content element.
* @param {!Event} event click event.
* @private
*/
handleRootClick_(event) {
const target = dev().assertElement(event.target);
if (
this.expandedItem_ &&
!this.expandedItem_.contains(target) &&
// since amp-video immediately removes its mask on first click, this check
// prevents menu from collapsing due to target no longer being attached.
isConnectedNode(target)
) {
this.collapse_();
}
}

/**
* Handle keydown event on document element to collapse mega menu on ESC.
* @param {!Event} event keydown event.
* @private
*/
handleRootKeyDown_(event) {
if (event.key === Keys.ESCAPE && this.collapse_()) {
event.preventDefault();
}
}

/**
Expand All @@ -206,15 +249,41 @@ export class AmpMegaMenu extends AMP.BaseElement {
* @private
*/
handleHeadingClick_(event) {
if (!this.shouldHandleClick_(event)) {
return;
}
event.preventDefault();
event.stopPropagation();
const item = dev().assertElement(event.target.parentElement);
const item = dev().assertElement(event.currentTarget.parentElement);
const previousItem = this.collapse_();
if (item != previousItem) {
this.expand_(item);
}
}

/**
* We should support clicks on any children of the heading except for on
* links or elements with tap targets, which should not have their default
* behavior overidden.
* @param {!Event} event
* @return {boolean}
* @private
*/
shouldHandleClick_(event) {
const target = dev().assertElement(event.target);
const currentTarget = dev().assertElement(event.currentTarget);
const hasAnchor = !!closest(target, e => e.tagName == 'A', currentTarget);
leafsy marked this conversation as resolved.
Show resolved Hide resolved
if (hasAnchor) {
return false;
}
const hasTapAction = this.action_.hasAction(target, 'tap', currentTarget);
if (hasTapAction) {
return false;
}
// do not handle click if heading has no associated content element.
return currentTarget.hasAttribute('aria-haspopup');
}

/**
* Handler for key presses on an item heading.
* @param {!Event} event keydown event.
Expand Down Expand Up @@ -245,7 +314,7 @@ export class AmpMegaMenu extends AMP.BaseElement {
* @private
*/
handleNavigationKeyDown_(event) {
const item = dev().assertElement(event.target.parentElement);
const item = dev().assertElement(event.currentTarget.parentElement);
const index = this.items_.indexOf(item);
if (index !== -1) {
event.preventDefault();
Expand Down Expand Up @@ -282,6 +351,9 @@ export class AmpMegaMenu extends AMP.BaseElement {
item.querySelector('.i-amphtml-screen-reader')
);
tryFocus(screenReaderCloseButton);
// add event listeners on the html element for closing the menu
this.documentElement_.addEventListener('click', this.rootClickHandler_);
this.documentElement_.addEventListener('keydown', this.rootKeyDownHandler_);
this.expandedItem_ = item;
}

Expand All @@ -305,7 +377,16 @@ export class AmpMegaMenu extends AMP.BaseElement {
this.maskElement_.removeAttribute('open');
const heading = this.getItemHeading_(item);
heading.setAttribute('aria-expanded', 'false');
tryFocus(heading);
// shift focus to heading only if it's currently inside the item.
if (item.contains(this.document_.activeElement)) {
tryFocus(heading);
}
// remove event listeners on the html element
this.documentElement_.removeEventListener('click', this.rootClickHandler_);
this.documentElement_.removeEventListener(
'keydown',
this.rootKeyDownHandler_
);
this.expandedItem_ = null;
return item;
}
Expand Down
23 changes: 19 additions & 4 deletions extensions/amp-mega-menu/0.1/test/test-amp-mega-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@ describes.realWin(
},
},
env => {
let win, doc, element;
let win, doc, element, sandbox;

beforeEach(() => {
win = env.win;
doc = win.document;
sandbox = env.sandbox;

element = getAmpMegaMenu();
doc.body.appendChild(element);
Expand All @@ -53,12 +54,11 @@ describes.realWin(
<div id="content1" role="dialog">Loreum ipsum</div>
</li>
<li>
<button id="heading2">Menu Item 2</button>
<div id="heading2" role="button">Menu Item 2</div>
<div id="content2" role="dialog">Loreum ipsum</div>
</li>
<li>
<button id="heading3">Menu Item 3</button>
<div id="content3" role="dialog">Loreum ipsum</div>
</li>
</ul>
</nav>
Expand All @@ -82,7 +82,7 @@ describes.realWin(
const headingClass = '.i-amphtml-mega-menu-heading';
expect(element.querySelectorAll(headingClass).length).to.equal(3);
const contentClass = '.i-amphtml-mega-menu-content';
expect(element.querySelectorAll(contentClass).length).to.equal(3);
expect(element.querySelectorAll(contentClass).length).to.equal(2);
});

it('should expand when heading of a collapsed menu item is clicked', async () => {
Expand Down Expand Up @@ -197,5 +197,20 @@ describes.realWin(
heading3.dispatchEvent(rightKey);
expect(doc.activeElement).to.equal(heading1);
});

it('should remove event listeners on root element when menu is closed', async () => {
await element.build();
await element.layoutCallback();
await element.unlayoutCallback();
const impl = element.implementation_;
const clickEvent = new Event('click');
const rootClickSpy = sandbox.spy(impl, 'handleRootClick_');
doc.documentElement.dispatchEvent(clickEvent);
expect(rootClickSpy).to.not.be.called;
const keydownEvent = new KeyboardEvent('keydown');
const rootKeyDownSpy = sandbox.spy(impl, 'handleRootKeyDown_');
doc.documentElement.dispatchEvent(keydownEvent);
expect(rootKeyDownSpy).to.not.be.called;
});
}
);