From 81eb9961f1208820932467c2f023e6340d4cfd26 Mon Sep 17 00:00:00 2001 From: Steven Ye <8905737+leafsy@users.noreply.github.com> Date: Thu, 24 Oct 2019 18:30:06 -0400 Subject: [PATCH] Update sidebar v0.2 + nested menu and remove history API (#25048) * change sidebar version, update naming and remove history api from nested menu --- build-system/compile/bundles.config.js | 4 +- .../0.1/amp-nested-menu.css} | 16 +- .../0.1/amp-nested-menu.js} | 142 +++++++++++------- .../0.1/test/test-amp-nested-menu.js | 126 ++++++++++++++++ .../{amp-drilldown => amp-nested-menu}/OWNERS | 0 .../amp-sidebar/{1.0 => 0.2}/amp-sidebar.css | 0 .../amp-sidebar/{1.0 => 0.2}/amp-sidebar.js | 35 +++-- .../amp-sidebar/{1.0 => 0.2}/amp-sidebar.md | 0 .../amp-sidebar/{1.0 => 0.2}/autoscroll.js | 0 .../{1.0 => 0.2}/swipe-to-dismiss.js | 0 .../{1.0 => 0.2}/test-e2e/test-amp-sidebar.js | 0 .../test/integration/test-amp-sidebar.js | 0 .../{1.0 => 0.2}/test/test-amp-sidebar.js | 10 +- .../{1.0 => 0.2}/test/test-toolbar.js | 0 .../amp-sidebar/{1.0 => 0.2}/toolbar.js | 0 extensions/amp-sidebar/{1.0 => 0.2}/utils.js | 0 src/sanitation.js | 8 +- .../amp-sidebar-nested-menus.amp.html | 32 ++-- tools/experiments/experiments-config.js | 2 + 19 files changed, 268 insertions(+), 107 deletions(-) rename extensions/{amp-drilldown/0.1/amp-drilldown.css => amp-nested-menu/0.1/amp-nested-menu.css} (80%) rename extensions/{amp-drilldown/0.1/amp-drilldown.js => amp-nested-menu/0.1/amp-nested-menu.js} (56%) create mode 100644 extensions/amp-nested-menu/0.1/test/test-amp-nested-menu.js rename extensions/{amp-drilldown => amp-nested-menu}/OWNERS (100%) rename extensions/amp-sidebar/{1.0 => 0.2}/amp-sidebar.css (100%) rename extensions/amp-sidebar/{1.0 => 0.2}/amp-sidebar.js (95%) rename extensions/amp-sidebar/{1.0 => 0.2}/amp-sidebar.md (100%) rename extensions/amp-sidebar/{1.0 => 0.2}/autoscroll.js (100%) rename extensions/amp-sidebar/{1.0 => 0.2}/swipe-to-dismiss.js (100%) rename extensions/amp-sidebar/{1.0 => 0.2}/test-e2e/test-amp-sidebar.js (100%) rename extensions/amp-sidebar/{1.0 => 0.2}/test/integration/test-amp-sidebar.js (100%) rename extensions/amp-sidebar/{1.0 => 0.2}/test/test-amp-sidebar.js (99%) rename extensions/amp-sidebar/{1.0 => 0.2}/test/test-toolbar.js (100%) rename extensions/amp-sidebar/{1.0 => 0.2}/toolbar.js (100%) rename extensions/amp-sidebar/{1.0 => 0.2}/utils.js (100%) diff --git a/build-system/compile/bundles.config.js b/build-system/compile/bundles.config.js index a707685616bb..0f27010adcc9 100644 --- a/build-system/compile/bundles.config.js +++ b/build-system/compile/bundles.config.js @@ -730,7 +730,7 @@ exports.extensionBundles = [ type: TYPES.MISC, }, { - name: 'amp-drilldown', + name: 'amp-nested-menu', version: '0.1', latestVersion: '0.1', options: {hasCss: true}, @@ -808,7 +808,7 @@ exports.extensionBundles = [ }, { name: 'amp-sidebar', - version: ['0.1', '1.0'], + version: ['0.1', '0.2'], latestVersion: '0.1', options: {hasCss: true}, type: TYPES.MISC, diff --git a/extensions/amp-drilldown/0.1/amp-drilldown.css b/extensions/amp-nested-menu/0.1/amp-nested-menu.css similarity index 80% rename from extensions/amp-drilldown/0.1/amp-drilldown.css rename to extensions/amp-nested-menu/0.1/amp-nested-menu.css index 2543791da5b3..38fe88b01e1f 100644 --- a/extensions/amp-drilldown/0.1/amp-drilldown.css +++ b/extensions/amp-nested-menu/0.1/amp-nested-menu.css @@ -14,8 +14,8 @@ * limitations under the License. */ -amp-drilldown, -amp-drilldown [amp-drilldown-submenu] { +amp-nested-menu, +amp-nested-menu [amp-nested-submenu] { position: absolute !important; top: 0 !important; width: 100% !important; @@ -23,26 +23,26 @@ amp-drilldown [amp-drilldown-submenu] { transform: translateX(0) !important; } -amp-drilldown.i-amphtml-layout-size-defined { +amp-nested-menu.i-amphtml-layout-size-defined { overflow: visible !important; } -amp-drilldown [amp-drilldown-submenu] { +amp-nested-menu [amp-nested-submenu] { left: 100% !important; opacity: 0 !important; visibility: hidden !important; transition: transform 233ms, opacity 233ms, visibility 0s 233ms; } -amp-drilldown, -amp-drilldown [amp-drilldown-submenu][open] { +amp-nested-menu, +amp-nested-menu [amp-nested-submenu][open] { opacity: 1 !important; visibility: visible !important; transition: transform 233ms; } -amp-drilldown[child-open], -amp-drilldown [amp-drilldown-submenu][child-open] { +amp-nested-menu[child-open], +amp-nested-menu [amp-nested-submenu][child-open] { transform: translateX(-100%) !important; visibility: hidden !important; transition: transform 233ms, visibility 0s 233ms; diff --git a/extensions/amp-drilldown/0.1/amp-drilldown.js b/extensions/amp-nested-menu/0.1/amp-nested-menu.js similarity index 56% rename from extensions/amp-drilldown/0.1/amp-drilldown.js rename to extensions/amp-nested-menu/0.1/amp-nested-menu.js index 705ffb4b511b..bda671cd7d75 100644 --- a/extensions/amp-drilldown/0.1/amp-drilldown.js +++ b/extensions/amp-nested-menu/0.1/amp-nested-menu.js @@ -14,18 +14,25 @@ * limitations under the License. */ -import {CSS} from '../../../build/amp-drilldown-0.1.css'; +import {CSS} from '../../../build/amp-nested-menu-0.1.css'; import {Keys} from '../../../src/utils/key-codes'; import {Layout} from '../../../src/layout'; import {Services} from '../../../src/services'; -import {closestAncestorElementBySelector} from '../../../src/dom'; +import { + closestAncestorElementBySelector, + scopedQuerySelector, + tryFocus, +} from '../../../src/dom'; import {dev, userAssert} from '../../../src/log'; import {isExperimentOn} from '../../../src/experiments'; +import {toArray} from '../../../src/types'; -const TAG = 'amp-drilldown'; +const TAG = 'amp-nested-menu'; -// TODO(#24668): add unit tests for click handlers and history. -export class AmpDrilldown extends AMP.BaseElement { +/** @private @const {number} */ +const ANIMATION_TIMEOUT = 350; + +export class AmpNestedMenu extends AMP.BaseElement { /** @param {!AmpElement} element */ constructor(element) { super(element); @@ -36,12 +43,21 @@ export class AmpDrilldown extends AMP.BaseElement { /** @private @const {!Element} */ this.documentElement_ = this.document_.documentElement; - /** @private {Array} */ - this.historyIds_ = []; + /** @private {?../../../src/service/action-impl.ActionService} */ + this.action_ = null; /** @private {?Element} */ this.currentSubmenu_ = null; + /** @private {?Array} */ + this.submenuElements_ = null; + + /** @private {?Array} */ + this.submenuOpenElements_ = null; + + /** @private {?Array} */ + this.submenuCloseElements_ = null; + /** @private {function(!Event)} */ this.submenuOpenHandler_ = this.handleSubmenuOpenClick_.bind(this); @@ -54,10 +70,39 @@ export class AmpDrilldown extends AMP.BaseElement { /** @override */ buildCallback() { + // TODO(#25022): remove this assert when cleaning up experiment post launch. userAssert( isExperimentOn(this.win, 'amp-sidebar-v2'), - 'Turning on the amp-sidebar-v2 experiment is necessary to use the amp-drilldown component.' + `Turning on the amp-sidebar-v2 experiment is necessary to use the ${TAG} component.` + ); + + this.action_ = Services.actionServiceForDoc(this.element); + + this.submenuElements_ = toArray( + this.element.querySelectorAll('[amp-nested-submenu]') + ); + this.submenuElements_.forEach(submenu => { + // Make submenus programmatically focusable for a11y. + submenu.tabIndex = -1; + }); + this.submenuOpenElements_ = toArray( + this.element.querySelectorAll('[amp-nested-submenu-open]') + ); + this.submenuCloseElements_ = toArray( + this.element.querySelectorAll('[amp-nested-submenu-close]') ); + this.submenuOpenElements_ + .concat(this.submenuCloseElements_) + .forEach(element => { + if (!element.hasAttribute('tabindex')) { + element.setAttribute('tabindex', 0); + } + element.setAttribute('role', 'button'); + userAssert( + this.action_.hasAction(element, 'tap') == false, + 'submenu open/close buttons should not have tap actions registered.' + ); + }); } /** @override */ @@ -81,16 +126,10 @@ export class AmpDrilldown extends AMP.BaseElement { * Add event listeners on submenu open/close elements. */ registerEventListeners_() { - const openElements = this.element.querySelectorAll( - '[amp-drilldown-submenu-open]' - ); - openElements.forEach(element => { + this.submenuOpenElements_.forEach(element => { element.addEventListener('click', this.submenuOpenHandler_); }); - const closeElements = this.element.querySelectorAll( - '[amp-drilldown-submenu-close]' - ); - closeElements.forEach(element => { + this.submenuCloseElements_.forEach(element => { element.addEventListener('click', this.submenuCloseHandler_); }); @@ -101,16 +140,10 @@ export class AmpDrilldown extends AMP.BaseElement { * Remove listeners on all submenu open/close elements. */ unregisterEventListeners_() { - const openElements = this.element.querySelectorAll( - '[amp-drilldown-submenu-open]' - ); - openElements.forEach(element => { + this.submenuOpenElements_.forEach(element => { element.removeEventListener('click', this.submenuOpenHandler_); }); - const closeElements = this.element.querySelectorAll( - '[amp-drilldown-submenu-close]' - ); - closeElements.forEach(element => { + this.submenuCloseElements_.forEach(element => { element.removeEventListener('click', this.submenuCloseHandler_); }); @@ -122,10 +155,8 @@ export class AmpDrilldown extends AMP.BaseElement { * @param {Event} e */ handleSubmenuOpenClick_(e) { - const submenuOpen = dev().assertElement(e.target); - const submenu = submenuOpen.parentElement.querySelector( - '[amp-drilldown-submenu]' - ); + const submenuParent = dev().assertElement(e.currentTarget.parentElement); + const submenu = scopedQuerySelector(submenuParent, '>[amp-nested-submenu]'); if (submenu) { this.open_(submenu); } @@ -139,16 +170,18 @@ export class AmpDrilldown extends AMP.BaseElement { if (submenu.hasAttribute('open')) { return; } - const submenuParent = this.getParentMenu_(submenu); - if (submenuParent) { - submenuParent.setAttribute('child-open', ''); + const parentMenu = this.getParentMenu_(submenu); + if (parentMenu) { + parentMenu.setAttribute('child-open', ''); submenu.setAttribute('open', ''); - this.getHistory_() - .push(() => this.close_(submenu)) - .then(historyId => { - this.historyIds_.push(historyId); - }); this.currentSubmenu_ = submenu; + // move focus to close element after submenu fully opens. + Services.timerFor(this.win).delay(() => { + const submenuClose = dev().assertElement( + submenu.querySelector('[amp-nested-submenu-close]') + ); + tryFocus(submenuClose); + }, ANIMATION_TIMEOUT); } } @@ -157,10 +190,10 @@ export class AmpDrilldown extends AMP.BaseElement { * @param {Event} e */ handleSubmenuCloseClick_(e) { - const submenuClose = dev().assertElement(e.target); + const submenuClose = dev().assertElement(e.currentTarget); const submenu = closestAncestorElementBySelector( submenuClose, - '[amp-drilldown-submenu]' + '[amp-nested-submenu]' ); if (submenu) { this.close_(submenu); @@ -175,15 +208,19 @@ export class AmpDrilldown extends AMP.BaseElement { if (!submenu.hasAttribute('open')) { return; } - const submenuParent = this.getParentMenu_(submenu); - if (submenuParent) { - submenuParent.removeAttribute('child-open'); + const parentMenu = this.getParentMenu_(submenu); + if (parentMenu) { + parentMenu.removeAttribute('child-open'); submenu.removeAttribute('open'); - if (this.historyIds_.length > 0) { - const lastHistoryId = this.historyIds_.pop(); - this.getHistory_().pop(lastHistoryId); - } - this.currentSubmenu_ = submenuParent; + this.currentSubmenu_ = parentMenu; + // move focus back to open element after submenu fully closes. + Services.timerFor(this.win).delay(() => { + const submenuParent = dev().assertElement(submenu.parentElement); + const submenuOpen = dev().assertElement( + scopedQuerySelector(submenuParent, '>[amp-nested-submenu-open]') + ); + tryFocus(submenuOpen); + }, ANIMATION_TIMEOUT); } } @@ -217,20 +254,11 @@ export class AmpDrilldown extends AMP.BaseElement { getParentMenu_(submenu) { return closestAncestorElementBySelector( dev().assertElement(submenu.parentElement), - 'amp-drilldown,[amp-drilldown-submenu]' + 'amp-nested-menu,[amp-nested-submenu]' ); } - - /** - * Returns the history for the ampdoc. - * @return {!../../../src/service/history-impl.History} - * @private - */ - getHistory_() { - return Services.historyForDoc(this.getAmpDoc()); - } } AMP.extension(TAG, '0.1', AMP => { - AMP.registerElement(TAG, AmpDrilldown, CSS); + AMP.registerElement(TAG, AmpNestedMenu, CSS); }); diff --git a/extensions/amp-nested-menu/0.1/test/test-amp-nested-menu.js b/extensions/amp-nested-menu/0.1/test/test-amp-nested-menu.js new file mode 100644 index 000000000000..c092884f41b9 --- /dev/null +++ b/extensions/amp-nested-menu/0.1/test/test-amp-nested-menu.js @@ -0,0 +1,126 @@ +/** + * Copyright 2019 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import '../amp-nested-menu'; +import * as lolex from 'lolex'; +import {Keys} from '../../../../src/utils/key-codes'; +import {htmlFor} from '../../../../src/static-template'; +import {toggleExperiment} from '../../../../src/experiments'; + +describes.realWin( + 'amp-nested-menu component', + { + amp: { + extensions: ['amp-nested-menu'], + }, + }, + env => { + let win, doc; + let clock; + + beforeEach(() => { + win = env.win; + doc = win.document; + clock = lolex.install({target: win}); + + // TODO(#25022): remove this toggle when cleaning up experiment post launch. + toggleExperiment(win, 'amp-sidebar-v2', true); + }); + + async function getNestedMenu() { + const element = doc.createElement('amp-nested-menu'); + element.setAttribute('layout', 'fill'); + const ul = doc.createElement('ul'); + + [1, 2, 3].forEach(i => { + const item = htmlFor(doc)` +
nested menu options: