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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Update event listeners for mega menu #25091

Merged
merged 4 commits into from Oct 23, 2019

Conversation

leafsy
Copy link
Contributor

@leafsy leafsy commented Oct 17, 2019

Resolves #25047

Update the event listener logic in amp-mega-menu to:

  1. keep references of event handlers for documentElement and add/remove them when menu is opened/closed (added unit tests);
  2. enable links and tap actions on children of menu headings by borrowing the shouldhandleClick method from amp-accordion;
  3. allow menu items without expandable content to still be navigable via arrow keys;
  4. some minor fixes from manual testing (e.g. prevent menu from collapsing when first clicking on an amp-video inside the menu);

updated firebase demo
Also added file under manual tests: pr-deployed link (must turn on the experiment amp-mega-menu)

extensions/amp-mega-menu/0.1/amp-mega-menu.js Show resolved Hide resolved
extensions/amp-mega-menu/0.1/amp-mega-menu.js Outdated Show resolved Hide resolved
extensions/amp-mega-menu/0.1/amp-mega-menu.js Outdated Show resolved Hide resolved
extensions/amp-mega-menu/0.1/amp-mega-menu.js Outdated Show resolved Hide resolved
* @private
*/
registerMenuItemListeners_() {
this.items_.forEach(item => {

Choose a reason for hiding this comment

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

Probably be better to use event delegation here, rather than adding a listener per item. That will allow for handling of changed DOM content (e.g. new headings added) automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the DOM update listener might still be necessary since we need to add several a11y attributes when first registering each menu item. Looking at the code again, since we're adding separate event listeners to the screen reader buttons anyway, maybe it's ok to keep the existing listeners on heading elements rather than using event delegation?

extensions/amp-mega-menu/0.1/amp-mega-menu.js Outdated Show resolved Hide resolved
'keydown',
this.documentKeyDownHandler_
);
this.unregisterMenuItemListeners_();

Choose a reason for hiding this comment

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

There is no reason to remove event listeners on unlayout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked offline; will remove the unregister function and add/remove the event listeners for documentElement inside the expand/collapse methods since these listeners are only needed when a menu is open.

extensions/amp-mega-menu/0.1/amp-mega-menu.js Show resolved Hide resolved
extensions/amp-mega-menu/0.1/amp-mega-menu.js Outdated Show resolved Hide resolved
@leafsy leafsy requested a review from sparhami October 21, 2019 23:43
}
const heading =
scopedQuerySelector(item, '> button') ||
scopedQuerySelector(item, '> [role=button]');
const content = scopedQuerySelector(item, '> [role=dialog]');
// 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.

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

Copy link
Contributor

@cathyxz cathyxz left a comment

Choose a reason for hiding this comment

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

Approving to unblock.

Nit: I would recommend making the registerMenuItems function idempotent to simplify your DOM_UPDATE code. But feel free to do this if you have extra time.

@leafsy leafsy merged commit 72e6f34 into ampproject:master Oct 23, 2019
@leafsy leafsy deleted the bugfix/mega-menu-listener branch October 23, 2019 21:50
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
* add manual test file

* simplify event listener code

* update logic for registering menu items
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unregister amp-mega-menu event listeners on unlayout
4 participants