From 5c6918120e5d3a5fa4e5b987d767663907c7f6ab Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Mon, 20 Mar 2017 10:41:52 -0500 Subject: [PATCH 01/19] Add Menu widget --- src/common/styles/widgets.css | 1 + src/menu/Menu.ts | 244 +++++++++++++++++++ src/menu/MenuItem.ts | 69 ++++++ src/menu/example/index.html | 26 ++ src/menu/example/index.ts | 171 +++++++++++++ src/menu/example/styles/app.css | 21 ++ src/menu/example/styles/app.css.d.ts | 2 + src/menu/example/styles/menu.css | 39 +++ src/menu/example/styles/menu.css.d.ts | 7 + src/menu/styles/menu.css | 39 +++ src/menu/styles/menu.css.d.ts | 13 + src/menu/tests/unit/Menu.ts | 336 ++++++++++++++++++++++++++ src/menu/tests/unit/MenuItem.ts | 126 ++++++++++ 13 files changed, 1094 insertions(+) create mode 100644 src/menu/Menu.ts create mode 100644 src/menu/MenuItem.ts create mode 100644 src/menu/example/index.html create mode 100644 src/menu/example/index.ts create mode 100644 src/menu/example/styles/app.css create mode 100644 src/menu/example/styles/app.css.d.ts create mode 100644 src/menu/example/styles/menu.css create mode 100644 src/menu/example/styles/menu.css.d.ts create mode 100644 src/menu/styles/menu.css create mode 100644 src/menu/styles/menu.css.d.ts create mode 100644 src/menu/tests/unit/Menu.ts create mode 100644 src/menu/tests/unit/MenuItem.ts diff --git a/src/common/styles/widgets.css b/src/common/styles/widgets.css index c04a49f80e..2c671158a8 100644 --- a/src/common/styles/widgets.css +++ b/src/common/styles/widgets.css @@ -10,3 +10,4 @@ @import '../../tabpane/styles/tabPane.m.css'; @import '../../textarea/styles/textarea.m.css'; @import '../../textinput/styles/textinput.m.css'; +@import '../../menu/styles/menu.m.css'; diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts new file mode 100644 index 0000000000..6f8d3b5fd6 --- /dev/null +++ b/src/menu/Menu.ts @@ -0,0 +1,244 @@ +import { assign } from '@dojo/core/lang'; +import { createTimer } from '@dojo/core/util'; +import uuid from '@dojo/core/uuid'; +import { Handle } from '@dojo/interfaces/core'; +import { v, w } from '@dojo/widget-core/d'; +import { DNode } from '@dojo/widget-core/interfaces'; +import ThemeableMixin, { theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable'; +import WidgetBase from '@dojo/widget-core/WidgetBase'; +import MenuItem, { MenuItemProperties } from './MenuItem'; +import * as css from './styles/menu.css'; + +export type Role = 'menu' | 'menubar'; + +export interface MenuProperties extends ThemeableProperties { + animate?: boolean; + disabled?: boolean; + expandOnClick?: boolean; + hideDelay?: number; + hidden?: boolean; + id?: string; + label?: string | MenuItemProperties; + nested?: boolean; + onRequestHide?: () => void; + onRequestShow?: () => void; + role?: Role; +} + +function getMenuHeight(menuElement: HTMLElement): number { + const maxHeight = parseInt(getComputedStyle(menuElement).getPropertyValue('max-height'), 10); + return Math.min(menuElement.scrollHeight, (isNaN(maxHeight) ? Infinity : maxHeight)); +} + +function addTransitionEndListener(element: HTMLElement, callback: (element: HTMLElement) => void) { + const listener = () => { + element.removeEventListener('transitionend', listener); + callback(element); + }; + element.addEventListener('transitionend', listener); +} + +@theme(css) +export class Menu extends ThemeableMixin(WidgetBase) { + protected _wasOpen = false; + private _hideTimer: Handle; + + onLabelClick() { + const { + disabled, + expandOnClick = true + } = this.properties; + + if (!disabled && expandOnClick) { + this._toggleDisplay(); + } + } + + onMenuMouseEnter() { + const { + disabled, + expandOnClick = true + } = this.properties; + + if (!disabled && !expandOnClick) { + this._hideTimer && this._hideTimer.destroy(); + this._toggleDisplay(true); + } + } + + onMenuMouseLeave() { + const { + disabled, + expandOnClick = true, + hideDelay = 300 + } = this.properties; + + if (!disabled && !expandOnClick) { + this._hideTimer && this._hideTimer.destroy(); + if (hideDelay) { + this._hideTimer = createTimer(() => { + this._toggleDisplay(false); + }, hideDelay); + this.own(this._hideTimer); + } + else { + this._toggleDisplay(false); + } + } + } + + render(): DNode { + const { + hidden = this._getDefaultHidden(), + id = uuid(), + nested, + role = 'menu' + } = this.properties; + + const label = this.renderLabel(id); + const menu = v('nav', { + id, + role, + classes: this.classes.apply(this, this._getMenuClasses(hidden, Boolean(label))), + afterUpdate: this._afterRender + }, this.children); + + if (label) { + return v('div', { + classes: this.classes(css.container, nested ? css.nestedMenuContainer : null), + onmouseenter: this.onMenuMouseEnter, + onmouseleave: this.onMenuMouseLeave + }, [ label, menu ]); + } + + return menu; + } + + renderLabel(id: string): DNode | void { + const { disabled, label, overrideClasses } = this.properties; + + if (label) { + const properties = typeof label === 'string' ? { label } : label; + return w(MenuItem, assign({ + disabled, + getAriaProperties: this._getLabelAriaProperties.bind(this, id), + hasMenu: true, + overrideClasses: overrideClasses || css, + onClick: this.onLabelClick + }, properties)); + } + } + + protected _afterRender(element: HTMLElement) { + const { animate = true, hidden = this._getDefaultHidden(), label } = this.properties; + + if (!label) { + return; + } + + if (!animate) { + element.style.height = null; + + if (hidden) { + element.classList.remove(css.afterVisible); + element.classList.add(css.afterHidden); + } + else { + element.classList.remove(css.afterHidden); + element.classList.add(css.afterVisible); + } + + return; + } + + this._animate(element, hidden); + } + + protected _animate(element: HTMLElement, hidden: boolean) { + if (hidden && !this._wasOpen) { + return; + } + + requestAnimationFrame(() => { + const height = getMenuHeight(element); + this._wasOpen = !hidden; + + if (hidden) { + const transition = element.style.transition; + element.style.transition = null; + + element.style.height = height + 'px'; + element.style.transition = transition; + requestAnimationFrame(() => { + addTransitionEndListener(element, (element: HTMLElement) => { + element.classList.add(css.afterHidden); + }); + + element.style.height = '0'; + }); + } + else { + addTransitionEndListener(element, (element: HTMLElement) => { + element.style.height = 'auto'; + element.classList.add(css.afterVisible); + }); + + element.style.height = `${height}px`; + } + }); + } + + protected _getDefaultHidden() { + const { disabled, label } = this.properties; + + if (label && disabled) { + return true; + } + + return label ? true : false; + } + + protected _getLabelAriaProperties(id: string): { [key: string]: string; } { + const { hidden = this._getDefaultHidden() } = this.properties; + return { + 'aria-controls': id, + 'aria-expanded': String(!hidden) + }; + } + + protected _getMenuClasses(hidden: boolean, isSubMenu: boolean) { + const { nested } = this.properties; + const classes = [ css.menu, hidden ? css.hidden : css.visible ]; + + if (nested) { + classes.push(css.nestedMenu); + } + + if (isSubMenu) { + classes.push(css.subMenu); + } + + return classes; + } + + protected _toggleDisplay(isHidden?: boolean) { + const { + onRequestHide, + onRequestShow + } = this.properties; + + if (typeof isHidden === 'undefined') { + const { hidden = this._getDefaultHidden() } = this.properties; + isHidden = hidden; + } + + if (isHidden) { + typeof onRequestShow === 'function' && onRequestShow(); + } + else { + typeof onRequestHide === 'function' && onRequestHide(); + } + } +} + +export default Menu; diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts new file mode 100644 index 0000000000..9cc9b0fdda --- /dev/null +++ b/src/menu/MenuItem.ts @@ -0,0 +1,69 @@ +import { assign } from '@dojo/core/lang'; +import { HNode } from '@dojo/widget-core/interfaces'; +import { v } from '@dojo/widget-core/d'; +import ThemeableMixin, { theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable'; +import WidgetBase from '@dojo/widget-core/WidgetBase'; +import * as css from './styles/menu.css'; + +export interface MenuItemProperties extends ThemeableProperties { + disabled?: boolean; + external?: boolean; + getAriaProperties?: () => { [key: string]: string; }; + hasMenu?: boolean; + label?: string; + onClick?: () => void; + selected?: boolean; + tabIndex?: number; + url?: string; +} + +@theme(css) +export class MenuItem extends ThemeableMixin(WidgetBase) { + onClick() { + const { disabled, onClick } = this.properties; + if (!disabled && typeof onClick === 'function') { + onClick(); + } + } + + render() { + const { + disabled, + external, + hasMenu = false, + getAriaProperties, + label, + selected, + tabIndex = 0, + url + } = this.properties; + + const ariaProperties = getAriaProperties && getAriaProperties() || Object.create(null); + const classes = this.classes( + hasMenu ? css.menuLabel : css.menuItem, + disabled ? css.disabled : null, + selected ? css.selected : null + ); + + const labelNode = v('a', assign(ariaProperties, { + 'aria-disabled': disabled, + classes, + href: url || undefined, + onclick: this.onClick, + role: 'menuitem', + tabIndex : disabled ? -1 : tabIndex, + target: url && external ? '_blank' : undefined + }), label ? [ label ] : undefined); + + if (this.children.length) { + const children = this.children as HNode[]; + return v('span', { + classes: this.classes(css.menuItem) + }, [ labelNode ].concat(children)); + } + + return labelNode; + } +} + +export default MenuItem; diff --git a/src/menu/example/index.html b/src/menu/example/index.html new file mode 100644 index 0000000000..09d6d7ab38 --- /dev/null +++ b/src/menu/example/index.html @@ -0,0 +1,26 @@ + + + + Menu Example + + + + + + + + + diff --git a/src/menu/example/index.ts b/src/menu/example/index.ts new file mode 100644 index 0000000000..d6665eab85 --- /dev/null +++ b/src/menu/example/index.ts @@ -0,0 +1,171 @@ +import { WidgetBase } from '@dojo/widget-core/WidgetBase'; +import { WidgetProperties } from '@dojo/widget-core/interfaces'; +import { ProjectorMixin } from '@dojo/widget-core/mixins/Projector'; +import { StatefulMixin } from '@dojo/widget-core/mixins/Stateful'; +import ThemeableMixin, { theme } from '@dojo/widget-core/mixins/Themeable'; +import { v, w } from '@dojo/widget-core/d'; +import Menu from '../Menu'; +import MenuItem from '../MenuItem'; +import * as appCss from './styles/app.css'; +import * as menuCss from './styles/menu.css'; + +const AppBase = StatefulMixin(ThemeableMixin(WidgetBase)); + +@theme(appCss) +export class App extends AppBase { + toggleDisabled(event: Event) { + this.setState({ disabled: ( event.target).checked }); + } + + toggleEvent(event: Event) { + this.setState({ + expandOnClick: !( event.target).checked, + packageMenuHidden: true + }); + } + + toggleSelected(key: string) { + const selectedKey = `${key}Selected`; + this.setState({ [selectedKey]: !this.state[selectedKey] }); + } + + render() { + return v('div', [ + v('form', { classes: this.classes(appCss.options) }, [ + v('div', [ + v('input', { + id: 'toggleEvent', + type: 'checkbox', + onchange: this.toggleEvent + }), + v('label', { + for: 'toggleEvent' + }, [ 'Show on hover' ]) + ]), + + v('div', [ + v('input', { + id: 'toggleDisabled', + type: 'checkbox', + onchange: this.toggleDisabled + }), + v('label', { + for: 'toggleDisabled' + }, [ 'Disable packages menu' ]) + ]) + ]), + + v('div', { + classes: this.classes(appCss.demos) + }, [ + this.renderFileMenu(), + this.renderDojoMenu() + ]) + ]); + } + + renderDojoMenu() { + const packages = [ + 'cli', + 'compose', + 'core', + 'has', + 'interfaces', + 'i18n', + 'loader', + 'routing', + 'shim', + 'stores', + 'streams', + 'widget-core', + 'widgets' + ]; + + const { + disabled, + expandOnClick, + packageMenuHidden + } = this.state; + + return w(Menu, { + key: 'DojoMenu', + overrideClasses: menuCss + }, [ + w(MenuItem, { + key: 'DojoMenuLabel', + label: 'Dojo 2', + overrideClasses: menuCss, + url: 'http://dojo.io', + external: true + }), + + w(Menu, { + disabled: disabled, + expandOnClick: expandOnClick, + hidden: packageMenuHidden, + key: 'menu1-sub1', + label: 'Dojo 2 Packages', + nested: true, + onRequestHide: () => { + this.setState({ packageMenuHidden: true }); + }, + onRequestShow: () => { + this.setState({ packageMenuHidden: false }); + }, + overrideClasses: menuCss + }, packages.map((label, i) => { + return w(MenuItem, { + label, + key: `menu1-sub1-item${i}`, + overrideClasses: menuCss, + url: `https://github.com/dojo/${label}`, + external: true + }); + })) + ]); + } + + renderFileMenu() { + function getKey(name: string) { + return name.charAt(0).toLowerCase() + name.replace(/\s/g, '').slice(1); + } + + return w(Menu, { + key: 'ChromeFileMenu', + overrideClasses: menuCss + }, [ + [ + 'New Tab', + 'New Window', + 'New Incognito Window', + 'Reopen Closed Tab', + 'Open File...', + 'Open Location...' + ], + [ + 'Close Window', + 'Close Tab', + 'Save Page As...' + ], + [ 'Email Page Location' ], + [ 'Print' ] + ].map(group => { + return v('div', group.map(name => { + const key = getKey(name); + return w(MenuItem, { + key, + label: name, + overrideClasses: menuCss, + disabled: name === 'Open Location...', + onRequestSelect: this.toggleSelected.bind(this, key), + selected: this.state[`${key}Selected`] + }); + })); + })); + } +} + +const Projector = ProjectorMixin(App); +const projector = new Projector(); + +projector.append(); diff --git a/src/menu/example/styles/app.css b/src/menu/example/styles/app.css new file mode 100644 index 0000000000..b5d6dbf38b --- /dev/null +++ b/src/menu/example/styles/app.css @@ -0,0 +1,21 @@ +.demos > * { + display: block; + margin-bottom: 25px; +} + +.options { + margin-bottom: 1.5em; +} + +.options > div { + display: inline-block; +} + +@media screen and (min-width: 500px) { + .demos > * { + float: left; + } + .demos > :first-child { + margin-right: 50px; + } +} diff --git a/src/menu/example/styles/app.css.d.ts b/src/menu/example/styles/app.css.d.ts new file mode 100644 index 0000000000..934f8a61e0 --- /dev/null +++ b/src/menu/example/styles/app.css.d.ts @@ -0,0 +1,2 @@ +export const demos: string; +export const options: string; diff --git a/src/menu/example/styles/menu.css b/src/menu/example/styles/menu.css new file mode 100644 index 0000000000..d43ff3947e --- /dev/null +++ b/src/menu/example/styles/menu.css @@ -0,0 +1,39 @@ +@import '../../../common/styles/variables.css'; + +.menu { + background: #fff; + box-shadow: 0 0 2px 1px rgba(0,0,0,0.3); + font-family: sans-serif; + font-size: 14px; + padding: 5px 0; +} + +.menuItem { + color: var(--title-bar-bg); + padding: 5px 10px; +} + +.menuLabel { + composes: menuItem; +} + +.disabled { + opacity: 0.3; +} + +.selected { + background-color: var(--title-bar-bg); + color: #fff; +} + +.subMenu { + box-shadow: 0 0; + box-sizing: border-box; + padding: 0 0 0 15px; +} + +.dropDown { + border: 1px solid rgba(0,0,0,0.3); + left: 100%; + top: 0; +} diff --git a/src/menu/example/styles/menu.css.d.ts b/src/menu/example/styles/menu.css.d.ts new file mode 100644 index 0000000000..ecf16eee45 --- /dev/null +++ b/src/menu/example/styles/menu.css.d.ts @@ -0,0 +1,7 @@ +export const menu: string; +export const menuItem: string; +export const menuLabel: string; +export const disabled: string; +export const selected: string; +export const subMenu: string; +export const dropDown: string; diff --git a/src/menu/styles/menu.css b/src/menu/styles/menu.css new file mode 100644 index 0000000000..d0b1be4c98 --- /dev/null +++ b/src/menu/styles/menu.css @@ -0,0 +1,39 @@ +.container { + box-sizing: border-box; + position: relative; +} + +.menu {} +.hidden {} +.visible {} +.afterHidden {} +.afterVisible {} + +.menuItem { + cursor: pointer; + display: block; +} + +.menuLabel { + composes: menuItem; +} + +.disabled { + cursor: default; +} + +/** + * Selected menu items. + */ +.selected {} + +.nestedMenuContainer, +.nestedMenu { + width: 100%; +} + +.subMenu { + height: 0; + overflow: hidden; + transition: height 0.5s ease-in-out; +} diff --git a/src/menu/styles/menu.css.d.ts b/src/menu/styles/menu.css.d.ts new file mode 100644 index 0000000000..2913357c9b --- /dev/null +++ b/src/menu/styles/menu.css.d.ts @@ -0,0 +1,13 @@ +export const container: string; +export const menu: string; +export const hidden: string; +export const visible: string; +export const afterHidden: string; +export const afterVisible: string; +export const menuItem: string; +export const menuLabel: string; +export const disabled: string; +export const selected: string; +export const nestedMenuContainer: string; +export const nestedMenu: string; +export const subMenu: string; diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts new file mode 100644 index 0000000000..b1964ed4e9 --- /dev/null +++ b/src/menu/tests/unit/Menu.ts @@ -0,0 +1,336 @@ +import * as registerSuite from 'intern!object'; +import * as assert from 'intern/chai!assert'; +import { VNode } from '@dojo/interfaces/vdom'; +import Menu from '../../Menu'; +import * as baseCss from '../../../common/styles/base.css'; +import * as css from '../../styles/menu.css'; + +registerSuite({ + name: 'Menu', + + 'Should construct menu with passed properties'() { + const menu = new Menu(); + menu.setProperties({ + key: 'foo', + disabled: false, + expandOnClick: false, + hidden: true + }); + + assert.strictEqual(menu.properties.key, 'foo'); + assert.isFalse(menu.properties.disabled); + assert.isFalse(menu.properties.expandOnClick); + assert.isTrue(menu.properties.hidden); + }, + + label: { + 'renders the menu within a container'() { + const menu = new Menu(); + menu.setProperties({ + hidden: false, + label: 'Menu label' + }); + const vnode = menu.__render__(); + + assert.strictEqual(vnode.vnodeSelector, 'div', 'container node should be a div'); + assert.lengthOf(vnode.children, 2); + }, + + 'renders the label as a link to the container'() { + const menu = new Menu(); + menu.setProperties({ + label: 'Menu label' + }); + const vnode = menu.__render__(); + const label = vnode.children![0]; + + assert.strictEqual(label.vnodeSelector, 'a', 'label node should be a link'); + assert.strictEqual(label.text, 'Menu label', 'label node should have the label text'); + }, + + 'does not use a container if there is no label'() { + const menu = new Menu(); + menu.setChildren([ 'first', 'second', 'third' ]); + const vnode = menu.__render__(); + + assert.strictEqual(vnode.vnodeSelector, 'nav', 'menu node should be a nav'); + assert.lengthOf(vnode.children, 3, 'menu node should have all children'); + }, + + 'allows label properties instead of a label string'() { + const menu = new Menu(); + menu.setProperties({ + label: { label: 'Menu label' } + }); + const vnode = menu.__render__(); + const label = vnode.children![0]; + + assert.strictEqual(label.text, 'Menu label', 'label node should have the label text'); + } + }, + + onRequestShow() { + const menu = new Menu(); + menu.setProperties({ + label: 'Menu label', + onRequestShow() { + menu.setProperties({ hidden: false }); + } + }); + menu.onLabelClick(); + + assert.isFalse(menu.properties.hidden, 'menu should not be hidden'); + }, + + onRequestHide() { + const menu = new Menu(); + menu.setProperties({ + hidden: false, + label: 'Menu label', + onRequestHide() { + menu.setProperties({ hidden: true }); + } + }); + menu.onLabelClick(); + + assert.isTrue(menu.properties.hidden, 'menu should be hidden'); + }, + + onLabelClick() { + const menu = new Menu(); + menu.setProperties({ + expandOnClick: false, + hidden: true, + onRequestShow() { + menu.setProperties({ hidden: false }); + } + }); + menu.onLabelClick(); + + assert.isTrue(menu.properties.hidden, 'menu should not be shown on click when `expandOnClick` is false'); + }, + + onMenuMouseEnter: { + 'when `expandOnClick` is true'() { + const menu = new Menu(); + menu.setProperties({ + hidden: true, + onRequestShow() { + menu.setProperties({ hidden: false }); + } + }); + menu.onMenuMouseEnter(); + + assert.isTrue(menu.properties.hidden, 'mouseenter should be ignored'); + }, + + 'when `expandOnClick` is false'() { + const menu = new Menu(); + menu.setProperties({ + expandOnClick: false, + hidden: true, + onRequestShow() { + menu.setProperties({ hidden: false }); + } + }); + + menu.onMenuMouseEnter(); + assert.isFalse(menu.properties.hidden, 'mouseenter should not be ignored'); + } + }, + + onMenuMouseLeave: { + 'when `expandOnClick` is true'() { + const menu = new Menu(); + menu.setProperties({ + hidden: false, + hideDelay: 0, + onRequestHide() { + menu.setProperties({ hidden: true }); + } + }); + menu.onMenuMouseLeave(); + + assert.isFalse(menu.properties.hidden, 'mouseleave should be ignored'); + }, + + 'when `expandOnClick` is false'() { + const menu = new Menu(); + menu.setProperties({ + expandOnClick: false, + hidden: false, + hideDelay: 0, + onRequestHide() { + menu.setProperties({ hidden: true }); + } + }); + + menu.onMenuMouseLeave(); + assert.isTrue(menu.properties.hidden, 'mouseleave should not be ignored'); + } + }, + + disabled() { + const menu = new Menu(); + menu.setProperties({ + disabled: true, + label: 'Menu label', + onRequestShow() { + menu.setProperties({ hidden: false }); + } + }); + menu.__render__(); + menu.onLabelClick(); + + assert.isUndefined(menu.properties.hidden, 'menu should not be displayed when disabled'); + }, + + hidden: { + 'hidden by default with a label'() { + const menu = new Menu(); + menu.setProperties({ label: 'Menu label' }); + const vnode: any = menu.__render__(); + + assert.lengthOf(vnode.children, 1, 'menu not rendered'); + }, + + 'displayed by default with a label'() { + const menu = new Menu(); + const vnode: any = menu.__render__(); + + assert.notOk(vnode.properties.classes[baseCss.visuallyHidden]); + }, + + 'can still be hidden without a label'() { + const menu = new Menu(); + menu.setProperties({ hidden: true }); + const vnode: any = menu.__render__(); + + assert.isTrue(vnode.properties.classes[baseCss.visuallyHidden]); + } + }, + + hideDelay: { + 'menu not hidden immediately'(this: any) { + const dfd = this.async(500); + const menu = new Menu(); + menu.setProperties({ + expandOnClick: false, + hidden: false, + hideDelay: 200, + onRequestHide() { + menu.setProperties({ hidden: true }); + } + }); + + menu.onMenuMouseLeave(); + assert.isFalse(menu.properties.hidden, 'menu should not be hidden immediately'); + setTimeout(dfd.callback(() => { + assert.isTrue(menu.properties.hidden, 'menu should be hidden after a delay'); + }), 200); + }, + + 'after show request'(this: any) { + const dfd = this.async(500); + const menu = new Menu(); + menu.setProperties({ + expandOnClick: false, + hidden: false, + hideDelay: 200, + onRequestHide() { + menu.setProperties({ hidden: true }); + } + }); + + menu.onMenuMouseLeave(); + menu.onMenuMouseEnter(); + setTimeout(dfd.callback(() => { + assert.isFalse(menu.properties.hidden, 'menu should not be hidden after show request'); + }), 200); + }, + + 'subsequent hides'(this: any) { + const dfd = this.async(500); + const menu = new Menu(); + let callCount = 0; + + menu.setProperties({ + expandOnClick: false, + hidden: false, + hideDelay: 200, + onRequestHide() { + callCount++; + menu.setProperties({ hidden: true }); + } + }); + + menu.onMenuMouseLeave(); + menu.onMenuMouseLeave(); + menu.onMenuMouseLeave(); + menu.onMenuMouseLeave(); + menu.onMenuMouseLeave(); + + setTimeout(dfd.callback(() => { + assert.strictEqual(callCount, 1, 'hide request should be called once'); + }), 200); + }, + + 'after destroy'(this: any) { + const dfd = this.async(500); + const menu = new Menu(); + menu.setProperties({ + expandOnClick: false, + hidden: false, + onRequestHide() { + menu.setProperties({ hidden: true }); + } + }); + + menu.onMenuMouseLeave(); + menu.destroy(); + setTimeout(dfd.callback(() => { + assert.isFalse(menu.properties.hidden, 'menu should not be hidden after the menu is destroyed'); + }), 300); + } + }, + + id() { + const menu = new Menu(); + let vnode: any = menu.__render__(); + + assert.isString(vnode.properties.id, 'id should be generated'); + + menu.setProperties({ id: 'menu-42' }); + vnode = menu.__render__(); + assert.strictEqual(vnode.properties.id, 'menu-42'); + }, + + role() { + const menu = new Menu(); + let vnode: any = menu.__render__(); + + assert.strictEqual(vnode.properties.role, 'menu', 'role should default to "menu"'); + + menu.setProperties({ role: 'menubar' }); + vnode = menu.__render__(); + assert.strictEqual(vnode.properties.role, 'menubar'); + }, + + nested: { + 'adds `nestedMenu` class to menu when there is no label'() { + const menu = new Menu(); + menu.setProperties({ nested: true }); + const vnode: any = menu.__render__(); + + assert.isTrue(vnode.properties.classes[css.nestedMenu]); + }, + + 'adds `nestedMenuContainer` class to container when there is a label'() { + const menu = new Menu(); + menu.setProperties({ label: 'Menu Label', nested: true }); + const vnode: any = menu.__render__(); + + assert.isTrue(vnode.properties.classes[css.nestedMenuContainer]); + } + } +}); diff --git a/src/menu/tests/unit/MenuItem.ts b/src/menu/tests/unit/MenuItem.ts new file mode 100644 index 0000000000..a59bab98ed --- /dev/null +++ b/src/menu/tests/unit/MenuItem.ts @@ -0,0 +1,126 @@ +import * as registerSuite from 'intern!object'; +import * as assert from 'intern/chai!assert'; +import MenuItem from '../../MenuItem'; +import * as css from '../../styles/menu.css'; + +registerSuite({ + name: 'MenuItem', + + 'Should construct menu item with passed properties'() { + const item = new MenuItem(); + item.setProperties({ + key: 'foo', + disabled: false, + external: true, + label: 'Label', + selected: false, + url: 'http://dojo.io' + }); + + assert.strictEqual(item.properties.key, 'foo'); + assert.isFalse(item.properties.disabled); + assert.isTrue(item.properties.external); + assert.strictEqual(item.properties.label, 'Label'); + assert.isFalse(item.properties.selected); + assert.strictEqual(item.properties.url, 'http://dojo.io'); + }, + + children: { + 'without children'() { + const item = new MenuItem(); + const vnode: any = item.__render__(); + + assert.strictEqual(vnode.vnodeSelector, 'a'); + assert.lengthOf(vnode.children, 0); + }, + + 'with children'() { + const item = new MenuItem(); + item.setChildren([ 'Child' ]); + const vnode: any = item.__render__(); + + assert.strictEqual(vnode.vnodeSelector, 'span'); + assert.isTrue(vnode.properties.classes[css.menuItem]); + assert.lengthOf(vnode.children, 2); + } + }, + + onClick: { + 'when disabled'() { + const item = new MenuItem(); + let called = false; + item.setProperties({ + disabled: true, + onClick() { + called = true; + } + }); + + item.onClick(); + assert.isFalse(called, '`onClick` should not be called when the menu item is disabled.'); + }, + + 'when not disabled'() { + const item = new MenuItem(); + let called = false; + item.setProperties({ + onClick() { + called = true; + } + }); + + item.onClick(); + assert.isTrue(called, '`onClick` should be called when the menu item is enabled.'); + } + }, + + disabled() { + const item = new MenuItem(); + let vnode: any = item.__render__(); + + assert.notOk(vnode.properties.classes[css.disabled]); + + item.setProperties({ disabled: true }); + vnode = item.__render__(); + assert.isTrue(vnode.properties.classes[css.disabled]); + }, + + external() { + const item = new MenuItem(); + item.setProperties({ external: true }); + let vnode: any = item.__render__(); + + assert.notOk(vnode.properties.target, 'target should not be set without a url'); + + item.setProperties({ external: true, url: 'http://dojo.io' }); + vnode = item.__render__(); + assert.strictEqual(vnode.properties.target, '_blank'); + }, + + label() { + const item = new MenuItem(); + item.setProperties({ label: 'Label' }); + const vnode: any = item.__render__(); + + assert.strictEqual(vnode.text, 'Label'); + }, + + selected() { + const item = new MenuItem(); + let vnode: any = item.__render__(); + + assert.notOk(vnode.properties.classes[css.selected]); + + item.setProperties({ selected: true }); + vnode = item.__render__(); + assert.isTrue(vnode.properties.classes[css.selected]); + }, + + url() { + const item = new MenuItem(); + item.setProperties({ url: 'http://dojo.io' }); + const vnode: any = item.__render__(); + + assert.strictEqual(vnode.properties.href, 'http://dojo.io'); + } +}); From de3ab26db867215360818da552909bfcaa59d050 Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Mon, 20 Mar 2017 11:39:55 -0500 Subject: [PATCH 02/19] Add keypress and focusin events to the menu widget --- src/menu/Menu.ts | 22 ++++++++++++++++++++-- src/menu/MenuItem.ts | 15 ++++++++++++--- src/menu/example/index.ts | 5 ++++- src/menu/example/styles/menu.css | 14 +++++++------- src/menu/example/styles/menu.css.d.ts | 1 - src/menu/tests/unit/MenuItem.ts | 4 ++-- 6 files changed, 45 insertions(+), 16 deletions(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index 6f8d3b5fd6..7f8f6c1695 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -54,6 +54,14 @@ export class Menu extends ThemeableMixin(WidgetBase) { } } + onLabelKeypress(event: KeyboardEvent) { + const { disabled } = this.properties; + + if (!disabled && event.key === 'Enter') { + this._toggleDisplay(); + } + } + onMenuMouseEnter() { const { disabled, @@ -87,6 +95,14 @@ export class Menu extends ThemeableMixin(WidgetBase) { } } + onMenuFocus() { + const { hidden = this._getDefaultHidden() } = this.properties; + if (hidden) { + const onRequestShow = this.properties.onRequestShow; + onRequestShow && onRequestShow(); + } + } + render(): DNode { const { hidden = this._getDefaultHidden(), @@ -100,7 +116,8 @@ export class Menu extends ThemeableMixin(WidgetBase) { id, role, classes: this.classes.apply(this, this._getMenuClasses(hidden, Boolean(label))), - afterUpdate: this._afterRender + afterUpdate: this._afterRender, + onfocusin: this.onMenuFocus }, this.children); if (label) { @@ -124,7 +141,8 @@ export class Menu extends ThemeableMixin(WidgetBase) { getAriaProperties: this._getLabelAriaProperties.bind(this, id), hasMenu: true, overrideClasses: overrideClasses || css, - onClick: this.onLabelClick + onClick: this.onLabelClick, + onKeypress: this.onLabelKeypress }, properties)); } } diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts index 9cc9b0fdda..0f79f0dc24 100644 --- a/src/menu/MenuItem.ts +++ b/src/menu/MenuItem.ts @@ -11,7 +11,8 @@ export interface MenuItemProperties extends ThemeableProperties { getAriaProperties?: () => { [key: string]: string; }; hasMenu?: boolean; label?: string; - onClick?: () => void; + onClick?: (event: MouseEvent) => void; + onKeypress?: (event: KeyboardEvent) => void; selected?: boolean; tabIndex?: number; url?: string; @@ -19,10 +20,17 @@ export interface MenuItemProperties extends ThemeableProperties { @theme(css) export class MenuItem extends ThemeableMixin(WidgetBase) { - onClick() { + onClick(event: MouseEvent) { const { disabled, onClick } = this.properties; if (!disabled && typeof onClick === 'function') { - onClick(); + onClick(event); + } + } + + onKeypress(event: KeyboardEvent) { + const { disabled, onKeypress } = this.properties; + if (!disabled && typeof onKeypress === 'function') { + onKeypress(event); } } @@ -50,6 +58,7 @@ export class MenuItem extends ThemeableMixin(WidgetBase) { classes, href: url || undefined, onclick: this.onClick, + onkeypress: this.onKeypress, role: 'menuitem', tabIndex : disabled ? -1 : tabIndex, target: url && external ? '_blank' : undefined diff --git a/src/menu/example/index.ts b/src/menu/example/index.ts index d6665eab85..2ed3fd0999 100644 --- a/src/menu/example/index.ts +++ b/src/menu/example/index.ts @@ -14,7 +14,10 @@ const AppBase = StatefulMixin(ThemeableMixin(WidgetBase)); @theme(appCss) export class App extends AppBase { toggleDisabled(event: Event) { - this.setState({ disabled: ( event.target).checked }); + this.setState({ + disabled: ( event.target).checked, + packageMenuHidden: true + }); } toggleEvent(event: Event) { diff --git a/src/menu/example/styles/menu.css b/src/menu/example/styles/menu.css index d43ff3947e..c77ad91879 100644 --- a/src/menu/example/styles/menu.css +++ b/src/menu/example/styles/menu.css @@ -13,6 +13,12 @@ padding: 5px 10px; } +.menuItem:focus, +.menuLabel:focus { + box-shadow: inset var(--input-focus-shadow); + outline: none; +} + .menuLabel { composes: menuItem; } @@ -29,11 +35,5 @@ .subMenu { box-shadow: 0 0; box-sizing: border-box; - padding: 0 0 0 15px; -} - -.dropDown { - border: 1px solid rgba(0,0,0,0.3); - left: 100%; - top: 0; + padding: 0 15px; } diff --git a/src/menu/example/styles/menu.css.d.ts b/src/menu/example/styles/menu.css.d.ts index ecf16eee45..4c83bfc878 100644 --- a/src/menu/example/styles/menu.css.d.ts +++ b/src/menu/example/styles/menu.css.d.ts @@ -4,4 +4,3 @@ export const menuLabel: string; export const disabled: string; export const selected: string; export const subMenu: string; -export const dropDown: string; diff --git a/src/menu/tests/unit/MenuItem.ts b/src/menu/tests/unit/MenuItem.ts index a59bab98ed..6da9773a3e 100644 --- a/src/menu/tests/unit/MenuItem.ts +++ b/src/menu/tests/unit/MenuItem.ts @@ -56,7 +56,7 @@ registerSuite({ } }); - item.onClick(); + item.onClick( {}); assert.isFalse(called, '`onClick` should not be called when the menu item is disabled.'); }, @@ -69,7 +69,7 @@ registerSuite({ } }); - item.onClick(); + item.onClick( {}); assert.isTrue(called, '`onClick` should be called when the menu item is enabled.'); } }, From aaa533d9649d56a6d89061bebee414bd84fbbcaf Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Mon, 20 Mar 2017 16:26:31 -0500 Subject: [PATCH 03/19] Fix animation when menu starts opened. --- src/common/tests/unit/all.ts | 2 ++ src/menu/Menu.ts | 54 +++++++++++++++-------------------- src/menu/example/index.ts | 17 +++++++++++ src/menu/styles/menu.css | 8 ++---- src/menu/styles/menu.css.d.ts | 6 ++-- src/menu/tests/unit/Menu.ts | 9 +++--- 6 files changed, 51 insertions(+), 45 deletions(-) diff --git a/src/common/tests/unit/all.ts b/src/common/tests/unit/all.ts index 912278cedb..b26d4df2ab 100644 --- a/src/common/tests/unit/all.ts +++ b/src/common/tests/unit/all.ts @@ -14,3 +14,5 @@ import '../../../textarea/tests/unit/Textarea'; import '../../../checkbox/tests/unit/Checkbox'; import '../../../radio/tests/unit/Radio'; import '../../../slider/tests/unit/Slider'; +import '../../../menu/tests/unit/Menu'; +import '../../../menu/tests/unit/MenuItem'; diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index 7f8f6c1695..f7e0129ad4 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -30,18 +30,11 @@ function getMenuHeight(menuElement: HTMLElement): number { return Math.min(menuElement.scrollHeight, (isNaN(maxHeight) ? Infinity : maxHeight)); } -function addTransitionEndListener(element: HTMLElement, callback: (element: HTMLElement) => void) { - const listener = () => { - element.removeEventListener('transitionend', listener); - callback(element); - }; - element.addEventListener('transitionend', listener); -} - @theme(css) export class Menu extends ThemeableMixin(WidgetBase) { protected _wasOpen = false; private _hideTimer: Handle; + private _initialRender = true; onLabelClick() { const { @@ -116,7 +109,8 @@ export class Menu extends ThemeableMixin(WidgetBase) { id, role, classes: this.classes.apply(this, this._getMenuClasses(hidden, Boolean(label))), - afterUpdate: this._afterRender, + afterCreate: this._afterCreate, + afterUpdate: this._afterUpdate, onfocusin: this.onMenuFocus }, this.children); @@ -147,7 +141,20 @@ export class Menu extends ThemeableMixin(WidgetBase) { } } - protected _afterRender(element: HTMLElement) { + protected _afterCreate(element: HTMLElement) { + const { animate = true } = this.properties; + if (animate && this._initialRender) { + const hidden = element.classList.contains(css.hidden); + this._wasOpen = !hidden; + + if (hidden) { + element.style.height = '0'; + } + } + this._initialRender = false; + } + + protected _afterUpdate(element: HTMLElement) { const { animate = true, hidden = this._getDefaultHidden(), label } = this.properties; if (!label) { @@ -156,16 +163,6 @@ export class Menu extends ThemeableMixin(WidgetBase) { if (!animate) { element.style.height = null; - - if (hidden) { - element.classList.remove(css.afterVisible); - element.classList.add(css.afterHidden); - } - else { - element.classList.remove(css.afterHidden); - element.classList.add(css.afterVisible); - } - return; } @@ -188,19 +185,10 @@ export class Menu extends ThemeableMixin(WidgetBase) { element.style.height = height + 'px'; element.style.transition = transition; requestAnimationFrame(() => { - addTransitionEndListener(element, (element: HTMLElement) => { - element.classList.add(css.afterHidden); - }); - element.style.height = '0'; }); } else { - addTransitionEndListener(element, (element: HTMLElement) => { - element.style.height = 'auto'; - element.classList.add(css.afterVisible); - }); - element.style.height = `${height}px`; } }); @@ -225,8 +213,12 @@ export class Menu extends ThemeableMixin(WidgetBase) { } protected _getMenuClasses(hidden: boolean, isSubMenu: boolean) { - const { nested } = this.properties; - const classes = [ css.menu, hidden ? css.hidden : css.visible ]; + const { animate = true, nested } = this.properties; + const classes = [ css.menu ]; + + if (this._initialRender || !animate || !isSubMenu) { + classes.push(hidden ? css.hidden : css.visible); + } if (nested) { classes.push(css.nestedMenu); diff --git a/src/menu/example/index.ts b/src/menu/example/index.ts index 2ed3fd0999..b7efa40170 100644 --- a/src/menu/example/index.ts +++ b/src/menu/example/index.ts @@ -13,6 +13,10 @@ const AppBase = StatefulMixin(ThemeableMixin(WidgetBase)); @theme(appCss) export class App extends AppBase { + toggleAnimation(event: Event) { + this.setState({ animate: !( event.target).checked }); + } + toggleDisabled(event: Event) { this.setState({ disabled: ( event.target).checked, @@ -55,6 +59,17 @@ export class App extends AppBase { v('label', { for: 'toggleDisabled' }, [ 'Disable packages menu' ]) + ]), + + v('div', [ + v('input', { + id: 'toggleAnimation', + type: 'checkbox', + onchange: this.toggleAnimation + }), + v('label', { + for: 'toggleAnimation' + }, [ 'Disable package menu animation' ]) ]) ]), @@ -85,6 +100,7 @@ export class App extends AppBase { ]; const { + animate = true, disabled, expandOnClick, packageMenuHidden @@ -103,6 +119,7 @@ export class App extends AppBase { }), w(Menu, { + animate: animate, disabled: disabled, expandOnClick: expandOnClick, hidden: packageMenuHidden, diff --git a/src/menu/styles/menu.css b/src/menu/styles/menu.css index d0b1be4c98..ec82a77493 100644 --- a/src/menu/styles/menu.css +++ b/src/menu/styles/menu.css @@ -4,10 +4,6 @@ } .menu {} -.hidden {} -.visible {} -.afterHidden {} -.afterVisible {} .menuItem { cursor: pointer; @@ -33,7 +29,9 @@ } .subMenu { - height: 0; overflow: hidden; transition: height 0.5s ease-in-out; } + +.hidden { height: 0; } +.visible { height: auto; } diff --git a/src/menu/styles/menu.css.d.ts b/src/menu/styles/menu.css.d.ts index 2913357c9b..0c976377e5 100644 --- a/src/menu/styles/menu.css.d.ts +++ b/src/menu/styles/menu.css.d.ts @@ -1,9 +1,5 @@ export const container: string; export const menu: string; -export const hidden: string; -export const visible: string; -export const afterHidden: string; -export const afterVisible: string; export const menuItem: string; export const menuLabel: string; export const disabled: string; @@ -11,3 +7,5 @@ export const selected: string; export const nestedMenuContainer: string; export const nestedMenu: string; export const subMenu: string; +export const hidden: string; +export const visible: string; diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index b1964ed4e9..2fb2809a77 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -2,7 +2,6 @@ import * as registerSuite from 'intern!object'; import * as assert from 'intern/chai!assert'; import { VNode } from '@dojo/interfaces/vdom'; import Menu from '../../Menu'; -import * as baseCss from '../../../common/styles/base.css'; import * as css from '../../styles/menu.css'; registerSuite({ @@ -191,14 +190,14 @@ registerSuite({ menu.setProperties({ label: 'Menu label' }); const vnode: any = menu.__render__(); - assert.lengthOf(vnode.children, 1, 'menu not rendered'); + assert.isTrue(vnode.children[1].properties.classes[css.hidden]); }, - 'displayed by default with a label'() { + 'displayed by default without a label'() { const menu = new Menu(); const vnode: any = menu.__render__(); - assert.notOk(vnode.properties.classes[baseCss.visuallyHidden]); + assert.notOk(vnode.properties.classes[css.hidden]); }, 'can still be hidden without a label'() { @@ -206,7 +205,7 @@ registerSuite({ menu.setProperties({ hidden: true }); const vnode: any = menu.__render__(); - assert.isTrue(vnode.properties.classes[baseCss.visuallyHidden]); + assert.isTrue(vnode.properties.classes[css.hidden]); } }, From 38500bcd7bf72567bc5154896f438cdc940c3a77 Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Mon, 20 Mar 2017 20:54:31 -0500 Subject: [PATCH 04/19] Update menu widgets tests --- src/common/tests/intern.ts | 3 +- src/menu/Menu.ts | 26 +-- src/menu/tests/unit/Menu.ts | 388 +++++++++++++++++++++++++++++++- src/menu/tests/unit/MenuItem.ts | 88 ++++++++ 4 files changed, 489 insertions(+), 16 deletions(-) diff --git a/src/common/tests/intern.ts b/src/common/tests/intern.ts index d0f67a1312..67cf4a4964 100644 --- a/src/common/tests/intern.ts +++ b/src/common/tests/intern.ts @@ -56,7 +56,8 @@ export const loaderOptions = { { name: '@dojo', location: 'node_modules/@dojo' }, { name: 'grunt-dojo2', location: 'node_modules/grunt-dojo2'}, { name: 'pepjs', location: 'node_modules/pepjs/dist', main: 'pep' }, - { name: 'maquette', location: 'node_modules/maquette/dist', main: 'maquette' } + { name: 'maquette', location: 'node_modules/maquette/dist', main: 'maquette' }, + { name: 'sinon', location: 'node_modules/sinon/pkg', main: 'sinon' } ] }; diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index f7e0129ad4..2779a4900c 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -55,6 +55,14 @@ export class Menu extends ThemeableMixin(WidgetBase) { } } + onMenuFocus() { + const { disabled, hidden = this._getDefaultHidden() } = this.properties; + if (!disabled && hidden) { + const onRequestShow = this.properties.onRequestShow; + onRequestShow && onRequestShow(); + } + } + onMenuMouseEnter() { const { disabled, @@ -88,14 +96,6 @@ export class Menu extends ThemeableMixin(WidgetBase) { } } - onMenuFocus() { - const { hidden = this._getDefaultHidden() } = this.properties; - if (hidden) { - const onRequestShow = this.properties.onRequestShow; - onRequestShow && onRequestShow(); - } - } - render(): DNode { const { hidden = this._getDefaultHidden(), @@ -143,7 +143,9 @@ export class Menu extends ThemeableMixin(WidgetBase) { protected _afterCreate(element: HTMLElement) { const { animate = true } = this.properties; - if (animate && this._initialRender) { + this._initialRender = false; + + if (animate) { const hidden = element.classList.contains(css.hidden); this._wasOpen = !hidden; @@ -151,7 +153,6 @@ export class Menu extends ThemeableMixin(WidgetBase) { element.style.height = '0'; } } - this._initialRender = false; } protected _afterUpdate(element: HTMLElement) { @@ -162,6 +163,7 @@ export class Menu extends ThemeableMixin(WidgetBase) { } if (!animate) { + // In case `animate` was previously `true`, remove any `height` property set on the node. element.style.height = null; return; } @@ -170,10 +172,6 @@ export class Menu extends ThemeableMixin(WidgetBase) { } protected _animate(element: HTMLElement, hidden: boolean) { - if (hidden && !this._wasOpen) { - return; - } - requestAnimationFrame(() => { const height = getMenuHeight(element); this._wasOpen = !hidden; diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 2fb2809a77..81a7bcf1b0 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -1,12 +1,102 @@ +import has from '@dojo/has/has'; +import { VNode } from '@dojo/interfaces/vdom'; import * as registerSuite from 'intern!object'; import * as assert from 'intern/chai!assert'; -import { VNode } from '@dojo/interfaces/vdom'; +import * as sinon from 'sinon'; import Menu from '../../Menu'; import * as css from '../../styles/menu.css'; +function getStyle(element: any) { + return { + getPropertyValue(name: string) { + return element.style[name]; + } + }; +} + +function raf(callback: () => void) { + callback(); +} + +function getMockNavElement() { + const classes: string[] = []; + const styleHistory: { [key: string]: (string | null)[]; } = { + height: [ null ], + 'max-height': [ null ], + transition: [ '0.5s' ] + }; + const styles = Object.create(null); + const getDefinition = (name: string) => { + const group = styleHistory[name]; + return { + get() { + return group[group.length - 1]; + }, + set(value: string) { + group.push(value); + } + }; + }; + Object.defineProperty(styles, 'height', getDefinition('height')); + Object.defineProperty(styles, 'max-height', getDefinition('max-height')); + Object.defineProperty(styles, 'transition', getDefinition('transition')); + + return { + get styleHistory() { + return styleHistory; + }, + get scrollHeight(){ + return 300; + }, + style: styles, + classList: { + add(name: string) { + if (classes.indexOf(name) < 0) { + classes.push(name); + } + }, + contains(name: string) { + return classes.indexOf(name) > -1; + }, + remove(name: string) { + const index = classes.indexOf(name); + if (index > -1) { + classes.splice(index, 1); + } + } + } + }; +} + registerSuite({ name: 'Menu', + setup() { + if (has('host-node')) { + ( global).requestAnimationFrame = function (callback: () => void) { + callback(); + }; + ( global).getComputedStyle = function (element: any) { + return { + getPropertyValue(name: string) { + return element.style[name]; + } + }; + }; + } + else if (has('host-browser')) { + sinon.stub(window, 'requestAnimationFrame', raf); + sinon.stub(window, 'getComputedStyle', getStyle); + } + }, + + teardown() { + if (has('host-browser')) { + ( window.requestAnimationFrame).restore(); + ( window.getComputedStyle).restore(); + } + }, + 'Should construct menu with passed properties'() { const menu = new Menu(); menu.setProperties({ @@ -22,6 +112,226 @@ registerSuite({ assert.isTrue(menu.properties.hidden); }, + animate: { + 'without a label': { + 'state classes added immediately'() { + const menu = new Menu(); + menu.setProperties({ + hidden: true + }); + + let vnode: any = menu.__render__(); + let element: any = getMockNavElement(); + + vnode.properties.afterCreate.call(menu, element); + vnode.properties.afterUpdate.call(menu, element); + assert.isTrue(vnode.properties.classes[css.hidden]); + + menu.setProperties({ + hidden: false + }); + + vnode = menu.__render__(); + element = getMockNavElement(); + + vnode.properties.afterCreate.call(menu, element); + vnode.properties.afterUpdate.call(menu, element); + assert.isTrue(vnode.properties.classes[css.visible]); + }, + + 'not animated'() { + const menu = new Menu(); + const vnode: any = menu.__render__(); + const element = getMockNavElement(); + + vnode.properties.afterCreate.call(menu, element); + vnode.properties.afterUpdate.call(menu, element); + + const styleHistory = element.styleHistory; + assert.sameMembers(styleHistory.height, [ null ]); + } + }, + + 'when false': { + 'state classes added immediately'() { + const menu = new Menu(); + menu.setProperties({ + animate: false, + label: 'Menu label' + }); + + let vnode: any = menu.__render__(); + let menuNode = vnode.children[1]; + let element: any = getMockNavElement(); + + menuNode.properties.afterCreate.call(menu, element); + menuNode.properties.afterUpdate.call(menu, element); + assert.isTrue(menuNode.properties.classes[css.hidden]); + + menu.setProperties({ + animate: false, + label: 'Menu label', + hidden: false + }); + + vnode = menu.__render__(); + menuNode = vnode.children[1]; + element = getMockNavElement(); + + menuNode.properties.afterCreate.call(menu, element); + menuNode.properties.afterUpdate.call(menu, element); + assert.isTrue(menuNode.properties.classes[css.visible]); + }, + + 'style.height not reset on initialization'() { + const menu = new Menu(); + menu.setProperties({ + animate: false, + label: 'Menu label' + }); + const vnode: any = menu.__render__(); + const menuNode = vnode.children[1]; + const element: any = getMockNavElement(); + menuNode.properties.afterCreate.call(menu, element); + + assert.isNull(element.style.height, 'style.height should not be modified'); + }, + + 'style.height removed on subsequent renders'() { + const menu = new Menu(); + const element = getMockNavElement(); + menu.setProperties({ + animate: false, + label: 'Menu label' + }); + + let vnode: any = menu.__render__(); + let menuNode = vnode.children[1]; + menuNode.properties.afterCreate.call(menu, element); + menuNode.properties.afterUpdate.call(menu, element); + + const styleHistory = element.styleHistory; + assert.sameMembers(styleHistory.height, [ null, null ], 'style.height should be reset'); + } + }, + + 'when true': { + 'state classes not added immediately after the initial render'() { + const menu = new Menu(); + menu.setProperties({ + label: 'Menu label' + }); + + const element = getMockNavElement(); + let vnode: any = menu.__render__(); + let menuNode = vnode.children[1]; + + menuNode.properties.afterCreate.call(menu, element); + + menu.setProperties({ + label: 'Other label' + }); + vnode = menu.__render__(); + menuNode = vnode.children[1]; + menuNode.properties.afterUpdate.call(menu, element); + + assert.notOk(menuNode.properties.classes[css.hidden]); + }, + + 'style.height zeroed when hidden'() { + const menu = new Menu(); + menu.setProperties({ + label: 'Menu label' + }); + + const vnode: any = menu.__render__(); + const menuNode = vnode.children[1]; + let element = getMockNavElement(); + + element.classList.add(css.hidden); + menuNode.properties.afterCreate.call(menu, element); + assert.strictEqual(element.style.height, '0'); + + element = getMockNavElement(); + menuNode.properties.afterCreate.call(menu, element); + assert.isNull(element.style.height); + }, + + 'collapsed from the scroll height to 0'() { + const menu = new Menu(); + menu.setProperties({ + label: 'Menu label' + }); + + const vnode: any = menu.__render__(); + const menuNode = vnode.children[1]; + const element = getMockNavElement(); + + menuNode.properties.afterCreate.call(menu, element); + menuNode.properties.afterUpdate.call(menu, element); + + const styleHistory = element.styleHistory; + assert.sameMembers(styleHistory.height, [ null, '300px', '0' ]); + assert.sameMembers(styleHistory.transition, [ '0.5s', null, '0.5s' ]); + }, + + 'collapsed from the max-height if it is set'() { + const menu = new Menu(); + menu.setProperties({ + label: 'Menu label' + }); + + const vnode: any = menu.__render__(); + const menuNode = vnode.children[1]; + const element = getMockNavElement(); + element.style['max-height'] = '100px'; + + menuNode.properties.afterCreate.call(menu, element); + menuNode.properties.afterUpdate.call(menu, element); + + const styleHistory = element.styleHistory; + assert.sameMembers(styleHistory.height, [ null, '100px', '0' ]); + }, + + 'expanded to the scroll height'() { + const menu = new Menu(); + menu.setProperties({ + hidden: false, + label: 'Menu label' + }); + + const vnode: any = menu.__render__(); + const menuNode = vnode.children[1]; + const element = getMockNavElement(); + + menuNode.properties.afterCreate.call(menu, element); + menuNode.properties.afterUpdate.call(menu, element); + + const styleHistory = element.styleHistory; + assert.sameMembers(styleHistory.height, [ null, '300px' ]); + }, + + 'animates to the max-height when set'() { + const menu = new Menu(); + menu.setProperties({ + hidden: false, + label: 'Menu label' + }); + + const vnode: any = menu.__render__(); + const menuNode = vnode.children[1]; + const element = getMockNavElement(); + element.style['max-height'] = '100px'; + + menuNode.properties.afterCreate.call(menu, element); + menuNode.properties.afterUpdate.call(menu, element); + + const styleHistory = element.styleHistory; + assert.sameMembers(styleHistory.height, [ null, '100px' ]); + } + } + }, + label: { 'renders the menu within a container'() { const menu = new Menu(); @@ -109,6 +419,82 @@ registerSuite({ assert.isTrue(menu.properties.hidden, 'menu should not be shown on click when `expandOnClick` is false'); }, + onLabelKeypress: { + 'when disabled'() { + const menu = new Menu(); + menu.setProperties({ + disabled: true, + hidden: true, + onRequestShow() { + menu.setProperties({ hidden: false }); + } + }); + + menu.onLabelKeypress( { key: 'Enter' }); + assert.isTrue(menu.properties.hidden, 'menu should remain hidden when disabled'); + }, + + 'when enabled'() { + const menu = new Menu(); + menu.setProperties({ + hidden: true, + onRequestShow() { + menu.setProperties({ hidden: false }); + } + }); + + menu.onLabelKeypress( { key: 'Enter' }); + assert.isFalse(menu.properties.hidden); + } + }, + + onMenuFocus: { + 'when disabled'() { + const menu = new Menu(); + menu.setProperties({ + disabled: true, + hidden: true, + onRequestShow() { + menu.setProperties({ hidden: false }); + } + }); + + menu.onMenuFocus(); + assert.isTrue(menu.properties.hidden, 'menu should remain hidden when disabled'); + }, + + 'when enabled and hidden'() { + const menu = new Menu(); + menu.setProperties({ + hidden: true, + onRequestShow() { + menu.setProperties({ hidden: false }); + } + }); + + menu.onMenuFocus(); + assert.isFalse(menu.properties.hidden, 'menu should open when focused'); + }, + + 'when enabled and visible'() { + const menu = new Menu(); + let hideCalled = false; + let showCalled = false; + menu.setProperties({ + onRequestHide() { + hideCalled = true; + }, + onRequestShow() { + showCalled = true; + } + }); + + menu.onMenuFocus(); + assert.isFalse(hideCalled, 'onRequestHide not called'); + assert.isFalse(showCalled, 'onRequestShow not called'); + } + }, + onMenuMouseEnter: { 'when `expandOnClick` is true'() { const menu = new Menu(); diff --git a/src/menu/tests/unit/MenuItem.ts b/src/menu/tests/unit/MenuItem.ts index 6da9773a3e..3bc80407b8 100644 --- a/src/menu/tests/unit/MenuItem.ts +++ b/src/menu/tests/unit/MenuItem.ts @@ -74,6 +74,35 @@ registerSuite({ } }, + onKeypress: { + 'when disabled'() { + const item = new MenuItem(); + let event: any; + item.setProperties({ + disabled: true, + onKeypress(_event: any) { + event = _event; + } + }); + + item.onKeypress( { type: 'keypress' }); + assert.isUndefined(event, '`onKeypress` should not be called when the menu item is disabled.'); + }, + + 'when enabled'() { + const item = new MenuItem(); + let event: any; + item.setProperties({ + onKeypress(_event: any) { + event = _event; + } + }); + + item.onKeypress( { type: 'keypress' }); + assert.strictEqual(event!.type, 'keypress', '`onKeypress` should be called when the menu item is enabled.'); + } + }, + disabled() { const item = new MenuItem(); let vnode: any = item.__render__(); @@ -97,6 +126,40 @@ registerSuite({ assert.strictEqual(vnode.properties.target, '_blank'); }, + getAriaProperties() { + const item = new MenuItem(); + item.setProperties({ + getAriaProperties() { + return { 'aria-expanded': 'false' }; + } + }); + const vnode: any = item.__render__(); + assert.strictEqual(vnode.properties['aria-expanded'], 'false', + 'Aria properties should be added to the node'); + }, + + hasMenu: { + 'when false'() { + const item = new MenuItem(); + const vnode: any = item.__render__(); + + assert.isTrue(vnode.properties.classes[css.menuItem]); + assert.notOk(vnode.properties.classes[css.menuLabel]); + }, + + 'when true'() { + const item = new MenuItem(); + item.setProperties({ hasMenu: true }); + + const vnode: any = item.__render__(); + const classes = css.menuLabel.split(' '); + + classes.forEach((className: string) => { + assert.isTrue(vnode.properties.classes[className]); + }); + } + }, + label() { const item = new MenuItem(); item.setProperties({ label: 'Label' }); @@ -116,6 +179,31 @@ registerSuite({ assert.isTrue(vnode.properties.classes[css.selected]); }, + tabIndex: { + 'when disabled'() { + const item = new MenuItem(); + item.setProperties({ disabled: true, tabIndex: 1 }); + const vnode: any = item.__render__(); + + assert.strictEqual(vnode.properties.tabIndex, -1, 'Specified tabIndex should be ignored'); + }, + + 'when enabled'() { + const item = new MenuItem(); + item.setProperties({ tabIndex: 1 }); + const vnode: any = item.__render__(); + + assert.strictEqual(vnode.properties.tabIndex, 1); + }, + + 'defaults to 0'() { + const item = new MenuItem(); + const vnode: any = item.__render__(); + + assert.strictEqual(vnode.properties.tabIndex, 0); + } + }, + url() { const item = new MenuItem(); item.setProperties({ url: 'http://dojo.io' }); From 268d7df225a002d417d0626d464d4783309fe783 Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Mon, 20 Mar 2017 21:59:34 -0500 Subject: [PATCH 05/19] Add menu comments and `animate` function property --- src/menu/Menu.ts | 61 ++++++++++++++++++++++++++++++++++++- src/menu/MenuItem.ts | 40 ++++++++++++++++++++++++ src/menu/tests/unit/Menu.ts | 21 +++++++++++++ 3 files changed, 121 insertions(+), 1 deletion(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index 2779a4900c..a4d8b4f047 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -12,16 +12,68 @@ import * as css from './styles/menu.css'; export type Role = 'menu' | 'menubar'; export interface MenuProperties extends ThemeableProperties { - animate?: boolean; + /** + * Only applies to nested menus. Either a flag indicating whether the widget instance should handle animating + * between the visible and hidden states, or a function that manually handles the animation. If true (the default), + * then the menu will slide into and out of view like an accordion. If false, then any animation should be handled + * in the CSS, as the menu will just snap open/shut. + */ + animate?: boolean | ((element: HTMLElement, hidden: boolean) => void); + + /** + * Indicates whether the menu is disabled. If true, then the widget will ignore events. + */ disabled?: boolean; + + /** + * Indicates whether the menu should be displayed/hidden via a click event. If false, then the menu is toggled + * via a hover event. Defaults to true. + */ expandOnClick?: boolean; + + /** + * Only applies to menus toggled into view with a hover event. The amount of time (in milliseconds) after a + * mouseleave event that should pass before the menu is hidden. Defaults to 300ms. + */ hideDelay?: number; + + /** + * Whether the menu is hidden. Defaults to true if a label is specified (i.e., the menu is controlled by a + * a link); false otherwise. + */ hidden?: boolean; + + /** + * The widget ID. Defaults to a random string. + */ id?: string; + + /** + * The text or properties for a MenuItem widget that is used to control the menu. + */ label?: string | MenuItemProperties; + + /** + * Indicates whether the menu is nested within another menu. Useful for styling, this does not affect + * functionality. Defaults to false. + */ nested?: boolean; + + /** + * Needed only when a label is used. Called when the menu is displayed, and the label is triggered. + * This method should be used to update the widget's `hidden` property. + */ onRequestHide?: () => void; + + /** + * Needed only when a label is used. Called when the menu is hidden, and the label is triggered. + * This method should be used to update the widget's `hidden` property. + */ onRequestShow?: () => void; + + /** + * The value to use for the menu's `role` property. Defaults to "menu". + */ role?: Role; } @@ -168,15 +220,22 @@ export class Menu extends ThemeableMixin(WidgetBase) { return; } + if (typeof animate === 'function') { + return animate(element, hidden); + } + this._animate(element, hidden); } protected _animate(element: HTMLElement, hidden: boolean) { + // Assuming the menu has an `auto` height, manually apply the scroll height (or max-height if specified), + // and animate to and from that. requestAnimationFrame(() => { const height = getMenuHeight(element); this._wasOpen = !hidden; if (hidden) { + // Temporarily remove any transition to prevent the element from animating to `height`. const transition = element.style.transition; element.style.transition = null; diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts index 0f79f0dc24..e35fc21cd1 100644 --- a/src/menu/MenuItem.ts +++ b/src/menu/MenuItem.ts @@ -6,15 +6,55 @@ import WidgetBase from '@dojo/widget-core/WidgetBase'; import * as css from './styles/menu.css'; export interface MenuItemProperties extends ThemeableProperties { + /** + * Indicates whether the menu is disabled. If true, then the widget will ignore events. + */ disabled?: boolean; + + /** + * Applies only when a URL is provided. If `true`, the URL will be opened in a new window. + */ external?: boolean; + + /** + * Returns an object of aria properties to apply to the widget's DOM element. + */ getAriaProperties?: () => { [key: string]: string; }; + + /** + * A flag indicating whether the widget is used as the label for a menu widget. If `true`, + * then the `menuLabel` CSS class is applied instead of the `menuItem` class. + */ hasMenu?: boolean; + + /** + * The widget text content. + */ label?: string; + + /** + * An event handler for click events. + */ onClick?: (event: MouseEvent) => void; + + /** + * An event handler for keypress events. + */ onKeypress?: (event: KeyboardEvent) => void; + + /** + * Indicates whether the widget is selected. + */ selected?: boolean; + + /** + * The tab index. Defaults to 0, and is forced to -1 if the widget is disabled. + */ tabIndex?: number; + + /** + * A URL to navigate to on click. + */ url?: string; } diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 81a7bcf1b0..00c56185bc 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -329,6 +329,27 @@ registerSuite({ const styleHistory = element.styleHistory; assert.sameMembers(styleHistory.height, [ null, '100px' ]); } + }, + + 'when a function'() { + const menu = new Menu(); + let args: any[] = []; + menu.setProperties({ + hidden: false, + label: 'Menu label', + animate(element, hidden) { + args = [ element, hidden ]; + } + }); + + const vnode: any = menu.__render__(); + const menuNode = vnode.children[1]; + const element = getMockNavElement(); + + menuNode.properties.afterCreate.call(menu, element); + menuNode.properties.afterUpdate.call(menu, element); + + assert.sameMembers(args, [ element, false ], 'The custom animate function should be called'); } }, From 1219058f0362dfd1ec4370b3d8b5f79f7a49201b Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Tue, 21 Mar 2017 10:21:59 -0500 Subject: [PATCH 06/19] Menu updates - Use `properties.hidden` directly instead of passing it around. - Disambiguate `isHidden` to `requestShow` in `_toggleDisplay`. - Create an export base classes from Menu and MenuItem. - Remove underscore prefix from protected class members. - Fix doc comments for Menu/MenuItem properties. - Fix MenuItem child type. --- src/menu/Menu.ts | 166 ++++++++++++++++++------------------ src/menu/MenuItem.ts | 86 +++++++++---------- src/menu/tests/unit/Menu.ts | 2 +- 3 files changed, 125 insertions(+), 129 deletions(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index a4d8b4f047..fb0d8675ab 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -11,69 +11,64 @@ import * as css from './styles/menu.css'; export type Role = 'menu' | 'menubar'; +/** + * @type MenuProperties + * + * Properties that can be set on a Menu component + * + * @property animate + * Only applies to nested menus. Either a flag indicating whether the widget instance should handle animating + * between the visible and hidden states, or a function that manually handles the animation. If true (the default), + * then the menu will slide into and out of view like an accordion. If false, then any animation should be handled + * in the CSS, as the menu will just snap open/shut. + * + * @property disabled + * Indicates whether the menu is disabled. If true, then the widget will ignore events. + * + * @property expandOnClick + * Indicates whether the menu should be displayed/hidden via a click event. If false, then the menu is toggled + * via a hover event. Defaults to true. + * + * @property hideDelay + * Only applies to menus toggled into view with a hover event. The amount of time (in milliseconds) after a + * mouseleave event that should pass before the menu is hidden. Defaults to 300ms. + * + * @property hidden + * Whether the menu is hidden. Defaults to true if a label is specified (i.e., the menu is controlled by a + * link); false otherwise. + * + * @property id + * The widget ID. Defaults to a random string. + * + * @property label + * The text or properties for a MenuItem widget that is used to control the menu. + * + * @property nested + * Indicates whether the menu is nested within another menu. Useful for styling, this does not affect + * functionality. Defaults to false. + * + * @property onRequestHide + * Needed only when a label is used. Called when the menu is displayed, and the label is triggered. + * This method should be used to update the widget's `hidden` property. + * + * @property onRequestShow + * Needed only when a label is used. Called when the menu is hidden, and the label is triggered. + * This method should be used to update the widget's `hidden` property. + * + * @property role + * The value to use for the menu's `role` property. Defaults to "menu". + */ export interface MenuProperties extends ThemeableProperties { - /** - * Only applies to nested menus. Either a flag indicating whether the widget instance should handle animating - * between the visible and hidden states, or a function that manually handles the animation. If true (the default), - * then the menu will slide into and out of view like an accordion. If false, then any animation should be handled - * in the CSS, as the menu will just snap open/shut. - */ animate?: boolean | ((element: HTMLElement, hidden: boolean) => void); - - /** - * Indicates whether the menu is disabled. If true, then the widget will ignore events. - */ disabled?: boolean; - - /** - * Indicates whether the menu should be displayed/hidden via a click event. If false, then the menu is toggled - * via a hover event. Defaults to true. - */ expandOnClick?: boolean; - - /** - * Only applies to menus toggled into view with a hover event. The amount of time (in milliseconds) after a - * mouseleave event that should pass before the menu is hidden. Defaults to 300ms. - */ hideDelay?: number; - - /** - * Whether the menu is hidden. Defaults to true if a label is specified (i.e., the menu is controlled by a - * a link); false otherwise. - */ hidden?: boolean; - - /** - * The widget ID. Defaults to a random string. - */ id?: string; - - /** - * The text or properties for a MenuItem widget that is used to control the menu. - */ label?: string | MenuItemProperties; - - /** - * Indicates whether the menu is nested within another menu. Useful for styling, this does not affect - * functionality. Defaults to false. - */ nested?: boolean; - - /** - * Needed only when a label is used. Called when the menu is displayed, and the label is triggered. - * This method should be used to update the widget's `hidden` property. - */ onRequestHide?: () => void; - - /** - * Needed only when a label is used. Called when the menu is hidden, and the label is triggered. - * This method should be used to update the widget's `hidden` property. - */ onRequestShow?: () => void; - - /** - * The value to use for the menu's `role` property. Defaults to "menu". - */ role?: Role; } @@ -82,9 +77,11 @@ function getMenuHeight(menuElement: HTMLElement): number { return Math.min(menuElement.scrollHeight, (isNaN(maxHeight) ? Infinity : maxHeight)); } +export const MenuBase = ThemeableMixin(WidgetBase); + @theme(css) -export class Menu extends ThemeableMixin(WidgetBase) { - protected _wasOpen = false; +export class Menu extends MenuBase { + protected wasOpen = false; private _hideTimer: Handle; private _initialRender = true; @@ -95,7 +92,7 @@ export class Menu extends ThemeableMixin(WidgetBase) { } = this.properties; if (!disabled && expandOnClick) { - this._toggleDisplay(); + this.toggleDisplay(); } } @@ -103,12 +100,12 @@ export class Menu extends ThemeableMixin(WidgetBase) { const { disabled } = this.properties; if (!disabled && event.key === 'Enter') { - this._toggleDisplay(); + this.toggleDisplay(); } } onMenuFocus() { - const { disabled, hidden = this._getDefaultHidden() } = this.properties; + const { disabled, hidden = this.getDefaultHidden() } = this.properties; if (!disabled && hidden) { const onRequestShow = this.properties.onRequestShow; onRequestShow && onRequestShow(); @@ -123,7 +120,7 @@ export class Menu extends ThemeableMixin(WidgetBase) { if (!disabled && !expandOnClick) { this._hideTimer && this._hideTimer.destroy(); - this._toggleDisplay(true); + this.toggleDisplay(true); } } @@ -138,19 +135,18 @@ export class Menu extends ThemeableMixin(WidgetBase) { this._hideTimer && this._hideTimer.destroy(); if (hideDelay) { this._hideTimer = createTimer(() => { - this._toggleDisplay(false); + this.toggleDisplay(false); }, hideDelay); this.own(this._hideTimer); } else { - this._toggleDisplay(false); + this.toggleDisplay(false); } } } render(): DNode { const { - hidden = this._getDefaultHidden(), id = uuid(), nested, role = 'menu' @@ -160,9 +156,9 @@ export class Menu extends ThemeableMixin(WidgetBase) { const menu = v('nav', { id, role, - classes: this.classes.apply(this, this._getMenuClasses(hidden, Boolean(label))), - afterCreate: this._afterCreate, - afterUpdate: this._afterUpdate, + classes: this.classes.apply(this, this.getMenuClasses()), + afterCreate: this.afterCreate, + afterUpdate: this.afterUpdate, onfocusin: this.onMenuFocus }, this.children); @@ -184,7 +180,7 @@ export class Menu extends ThemeableMixin(WidgetBase) { const properties = typeof label === 'string' ? { label } : label; return w(MenuItem, assign({ disabled, - getAriaProperties: this._getLabelAriaProperties.bind(this, id), + getAriaProperties: this.getLabelAriaProperties.bind(this, id), hasMenu: true, overrideClasses: overrideClasses || css, onClick: this.onLabelClick, @@ -193,13 +189,12 @@ export class Menu extends ThemeableMixin(WidgetBase) { } } - protected _afterCreate(element: HTMLElement) { - const { animate = true } = this.properties; + protected afterCreate(element: HTMLElement) { + const { animate = true, hidden = this.getDefaultHidden() } = this.properties; this._initialRender = false; if (animate) { - const hidden = element.classList.contains(css.hidden); - this._wasOpen = !hidden; + this.wasOpen = !hidden; if (hidden) { element.style.height = '0'; @@ -207,8 +202,8 @@ export class Menu extends ThemeableMixin(WidgetBase) { } } - protected _afterUpdate(element: HTMLElement) { - const { animate = true, hidden = this._getDefaultHidden(), label } = this.properties; + protected afterUpdate(element: HTMLElement) { + const { animate = true, hidden = this.getDefaultHidden(), label } = this.properties; if (!label) { return; @@ -224,15 +219,17 @@ export class Menu extends ThemeableMixin(WidgetBase) { return animate(element, hidden); } - this._animate(element, hidden); + this.animate(element); } - protected _animate(element: HTMLElement, hidden: boolean) { + protected animate(element: HTMLElement) { + const { hidden = this.getDefaultHidden() } = this.properties; + // Assuming the menu has an `auto` height, manually apply the scroll height (or max-height if specified), // and animate to and from that. requestAnimationFrame(() => { const height = getMenuHeight(element); - this._wasOpen = !hidden; + this.wasOpen = !hidden; if (hidden) { // Temporarily remove any transition to prevent the element from animating to `height`. @@ -251,7 +248,7 @@ export class Menu extends ThemeableMixin(WidgetBase) { }); } - protected _getDefaultHidden() { + protected getDefaultHidden() { const { disabled, label } = this.properties; if (label && disabled) { @@ -261,16 +258,17 @@ export class Menu extends ThemeableMixin(WidgetBase) { return label ? true : false; } - protected _getLabelAriaProperties(id: string): { [key: string]: string; } { - const { hidden = this._getDefaultHidden() } = this.properties; + protected getLabelAriaProperties(id: string): { [key: string]: string; } { + const { hidden = this.getDefaultHidden() } = this.properties; return { 'aria-controls': id, 'aria-expanded': String(!hidden) }; } - protected _getMenuClasses(hidden: boolean, isSubMenu: boolean) { - const { animate = true, nested } = this.properties; + protected getMenuClasses() { + const { animate = true, hidden = this.getDefaultHidden(), label, nested } = this.properties; + const isSubMenu = Boolean(label); const classes = [ css.menu ]; if (this._initialRender || !animate || !isSubMenu) { @@ -288,18 +286,18 @@ export class Menu extends ThemeableMixin(WidgetBase) { return classes; } - protected _toggleDisplay(isHidden?: boolean) { + protected toggleDisplay(requestShow?: boolean) { const { onRequestHide, onRequestShow } = this.properties; - if (typeof isHidden === 'undefined') { - const { hidden = this._getDefaultHidden() } = this.properties; - isHidden = hidden; + if (typeof requestShow === 'undefined') { + const { hidden = this.getDefaultHidden() } = this.properties; + requestShow = hidden; } - if (isHidden) { + if (requestShow) { typeof onRequestShow === 'function' && onRequestShow(); } else { diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts index e35fc21cd1..df8fe74eff 100644 --- a/src/menu/MenuItem.ts +++ b/src/menu/MenuItem.ts @@ -1,65 +1,63 @@ import { assign } from '@dojo/core/lang'; -import { HNode } from '@dojo/widget-core/interfaces'; +import { DNode } from '@dojo/widget-core/interfaces'; import { v } from '@dojo/widget-core/d'; import ThemeableMixin, { theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable'; import WidgetBase from '@dojo/widget-core/WidgetBase'; import * as css from './styles/menu.css'; +/** + * @type MenuItemProperties + * + * Properties that can be set on a MenuItem component + * + * @property disabled + * Indicates whether the menu is disabled. If true, then the widget will ignore events. + * + * @property external + * Applies only when a URL is provided. If `true`, the URL will be opened in a new window. + * + * @property getAriaProperties + * Returns an object of aria properties to apply to the widget's DOM element. + * + * @property hasMenu + * A flag indicating whether the widget is used as the label for a menu widget. If `true`, + * then the `menuLabel` CSS class is applied instead of the `menuItem` class. + * + * @property label + * The widget text content. + * + * @property onClick + * An event handler for click events. + * + * @property onKeypress + * An event handler for keypress events. + * + * @property selected + * Indicates whether the widget is selected. + * + * @property tabIndex + * The tab index. Defaults to 0, and is forced to -1 if the widget is disabled. + * + * @property url + * A URL to navigate to on click. + */ export interface MenuItemProperties extends ThemeableProperties { - /** - * Indicates whether the menu is disabled. If true, then the widget will ignore events. - */ disabled?: boolean; - - /** - * Applies only when a URL is provided. If `true`, the URL will be opened in a new window. - */ external?: boolean; - - /** - * Returns an object of aria properties to apply to the widget's DOM element. - */ getAriaProperties?: () => { [key: string]: string; }; - - /** - * A flag indicating whether the widget is used as the label for a menu widget. If `true`, - * then the `menuLabel` CSS class is applied instead of the `menuItem` class. - */ hasMenu?: boolean; - - /** - * The widget text content. - */ label?: string; - - /** - * An event handler for click events. - */ onClick?: (event: MouseEvent) => void; - - /** - * An event handler for keypress events. - */ onKeypress?: (event: KeyboardEvent) => void; - - /** - * Indicates whether the widget is selected. - */ selected?: boolean; - - /** - * The tab index. Defaults to 0, and is forced to -1 if the widget is disabled. - */ tabIndex?: number; - - /** - * A URL to navigate to on click. - */ url?: string; } +export const MenuItemBase = ThemeableMixin(WidgetBase); + @theme(css) -export class MenuItem extends ThemeableMixin(WidgetBase) { +export class MenuItem extends MenuItemBase { onClick(event: MouseEvent) { const { disabled, onClick } = this.properties; if (!disabled && typeof onClick === 'function') { @@ -105,10 +103,10 @@ export class MenuItem extends ThemeableMixin(WidgetBase) { }), label ? [ label ] : undefined); if (this.children.length) { - const children = this.children as HNode[]; + const children: DNode[] = [ labelNode ]; return v('span', { classes: this.classes(css.menuItem) - }, [ labelNode ].concat(children)); + }, children.concat(this.children)); } return labelNode; diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 00c56185bc..98cc17cb2f 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -248,10 +248,10 @@ registerSuite({ const menuNode = vnode.children[1]; let element = getMockNavElement(); - element.classList.add(css.hidden); menuNode.properties.afterCreate.call(menu, element); assert.strictEqual(element.style.height, '0'); + menu.setProperties({ hidden: false, label: 'Menu label' }); element = getMockNavElement(); menuNode.properties.afterCreate.call(menu, element); assert.isNull(element.style.height); From 48398b0bf2424d5cb857ce79576eb4035d45919a Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Tue, 21 Mar 2017 20:37:24 -0500 Subject: [PATCH 07/19] Menu and MenuItem updates - Remove the ability to specify `animate` as a function. - Remove `getAriaProperties` in favor of individual properties. - Simplify the MenuItem widget. --- src/menu/Menu.ts | 39 ++++++---------- src/menu/MenuItem.ts | 56 +++++++++-------------- src/menu/example/index.ts | 24 +++++----- src/menu/tests/unit/Menu.ts | 36 +-------------- src/menu/tests/unit/MenuItem.ts | 80 +++++++++------------------------ 5 files changed, 71 insertions(+), 164 deletions(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index fb0d8675ab..cba2751d14 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -6,7 +6,7 @@ import { v, w } from '@dojo/widget-core/d'; import { DNode } from '@dojo/widget-core/interfaces'; import ThemeableMixin, { theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable'; import WidgetBase from '@dojo/widget-core/WidgetBase'; -import MenuItem, { MenuItemProperties } from './MenuItem'; +import MenuItem from './MenuItem'; import * as css from './styles/menu.css'; export type Role = 'menu' | 'menubar'; @@ -17,10 +17,9 @@ export type Role = 'menu' | 'menubar'; * Properties that can be set on a Menu component * * @property animate - * Only applies to nested menus. Either a flag indicating whether the widget instance should handle animating - * between the visible and hidden states, or a function that manually handles the animation. If true (the default), - * then the menu will slide into and out of view like an accordion. If false, then any animation should be handled - * in the CSS, as the menu will just snap open/shut. + * Only applies to nested menus. A flag indicating whether the widget instance should handle animating between the + * visible and hidden states. If true (the default), then the menu will slide into and out of view like an accordion. + * If false, then any animation should be handled in the CSS, as the menu will just snap open/shut. * * @property disabled * Indicates whether the menu is disabled. If true, then the widget will ignore events. @@ -41,7 +40,7 @@ export type Role = 'menu' | 'menubar'; * The widget ID. Defaults to a random string. * * @property label - * The text or properties for a MenuItem widget that is used to control the menu. + * A widget that will be wrapped in a MenuItem widget that will be used to control the menu. * * @property nested * Indicates whether the menu is nested within another menu. Useful for styling, this does not affect @@ -59,13 +58,13 @@ export type Role = 'menu' | 'menubar'; * The value to use for the menu's `role` property. Defaults to "menu". */ export interface MenuProperties extends ThemeableProperties { - animate?: boolean | ((element: HTMLElement, hidden: boolean) => void); + animate?: boolean; disabled?: boolean; expandOnClick?: boolean; hideDelay?: number; hidden?: boolean; id?: string; - label?: string | MenuItemProperties; + label?: DNode; nested?: boolean; onRequestHide?: () => void; onRequestShow?: () => void; @@ -174,18 +173,18 @@ export class Menu extends MenuBase { } renderLabel(id: string): DNode | void { - const { disabled, label, overrideClasses } = this.properties; + const { disabled, hidden = this.getDefaultHidden(), label, overrideClasses } = this.properties; if (label) { - const properties = typeof label === 'string' ? { label } : label; - return w(MenuItem, assign({ + return w(MenuItem, { + controls: id, disabled, - getAriaProperties: this.getLabelAriaProperties.bind(this, id), + expanded: !hidden, hasMenu: true, overrideClasses: overrideClasses || css, onClick: this.onLabelClick, onKeypress: this.onLabelKeypress - }, properties)); + }, [ label ]); } } @@ -203,7 +202,7 @@ export class Menu extends MenuBase { } protected afterUpdate(element: HTMLElement) { - const { animate = true, hidden = this.getDefaultHidden(), label } = this.properties; + const { animate = true, label } = this.properties; if (!label) { return; @@ -215,10 +214,6 @@ export class Menu extends MenuBase { return; } - if (typeof animate === 'function') { - return animate(element, hidden); - } - this.animate(element); } @@ -258,14 +253,6 @@ export class Menu extends MenuBase { return label ? true : false; } - protected getLabelAriaProperties(id: string): { [key: string]: string; } { - const { hidden = this.getDefaultHidden() } = this.properties; - return { - 'aria-controls': id, - 'aria-expanded': String(!hidden) - }; - } - protected getMenuClasses() { const { animate = true, hidden = this.getDefaultHidden(), label, nested } = this.properties; const isSubMenu = Boolean(label); diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts index df8fe74eff..a6cc908c78 100644 --- a/src/menu/MenuItem.ts +++ b/src/menu/MenuItem.ts @@ -1,5 +1,3 @@ -import { assign } from '@dojo/core/lang'; -import { DNode } from '@dojo/widget-core/interfaces'; import { v } from '@dojo/widget-core/d'; import ThemeableMixin, { theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable'; import WidgetBase from '@dojo/widget-core/WidgetBase'; @@ -10,22 +8,22 @@ import * as css from './styles/menu.css'; * * Properties that can be set on a MenuItem component * + * @property controls + * ID of an element that this input controls + * * @property disabled * Indicates whether the menu is disabled. If true, then the widget will ignore events. * - * @property external - * Applies only when a URL is provided. If `true`, the URL will be opened in a new window. + * @property expanded + * A flag indicating whether a widget controlled by `this` is expanded. * - * @property getAriaProperties - * Returns an object of aria properties to apply to the widget's DOM element. + * @property hasDropDown + * A flag indicating whether the widget has a drop down child. * * @property hasMenu * A flag indicating whether the widget is used as the label for a menu widget. If `true`, * then the `menuLabel` CSS class is applied instead of the `menuItem` class. * - * @property label - * The widget text content. - * * @property onClick * An event handler for click events. * @@ -37,21 +35,17 @@ import * as css from './styles/menu.css'; * * @property tabIndex * The tab index. Defaults to 0, and is forced to -1 if the widget is disabled. - * - * @property url - * A URL to navigate to on click. */ export interface MenuItemProperties extends ThemeableProperties { + controls?: string; disabled?: boolean; - external?: boolean; - getAriaProperties?: () => { [key: string]: string; }; + expanded?: boolean; + hasDropDown?: boolean; hasMenu?: boolean; - label?: string; onClick?: (event: MouseEvent) => void; onKeypress?: (event: KeyboardEvent) => void; selected?: boolean; tabIndex?: number; - url?: string; } export const MenuItemBase = ThemeableMixin(WidgetBase); @@ -74,42 +68,32 @@ export class MenuItem extends MenuItemBase { render() { const { + controls, disabled, - external, + expanded, + hasDropDown = false, hasMenu = false, - getAriaProperties, - label, selected, - tabIndex = 0, - url + tabIndex = 0 } = this.properties; - const ariaProperties = getAriaProperties && getAriaProperties() || Object.create(null); const classes = this.classes( hasMenu ? css.menuLabel : css.menuItem, disabled ? css.disabled : null, selected ? css.selected : null ); - const labelNode = v('a', assign(ariaProperties, { + return v('span', { + 'aria-controls': controls, + 'aria-expanded': expanded, + 'aria-hasdropdown': hasDropDown, 'aria-disabled': disabled, classes, - href: url || undefined, onclick: this.onClick, onkeypress: this.onKeypress, role: 'menuitem', - tabIndex : disabled ? -1 : tabIndex, - target: url && external ? '_blank' : undefined - }), label ? [ label ] : undefined); - - if (this.children.length) { - const children: DNode[] = [ labelNode ]; - return v('span', { - classes: this.classes(css.menuItem) - }, children.concat(this.children)); - } - - return labelNode; + tabIndex : disabled ? -1 : tabIndex + }, this.children); } } diff --git a/src/menu/example/index.ts b/src/menu/example/index.ts index b7efa40170..cf917eae09 100644 --- a/src/menu/example/index.ts +++ b/src/menu/example/index.ts @@ -112,11 +112,13 @@ export class App extends AppBase { }, [ w(MenuItem, { key: 'DojoMenuLabel', - label: 'Dojo 2', - overrideClasses: menuCss, - url: 'http://dojo.io', - external: true - }), + overrideClasses: menuCss + }, [ + v('a', { + href: 'http://dojo.io', + target: '_blank' + }, [ 'Dojo 2' ]) + ]), w(Menu, { animate: animate, @@ -135,12 +137,14 @@ export class App extends AppBase { overrideClasses: menuCss }, packages.map((label, i) => { return w(MenuItem, { - label, key: `menu1-sub1-item${i}`, - overrideClasses: menuCss, - url: `https://github.com/dojo/${label}`, - external: true - }); + overrideClasses: menuCss + }, [ + v('a', { + href: `https://github.com/dojo/${label}`, + target: '_blank' + }, [ label ]) + ]); })) ]); } diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 98cc17cb2f..4d0e025c6d 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -329,32 +329,11 @@ registerSuite({ const styleHistory = element.styleHistory; assert.sameMembers(styleHistory.height, [ null, '100px' ]); } - }, - - 'when a function'() { - const menu = new Menu(); - let args: any[] = []; - menu.setProperties({ - hidden: false, - label: 'Menu label', - animate(element, hidden) { - args = [ element, hidden ]; - } - }); - - const vnode: any = menu.__render__(); - const menuNode = vnode.children[1]; - const element = getMockNavElement(); - - menuNode.properties.afterCreate.call(menu, element); - menuNode.properties.afterUpdate.call(menu, element); - - assert.sameMembers(args, [ element, false ], 'The custom animate function should be called'); } }, label: { - 'renders the menu within a container'() { + 'renders the menu within a MenuItem'() { const menu = new Menu(); menu.setProperties({ hidden: false, @@ -374,7 +353,7 @@ registerSuite({ const vnode = menu.__render__(); const label = vnode.children![0]; - assert.strictEqual(label.vnodeSelector, 'a', 'label node should be a link'); + assert.strictEqual(label.vnodeSelector, 'span', 'label node should be a '); assert.strictEqual(label.text, 'Menu label', 'label node should have the label text'); }, @@ -385,17 +364,6 @@ registerSuite({ assert.strictEqual(vnode.vnodeSelector, 'nav', 'menu node should be a nav'); assert.lengthOf(vnode.children, 3, 'menu node should have all children'); - }, - - 'allows label properties instead of a label string'() { - const menu = new Menu(); - menu.setProperties({ - label: { label: 'Menu label' } - }); - const vnode = menu.__render__(); - const label = vnode.children![0]; - - assert.strictEqual(label.text, 'Menu label', 'label node should have the label text'); } }, diff --git a/src/menu/tests/unit/MenuItem.ts b/src/menu/tests/unit/MenuItem.ts index 3bc80407b8..745723761b 100644 --- a/src/menu/tests/unit/MenuItem.ts +++ b/src/menu/tests/unit/MenuItem.ts @@ -11,38 +11,12 @@ registerSuite({ item.setProperties({ key: 'foo', disabled: false, - external: true, - label: 'Label', - selected: false, - url: 'http://dojo.io' + selected: false }); assert.strictEqual(item.properties.key, 'foo'); assert.isFalse(item.properties.disabled); - assert.isTrue(item.properties.external); - assert.strictEqual(item.properties.label, 'Label'); assert.isFalse(item.properties.selected); - assert.strictEqual(item.properties.url, 'http://dojo.io'); - }, - - children: { - 'without children'() { - const item = new MenuItem(); - const vnode: any = item.__render__(); - - assert.strictEqual(vnode.vnodeSelector, 'a'); - assert.lengthOf(vnode.children, 0); - }, - - 'with children'() { - const item = new MenuItem(); - item.setChildren([ 'Child' ]); - const vnode: any = item.__render__(); - - assert.strictEqual(vnode.vnodeSelector, 'span'); - assert.isTrue(vnode.properties.classes[css.menuItem]); - assert.lengthOf(vnode.children, 2); - } }, onClick: { @@ -103,6 +77,16 @@ registerSuite({ } }, + controls() { + const item = new MenuItem(); + item.setProperties({ + controls: 'uuid-12345' + }); + const vnode: any = item.__render__(); + assert.strictEqual(vnode.properties['aria-controls'], 'uuid-12345', + '`controls` should be assigned to the `aria-controls` attribute'); + }, + disabled() { const item = new MenuItem(); let vnode: any = item.__render__(); @@ -114,28 +98,24 @@ registerSuite({ assert.isTrue(vnode.properties.classes[css.disabled]); }, - external() { + expanded() { const item = new MenuItem(); - item.setProperties({ external: true }); - let vnode: any = item.__render__(); - - assert.notOk(vnode.properties.target, 'target should not be set without a url'); - - item.setProperties({ external: true, url: 'http://dojo.io' }); - vnode = item.__render__(); - assert.strictEqual(vnode.properties.target, '_blank'); + item.setProperties({ + expanded: true + }); + const vnode: any = item.__render__(); + assert.strictEqual(vnode.properties['aria-expanded'], true, + '`expanded` should be assigned to the `aria-expanded` attribute'); }, - getAriaProperties() { + hasDropDown() { const item = new MenuItem(); item.setProperties({ - getAriaProperties() { - return { 'aria-expanded': 'false' }; - } + hasDropDown: true }); const vnode: any = item.__render__(); - assert.strictEqual(vnode.properties['aria-expanded'], 'false', - 'Aria properties should be added to the node'); + assert.strictEqual(vnode.properties['aria-hasdropdown'], true, + '`hasDropDown` should be assigned to the `aria-hasdropdown` attribute'); }, hasMenu: { @@ -160,14 +140,6 @@ registerSuite({ } }, - label() { - const item = new MenuItem(); - item.setProperties({ label: 'Label' }); - const vnode: any = item.__render__(); - - assert.strictEqual(vnode.text, 'Label'); - }, - selected() { const item = new MenuItem(); let vnode: any = item.__render__(); @@ -202,13 +174,5 @@ registerSuite({ assert.strictEqual(vnode.properties.tabIndex, 0); } - }, - - url() { - const item = new MenuItem(); - item.setProperties({ url: 'http://dojo.io' }); - const vnode: any = item.__render__(); - - assert.strictEqual(vnode.properties.href, 'http://dojo.io'); } }); From a0292e51c5f55aee43b221aecc079c4db7257bef Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Tue, 21 Mar 2017 21:16:38 -0500 Subject: [PATCH 08/19] Additional Menu/MenuItem updates - Remove unused import - Remove menu/example.html - Fix example page - Use `KeyboardEvent#keyCode` where `key` is unavailable - Consolidate menu CSS into a single module - Ensure menu is scrolled to top when animating open --- src/menu/Menu.ts | 8 +- src/menu/example/index.html | 26 ----- src/menu/example/index.ts | 160 +++++++++++++------------- src/menu/example/styles/app.css | 21 ---- src/menu/example/styles/app.css.d.ts | 2 - src/menu/example/styles/menu.css | 39 ------- src/menu/example/styles/menu.css.d.ts | 6 - src/menu/styles/menu.css | 30 ++++- src/menu/tests/unit/Menu.ts | 2 + 9 files changed, 111 insertions(+), 183 deletions(-) delete mode 100644 src/menu/example/index.html delete mode 100644 src/menu/example/styles/app.css delete mode 100644 src/menu/example/styles/app.css.d.ts delete mode 100644 src/menu/example/styles/menu.css delete mode 100644 src/menu/example/styles/menu.css.d.ts diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index cba2751d14..fc614bda64 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -1,4 +1,3 @@ -import { assign } from '@dojo/core/lang'; import { createTimer } from '@dojo/core/util'; import uuid from '@dojo/core/uuid'; import { Handle } from '@dojo/interfaces/core'; @@ -97,8 +96,9 @@ export class Menu extends MenuBase { onLabelKeypress(event: KeyboardEvent) { const { disabled } = this.properties; + const key = 'key' in event ? event.key : event.keyCode; - if (!disabled && event.key === 'Enter') { + if (!disabled && key === 'Enter') { this.toggleDisplay(); } } @@ -106,8 +106,7 @@ export class Menu extends MenuBase { onMenuFocus() { const { disabled, hidden = this.getDefaultHidden() } = this.properties; if (!disabled && hidden) { - const onRequestShow = this.properties.onRequestShow; - onRequestShow && onRequestShow(); + this.toggleDisplay(true); } } @@ -238,6 +237,7 @@ export class Menu extends MenuBase { }); } else { + element.scrollTop = 0; element.style.height = `${height}px`; } }); diff --git a/src/menu/example/index.html b/src/menu/example/index.html deleted file mode 100644 index 09d6d7ab38..0000000000 --- a/src/menu/example/index.html +++ /dev/null @@ -1,26 +0,0 @@ - - - - Menu Example - - - - - - - - - diff --git a/src/menu/example/index.ts b/src/menu/example/index.ts index cf917eae09..8f01a85db8 100644 --- a/src/menu/example/index.ts +++ b/src/menu/example/index.ts @@ -2,16 +2,12 @@ import { WidgetBase } from '@dojo/widget-core/WidgetBase'; import { WidgetProperties } from '@dojo/widget-core/interfaces'; import { ProjectorMixin } from '@dojo/widget-core/mixins/Projector'; import { StatefulMixin } from '@dojo/widget-core/mixins/Stateful'; -import ThemeableMixin, { theme } from '@dojo/widget-core/mixins/Themeable'; import { v, w } from '@dojo/widget-core/d'; import Menu from '../Menu'; import MenuItem from '../MenuItem'; -import * as appCss from './styles/app.css'; -import * as menuCss from './styles/menu.css'; -const AppBase = StatefulMixin(ThemeableMixin(WidgetBase)); +const AppBase = StatefulMixin(WidgetBase); -@theme(appCss) export class App extends AppBase { toggleAnimation(event: Event) { this.setState({ animate: !( event.target).checked }); @@ -38,8 +34,8 @@ export class App extends AppBase { render() { return v('div', [ - v('form', { classes: this.classes(appCss.options) }, [ - v('div', [ + v('form', { style: 'margin-bottom: 1.5em' }, [ + v('span', [ v('input', { id: 'toggleEvent', type: 'checkbox', @@ -50,7 +46,7 @@ export class App extends AppBase { }, [ 'Show on hover' ]) ]), - v('div', [ + v('span', [ v('input', { id: 'toggleDisabled', type: 'checkbox', @@ -61,7 +57,7 @@ export class App extends AppBase { }, [ 'Disable packages menu' ]) ]), - v('div', [ + v('span', [ v('input', { id: 'toggleAnimation', type: 'checkbox', @@ -73,9 +69,7 @@ export class App extends AppBase { ]) ]), - v('div', { - classes: this.classes(appCss.demos) - }, [ + v('div', [ this.renderFileMenu(), this.renderDojoMenu() ]) @@ -106,46 +100,46 @@ export class App extends AppBase { packageMenuHidden } = this.state; - return w(Menu, { - key: 'DojoMenu', - overrideClasses: menuCss - }, [ - w(MenuItem, { - key: 'DojoMenuLabel', - overrideClasses: menuCss - }, [ - v('a', { - href: 'http://dojo.io', - target: '_blank' - }, [ 'Dojo 2' ]) - ]), - + return v('div', { style: 'float: left;' }, [ w(Menu, { - animate: animate, - disabled: disabled, - expandOnClick: expandOnClick, - hidden: packageMenuHidden, - key: 'menu1-sub1', - label: 'Dojo 2 Packages', - nested: true, - onRequestHide: () => { - this.setState({ packageMenuHidden: true }); - }, - onRequestShow: () => { - this.setState({ packageMenuHidden: false }); - }, - overrideClasses: menuCss - }, packages.map((label, i) => { - return w(MenuItem, { - key: `menu1-sub1-item${i}`, - overrideClasses: menuCss + key: 'DojoMenu' + }, [ + w(MenuItem, { + key: 'DojoMenuLabel', + tabIndex: -1 }, [ v('a', { - href: `https://github.com/dojo/${label}`, + href: 'http://dojo.io', target: '_blank' - }, [ label ]) - ]); - })) + }, [ 'Dojo 2' ]) + ]), + + w(Menu, { + animate: animate, + disabled: disabled, + expandOnClick: expandOnClick, + hidden: packageMenuHidden, + key: 'menu1-sub1', + label: 'Dojo 2 Packages', + nested: true, + onRequestHide: () => { + this.setState({ packageMenuHidden: true }); + }, + onRequestShow: () => { + this.setState({ packageMenuHidden: false }); + } + }, packages.map((label, i) => { + return w(MenuItem, { + key: `menu1-sub1-item${i}`, + tabIndex: -1 + }, [ + v('a', { + href: `https://github.com/dojo/${label}`, + target: '_blank' + }, [ label ]) + ]); + })) + ]) ]); } @@ -154,38 +148,44 @@ export class App extends AppBase { return name.charAt(0).toLowerCase() + name.replace(/\s/g, '').slice(1); } - return w(Menu, { - key: 'ChromeFileMenu', - overrideClasses: menuCss - }, [ - [ - 'New Tab', - 'New Window', - 'New Incognito Window', - 'Reopen Closed Tab', - 'Open File...', - 'Open Location...' - ], - [ - 'Close Window', - 'Close Tab', - 'Save Page As...' - ], - [ 'Email Page Location' ], - [ 'Print' ] - ].map(group => { - return v('div', group.map(name => { - const key = getKey(name); - return w(MenuItem, { - key, - label: name, - overrideClasses: menuCss, - disabled: name === 'Open Location...', - onRequestSelect: this.toggleSelected.bind(this, key), - selected: this.state[`${key}Selected`] - }); - })); - })); + return v('div', { style: 'float: left; margin-right: 50px;' }, [ + w(Menu, { + key: 'ChromeFileMenu' + }, [ + [ + 'New Tab', + 'New Window', + 'New Incognito Window', + 'Reopen Closed Tab', + 'Open File...', + 'Open Location...' + ], + [ + 'Close Window', + 'Close Tab', + 'Save Page As...' + ], + [ 'Email Page Location' ], + [ 'Print' ] + ].map(group => { + return v('div', group.map(name => { + const key = getKey(name); + const toggleSelected: () => void = this.toggleSelected.bind(this, key); + return w(MenuItem, { + key, + disabled: name === 'Open Location...', + onKeypress: (event: KeyboardEvent) => { + const pressed = 'key' in event ? event.key : event.keyCode; + if (pressed === 'Enter') { + toggleSelected(); + } + }, + onClick: toggleSelected, + selected: this.state[`${key}Selected`] + }, [ name ]); + })); + })) + ]); } } diff --git a/src/menu/example/styles/app.css b/src/menu/example/styles/app.css deleted file mode 100644 index b5d6dbf38b..0000000000 --- a/src/menu/example/styles/app.css +++ /dev/null @@ -1,21 +0,0 @@ -.demos > * { - display: block; - margin-bottom: 25px; -} - -.options { - margin-bottom: 1.5em; -} - -.options > div { - display: inline-block; -} - -@media screen and (min-width: 500px) { - .demos > * { - float: left; - } - .demos > :first-child { - margin-right: 50px; - } -} diff --git a/src/menu/example/styles/app.css.d.ts b/src/menu/example/styles/app.css.d.ts deleted file mode 100644 index 934f8a61e0..0000000000 --- a/src/menu/example/styles/app.css.d.ts +++ /dev/null @@ -1,2 +0,0 @@ -export const demos: string; -export const options: string; diff --git a/src/menu/example/styles/menu.css b/src/menu/example/styles/menu.css deleted file mode 100644 index c77ad91879..0000000000 --- a/src/menu/example/styles/menu.css +++ /dev/null @@ -1,39 +0,0 @@ -@import '../../../common/styles/variables.css'; - -.menu { - background: #fff; - box-shadow: 0 0 2px 1px rgba(0,0,0,0.3); - font-family: sans-serif; - font-size: 14px; - padding: 5px 0; -} - -.menuItem { - color: var(--title-bar-bg); - padding: 5px 10px; -} - -.menuItem:focus, -.menuLabel:focus { - box-shadow: inset var(--input-focus-shadow); - outline: none; -} - -.menuLabel { - composes: menuItem; -} - -.disabled { - opacity: 0.3; -} - -.selected { - background-color: var(--title-bar-bg); - color: #fff; -} - -.subMenu { - box-shadow: 0 0; - box-sizing: border-box; - padding: 0 15px; -} diff --git a/src/menu/example/styles/menu.css.d.ts b/src/menu/example/styles/menu.css.d.ts deleted file mode 100644 index 4c83bfc878..0000000000 --- a/src/menu/example/styles/menu.css.d.ts +++ /dev/null @@ -1,6 +0,0 @@ -export const menu: string; -export const menuItem: string; -export const menuLabel: string; -export const disabled: string; -export const selected: string; -export const subMenu: string; diff --git a/src/menu/styles/menu.css b/src/menu/styles/menu.css index ec82a77493..a08d9e766b 100644 --- a/src/menu/styles/menu.css +++ b/src/menu/styles/menu.css @@ -1,27 +1,44 @@ +@import '../../common/styles/variables.css'; + .container { box-sizing: border-box; position: relative; } -.menu {} +.menu { + background: #fff; + box-shadow: 0 0 2px 1px rgba(0,0,0,0.3); + font-family: sans-serif; + font-size: 14px; + padding: 5px 0; +} .menuItem { + color: var(--title-bar-bg); cursor: pointer; display: block; + padding: 5px 10px; } .menuLabel { composes: menuItem; } +.menuItem:focus, +.menuLabel:focus { + box-shadow: inset var(--input-focus-shadow); + outline: none; +} + .disabled { cursor: default; + opacity: 0.3; } -/** - * Selected menu items. - */ -.selected {} +.selected { + background-color: var(--title-bar-bg); + color: #fff; +} .nestedMenuContainer, .nestedMenu { @@ -29,7 +46,10 @@ } .subMenu { + box-shadow: 0 0; + box-sizing: border-box; overflow: hidden; + padding: 0 15px; transition: height 0.5s ease-in-out; } diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 4d0e025c6d..46b862df07 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -48,6 +48,7 @@ function getMockNavElement() { get scrollHeight(){ return 300; }, + scrollTop: null, style: styles, classList: { add(name: string) { @@ -309,6 +310,7 @@ registerSuite({ const styleHistory = element.styleHistory; assert.sameMembers(styleHistory.height, [ null, '300px' ]); + assert.strictEqual(element.scrollTop, 0, 'The nav should be scrolled top'); }, 'animates to the max-height when set'() { From 035c8e675010e0a39d152f2f531e8f3f9bd8766f Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Fri, 24 Mar 2017 09:51:08 -0500 Subject: [PATCH 09/19] Move menu.css to menu.m.css --- src/common/styles/widgets.css | 2 +- src/menu/Menu.ts | 2 +- src/menu/MenuItem.ts | 2 +- src/menu/styles/{menu.css => menu.m.css} | 0 src/menu/styles/{menu.css.d.ts => menu.m.css.d.ts} | 0 src/menu/tests/unit/Menu.ts | 2 +- src/menu/tests/unit/MenuItem.ts | 2 +- 7 files changed, 5 insertions(+), 5 deletions(-) rename src/menu/styles/{menu.css => menu.m.css} (100%) rename src/menu/styles/{menu.css.d.ts => menu.m.css.d.ts} (100%) diff --git a/src/common/styles/widgets.css b/src/common/styles/widgets.css index 2c671158a8..346548a0bb 100644 --- a/src/common/styles/widgets.css +++ b/src/common/styles/widgets.css @@ -4,10 +4,10 @@ @import '../../checkbox/styles/checkbox.m.css'; @import '../../combobox/styles/comboBox.m.css'; @import '../../dialog/styles/dialog.m.css'; +@import '../../menu/styles/menu.m.css'; @import '../../radio/styles/radio.m.css'; @import '../../slidepane/styles/slidePane.m.css'; @import '../../slider/styles/slider.m.css'; @import '../../tabpane/styles/tabPane.m.css'; @import '../../textarea/styles/textarea.m.css'; @import '../../textinput/styles/textinput.m.css'; -@import '../../menu/styles/menu.m.css'; diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index fc614bda64..377237f539 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -6,7 +6,7 @@ import { DNode } from '@dojo/widget-core/interfaces'; import ThemeableMixin, { theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable'; import WidgetBase from '@dojo/widget-core/WidgetBase'; import MenuItem from './MenuItem'; -import * as css from './styles/menu.css'; +import * as css from './styles/menu.m.css'; export type Role = 'menu' | 'menubar'; diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts index a6cc908c78..e0e4009927 100644 --- a/src/menu/MenuItem.ts +++ b/src/menu/MenuItem.ts @@ -1,7 +1,7 @@ import { v } from '@dojo/widget-core/d'; import ThemeableMixin, { theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable'; import WidgetBase from '@dojo/widget-core/WidgetBase'; -import * as css from './styles/menu.css'; +import * as css from './styles/menu.m.css'; /** * @type MenuItemProperties diff --git a/src/menu/styles/menu.css b/src/menu/styles/menu.m.css similarity index 100% rename from src/menu/styles/menu.css rename to src/menu/styles/menu.m.css diff --git a/src/menu/styles/menu.css.d.ts b/src/menu/styles/menu.m.css.d.ts similarity index 100% rename from src/menu/styles/menu.css.d.ts rename to src/menu/styles/menu.m.css.d.ts diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 46b862df07..4270bf6d6d 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -4,7 +4,7 @@ import * as registerSuite from 'intern!object'; import * as assert from 'intern/chai!assert'; import * as sinon from 'sinon'; import Menu from '../../Menu'; -import * as css from '../../styles/menu.css'; +import * as css from '../../styles/menu.m.css'; function getStyle(element: any) { return { diff --git a/src/menu/tests/unit/MenuItem.ts b/src/menu/tests/unit/MenuItem.ts index 745723761b..cb72f0e825 100644 --- a/src/menu/tests/unit/MenuItem.ts +++ b/src/menu/tests/unit/MenuItem.ts @@ -1,7 +1,7 @@ import * as registerSuite from 'intern!object'; import * as assert from 'intern/chai!assert'; import MenuItem from '../../MenuItem'; -import * as css from '../../styles/menu.css'; +import * as css from '../../styles/menu.m.css'; registerSuite({ name: 'MenuItem', From 8223d6be6f7d3f0c094fcdec40f00b86156bcc97 Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Tue, 28 Mar 2017 15:33:42 -0500 Subject: [PATCH 10/19] Move menu colors into common variables --- src/common/styles/variables.css | 3 +++ src/menu/styles/menu.m.css | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/common/styles/variables.css b/src/common/styles/variables.css index a61e859a3b..72c4b3c504 100644 --- a/src/common/styles/variables.css +++ b/src/common/styles/variables.css @@ -51,4 +51,7 @@ --button-icon-font: 'iconFont'; --button-icon-size: 1.4em; --button-padding: 0.75em 1.25em; + --menu-bg: var(--dojo-white); + --menu-selected-bg: var(--dojo-blue); + --menu-selected-color: var(--dojo-white); } diff --git a/src/menu/styles/menu.m.css b/src/menu/styles/menu.m.css index a08d9e766b..422a64b93d 100644 --- a/src/menu/styles/menu.m.css +++ b/src/menu/styles/menu.m.css @@ -6,7 +6,7 @@ } .menu { - background: #fff; + background: var(--menu-bg); box-shadow: 0 0 2px 1px rgba(0,0,0,0.3); font-family: sans-serif; font-size: 14px; @@ -36,8 +36,8 @@ } .selected { - background-color: var(--title-bar-bg); - color: #fff; + background-color: var(--menu-selected-bg); + color: var(--menu-selected-color); } .nestedMenuContainer, From 9e2e27705252e9548483daacb5ecad570133db80 Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Tue, 28 Mar 2017 16:23:01 -0500 Subject: [PATCH 11/19] Menu and MenuItem updates - Use `onElementCreated` and `onElementUpdated` - Correct use of `KeyboardEvent#keyCode` - Privacy updates --- src/menu/Menu.ts | 191 ++++++++++++++++---------------- src/menu/MenuItem.ts | 28 ++--- src/menu/tests/unit/Menu.ts | 124 ++++++++++----------- src/menu/tests/unit/MenuItem.ts | 8 +- 4 files changed, 177 insertions(+), 174 deletions(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index 377237f539..fa6b226fda 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -83,66 +83,6 @@ export class Menu extends MenuBase { private _hideTimer: Handle; private _initialRender = true; - onLabelClick() { - const { - disabled, - expandOnClick = true - } = this.properties; - - if (!disabled && expandOnClick) { - this.toggleDisplay(); - } - } - - onLabelKeypress(event: KeyboardEvent) { - const { disabled } = this.properties; - const key = 'key' in event ? event.key : event.keyCode; - - if (!disabled && key === 'Enter') { - this.toggleDisplay(); - } - } - - onMenuFocus() { - const { disabled, hidden = this.getDefaultHidden() } = this.properties; - if (!disabled && hidden) { - this.toggleDisplay(true); - } - } - - onMenuMouseEnter() { - const { - disabled, - expandOnClick = true - } = this.properties; - - if (!disabled && !expandOnClick) { - this._hideTimer && this._hideTimer.destroy(); - this.toggleDisplay(true); - } - } - - onMenuMouseLeave() { - const { - disabled, - expandOnClick = true, - hideDelay = 300 - } = this.properties; - - if (!disabled && !expandOnClick) { - this._hideTimer && this._hideTimer.destroy(); - if (hideDelay) { - this._hideTimer = createTimer(() => { - this.toggleDisplay(false); - }, hideDelay); - this.own(this._hideTimer); - } - else { - this.toggleDisplay(false); - } - } - } - render(): DNode { const { id = uuid(), @@ -152,12 +92,11 @@ export class Menu extends MenuBase { const label = this.renderLabel(id); const menu = v('nav', { - id, - role, classes: this.classes.apply(this, this.getMenuClasses()), - afterCreate: this.afterCreate, - afterUpdate: this.afterUpdate, - onfocusin: this.onMenuFocus + id, + key: 'menu', + onfocusin: this.onMenuFocus, + role }, this.children); if (label) { @@ -187,35 +126,6 @@ export class Menu extends MenuBase { } } - protected afterCreate(element: HTMLElement) { - const { animate = true, hidden = this.getDefaultHidden() } = this.properties; - this._initialRender = false; - - if (animate) { - this.wasOpen = !hidden; - - if (hidden) { - element.style.height = '0'; - } - } - } - - protected afterUpdate(element: HTMLElement) { - const { animate = true, label } = this.properties; - - if (!label) { - return; - } - - if (!animate) { - // In case `animate` was previously `true`, remove any `height` property set on the node. - element.style.height = null; - return; - } - - this.animate(element); - } - protected animate(element: HTMLElement) { const { hidden = this.getDefaultHidden() } = this.properties; @@ -273,6 +183,99 @@ export class Menu extends MenuBase { return classes; } + protected onElementCreated(element: HTMLElement, key: string) { + if (key === 'menu') { + const { animate = true, hidden = this.getDefaultHidden() } = this.properties; + this._initialRender = false; + + if (animate) { + this.wasOpen = !hidden; + + if (hidden) { + element.style.height = '0'; + } + } + } + } + + protected onElementUpdated(element: HTMLElement, key: string) { + if (key === 'menu') { + const { animate = true, label } = this.properties; + + if (!label) { + return; + } + + if (!animate) { + // In case `animate` was previously `true`, remove any `height` property set on the node. + element.style.height = null; + return; + } + + this.animate(element); + } + } + + protected onLabelClick() { + const { + disabled, + expandOnClick = true + } = this.properties; + + if (!disabled && expandOnClick) { + this.toggleDisplay(); + } + } + + protected onLabelKeypress(event: KeyboardEvent) { + const { disabled } = this.properties; + const key: string | number = 'key' in event ? event.key : event.keyCode; + + if (!disabled && (key === 'Enter' || key === 13)) { + this.toggleDisplay(); + } + } + + protected onMenuFocus() { + const { disabled, hidden = this.getDefaultHidden() } = this.properties; + if (!disabled && hidden) { + this.toggleDisplay(true); + } + } + + protected onMenuMouseEnter() { + const { + disabled, + expandOnClick = true + } = this.properties; + + if (!disabled && !expandOnClick) { + this._hideTimer && this._hideTimer.destroy(); + this.toggleDisplay(true); + } + } + + protected onMenuMouseLeave() { + const { + disabled, + expandOnClick = true, + hideDelay = 300 + } = this.properties; + + if (!disabled && !expandOnClick) { + this._hideTimer && this._hideTimer.destroy(); + if (hideDelay) { + this._hideTimer = createTimer(() => { + this.toggleDisplay(false); + }, hideDelay); + this.own(this._hideTimer); + } + else { + this.toggleDisplay(false); + } + } + } + protected toggleDisplay(requestShow?: boolean) { const { onRequestHide, diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts index e0e4009927..4171bce239 100644 --- a/src/menu/MenuItem.ts +++ b/src/menu/MenuItem.ts @@ -52,20 +52,6 @@ export const MenuItemBase = ThemeableMixin(WidgetBase); @theme(css) export class MenuItem extends MenuItemBase { - onClick(event: MouseEvent) { - const { disabled, onClick } = this.properties; - if (!disabled && typeof onClick === 'function') { - onClick(event); - } - } - - onKeypress(event: KeyboardEvent) { - const { disabled, onKeypress } = this.properties; - if (!disabled && typeof onKeypress === 'function') { - onKeypress(event); - } - } - render() { const { controls, @@ -95,6 +81,20 @@ export class MenuItem extends MenuItemBase { tabIndex : disabled ? -1 : tabIndex }, this.children); } + + protected onClick(event: MouseEvent) { + const { disabled, onClick } = this.properties; + if (!disabled && typeof onClick === 'function') { + onClick(event); + } + } + + protected onKeypress(event: KeyboardEvent) { + const { disabled, onKeypress } = this.properties; + if (!disabled && typeof onKeypress === 'function') { + onKeypress(event); + } + } } export default MenuItem; diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 4270bf6d6d..cfddc09fd2 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -124,8 +124,8 @@ registerSuite({ let vnode: any = menu.__render__(); let element: any = getMockNavElement(); - vnode.properties.afterCreate.call(menu, element); - vnode.properties.afterUpdate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); + ( menu).onElementUpdated(element, 'menu'); assert.isTrue(vnode.properties.classes[css.hidden]); menu.setProperties({ @@ -135,18 +135,17 @@ registerSuite({ vnode = menu.__render__(); element = getMockNavElement(); - vnode.properties.afterCreate.call(menu, element); - vnode.properties.afterUpdate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); + ( menu).onElementUpdated(element, 'menu'); assert.isTrue(vnode.properties.classes[css.visible]); }, 'not animated'() { const menu = new Menu(); - const vnode: any = menu.__render__(); const element = getMockNavElement(); - vnode.properties.afterCreate.call(menu, element); - vnode.properties.afterUpdate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); + ( menu).onElementUpdated(element, 'menu'); const styleHistory = element.styleHistory; assert.sameMembers(styleHistory.height, [ null ]); @@ -165,8 +164,8 @@ registerSuite({ let menuNode = vnode.children[1]; let element: any = getMockNavElement(); - menuNode.properties.afterCreate.call(menu, element); - menuNode.properties.afterUpdate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); + ( menu).onElementUpdated(element, 'menu'); assert.isTrue(menuNode.properties.classes[css.hidden]); menu.setProperties({ @@ -179,8 +178,8 @@ registerSuite({ menuNode = vnode.children[1]; element = getMockNavElement(); - menuNode.properties.afterCreate.call(menu, element); - menuNode.properties.afterUpdate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); + ( menu).onElementUpdated(element, 'menu'); assert.isTrue(menuNode.properties.classes[css.visible]); }, @@ -190,10 +189,8 @@ registerSuite({ animate: false, label: 'Menu label' }); - const vnode: any = menu.__render__(); - const menuNode = vnode.children[1]; const element: any = getMockNavElement(); - menuNode.properties.afterCreate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); assert.isNull(element.style.height, 'style.height should not be modified'); }, @@ -206,10 +203,8 @@ registerSuite({ label: 'Menu label' }); - let vnode: any = menu.__render__(); - let menuNode = vnode.children[1]; - menuNode.properties.afterCreate.call(menu, element); - menuNode.properties.afterUpdate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); + ( menu).onElementUpdated(element, 'menu'); const styleHistory = element.styleHistory; assert.sameMembers(styleHistory.height, [ null, null ], 'style.height should be reset'); @@ -227,14 +222,16 @@ registerSuite({ let vnode: any = menu.__render__(); let menuNode = vnode.children[1]; - menuNode.properties.afterCreate.call(menu, element); + ( menu).onElementCreated(element); + ( menu).onElementCreated(element, 'menu'); menu.setProperties({ label: 'Other label' }); vnode = menu.__render__(); menuNode = vnode.children[1]; - menuNode.properties.afterUpdate.call(menu, element); + ( menu).onElementUpdated(element); + ( menu).onElementUpdated(element, 'menu'); assert.notOk(menuNode.properties.classes[css.hidden]); }, @@ -245,16 +242,14 @@ registerSuite({ label: 'Menu label' }); - const vnode: any = menu.__render__(); - const menuNode = vnode.children[1]; let element = getMockNavElement(); - menuNode.properties.afterCreate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); assert.strictEqual(element.style.height, '0'); menu.setProperties({ hidden: false, label: 'Menu label' }); element = getMockNavElement(); - menuNode.properties.afterCreate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); assert.isNull(element.style.height); }, @@ -264,12 +259,10 @@ registerSuite({ label: 'Menu label' }); - const vnode: any = menu.__render__(); - const menuNode = vnode.children[1]; const element = getMockNavElement(); - menuNode.properties.afterCreate.call(menu, element); - menuNode.properties.afterUpdate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); + ( menu).onElementUpdated(element, 'menu'); const styleHistory = element.styleHistory; assert.sameMembers(styleHistory.height, [ null, '300px', '0' ]); @@ -282,13 +275,11 @@ registerSuite({ label: 'Menu label' }); - const vnode: any = menu.__render__(); - const menuNode = vnode.children[1]; const element = getMockNavElement(); element.style['max-height'] = '100px'; - menuNode.properties.afterCreate.call(menu, element); - menuNode.properties.afterUpdate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); + ( menu).onElementUpdated(element, 'menu'); const styleHistory = element.styleHistory; assert.sameMembers(styleHistory.height, [ null, '100px', '0' ]); @@ -301,12 +292,10 @@ registerSuite({ label: 'Menu label' }); - const vnode: any = menu.__render__(); - const menuNode = vnode.children[1]; const element = getMockNavElement(); - menuNode.properties.afterCreate.call(menu, element); - menuNode.properties.afterUpdate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); + ( menu).onElementUpdated(element, 'menu'); const styleHistory = element.styleHistory; assert.sameMembers(styleHistory.height, [ null, '300px' ]); @@ -320,13 +309,11 @@ registerSuite({ label: 'Menu label' }); - const vnode: any = menu.__render__(); - const menuNode = vnode.children[1]; const element = getMockNavElement(); element.style['max-height'] = '100px'; - menuNode.properties.afterCreate.call(menu, element); - menuNode.properties.afterUpdate.call(menu, element); + ( menu).onElementCreated(element, 'menu'); + ( menu).onElementUpdated(element, 'menu'); const styleHistory = element.styleHistory; assert.sameMembers(styleHistory.height, [ null, '100px' ]); @@ -377,7 +364,7 @@ registerSuite({ menu.setProperties({ hidden: false }); } }); - menu.onLabelClick(); + ( menu).onLabelClick(); assert.isFalse(menu.properties.hidden, 'menu should not be hidden'); }, @@ -391,7 +378,7 @@ registerSuite({ menu.setProperties({ hidden: true }); } }); - menu.onLabelClick(); + ( menu).onLabelClick(); assert.isTrue(menu.properties.hidden, 'menu should be hidden'); }, @@ -405,7 +392,7 @@ registerSuite({ menu.setProperties({ hidden: false }); } }); - menu.onLabelClick(); + ( menu).onLabelClick(); assert.isTrue(menu.properties.hidden, 'menu should not be shown on click when `expandOnClick` is false'); }, @@ -421,7 +408,7 @@ registerSuite({ } }); - menu.onLabelKeypress( { key: 'Enter' }); + ( menu).onLabelKeypress( { key: 'Enter' }); assert.isTrue(menu.properties.hidden, 'menu should remain hidden when disabled'); }, @@ -434,7 +421,20 @@ registerSuite({ } }); - menu.onLabelKeypress( { key: 'Enter' }); + ( menu).onLabelKeypress( { key: 'Enter' }); + assert.isFalse(menu.properties.hidden); + }, + + 'when `key` is not supported'() { + const menu = new Menu(); + menu.setProperties({ + hidden: true, + onRequestShow() { + menu.setProperties({ hidden: false }); + } + }); + + ( menu).onLabelKeypress( { keyCode: 13 }); assert.isFalse(menu.properties.hidden); } }, @@ -450,7 +450,7 @@ registerSuite({ } }); - menu.onMenuFocus(); + ( menu).onMenuFocus(); assert.isTrue(menu.properties.hidden, 'menu should remain hidden when disabled'); }, @@ -463,7 +463,7 @@ registerSuite({ } }); - menu.onMenuFocus(); + ( menu).onMenuFocus(); assert.isFalse(menu.properties.hidden, 'menu should open when focused'); }, @@ -480,7 +480,7 @@ registerSuite({ } }); - menu.onMenuFocus(); + ( menu).onMenuFocus(); assert.isFalse(hideCalled, 'onRequestHide not called'); assert.isFalse(showCalled, 'onRequestShow not called'); } @@ -495,7 +495,7 @@ registerSuite({ menu.setProperties({ hidden: false }); } }); - menu.onMenuMouseEnter(); + ( menu).onMenuMouseEnter(); assert.isTrue(menu.properties.hidden, 'mouseenter should be ignored'); }, @@ -510,7 +510,7 @@ registerSuite({ } }); - menu.onMenuMouseEnter(); + ( menu).onMenuMouseEnter(); assert.isFalse(menu.properties.hidden, 'mouseenter should not be ignored'); } }, @@ -525,7 +525,7 @@ registerSuite({ menu.setProperties({ hidden: true }); } }); - menu.onMenuMouseLeave(); + ( menu).onMenuMouseLeave(); assert.isFalse(menu.properties.hidden, 'mouseleave should be ignored'); }, @@ -541,7 +541,7 @@ registerSuite({ } }); - menu.onMenuMouseLeave(); + ( menu).onMenuMouseLeave(); assert.isTrue(menu.properties.hidden, 'mouseleave should not be ignored'); } }, @@ -556,7 +556,7 @@ registerSuite({ } }); menu.__render__(); - menu.onLabelClick(); + ( menu).onLabelClick(); assert.isUndefined(menu.properties.hidden, 'menu should not be displayed when disabled'); }, @@ -599,7 +599,7 @@ registerSuite({ } }); - menu.onMenuMouseLeave(); + ( menu).onMenuMouseLeave(); assert.isFalse(menu.properties.hidden, 'menu should not be hidden immediately'); setTimeout(dfd.callback(() => { assert.isTrue(menu.properties.hidden, 'menu should be hidden after a delay'); @@ -618,8 +618,8 @@ registerSuite({ } }); - menu.onMenuMouseLeave(); - menu.onMenuMouseEnter(); + ( menu).onMenuMouseLeave(); + ( menu).onMenuMouseEnter(); setTimeout(dfd.callback(() => { assert.isFalse(menu.properties.hidden, 'menu should not be hidden after show request'); }), 200); @@ -640,11 +640,11 @@ registerSuite({ } }); - menu.onMenuMouseLeave(); - menu.onMenuMouseLeave(); - menu.onMenuMouseLeave(); - menu.onMenuMouseLeave(); - menu.onMenuMouseLeave(); + ( menu).onMenuMouseLeave(); + ( menu).onMenuMouseLeave(); + ( menu).onMenuMouseLeave(); + ( menu).onMenuMouseLeave(); + ( menu).onMenuMouseLeave(); setTimeout(dfd.callback(() => { assert.strictEqual(callCount, 1, 'hide request should be called once'); @@ -662,7 +662,7 @@ registerSuite({ } }); - menu.onMenuMouseLeave(); + ( menu).onMenuMouseLeave(); menu.destroy(); setTimeout(dfd.callback(() => { assert.isFalse(menu.properties.hidden, 'menu should not be hidden after the menu is destroyed'); diff --git a/src/menu/tests/unit/MenuItem.ts b/src/menu/tests/unit/MenuItem.ts index cb72f0e825..a5fd52c298 100644 --- a/src/menu/tests/unit/MenuItem.ts +++ b/src/menu/tests/unit/MenuItem.ts @@ -30,7 +30,7 @@ registerSuite({ } }); - item.onClick( {}); + ( item).onClick( {}); assert.isFalse(called, '`onClick` should not be called when the menu item is disabled.'); }, @@ -43,7 +43,7 @@ registerSuite({ } }); - item.onClick( {}); + ( item).onClick( {}); assert.isTrue(called, '`onClick` should be called when the menu item is enabled.'); } }, @@ -59,7 +59,7 @@ registerSuite({ } }); - item.onKeypress( { type: 'keypress' }); + ( item).onKeypress( { type: 'keypress' }); assert.isUndefined(event, '`onKeypress` should not be called when the menu item is disabled.'); }, @@ -72,7 +72,7 @@ registerSuite({ } }); - item.onKeypress( { type: 'keypress' }); + ( item).onKeypress( { type: 'keypress' }); assert.strictEqual(event!.type, 'keypress', '`onKeypress` should be called when the menu item is enabled.'); } }, From f57452a08732bedf2f6d664f54c80ecb02515f06 Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Wed, 29 Mar 2017 16:50:55 -0500 Subject: [PATCH 12/19] Menu and MenuItem updates `MenuItem`: - Allow tag and vnode properties to be set. - Fix ARIA properties. - Allow different `role`s to be applied. `Menu`: - Update keyboard handling to more accurately match the WAI-ARIA spec. - Add handling for right/left arrows, space key. - Internalize active index management. - Add an `orientation` property. --- src/menu/Menu.ts | 228 +++++++++++++++++++++------- src/menu/MenuItem.ts | 103 +++++++------ src/menu/example/index.ts | 97 ++++++------ src/menu/styles/menu.m.css | 9 ++ src/menu/styles/menu.m.css.d.ts | 1 + src/menu/tests/unit/Menu.ts | 257 ++++++++++++++++++++++++++++---- src/menu/tests/unit/MenuItem.ts | 149 +++++++++++++----- 7 files changed, 632 insertions(+), 212 deletions(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index fa6b226fda..4a80f9a582 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -1,62 +1,39 @@ +import { assign } from '@dojo/core/lang'; import { createTimer } from '@dojo/core/util'; import uuid from '@dojo/core/uuid'; import { Handle } from '@dojo/interfaces/core'; import { v, w } from '@dojo/widget-core/d'; import { DNode } from '@dojo/widget-core/interfaces'; +import StatefulMixin from '@dojo/widget-core/mixins/Stateful'; import ThemeableMixin, { theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable'; import WidgetBase from '@dojo/widget-core/WidgetBase'; import MenuItem from './MenuItem'; import * as css from './styles/menu.m.css'; +export type Orientation = 'horizontal' | 'vertical'; + export type Role = 'menu' | 'menubar'; /** * @type MenuProperties * - * Properties that can be set on a Menu component - * - * @property animate - * Only applies to nested menus. A flag indicating whether the widget instance should handle animating between the - * visible and hidden states. If true (the default), then the menu will slide into and out of view like an accordion. - * If false, then any animation should be handled in the CSS, as the menu will just snap open/shut. - * - * @property disabled - * Indicates whether the menu is disabled. If true, then the widget will ignore events. - * - * @property expandOnClick - * Indicates whether the menu should be displayed/hidden via a click event. If false, then the menu is toggled - * via a hover event. Defaults to true. - * - * @property hideDelay - * Only applies to menus toggled into view with a hover event. The amount of time (in milliseconds) after a - * mouseleave event that should pass before the menu is hidden. Defaults to 300ms. - * - * @property hidden - * Whether the menu is hidden. Defaults to true if a label is specified (i.e., the menu is controlled by a - * link); false otherwise. - * - * @property id - * The widget ID. Defaults to a random string. + * Properties that can be set on a Menu component. * - * @property label - * A widget that will be wrapped in a MenuItem widget that will be used to control the menu. - * - * @property nested - * Indicates whether the menu is nested within another menu. Useful for styling, this does not affect - * functionality. Defaults to false. - * - * @property onRequestHide - * Needed only when a label is used. Called when the menu is displayed, and the label is triggered. - * This method should be used to update the widget's `hidden` property. - * - * @property onRequestShow - * Needed only when a label is used. Called when the menu is hidden, and the label is triggered. - * This method should be used to update the widget's `hidden` property. - * - * @property role - * The value to use for the menu's `role` property. Defaults to "menu". + * @property active Determines whether the menu trigger is active (should have focus). + * @property animate Determines whether animation should be handled internally. + * @property disabled Determines whether the menu is disabled. + * @property expandOnClick Determines whether a menu is displayed on click (default) or hover. + * @property hideDelay The amount of time (in milliseconds) after mouseleave before hiding the menu. + * @property hidden Determines whether the menu is hidden. + * @property id The widget ID. Defaults to a random string. + * @property label A DNode to use as the trigger for a nested menu. + * @property nested Indicates whether the menu is nested within another menu. + * @property onRequestHide Called when the menu is displayed and the trigger is activated. + * @property onRequestShow Called when the menu is hidden and the trigger is activated. + * @property role The value to use for the menu's `role` property. Defaults to 'menu'. */ export interface MenuProperties extends ThemeableProperties { + active?: boolean; animate?: boolean; disabled?: boolean; expandOnClick?: boolean; @@ -67,6 +44,7 @@ export interface MenuProperties extends ThemeableProperties { nested?: boolean; onRequestHide?: () => void; onRequestShow?: () => void; + orientation?: Orientation; role?: Role; } @@ -75,13 +53,42 @@ function getMenuHeight(menuElement: HTMLElement): number { return Math.min(menuElement.scrollHeight, (isNaN(maxHeight) ? Infinity : maxHeight)); } -export const MenuBase = ThemeableMixin(WidgetBase); +export const enum Operation { + decrease, + increase, + reset +} + +const commonKeys = { + enter: 13, + escape: 27, + tab: 9 +}; + +const horizontalKeys = assign({ + ascend: 38, // up arrow + decrease: 37, // left arrow + descend: 40, // down arrow + increase: 39 // right arrow +}, commonKeys); + +const verticalKeys = assign({ + ascend: 37, // left arrow + decrease: 38, // up arrow + descend: 39, // right arrow + increase: 40 // down arrow +}, commonKeys); + +export const MenuBase = StatefulMixin(ThemeableMixin(WidgetBase)); @theme(css) export class Menu extends MenuBase { protected wasOpen = false; + private _activeIndex = 0; + private _domNode: HTMLElement; private _hideTimer: Handle; private _initialRender = true; + private _isLabelActive = false; render(): DNode { const { @@ -91,15 +98,19 @@ export class Menu extends MenuBase { } = this.properties; const label = this.renderLabel(id); - const menu = v('nav', { + const menu = v('div', { classes: this.classes.apply(this, this.getMenuClasses()), id, key: 'menu', - onfocusin: this.onMenuFocus, - role - }, this.children); + onfocus: this.onMenuFocus, + onfocusout: this.onMenuFocusout, + onkeydown: this.onMenuKeydown, + role, + tabIndex: this.state.active || label ? -1 : 0 + }, this.renderChildren()); if (label) { + this._isLabelActive = false; return v('div', { classes: this.classes(css.container, nested ? css.nestedMenuContainer : null), onmouseenter: this.onMenuMouseEnter, @@ -111,17 +122,19 @@ export class Menu extends MenuBase { } renderLabel(id: string): DNode | void { - const { disabled, hidden = this.getDefaultHidden(), label, overrideClasses } = this.properties; + const { active, disabled, hidden = this.getDefaultHidden(), label, overrideClasses } = this.properties; + const labelActive = this._isLabelActive || active; if (label) { return w(MenuItem, { + active: labelActive, controls: id, disabled, expanded: !hidden, hasMenu: true, overrideClasses: overrideClasses || css, onClick: this.onLabelClick, - onKeypress: this.onLabelKeypress + onKeydown: this.onLabelKeydown }, [ label ]); } } @@ -153,6 +166,15 @@ export class Menu extends MenuBase { }); } + protected exitMenu() { + const { label, hidden = this.getDefaultHidden() } = this.properties; + + if (label && !hidden) { + this._isLabelActive = true; + this.setState({ active: false }); + } + } + protected getDefaultHidden() { const { disabled, label } = this.properties; @@ -163,8 +185,19 @@ export class Menu extends MenuBase { return label ? true : false; } + protected getDefaultOrientation(): Orientation { + const { role = 'menu' } = this.properties; + return role === 'menubar' ? 'horizontal' : 'vertical'; + } + protected getMenuClasses() { - const { animate = true, hidden = this.getDefaultHidden(), label, nested } = this.properties; + const { + animate = true, + hidden = this.getDefaultHidden(), + label, + orientation = this.getDefaultOrientation(), + nested + } = this.properties; const isSubMenu = Boolean(label); const classes = [ css.menu ]; @@ -172,6 +205,10 @@ export class Menu extends MenuBase { classes.push(hidden ? css.hidden : css.visible); } + if (orientation === 'horizontal') { + classes.push(css.horizontal); + } + if (nested) { classes.push(css.nestedMenu); } @@ -183,10 +220,27 @@ export class Menu extends MenuBase { return classes; } + protected moveActiveIndex(operation: Operation) { + if (operation === Operation.reset) { + this._activeIndex = 0; + this.toggleDisplay(false); + } + else { + const max = this.children.length; + const previousIndex = this._activeIndex; + this._activeIndex = operation === Operation.decrease ? + previousIndex - 1 < 0 ? max - 1 : previousIndex - 1 : + Math.min(previousIndex + 1, max) % max; + + this.invalidate(); + } + } + protected onElementCreated(element: HTMLElement, key: string) { if (key === 'menu') { const { animate = true, hidden = this.getDefaultHidden() } = this.properties; this._initialRender = false; + this._domNode = element; if (animate) { this.wasOpen = !hidden; @@ -227,19 +281,62 @@ export class Menu extends MenuBase { } } - protected onLabelKeypress(event: KeyboardEvent) { - const { disabled } = this.properties; - const key: string | number = 'key' in event ? event.key : event.keyCode; + protected onLabelKeydown(event: KeyboardEvent) { + const { disabled, hidden = this.getDefaultHidden(), label, orientation = this.getDefaultOrientation() } = this.properties; + const keys = orientation === 'horizontal' ? horizontalKeys : verticalKeys; - if (!disabled && (key === 'Enter' || key === 13)) { - this.toggleDisplay(); + if (!disabled) { + const key = event.keyCode; + + if (key === keys.enter) { // enter + this.toggleDisplay(); + } + else if (key === keys.descend && label && !hidden) { + this.setState({ active: true }); + } } } protected onMenuFocus() { - const { disabled, hidden = this.getDefaultHidden() } = this.properties; - if (!disabled && hidden) { - this.toggleDisplay(true); + this.setState({ active: true }); + } + + protected onMenuFocusout() { + if (this.properties.label) { + requestAnimationFrame(() => { + if (!this._domNode.contains(document.activeElement)) { + this.setState({ active: false }); + } + }); + } + } + + protected onMenuKeydown(event: KeyboardEvent) { + const { orientation = this.getDefaultOrientation() } = this.properties; + const keys = orientation === 'horizontal' ? horizontalKeys : verticalKeys; + + switch (event.keyCode) { + case keys.tab: + event.stopPropagation(); + this.setState({ active: false }); + break; + case keys.ascend: + event.stopPropagation(); + this.exitMenu(); + break; + case keys.decrease: + event.stopPropagation(); + this.moveActiveIndex(Operation.decrease); + break; + case keys.increase: + event.stopPropagation(); + this.moveActiveIndex(Operation.increase); + break; + case keys.escape: + event.stopPropagation(); + this._isLabelActive = true; + this.moveActiveIndex(Operation.reset); + break; } } @@ -276,6 +373,21 @@ export class Menu extends MenuBase { } } + protected renderChildren() { + const { hidden = this.getDefaultHidden() } = this.properties; + const activeIndex = this.state.active && !this._isLabelActive ? this._activeIndex : null; + + if (!hidden) { + this.children.forEach((child: any, i) => { + if (child && child.properties) { + child.properties.active = i === activeIndex; + } + }); + } + + return this.children; + } + protected toggleDisplay(requestShow?: boolean) { const { onRequestHide, @@ -288,9 +400,11 @@ export class Menu extends MenuBase { } if (requestShow) { + this.setState({ active: true }); typeof onRequestShow === 'function' && onRequestShow(); } else { + this.setState({ active: false }); typeof onRequestHide === 'function' && onRequestHide(); } } diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts index 4171bce239..a23307c30f 100644 --- a/src/menu/MenuItem.ts +++ b/src/menu/MenuItem.ts @@ -1,66 +1,67 @@ +import { assign } from '@dojo/core/lang'; import { v } from '@dojo/widget-core/d'; +import { VirtualDomProperties } from '@dojo/widget-core/interfaces'; import ThemeableMixin, { theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable'; import WidgetBase from '@dojo/widget-core/WidgetBase'; import * as css from './styles/menu.m.css'; +export type MenuItemType = 'item' | 'checkbox' | 'radio'; + /** * @type MenuItemProperties * - * Properties that can be set on a MenuItem component - * - * @property controls - * ID of an element that this input controls - * - * @property disabled - * Indicates whether the menu is disabled. If true, then the widget will ignore events. - * - * @property expanded - * A flag indicating whether a widget controlled by `this` is expanded. - * - * @property hasDropDown - * A flag indicating whether the widget has a drop down child. - * - * @property hasMenu - * A flag indicating whether the widget is used as the label for a menu widget. If `true`, - * then the `menuLabel` CSS class is applied instead of the `menuItem` class. + * Properties that can be set on a MenuItem component. * - * @property onClick - * An event handler for click events. - * - * @property onKeypress - * An event handler for keypress events. - * - * @property selected - * Indicates whether the widget is selected. - * - * @property tabIndex - * The tab index. Defaults to 0, and is forced to -1 if the widget is disabled. + * @property active Determines whether the item should receive focus. + * @property controls The ID of an element that this input controls. + * @property disabled Indicates whether the item is disabled. + * @property expanded Indicates whether a widget controlled by `this` is expanded. + * @property hasMenu Indicates whether the widget is used as the label for a menu. + * @property hasPopup Indicates whether the widget has a drop down child. + * @property onClick Called when the widget is activated via a click or space key. + * @property onKeydown Called when keys are pressed while the widget has focus. + * @property properties Additional properties for the widget's vnode. + * @property selected Determines whether the widget is marked as selected. + * @property tag The HTML tag name to use for the widget's vnode. Defaults to 'span'. + * @property type The type of the menu item. Defaults to 'item'. */ export interface MenuItemProperties extends ThemeableProperties { + active?: boolean; controls?: string; disabled?: boolean; expanded?: boolean; - hasDropDown?: boolean; hasMenu?: boolean; + hasPopup?: boolean; onClick?: (event: MouseEvent) => void; - onKeypress?: (event: KeyboardEvent) => void; + onKeydown?: (event: KeyboardEvent) => void; + properties?: VirtualDomProperties; selected?: boolean; - tabIndex?: number; + tag?: string; + type?: MenuItemType; } export const MenuItemBase = ThemeableMixin(WidgetBase); +const roleMap: { [key: string]: string } = { + item: 'menuitem', + checkbox: 'menuitemcheckbox', + radio: 'menuitemradio' +}; + @theme(css) export class MenuItem extends MenuItemBase { render() { const { + active = false, controls, - disabled, - expanded, - hasDropDown = false, + disabled = false, + expanded = false, + hasPopup = false, hasMenu = false, + properties, selected, - tabIndex = 0 + tag = 'span', + type = 'item' } = this.properties; const classes = this.classes( @@ -69,17 +70,18 @@ export class MenuItem extends MenuItemBase { selected ? css.selected : null ); - return v('span', { + return v(tag, assign(Object.create(null), properties, { 'aria-controls': controls, - 'aria-expanded': expanded, - 'aria-hasdropdown': hasDropDown, - 'aria-disabled': disabled, + 'aria-expanded': String(expanded), + 'aria-haspopup': hasPopup ? 'true' : undefined, + 'aria-disabled': String(disabled), classes, + key: 'menu-item', onclick: this.onClick, - onkeypress: this.onKeypress, - role: 'menuitem', - tabIndex : disabled ? -1 : tabIndex - }, this.children); + onkeydown: this.onKeydown, + role: roleMap[type], + tabIndex: active ? 0 : -1 + }), this.children); } protected onClick(event: MouseEvent) { @@ -89,10 +91,17 @@ export class MenuItem extends MenuItemBase { } } - protected onKeypress(event: KeyboardEvent) { - const { disabled, onKeypress } = this.properties; - if (!disabled && typeof onKeypress === 'function') { - onKeypress(event); + protected onElementUpdated(element: HTMLElement) { + this.properties.active && element.focus(); + } + + protected onKeydown(event: KeyboardEvent) { + const { disabled, onKeydown } = this.properties; + if (!disabled) { + if (event.keyCode === 32) { // space + ( event.target).click(); + } + typeof onKeydown === 'function' && onKeydown(event); } } } diff --git a/src/menu/example/index.ts b/src/menu/example/index.ts index 8f01a85db8..504343e2bd 100644 --- a/src/menu/example/index.ts +++ b/src/menu/example/index.ts @@ -3,7 +3,7 @@ import { WidgetProperties } from '@dojo/widget-core/interfaces'; import { ProjectorMixin } from '@dojo/widget-core/mixins/Projector'; import { StatefulMixin } from '@dojo/widget-core/mixins/Stateful'; import { v, w } from '@dojo/widget-core/d'; -import Menu from '../Menu'; +import Menu, { Orientation } from '../Menu'; import MenuItem from '../MenuItem'; const AppBase = StatefulMixin(WidgetBase); @@ -27,6 +27,10 @@ export class App extends AppBase { }); } + toggleOrientation(event: Event) { + this.setState({ isFileMenuHorizontal: ( event.target).checked }); + } + toggleSelected(key: string) { const selectedKey = `${key}Selected`; this.setState({ [selectedKey]: !this.state[selectedKey] }); @@ -66,6 +70,17 @@ export class App extends AppBase { v('label', { for: 'toggleAnimation' }, [ 'Disable package menu animation' ]) + ]), + + v('span', [ + v('input', { + id: 'toggleOrientation', + type: 'checkbox', + onchange: this.toggleOrientation + }), + v('label', { + for: 'toggleOrientation' + }, [ 'Render file menu horizontally' ]) ]) ]), @@ -106,13 +121,12 @@ export class App extends AppBase { }, [ w(MenuItem, { key: 'DojoMenuLabel', - tabIndex: -1 - }, [ - v('a', { + tag: 'a', + properties: { href: 'http://dojo.io', target: '_blank' - }, [ 'Dojo 2' ]) - ]), + } + }, [ 'Dojo 2' ]), w(Menu, { animate: animate, @@ -131,13 +145,12 @@ export class App extends AppBase { }, packages.map((label, i) => { return w(MenuItem, { key: `menu1-sub1-item${i}`, - tabIndex: -1 - }, [ - v('a', { + tag: 'a', + properties: { href: `https://github.com/dojo/${label}`, target: '_blank' - }, [ label ]) - ]); + } + }, [ label ]); })) ]) ]); @@ -148,42 +161,36 @@ export class App extends AppBase { return name.charAt(0).toLowerCase() + name.replace(/\s/g, '').slice(1); } - return v('div', { style: 'float: left; margin-right: 50px;' }, [ + return v('div', { style: 'float: left; margin: 0 50px 50px 0;' }, [ w(Menu, { - key: 'ChromeFileMenu' + key: 'ChromeFileMenu', + orientation: this.state['isFileMenuHorizontal'] ? 'horizontal' : 'vertical' as Orientation }, [ - [ - 'New Tab', - 'New Window', - 'New Incognito Window', - 'Reopen Closed Tab', - 'Open File...', - 'Open Location...' - ], - [ - 'Close Window', - 'Close Tab', - 'Save Page As...' - ], - [ 'Email Page Location' ], - [ 'Print' ] - ].map(group => { - return v('div', group.map(name => { - const key = getKey(name); - const toggleSelected: () => void = this.toggleSelected.bind(this, key); - return w(MenuItem, { - key, - disabled: name === 'Open Location...', - onKeypress: (event: KeyboardEvent) => { - const pressed = 'key' in event ? event.key : event.keyCode; - if (pressed === 'Enter') { - toggleSelected(); - } - }, - onClick: toggleSelected, - selected: this.state[`${key}Selected`] - }, [ name ]); - })); + 'New Tab', + 'New Window', + 'New Incognito Window', + 'Reopen Closed Tab', + 'Open File...', + 'Open Location...', + 'Close Window', + 'Close Tab', + 'Save Page As...', + 'Email Page Location', + 'Print' + ].map((name, i) => { + const key = getKey(name); + const toggleSelected: () => void = this.toggleSelected.bind(this, key); + return w(MenuItem, { + key, + disabled: name === 'Open Location...', + onKeydown: (event: KeyboardEvent) => { + if (event.keyCode === 13) { + toggleSelected(); + } + }, + onClick: toggleSelected, + selected: this.state[`${key}Selected`] + }, [ name ]); })) ]); } diff --git a/src/menu/styles/menu.m.css b/src/menu/styles/menu.m.css index 422a64b93d..75ce3cbfac 100644 --- a/src/menu/styles/menu.m.css +++ b/src/menu/styles/menu.m.css @@ -13,6 +13,10 @@ padding: 5px 0; } +.menu:focus { + outline: none; +} + .menuItem { color: var(--title-bar-bg); cursor: pointer; @@ -55,3 +59,8 @@ .hidden { height: 0; } .visible { height: auto; } + +.horizontal { + display: flex; + flex-direction: row; +} diff --git a/src/menu/styles/menu.m.css.d.ts b/src/menu/styles/menu.m.css.d.ts index 0c976377e5..921e9a1c16 100644 --- a/src/menu/styles/menu.m.css.d.ts +++ b/src/menu/styles/menu.m.css.d.ts @@ -9,3 +9,4 @@ export const nestedMenu: string; export const subMenu: string; export const hidden: string; export const visible: string; +export const horizontal: string; diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index cfddc09fd2..5bc3cb5df4 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -1,9 +1,11 @@ import has from '@dojo/has/has'; import { VNode } from '@dojo/interfaces/vdom'; +import { w } from '@dojo/widget-core/d'; import * as registerSuite from 'intern!object'; import * as assert from 'intern/chai!assert'; import * as sinon from 'sinon'; -import Menu from '../../Menu'; +import Menu, { Orientation } from '../../Menu'; +import MenuItem from '../../MenuItem'; import * as css from '../../styles/menu.m.css'; function getStyle(element: any) { @@ -50,6 +52,7 @@ function getMockNavElement() { }, scrollTop: null, style: styles, + contains: () => true, classList: { add(name: string) { if (classes.indexOf(name) < 0) { @@ -74,6 +77,7 @@ registerSuite({ setup() { if (has('host-node')) { + ( global).document = Object.create(null); ( global).requestAnimationFrame = function (callback: () => void) { callback(); }; @@ -351,11 +355,24 @@ registerSuite({ menu.setChildren([ 'first', 'second', 'third' ]); const vnode = menu.__render__(); - assert.strictEqual(vnode.vnodeSelector, 'nav', 'menu node should be a nav'); assert.lengthOf(vnode.children, 3, 'menu node should have all children'); } }, + orientation() { + const menu = new Menu(); + let vnode: any = menu.__render__(); + assert.notOk(vnode.properties.classes[css.horizontal], 'should not have horizontal class by default'); + + menu.setProperties({ orientation: 'horizontal' }); + vnode = menu.__render__(); + assert.isTrue(vnode.properties.classes[css.horizontal], 'should have horizontal class'); + + menu.setProperties({ orientation: 'vertical' }); + vnode = menu.__render__(); + assert.notOk(vnode.properties.classes[css.horizontal], 'horizontal class should be removed'); + }, + onRequestShow() { const menu = new Menu(); menu.setProperties({ @@ -397,8 +414,8 @@ registerSuite({ assert.isTrue(menu.properties.hidden, 'menu should not be shown on click when `expandOnClick` is false'); }, - onLabelKeypress: { - 'when disabled'() { + onLabelKeydown: { + 'enter key: when disabled'() { const menu = new Menu(); menu.setProperties({ disabled: true, @@ -408,11 +425,11 @@ registerSuite({ } }); - ( menu).onLabelKeypress( { key: 'Enter' }); + ( menu).onLabelKeydown( { keyCode: 13 }); assert.isTrue(menu.properties.hidden, 'menu should remain hidden when disabled'); }, - 'when enabled'() { + 'enter key: when enabled'() { const menu = new Menu(); menu.setProperties({ hidden: true, @@ -421,21 +438,64 @@ registerSuite({ } }); - ( menu).onLabelKeypress( { key: 'Enter' }); + ( menu).onLabelKeydown( { keyCode: 13 }); assert.isFalse(menu.properties.hidden); }, - 'when `key` is not supported'() { + 'down arrow: horizontal orientation'() { const menu = new Menu(); + menu.setChildren([ + w(MenuItem, { key: 'child' }, [ 'child' ]) + ]); + menu.setProperties({ - hidden: true, - onRequestShow() { - menu.setProperties({ hidden: false }); - } + label: 'Menu label', + orientation: 'horizontal' }); + ( menu).onLabelKeydown( { keyCode: 40 }); + let vnode: any = menu.__render__(); + let unrenderedChild: any = menu.children[0]; - ( menu).onLabelKeypress( { keyCode: 13 }); - assert.isFalse(menu.properties.hidden); + assert.notOk(unrenderedChild.properties.active, 'the submenu should be activated'); + + menu.setProperties({ + hidden: false, + label: 'Menu label', + orientation: 'horizontal' + }); + ( menu).onLabelKeydown( { keyCode: 40 }); + const menuNode = vnode.children[1]; + vnode = menu.__render__(); + unrenderedChild = menu.children[0]; + + assert.strictEqual(menuNode.properties.tabIndex, -1, 'tab index should be -1 when the submenu has focus'); + assert.isTrue(unrenderedChild.properties.active, 'the submenu should be activated'); + }, + + 'right arrow: vertical orientation'() { + const menu = new Menu(); + menu.setChildren([ + w(MenuItem, { key: 'child' }, [ 'child' ]) + ]); + + menu.setProperties({ label: 'Menu label' }); + ( menu).onLabelKeydown( { keyCode: 39 }); + let vnode: any = menu.__render__(); + let unrenderedChild: any = menu.children[0]; + + assert.notOk(unrenderedChild.properties.active, 'the submenu should be activated'); + + menu.setProperties({ + hidden: false, + label: 'Menu label' + }); + ( menu).onLabelKeydown( { keyCode: 39 }); + vnode = menu.__render__(); + unrenderedChild = menu.children[0]; + const menuNode = vnode.children[1]; + + assert.strictEqual(menuNode.properties.tabIndex, -1, 'tab index should be -1 when the submenu has focus'); + assert.isTrue(unrenderedChild.properties.active, 'the submenu should be activated'); } }, @@ -454,19 +514,6 @@ registerSuite({ assert.isTrue(menu.properties.hidden, 'menu should remain hidden when disabled'); }, - 'when enabled and hidden'() { - const menu = new Menu(); - menu.setProperties({ - hidden: true, - onRequestShow() { - menu.setProperties({ hidden: false }); - } - }); - - ( menu).onMenuFocus(); - assert.isFalse(menu.properties.hidden, 'menu should open when focused'); - }, - 'when enabled and visible'() { const menu = new Menu(); let hideCalled = false; @@ -486,6 +533,162 @@ registerSuite({ } }, + onMenuFocusout: { + 'with a label: submenu has focus'() { + const menu = new Menu(); + menu.setProperties({ label: 'menu label' }); + const element: any = getMockNavElement(); + + ( menu).onElementCreated(element, 'menu'); + ( menu).onMenuFocus(); + ( menu).onMenuFocusout(); + + assert.isTrue(menu.state.active, 'menu should remain active'); + }, + + 'with a label: submenu loses focus'() { + const menu = new Menu(); + menu.setProperties({ label: 'menu label' }); + const element: any = getMockNavElement(); + element.contains = () => false; + + ( menu).onElementCreated(element, 'menu'); + ( menu).onMenuFocus(); + ( menu).onMenuFocusout(); + + assert.isFalse(menu.state.active, 'menu should be inactive'); + }, + + 'without a label'() { + const menu = new Menu(); + const element: any = getMockNavElement(); + element.contains = () => false; + + ( menu).onElementCreated(element, 'menu'); + ( menu).onMenuFocus(); + ( menu).onMenuFocusout(); + + assert.isTrue(menu.state.active, 'menu should remain active'); + } + }, + + onMenuKeydown: (() => { + const stopPropagation = () => {}; + + function getExitAssertion(keyCode = 37, orientation: Orientation = 'vertical') { + return function () { + const menu = new Menu(); + menu.setProperties({ orientation }); + ( menu).onMenuKeydown( { keyCode, stopPropagation }); + sinon.spy(menu, 'invalidate'); + + assert.isFalse(( menu.invalidate).called, 'menu should not be invalidated'); + + menu.setProperties({ label: 'menu label', orientation }); + ( menu).onMenuKeydown( { keyCode, stopPropagation }); + assert.isFalse(( menu.invalidate).called, 'menu should not be invalidated while hidden'); + + menu.setProperties({ label: 'menu label', hidden: false, orientation }); + const child = new MenuItem(); + child.setProperties({ active: true }); + menu.setChildren([ child ]); + ( menu).onMenuKeydown( { keyCode, stopPropagation }); + menu.__render__(); + + assert.isTrue(( menu.invalidate).called, 'menu should be invalidated'); + assert.isFalse(child.properties.active, 'menu item should not be active'); + }; + } + + function getDecreaseAssertion(keyCode = 38, orientation: Orientation = 'vertical') { + return function () { + const menu = new Menu(); + const first = new MenuItem(); + const second = new MenuItem(); + + menu.setProperties({ label: 'menu label', hidden: false, orientation }); + menu.setChildren( [ first, second ]); + ( menu).onMenuFocus(); + ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).renderChildren(); + + assert.isTrue(second.properties.active, 'active status should cycle from the first to last child'); + assert.notOk(first.properties.active, 'only one child should be active'); + + ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).renderChildren(); + + assert.notOk(second.properties.active, 'previously-active child should be inactive'); + assert.isTrue(first.properties.active, 'previous child should be active'); + }; + } + + function getIncreaseAssertion(keyCode = 40, orientation: Orientation = 'vertical') { + return function () { + const menu = new Menu(); + const first = new MenuItem(); + const second = new MenuItem(); + + menu.setProperties({ label: 'menu label', hidden: false, orientation }); + menu.setChildren( [ first, second ]); + ( menu).onMenuFocus(); + ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).renderChildren(); + + assert.isTrue(second.properties.active, 'next child should be active'); + assert.notOk(first.properties.active, 'only one child should be active'); + + ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).renderChildren(); + + assert.notOk(second.properties.active, 'previously-active child should be inactive'); + assert.isTrue(first.properties.active, 'active status should cycle from the last to first child'); + }; + } + + return { + 'common operations': { + 'tab key'() { + const menu = new Menu(); + const child = new MenuItem(); + child.setProperties({ active: true }); + menu.setChildren([ child ]); + ( menu).onMenuKeydown( { keyCode: 9, stopPropagation: () => {} }); + const vnode: any = menu.__render__(); + + assert.strictEqual(vnode.properties.tabIndex, 0, 'Menu should be focusable'); + assert.notOk(child.properties.active, 'Child should be marked as inactive'); + }, + + 'escape key'() { + const menu = new Menu(); + const onRequestHide = sinon.spy(); + menu.setProperties({ + label: 'menu label', + onRequestHide + }); + ( menu).onMenuKeydown( { keyCode: 27, stopPropagation: () => {} }); + const vnode: any = menu.__render__(); + + assert.strictEqual(vnode.children[0].properties.tabIndex, 0, 'label should be focusable'); + assert.isTrue(onRequestHide.called, 'menu should be hidden.'); + } + }, + + 'vertical orientation': { + 'left arrow key': getExitAssertion(), + 'up arrow key': getDecreaseAssertion(), + 'down arrow key': getIncreaseAssertion() + }, + + 'horizontal orientation': { + 'up arrow key': getExitAssertion(38, 'horizontal'), + 'left arrow key': getDecreaseAssertion(37, 'horizontal'), + 'right arrow key': getIncreaseAssertion(39, 'horizontal') + } + }; + })(), + onMenuMouseEnter: { 'when `expandOnClick` is true'() { const menu = new Menu(); diff --git a/src/menu/tests/unit/MenuItem.ts b/src/menu/tests/unit/MenuItem.ts index a5fd52c298..9401549116 100644 --- a/src/menu/tests/unit/MenuItem.ts +++ b/src/menu/tests/unit/MenuItem.ts @@ -1,5 +1,6 @@ import * as registerSuite from 'intern!object'; import * as assert from 'intern/chai!assert'; +import * as sinon from 'sinon'; import MenuItem from '../../MenuItem'; import * as css from '../../styles/menu.m.css'; @@ -19,6 +20,56 @@ registerSuite({ assert.isFalse(item.properties.selected); }, + properties: { + 'applies properties to the vnode'() { + const item = new MenuItem(); + item.setProperties({ + properties: { + 'data-custom': '12345' + } + }); + + const vnode: any = item.__render__(); + assert.strictEqual(vnode.properties['data-custom'], '12345'); + }, + + 'does not override static properties'() { + const item = new MenuItem(); + item.setProperties({ + controls: 'controls-base', + expanded: false, + hasPopup: false, + disabled: false, + properties: { + 'aria-controls': 'controls-custom', + 'aria-expanded': 'true', + 'aria-haspopup': 'true', + 'aria-disabled': 'true' + } + }); + + const vnode: any = item.__render__(); + const properties = vnode.properties; + + assert.strictEqual(properties['aria-controls'], 'controls-base'); + assert.strictEqual(properties['aria-expanded'], 'false'); + assert.isUndefined(properties['aria-haspopup']); + assert.strictEqual(properties['aria-disabled'], 'false'); + } + }, + + active() { + const item = new MenuItem(); + const focus = sinon.spy(); + + ( item).onElementUpdated( { focus }); + assert.isFalse(focus.called, 'element should not receive focus when `active` is false'); + + item.setProperties({ active: true }); + ( item).onElementUpdated( { focus }); + assert.isTrue(focus.called, 'element should receive focus when `active` is true'); + }, + onClick: { 'when disabled'() { const item = new MenuItem(); @@ -48,32 +99,43 @@ registerSuite({ } }, - onKeypress: { + onKeydown: { 'when disabled'() { const item = new MenuItem(); let event: any; item.setProperties({ disabled: true, - onKeypress(_event: any) { + onKeydown(_event: any) { event = _event; } }); - ( item).onKeypress( { type: 'keypress' }); - assert.isUndefined(event, '`onKeypress` should not be called when the menu item is disabled.'); + ( item).onKeydown( { type: 'keydown' }); + assert.isUndefined(event, '`onKeydown` should not be called when the menu item is disabled.'); }, 'when enabled'() { const item = new MenuItem(); let event: any; item.setProperties({ - onKeypress(_event: any) { + onKeydown(_event: any) { event = _event; } }); - ( item).onKeypress( { type: 'keypress' }); - assert.strictEqual(event!.type, 'keypress', '`onKeypress` should be called when the menu item is enabled.'); + ( item).onKeydown( { type: 'keydown' }); + assert.strictEqual(event!.type, 'keydown', '`onKeydown` should be called when the menu item is enabled.'); + }, + + 'space key'() { + const item = new MenuItem(); + const click = sinon.spy(); + ( item).onKeydown( { + keyCode: 32, + target: { click } + }); + + assert.isTrue(click.called, 'The event target\'s "click" method should be called.'); } }, @@ -104,20 +166,10 @@ registerSuite({ expanded: true }); const vnode: any = item.__render__(); - assert.strictEqual(vnode.properties['aria-expanded'], true, + assert.strictEqual(vnode.properties['aria-expanded'], 'true', '`expanded` should be assigned to the `aria-expanded` attribute'); }, - hasDropDown() { - const item = new MenuItem(); - item.setProperties({ - hasDropDown: true - }); - const vnode: any = item.__render__(); - assert.strictEqual(vnode.properties['aria-hasdropdown'], true, - '`hasDropDown` should be assigned to the `aria-hasdropdown` attribute'); - }, - hasMenu: { 'when false'() { const item = new MenuItem(); @@ -140,6 +192,25 @@ registerSuite({ } }, + hasPopup() { + const item = new MenuItem(); + item.setProperties({ + hasPopup: true + }); + let vnode: any = item.__render__(); + + assert.strictEqual(vnode.properties['aria-haspopup'], 'true', + '`hasPopup` should be assigned to the `aria-haspopup` attribute'); + + item.setProperties({ + hasPopup: false + }); + vnode = item.__render__(); + + assert.isUndefined(vnode.properties['aria-haspopup'], + 'the `aria-haspopup` attribute should be undefined'); + }, + selected() { const item = new MenuItem(); let vnode: any = item.__render__(); @@ -151,28 +222,34 @@ registerSuite({ assert.isTrue(vnode.properties.classes[css.selected]); }, - tabIndex: { - 'when disabled'() { - const item = new MenuItem(); - item.setProperties({ disabled: true, tabIndex: 1 }); - const vnode: any = item.__render__(); + tag() { + const item = new MenuItem(); + let vnode: any = item.__render__(); - assert.strictEqual(vnode.properties.tabIndex, -1, 'Specified tabIndex should be ignored'); - }, + assert.strictEqual(vnode.vnodeSelector, 'span', 'defaults to span'); - 'when enabled'() { - const item = new MenuItem(); - item.setProperties({ tabIndex: 1 }); - const vnode: any = item.__render__(); + item.setProperties({ + tag: 'a' + }); + vnode = item.__render__(); + assert.strictEqual(vnode.vnodeSelector, 'a'); + }, - assert.strictEqual(vnode.properties.tabIndex, 1); - }, + type() { + const item = new MenuItem(); + let vnode: any = item.__render__(); + assert.strictEqual(vnode.properties.role, 'menuitem', 'role should default to "menuitem"'); - 'defaults to 0'() { - const item = new MenuItem(); - const vnode: any = item.__render__(); + item.setProperties({ type: 'item' }); + vnode = item.__render__(); + assert.strictEqual(vnode.properties.role, 'menuitem', '"item" type should map to "menuitem" role'); - assert.strictEqual(vnode.properties.tabIndex, 0); - } + item.setProperties({ type: 'checkbox' }); + vnode = item.__render__(); + assert.strictEqual(vnode.properties.role, 'menuitemcheckbox', '"checkbox" type should map to "menuitemcheckbox" role'); + + item.setProperties({ type: 'radio' }); + vnode = item.__render__(); + assert.strictEqual(vnode.properties.role, 'menuitemradio', '"radio" type should map to "menuitemradio" role'); } }); From 3911c93cfcc666ce4e949efd26bbd5d04e45f9d1 Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Mon, 3 Apr 2017 18:21:02 -0500 Subject: [PATCH 13/19] Fix: animate only when `hidden` changes. --- src/menu/Menu.ts | 28 +++++++++++++++++----------- src/menu/tests/unit/Menu.ts | 27 ++++++++++----------------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index 4a80f9a582..fb08df297d 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -87,20 +87,25 @@ export class Menu extends MenuBase { private _activeIndex = 0; private _domNode: HTMLElement; private _hideTimer: Handle; + private _id = uuid(); private _initialRender = true; private _isLabelActive = false; render(): DNode { const { - id = uuid(), + id, nested, role = 'menu' } = this.properties; - const label = this.renderLabel(id); + if (id) { + this._id = id; + } + + const label = this.renderLabel(); const menu = v('div', { classes: this.classes.apply(this, this.getMenuClasses()), - id, + id: this._id, key: 'menu', onfocus: this.onMenuFocus, onfocusout: this.onMenuFocusout, @@ -121,14 +126,14 @@ export class Menu extends MenuBase { return menu; } - renderLabel(id: string): DNode | void { + renderLabel(): DNode | void { const { active, disabled, hidden = this.getDefaultHidden(), label, overrideClasses } = this.properties; const labelActive = this._isLabelActive || active; if (label) { return w(MenuItem, { active: labelActive, - controls: id, + controls: this._id, disabled, expanded: !hidden, hasMenu: true, @@ -142,6 +147,10 @@ export class Menu extends MenuBase { protected animate(element: HTMLElement) { const { hidden = this.getDefaultHidden() } = this.properties; + if (this.wasOpen !== hidden) { + return; + } + // Assuming the menu has an `auto` height, manually apply the scroll height (or max-height if specified), // and animate to and from that. requestAnimationFrame(() => { @@ -241,13 +250,10 @@ export class Menu extends MenuBase { const { animate = true, hidden = this.getDefaultHidden() } = this.properties; this._initialRender = false; this._domNode = element; + this.wasOpen = !hidden; - if (animate) { - this.wasOpen = !hidden; - - if (hidden) { - element.style.height = '0'; - } + if (animate && hidden) { + element.style.height = '0'; } } } diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 5bc3cb5df4..6c6f2d03f0 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -259,13 +259,11 @@ registerSuite({ 'collapsed from the scroll height to 0'() { const menu = new Menu(); - menu.setProperties({ - label: 'Menu label' - }); - + menu.setProperties({ hidden: false, label: 'Menu label' }); const element = getMockNavElement(); ( menu).onElementCreated(element, 'menu'); + menu.setProperties({ hidden: true, label: 'Menu label' }); ( menu).onElementUpdated(element, 'menu'); const styleHistory = element.styleHistory; @@ -275,14 +273,13 @@ registerSuite({ 'collapsed from the max-height if it is set'() { const menu = new Menu(); - menu.setProperties({ - label: 'Menu label' - }); + menu.setProperties({ hidden: false, label: 'Menu label' }); const element = getMockNavElement(); element.style['max-height'] = '100px'; ( menu).onElementCreated(element, 'menu'); + menu.setProperties({ label: 'Menu label', hidden: true }); ( menu).onElementUpdated(element, 'menu'); const styleHistory = element.styleHistory; @@ -291,36 +288,32 @@ registerSuite({ 'expanded to the scroll height'() { const menu = new Menu(); - menu.setProperties({ - hidden: false, - label: 'Menu label' - }); + menu.setProperties({ hidden: true, label: 'Menu label' }); const element = getMockNavElement(); ( menu).onElementCreated(element, 'menu'); + menu.setProperties({ hidden: false, label: 'Menu label' }); ( menu).onElementUpdated(element, 'menu'); const styleHistory = element.styleHistory; - assert.sameMembers(styleHistory.height, [ null, '300px' ]); + assert.sameMembers(styleHistory.height, [ null, '0', '300px' ]); assert.strictEqual(element.scrollTop, 0, 'The nav should be scrolled top'); }, 'animates to the max-height when set'() { const menu = new Menu(); - menu.setProperties({ - hidden: false, - label: 'Menu label' - }); + menu.setProperties({ hidden: true, label: 'Menu label' }); const element = getMockNavElement(); element.style['max-height'] = '100px'; ( menu).onElementCreated(element, 'menu'); + menu.setProperties({ hidden: false, label: 'Menu label' }); ( menu).onElementUpdated(element, 'menu'); const styleHistory = element.styleHistory; - assert.sameMembers(styleHistory.height, [ null, '100px' ]); + assert.sameMembers(styleHistory.height, [ null, '0', '100px' ]); } } }, From f26a42307bcb43f4472505564d275c8b45f10eba Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Tue, 4 Apr 2017 11:54:33 -0500 Subject: [PATCH 14/19] Menu/MenuItem updates - Rename event listener names for consistency with other widgets. - Change `container`/`nestedMenuContainer` CSS classes to `root`/`nestedMenuRoot`, respectively. - Use CSS variable for menu animation duration. - Simplify function property checks. - Only move focus from the label into the menu when navigating via the keyboard. - Add `hideOnActivate` options. - Always expand submenus when the designated "descend" key is pressed. --- src/menu/Menu.ts | 48 ++++++++----- src/menu/MenuItem.ts | 25 ++++--- src/menu/example/index.ts | 11 +-- src/menu/styles/menu.m.css | 6 +- src/menu/styles/menu.m.css.d.ts | 4 +- src/menu/tests/unit/Menu.ts | 124 ++++++++++++++++++++------------ src/menu/tests/unit/MenuItem.ts | 19 ++--- 7 files changed, 147 insertions(+), 90 deletions(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index fb08df297d..71ea3dc2e1 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -25,6 +25,7 @@ export type Role = 'menu' | 'menubar'; * @property expandOnClick Determines whether a menu is displayed on click (default) or hover. * @property hideDelay The amount of time (in milliseconds) after mouseleave before hiding the menu. * @property hidden Determines whether the menu is hidden. + * @property hideOnActivate Determines whether the menu should be hidden when an item is activated. Defaults to true. * @property id The widget ID. Defaults to a random string. * @property label A DNode to use as the trigger for a nested menu. * @property nested Indicates whether the menu is nested within another menu. @@ -39,6 +40,7 @@ export interface MenuProperties extends ThemeableProperties { expandOnClick?: boolean; hideDelay?: number; hidden?: boolean; + hideOnActivate?: boolean; id?: string; label?: DNode; nested?: boolean; @@ -62,6 +64,7 @@ export const enum Operation { const commonKeys = { enter: 13, escape: 27, + space: 32, tab: 9 }; @@ -107,9 +110,10 @@ export class Menu extends MenuBase { classes: this.classes.apply(this, this.getMenuClasses()), id: this._id, key: 'menu', + onclick: this.onItemActivate, onfocus: this.onMenuFocus, - onfocusout: this.onMenuFocusout, - onkeydown: this.onMenuKeydown, + onfocusout: this.onMenuFocusOut, + onkeydown: this.onMenuKeyDown, role, tabIndex: this.state.active || label ? -1 : 0 }, this.renderChildren()); @@ -117,7 +121,7 @@ export class Menu extends MenuBase { if (label) { this._isLabelActive = false; return v('div', { - classes: this.classes(css.container, nested ? css.nestedMenuContainer : null), + classes: this.classes(css.root, nested ? css.nestedMenuRoot : null), onmouseenter: this.onMenuMouseEnter, onmouseleave: this.onMenuMouseLeave }, [ label, menu ]); @@ -139,7 +143,7 @@ export class Menu extends MenuBase { hasMenu: true, overrideClasses: overrideClasses || css, onClick: this.onLabelClick, - onKeydown: this.onLabelKeydown + onKeyDown: this.onLabelKeyDown }, [ label ]); } } @@ -276,6 +280,14 @@ export class Menu extends MenuBase { } } + protected onItemActivate() { + const { hideOnActivate = true, role } = this.properties; + + if (role !== 'menubar' && hideOnActivate) { + this.toggleDisplay(false); + } + } + protected onLabelClick() { const { disabled, @@ -283,22 +295,23 @@ export class Menu extends MenuBase { } = this.properties; if (!disabled && expandOnClick) { + this._isLabelActive = true; this.toggleDisplay(); } } - protected onLabelKeydown(event: KeyboardEvent) { - const { disabled, hidden = this.getDefaultHidden(), label, orientation = this.getDefaultOrientation() } = this.properties; + protected onLabelKeyDown(event: KeyboardEvent) { + const { disabled, orientation = this.getDefaultOrientation() } = this.properties; const keys = orientation === 'horizontal' ? horizontalKeys : verticalKeys; if (!disabled) { const key = event.keyCode; - if (key === keys.enter) { // enter + if (key === keys.enter) { this.toggleDisplay(); } - else if (key === keys.descend && label && !hidden) { - this.setState({ active: true }); + else if (key === keys.descend) { + this.toggleDisplay(true); } } } @@ -307,7 +320,7 @@ export class Menu extends MenuBase { this.setState({ active: true }); } - protected onMenuFocusout() { + protected onMenuFocusOut() { if (this.properties.label) { requestAnimationFrame(() => { if (!this._domNode.contains(document.activeElement)) { @@ -317,11 +330,14 @@ export class Menu extends MenuBase { } } - protected onMenuKeydown(event: KeyboardEvent) { + protected onMenuKeyDown(event: KeyboardEvent) { const { orientation = this.getDefaultOrientation() } = this.properties; const keys = orientation === 'horizontal' ? horizontalKeys : verticalKeys; switch (event.keyCode) { + case keys.space: + this.onItemActivate(); + break; case keys.tab: event.stopPropagation(); this.setState({ active: false }); @@ -354,7 +370,7 @@ export class Menu extends MenuBase { if (!disabled && !expandOnClick) { this._hideTimer && this._hideTimer.destroy(); - this.toggleDisplay(true); + this.toggleDisplay(true, true); } } @@ -394,7 +410,7 @@ export class Menu extends MenuBase { return this.children; } - protected toggleDisplay(requestShow?: boolean) { + protected toggleDisplay(requestShow?: boolean, inactive?: boolean) { const { onRequestHide, onRequestShow @@ -406,12 +422,12 @@ export class Menu extends MenuBase { } if (requestShow) { - this.setState({ active: true }); - typeof onRequestShow === 'function' && onRequestShow(); + !inactive && this.setState({ active: true }); + onRequestShow && onRequestShow(); } else { this.setState({ active: false }); - typeof onRequestHide === 'function' && onRequestHide(); + onRequestHide && onRequestHide(); } } } diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts index a23307c30f..066a31d18a 100644 --- a/src/menu/MenuItem.ts +++ b/src/menu/MenuItem.ts @@ -19,7 +19,7 @@ export type MenuItemType = 'item' | 'checkbox' | 'radio'; * @property hasMenu Indicates whether the widget is used as the label for a menu. * @property hasPopup Indicates whether the widget has a drop down child. * @property onClick Called when the widget is activated via a click or space key. - * @property onKeydown Called when keys are pressed while the widget has focus. + * @property onKeyDown Called when keys are pressed while the widget has focus. * @property properties Additional properties for the widget's vnode. * @property selected Determines whether the widget is marked as selected. * @property tag The HTML tag name to use for the widget's vnode. Defaults to 'span'. @@ -33,7 +33,7 @@ export interface MenuItemProperties extends ThemeableProperties { hasMenu?: boolean; hasPopup?: boolean; onClick?: (event: MouseEvent) => void; - onKeydown?: (event: KeyboardEvent) => void; + onKeyDown?: (event: KeyboardEvent) => void; properties?: VirtualDomProperties; selected?: boolean; tag?: string; @@ -42,6 +42,7 @@ export interface MenuItemProperties extends ThemeableProperties { export const MenuItemBase = ThemeableMixin(WidgetBase); +const SPACE_KEY = 32; const roleMap: { [key: string]: string } = { item: 'menuitem', checkbox: 'menuitemcheckbox', @@ -76,9 +77,9 @@ export class MenuItem extends MenuItemBase { 'aria-haspopup': hasPopup ? 'true' : undefined, 'aria-disabled': String(disabled), classes, - key: 'menu-item', + key: 'root', onclick: this.onClick, - onkeydown: this.onKeydown, + onkeydown: this.onKeyDown, role: roleMap[type], tabIndex: active ? 0 : -1 }), this.children); @@ -86,22 +87,24 @@ export class MenuItem extends MenuItemBase { protected onClick(event: MouseEvent) { const { disabled, onClick } = this.properties; - if (!disabled && typeof onClick === 'function') { + if (!disabled && onClick) { onClick(event); } } - protected onElementUpdated(element: HTMLElement) { - this.properties.active && element.focus(); + protected onElementUpdated(element: HTMLElement, key: string) { + if (key === 'root') { + this.properties.active && element.focus(); + } } - protected onKeydown(event: KeyboardEvent) { - const { disabled, onKeydown } = this.properties; + protected onKeyDown(event: KeyboardEvent) { + const { disabled, onKeyDown } = this.properties; if (!disabled) { - if (event.keyCode === 32) { // space + if (event.keyCode === SPACE_KEY) { ( event.target).click(); } - typeof onKeydown === 'function' && onKeydown(event); + onKeyDown && onKeyDown(event); } } } diff --git a/src/menu/example/index.ts b/src/menu/example/index.ts index 504343e2bd..77c4abe494 100644 --- a/src/menu/example/index.ts +++ b/src/menu/example/index.ts @@ -3,7 +3,7 @@ import { WidgetProperties } from '@dojo/widget-core/interfaces'; import { ProjectorMixin } from '@dojo/widget-core/mixins/Projector'; import { StatefulMixin } from '@dojo/widget-core/mixins/Stateful'; import { v, w } from '@dojo/widget-core/d'; -import Menu, { Orientation } from '../Menu'; +import Menu, { Orientation, Role } from '../Menu'; import MenuItem from '../MenuItem'; const AppBase = StatefulMixin(WidgetBase); @@ -117,7 +117,9 @@ export class App extends AppBase { return v('div', { style: 'float: left;' }, [ w(Menu, { - key: 'DojoMenu' + key: 'DojoMenu', + orientation: 'vertical', + role: 'menubar' }, [ w(MenuItem, { key: 'DojoMenuLabel', @@ -164,7 +166,8 @@ export class App extends AppBase { return v('div', { style: 'float: left; margin: 0 50px 50px 0;' }, [ w(Menu, { key: 'ChromeFileMenu', - orientation: this.state['isFileMenuHorizontal'] ? 'horizontal' : 'vertical' as Orientation + orientation: this.state['isFileMenuHorizontal'] ? 'horizontal' : 'vertical' as Orientation, + role: 'menubar' }, [ 'New Tab', 'New Window', @@ -183,7 +186,7 @@ export class App extends AppBase { return w(MenuItem, { key, disabled: name === 'Open Location...', - onKeydown: (event: KeyboardEvent) => { + onKeyDown: (event: KeyboardEvent) => { if (event.keyCode === 13) { toggleSelected(); } diff --git a/src/menu/styles/menu.m.css b/src/menu/styles/menu.m.css index 75ce3cbfac..fab483012e 100644 --- a/src/menu/styles/menu.m.css +++ b/src/menu/styles/menu.m.css @@ -1,6 +1,6 @@ @import '../../common/styles/variables.css'; -.container { +.root { box-sizing: border-box; position: relative; } @@ -44,7 +44,7 @@ color: var(--menu-selected-color); } -.nestedMenuContainer, +.nestedMenuRoot, .nestedMenu { width: 100%; } @@ -54,7 +54,7 @@ box-sizing: border-box; overflow: hidden; padding: 0 15px; - transition: height 0.5s ease-in-out; + transition: height var(--short-animation-duration) ease-in-out; } .hidden { height: 0; } diff --git a/src/menu/styles/menu.m.css.d.ts b/src/menu/styles/menu.m.css.d.ts index 921e9a1c16..58165966d4 100644 --- a/src/menu/styles/menu.m.css.d.ts +++ b/src/menu/styles/menu.m.css.d.ts @@ -1,10 +1,10 @@ -export const container: string; +export const root: string; export const menu: string; export const menuItem: string; export const menuLabel: string; export const disabled: string; export const selected: string; -export const nestedMenuContainer: string; +export const nestedMenuRoot: string; export const nestedMenu: string; export const subMenu: string; export const hidden: string; diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 6c6f2d03f0..9f15423143 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -407,7 +407,7 @@ registerSuite({ assert.isTrue(menu.properties.hidden, 'menu should not be shown on click when `expandOnClick` is false'); }, - onLabelKeydown: { + onLabelKeyDown: { 'enter key: when disabled'() { const menu = new Menu(); menu.setProperties({ @@ -418,7 +418,7 @@ registerSuite({ } }); - ( menu).onLabelKeydown( { keyCode: 13 }); + ( menu).onLabelKeyDown( { keyCode: 13 }); assert.isTrue(menu.properties.hidden, 'menu should remain hidden when disabled'); }, @@ -431,7 +431,8 @@ registerSuite({ } }); - ( menu).onLabelKeydown( { keyCode: 13 }); + ( menu).onLabelKeyDown( { keyCode: 13 }); + ( menu).onLabelKeyDown( {}); assert.isFalse(menu.properties.hidden); }, @@ -445,24 +446,13 @@ registerSuite({ label: 'Menu label', orientation: 'horizontal' }); - ( menu).onLabelKeydown( { keyCode: 40 }); - let vnode: any = menu.__render__(); - let unrenderedChild: any = menu.children[0]; - - assert.notOk(unrenderedChild.properties.active, 'the submenu should be activated'); - - menu.setProperties({ - hidden: false, - label: 'Menu label', - orientation: 'horizontal' - }); - ( menu).onLabelKeydown( { keyCode: 40 }); + ( menu).onLabelKeyDown( { keyCode: 40 }); + ( menu).onLabelKeyDown( {}); + const vnode: any = menu.__render__(); const menuNode = vnode.children[1]; - vnode = menu.__render__(); - unrenderedChild = menu.children[0]; assert.strictEqual(menuNode.properties.tabIndex, -1, 'tab index should be -1 when the submenu has focus'); - assert.isTrue(unrenderedChild.properties.active, 'the submenu should be activated'); + assert.notOk(menu.properties.hidden, 'the submenu should not be hidden'); }, 'right arrow: vertical orientation'() { @@ -471,24 +461,17 @@ registerSuite({ w(MenuItem, { key: 'child' }, [ 'child' ]) ]); - menu.setProperties({ label: 'Menu label' }); - ( menu).onLabelKeydown( { keyCode: 39 }); - let vnode: any = menu.__render__(); - let unrenderedChild: any = menu.children[0]; - - assert.notOk(unrenderedChild.properties.active, 'the submenu should be activated'); - menu.setProperties({ hidden: false, label: 'Menu label' }); - ( menu).onLabelKeydown( { keyCode: 39 }); - vnode = menu.__render__(); - unrenderedChild = menu.children[0]; + ( menu).onLabelKeyDown( { keyCode: 39 }); + ( menu).onLabelKeyDown( {}); + const vnode: any = menu.__render__(); const menuNode = vnode.children[1]; assert.strictEqual(menuNode.properties.tabIndex, -1, 'tab index should be -1 when the submenu has focus'); - assert.isTrue(unrenderedChild.properties.active, 'the submenu should be activated'); + assert.notOk(menu.properties.hidden, 'the submenu should not be hidden'); } }, @@ -526,7 +509,7 @@ registerSuite({ } }, - onMenuFocusout: { + onMenuFocusOut: { 'with a label: submenu has focus'() { const menu = new Menu(); menu.setProperties({ label: 'menu label' }); @@ -534,7 +517,7 @@ registerSuite({ ( menu).onElementCreated(element, 'menu'); ( menu).onMenuFocus(); - ( menu).onMenuFocusout(); + ( menu).onMenuFocusOut(); assert.isTrue(menu.state.active, 'menu should remain active'); }, @@ -547,7 +530,7 @@ registerSuite({ ( menu).onElementCreated(element, 'menu'); ( menu).onMenuFocus(); - ( menu).onMenuFocusout(); + ( menu).onMenuFocusOut(); assert.isFalse(menu.state.active, 'menu should be inactive'); }, @@ -559,33 +542,33 @@ registerSuite({ ( menu).onElementCreated(element, 'menu'); ( menu).onMenuFocus(); - ( menu).onMenuFocusout(); + ( menu).onMenuFocusOut(); assert.isTrue(menu.state.active, 'menu should remain active'); } }, - onMenuKeydown: (() => { + onMenuKeyDown: (() => { const stopPropagation = () => {}; function getExitAssertion(keyCode = 37, orientation: Orientation = 'vertical') { return function () { const menu = new Menu(); menu.setProperties({ orientation }); - ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, stopPropagation }); sinon.spy(menu, 'invalidate'); assert.isFalse(( menu.invalidate).called, 'menu should not be invalidated'); menu.setProperties({ label: 'menu label', orientation }); - ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, stopPropagation }); assert.isFalse(( menu.invalidate).called, 'menu should not be invalidated while hidden'); menu.setProperties({ label: 'menu label', hidden: false, orientation }); const child = new MenuItem(); child.setProperties({ active: true }); menu.setChildren([ child ]); - ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, stopPropagation }); menu.__render__(); assert.isTrue(( menu.invalidate).called, 'menu should be invalidated'); @@ -602,13 +585,13 @@ registerSuite({ menu.setProperties({ label: 'menu label', hidden: false, orientation }); menu.setChildren( [ first, second ]); ( menu).onMenuFocus(); - ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, stopPropagation }); ( menu).renderChildren(); assert.isTrue(second.properties.active, 'active status should cycle from the first to last child'); assert.notOk(first.properties.active, 'only one child should be active'); - ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, stopPropagation }); ( menu).renderChildren(); assert.notOk(second.properties.active, 'previously-active child should be inactive'); @@ -625,13 +608,13 @@ registerSuite({ menu.setProperties({ label: 'menu label', hidden: false, orientation }); menu.setChildren( [ first, second ]); ( menu).onMenuFocus(); - ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, stopPropagation }); ( menu).renderChildren(); assert.isTrue(second.properties.active, 'next child should be active'); assert.notOk(first.properties.active, 'only one child should be active'); - ( menu).onMenuKeydown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, stopPropagation }); ( menu).renderChildren(); assert.notOk(second.properties.active, 'previously-active child should be inactive'); @@ -641,12 +624,31 @@ registerSuite({ return { 'common operations': { + 'space key'() { + const menu = new Menu(); + let onRequestHide = sinon.spy(); + + menu.setProperties({ onRequestHide }); + ( menu).onMenuKeyDown( { keyCode: 32 }); + assert.isTrue(onRequestHide.called, 'Menu should be hidden'); + + onRequestHide = sinon.spy(); + menu.setProperties({ hideOnActivate: false }); + ( menu).onMenuKeyDown( { keyCode: 32 }); + assert.isFalse(onRequestHide.called, 'Menu should not be hidden'); + + onRequestHide = sinon.spy(); + menu.setProperties({ hideOnActivate: true, role: 'menubar' }); + ( menu).onMenuKeyDown( { keyCode: 32 }); + assert.isFalse(onRequestHide.called, 'Menu should not be hidden'); + }, + 'tab key'() { const menu = new Menu(); const child = new MenuItem(); child.setProperties({ active: true }); menu.setChildren([ child ]); - ( menu).onMenuKeydown( { keyCode: 9, stopPropagation: () => {} }); + ( menu).onMenuKeyDown( { keyCode: 9, stopPropagation: () => {} }); const vnode: any = menu.__render__(); assert.strictEqual(vnode.properties.tabIndex, 0, 'Menu should be focusable'); @@ -660,7 +662,7 @@ registerSuite({ label: 'menu label', onRequestHide }); - ( menu).onMenuKeydown( { keyCode: 27, stopPropagation: () => {} }); + ( menu).onMenuKeyDown( { keyCode: 27, stopPropagation: () => {} }); const vnode: any = menu.__render__(); assert.strictEqual(vnode.children[0].properties.tabIndex, 0, 'label should be focusable'); @@ -866,6 +868,38 @@ registerSuite({ } }, + hideOnActivate: { + 'when true'() { + const menu = new Menu(); + const onRequestHide = sinon.spy(); + + menu.setProperties({ onRequestHide }); + ( menu).onItemActivate(); + + assert.isTrue(onRequestHide.called, 'Menu should be hidden'); + }, + + 'when false'() { + const menu = new Menu(); + const onRequestHide = sinon.spy(); + + menu.setProperties({ hideOnActivate: false, onRequestHide }); + ( menu).onItemActivate(); + + assert.isFalse(onRequestHide.called, 'Menu should not be hidden'); + }, + + 'ignored for menu bars'() { + const menu = new Menu(); + const onRequestHide = sinon.spy(); + + menu.setProperties({ onRequestHide, role: 'menubar' }); + ( menu).onItemActivate(); + + assert.isFalse(onRequestHide.called, 'Menu bars should not be hidden'); + } + }, + id() { const menu = new Menu(); let vnode: any = menu.__render__(); @@ -897,12 +931,12 @@ registerSuite({ assert.isTrue(vnode.properties.classes[css.nestedMenu]); }, - 'adds `nestedMenuContainer` class to container when there is a label'() { + 'adds `nestedMenuRoot` class to container when there is a label'() { const menu = new Menu(); menu.setProperties({ label: 'Menu Label', nested: true }); const vnode: any = menu.__render__(); - assert.isTrue(vnode.properties.classes[css.nestedMenuContainer]); + assert.isTrue(vnode.properties.classes[css.nestedMenuRoot]); } } }); diff --git a/src/menu/tests/unit/MenuItem.ts b/src/menu/tests/unit/MenuItem.ts index 9401549116..a049d9332e 100644 --- a/src/menu/tests/unit/MenuItem.ts +++ b/src/menu/tests/unit/MenuItem.ts @@ -63,10 +63,11 @@ registerSuite({ const focus = sinon.spy(); ( item).onElementUpdated( { focus }); + ( item).onElementUpdated( { focus }, 'root'); assert.isFalse(focus.called, 'element should not receive focus when `active` is false'); item.setProperties({ active: true }); - ( item).onElementUpdated( { focus }); + ( item).onElementUpdated( { focus }, 'root'); assert.isTrue(focus.called, 'element should receive focus when `active` is true'); }, @@ -99,38 +100,38 @@ registerSuite({ } }, - onKeydown: { + onKeyDown: { 'when disabled'() { const item = new MenuItem(); let event: any; item.setProperties({ disabled: true, - onKeydown(_event: any) { + onKeyDown(_event: any) { event = _event; } }); - ( item).onKeydown( { type: 'keydown' }); - assert.isUndefined(event, '`onKeydown` should not be called when the menu item is disabled.'); + ( item).onKeyDown( { type: 'keydown' }); + assert.isUndefined(event, '`onKeyDown` should not be called when the menu item is disabled.'); }, 'when enabled'() { const item = new MenuItem(); let event: any; item.setProperties({ - onKeydown(_event: any) { + onKeyDown(_event: any) { event = _event; } }); - ( item).onKeydown( { type: 'keydown' }); - assert.strictEqual(event!.type, 'keydown', '`onKeydown` should be called when the menu item is enabled.'); + ( item).onKeyDown( { type: 'keydown' }); + assert.strictEqual(event!.type, 'keydown', '`onKeyDown` should be called when the menu item is enabled.'); }, 'space key'() { const item = new MenuItem(); const click = sinon.spy(); - ( item).onKeydown( { + ( item).onKeyDown( { keyCode: 32, target: { click } }); From d291953ae8606f62ebdb6f573e559bba1ce8edc3 Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Tue, 4 Apr 2017 14:05:10 -0500 Subject: [PATCH 15/19] Add `aria-checked` property to checkbox/radio menu items --- src/menu/MenuItem.ts | 12 +++++++--- src/menu/tests/unit/MenuItem.ts | 40 +++++++++++++++++++++++++++------ 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts index 066a31d18a..ab0c597df7 100644 --- a/src/menu/MenuItem.ts +++ b/src/menu/MenuItem.ts @@ -60,7 +60,7 @@ export class MenuItem extends MenuItemBase { hasPopup = false, hasMenu = false, properties, - selected, + selected = false, tag = 'span', type = 'item' } = this.properties; @@ -71,7 +71,7 @@ export class MenuItem extends MenuItemBase { selected ? css.selected : null ); - return v(tag, assign(Object.create(null), properties, { + const itemProperties: { [key: string]: any; } = { 'aria-controls': controls, 'aria-expanded': String(expanded), 'aria-haspopup': hasPopup ? 'true' : undefined, @@ -82,7 +82,13 @@ export class MenuItem extends MenuItemBase { onkeydown: this.onKeyDown, role: roleMap[type], tabIndex: active ? 0 : -1 - }), this.children); + }; + + if (type === 'checkbox' || type === 'radio') { + itemProperties['aria-checked'] = String(selected); + } + + return v(tag, assign(Object.create(null), properties, itemProperties), this.children); } protected onClick(event: MouseEvent) { diff --git a/src/menu/tests/unit/MenuItem.ts b/src/menu/tests/unit/MenuItem.ts index a049d9332e..67079b4a98 100644 --- a/src/menu/tests/unit/MenuItem.ts +++ b/src/menu/tests/unit/MenuItem.ts @@ -212,15 +212,41 @@ registerSuite({ 'the `aria-haspopup` attribute should be undefined'); }, - selected() { - const item = new MenuItem(); - let vnode: any = item.__render__(); + selected: { + basic() { + const item = new MenuItem(); + let vnode: any = item.__render__(); - assert.notOk(vnode.properties.classes[css.selected]); + assert.notOk(vnode.properties.classes[css.selected]); - item.setProperties({ selected: true }); - vnode = item.__render__(); - assert.isTrue(vnode.properties.classes[css.selected]); + item.setProperties({ selected: true }); + vnode = item.__render__(); + assert.isTrue(vnode.properties.classes[css.selected]); + }, + + 'type="checkbox"'() { + const item = new MenuItem(); + item.setProperties({ type: 'checkbox' }); + let vnode: any = item.__render__(); + + assert.strictEqual(vnode.properties['aria-checked'], 'false'); + + item.setProperties({ type: 'checkbox', selected: true }); + vnode = item.__render__(); + assert.strictEqual(vnode.properties['aria-checked'], 'true'); + }, + + 'type="radio"'() { + const item = new MenuItem(); + item.setProperties({ type: 'radio' }); + let vnode: any = item.__render__(); + + assert.strictEqual(vnode.properties['aria-checked'], 'false'); + + item.setProperties({ type: 'radio', selected: true }); + vnode = item.__render__(); + assert.strictEqual(vnode.properties['aria-checked'], 'true'); + } }, tag() { From e5874fd04c7e6a39f67bb4ae41790c44aef3549b Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Tue, 4 Apr 2017 15:07:46 -0500 Subject: [PATCH 16/19] Add Menu widget README. --- src/menu/README.md | 164 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 164 insertions(+) create mode 100644 src/menu/README.md diff --git a/src/menu/README.md b/src/menu/README.md new file mode 100644 index 0000000000..a56d64dc20 --- /dev/null +++ b/src/menu/README.md @@ -0,0 +1,164 @@ +# `@dojo/widgets/menu/Menu` + +`Menu` is a widget that provides a list of actions, with the ability to nest menus. Common examples include horizontal menubars used for app navigation on wider screens, vertical menubars used in a `SlidePane` on smaller screens, and the reply/reply all/forward menus found in mail applications. Due to the generic nature of `Menu`, there are certain patterns that should be followed when using it to guarantee the best experience for all app users. + +## Basic Usage + +`Menu` is used in tandem with `MenuItem`, a helper widget that provides mouse and keyboard handlers for menu operations and ensures menu items have the proper ARIA roles and properties. + +```typescript +// Basic example +w(Menu, {}, [ + w(MenuItem, { + onClick: () => { + // reply + } + }, [ 'Reply' ]), + + w(MenuItem, { + onClick: () => { + // reply all + } + }, [ 'Reply All' ]), + + w(MenuItem, { + onClick: () => { + // forward + } + }, [ 'Forward' ]), +]); +``` + +```typescript +// Horizontal menubar +w(Menu, { + // "horizontal" is the default orientation for menubars, so the `orientation` property + // is not technically required here. + orientation: 'horizontal', + role: 'menubar' +}, [ + w(MenuItem, { + tag: 'a', + properties: { + href: 'http://dojo.io' + } + }, [ 'Dojo 2' ]), + + w(MenuItem, { + tag: 'a', + properties: { + href: 'https://github.com/dojo/widget-core' + } + }, [ 'Dojo 2 Widget Core' ]), + + w(MenuItem, { + tag: 'a', + properties: { + href: 'https://github.com/dojo/widgets' + } + }, [ 'Dojo 2 Widgets' ]) +]); +``` + +```typescript +// Menu toggled open/closed with a trigger +w(Menu, { + hidden: this.state.hidden, + label: 'More', + onRequestHide: () => { + this.setState({ hidden: true }); + }, + onRequestShow: () => { + this.setState({ hidden: false }); + } +}, [ + w(MenuItem, { + onClick: () => { + // reply + } + }, [ 'Reply' ]), + + w(MenuItem, { + onClick: () => { + // reply all + } + }, [ 'Reply All' ]), + + w(MenuItem, { + onClick: () => { + // forward + } + }, [ 'Forward' ]), +]); +``` + +## Accessibility + +### `Menu` types + +`Menu` supports two `role` properties: `'menu'` and `'menubar'`. If the menu will always be visible, use `'menubar'`. Otherwise, the default `'menu'` can be used. + +### `MenuItem` types + +`MenuItem` provides a simple `type` property that can be used to specify the item type. Its values are `'item'` (the default), `'checkbox'`, and `'radio'`. When the item type is a `'checkbox'` or `'radio'`, the `selected` property should be used to control state: + +```typescript +w(MenuItem, { + type: 'checkbox', + selected: this.state.italicSelected, + onClick: () => { + this.setState({ italicSelected: !this.state.italicSelected }); + } +}, [ 'Italic' ]); +``` + +When the `'radio'` type is used, however, the application must ensure that only one item within the menu is `selected` at a time: + +```typescript +w(Menu, {}, [ + w(MenuItem, { + type: 'radio', + selected: this.state.fontSize === 'small', + onClick: () => { + this.setState({ fontSize: 'small' }); + } + }, [ 'Small' ]), + + w(MenuItem, { + type: 'radio', + selected: this.state.fontSize === 'medium', + onClick: () => { + this.setState({ fontSize: 'medium' }); + } + }, [ 'Medium' ]), + + w(MenuItem, { + type: 'radio', + selected: this.state.fontSize === 'large', + onClick: () => { + this.setState({ fontSize: 'large' }); + } + }, [ 'Large' ]) +]); +``` + +### Keyboard Navigation + +`Menu` follows the [WAI-ARIA Authoring Practices](https://www.w3.org/TR/wai-aria-practices-1.1/#menu) for menus to the extent that the various use cases are described. As such, `Menu` provides the following keyboard interactions: + +**With `orientation="horizontal"`**: +- Pressing the left arrow key moves focus to the previous item. If the first item is currently selected, focus is moved to the last item. +- Pressing the right arrow key moves focus to the next item. If the last item is currently selected, focus is moved to the first item. +- Pressing the down arrow key when focus is on a trigger for a submenu moves focus into the submenu, placing the focus on the previously-focused item. If the submenu is hidden, it will be displayed. +- Pressing the up arrow key when focus is on an item within a submenu moves focus back to the trigger item. The next time the submenu receives focus, the previously-focused item receives the focus. Pressing the up arrow key has no effect in other circumstances. + +**With `orientation="vertical"` (the default)**: +- Pressing the up arrow key moves focus to the previous item. If the first item is currently selected, focus is moved to the last item. +- Pressing the down arrow key moves focus to the next item. If the last item is currently selected, focus is moved to the first item. +- Pressing the right arrow key when focus is on a trigger for a submenu moves focus into the submenu, placing the focus on the previously-focused item. If the submenu is hidden, it will be displayed. +- Pressing the left arrow key when focus is on an item within a submenu moves focus back to the trigger item. The next time the submenu receives focus, the previously-focused item receives the focus. Pressing the left arrow key has no effect in other circumstances. + +**Common operations**: +- Pressing the escape key within a submenu closes the submenu. When the submenu is re-opened, focus is placed on the first item. +- Pressing the enter key triggers an item's action. If the item is used as the trigger for a submenu, then the submenu is toggled opened/closed. +- Pressing the space key triggers an item's action. If the item is used as the trigger for a submenu, then the submenu is toggled opened/closed. From 68f287c3a2d858abb759bd2b256ff66a4f4c7e0c Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Tue, 4 Apr 2017 18:24:51 -0500 Subject: [PATCH 17/19] Menu/MenuItem updates - general cleanup - remove tabIndex from menu container - reset active index on hide - move focus on click --- src/menu/Menu.ts | 93 +++++++++++++++----------- src/menu/MenuItem.ts | 24 +++++-- src/menu/README.md | 17 ++--- src/menu/example/index.ts | 5 +- src/menu/tests/unit/Menu.ts | 111 +++++++++++++++++++++++++------- src/menu/tests/unit/MenuItem.ts | 15 +++-- 6 files changed, 180 insertions(+), 85 deletions(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index 71ea3dc2e1..431f0e4f48 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -23,6 +23,7 @@ export type Role = 'menu' | 'menubar'; * @property animate Determines whether animation should be handled internally. * @property disabled Determines whether the menu is disabled. * @property expandOnClick Determines whether a menu is displayed on click (default) or hover. + * @property focusable Determines whether the menu trigger can receive focus with tab key. * @property hideDelay The amount of time (in milliseconds) after mouseleave before hiding the menu. * @property hidden Determines whether the menu is hidden. * @property hideOnActivate Determines whether the menu should be hidden when an item is activated. Defaults to true. @@ -38,6 +39,7 @@ export interface MenuProperties extends ThemeableProperties { animate?: boolean; disabled?: boolean; expandOnClick?: boolean; + focusable?: boolean; hideDelay?: number; hidden?: boolean; hideOnActivate?: boolean; @@ -57,8 +59,7 @@ function getMenuHeight(menuElement: HTMLElement): number { export const enum Operation { decrease, - increase, - reset + increase } const commonKeys = { @@ -96,26 +97,22 @@ export class Menu extends MenuBase { render(): DNode { const { - id, + id = this._id, nested, role = 'menu' } = this.properties; - if (id) { - this._id = id; - } - const label = this.renderLabel(); const menu = v('div', { classes: this.classes.apply(this, this.getMenuClasses()), - id: this._id, + id, key: 'menu', onclick: this.onItemActivate, - onfocus: this.onMenuFocus, + onfocusin: this.onMenuFocus, onfocusout: this.onMenuFocusOut, onkeydown: this.onMenuKeyDown, - role, - tabIndex: this.state.active || label ? -1 : 0 + onmousedown: this.onMenuMouseDown, + role }, this.renderChildren()); if (label) { @@ -131,15 +128,24 @@ export class Menu extends MenuBase { } renderLabel(): DNode | void { - const { active, disabled, hidden = this.getDefaultHidden(), label, overrideClasses } = this.properties; + const { + active, + disabled, + focusable, + hidden = this.getDefaultHidden(), + id = this._id, + label, + overrideClasses + } = this.properties; const labelActive = this._isLabelActive || active; if (label) { return w(MenuItem, { active: labelActive, - controls: this._id, + controls: id, disabled, expanded: !hidden, + focusable, hasMenu: true, overrideClasses: overrideClasses || css, onClick: this.onLabelClick, @@ -189,13 +195,7 @@ export class Menu extends MenuBase { } protected getDefaultHidden() { - const { disabled, label } = this.properties; - - if (label && disabled) { - return true; - } - - return label ? true : false; + return this.properties.label ? true : false; } protected getDefaultOrientation(): Orientation { @@ -234,19 +234,13 @@ export class Menu extends MenuBase { } protected moveActiveIndex(operation: Operation) { - if (operation === Operation.reset) { - this._activeIndex = 0; - this.toggleDisplay(false); - } - else { - const max = this.children.length; - const previousIndex = this._activeIndex; - this._activeIndex = operation === Operation.decrease ? - previousIndex - 1 < 0 ? max - 1 : previousIndex - 1 : - Math.min(previousIndex + 1, max) % max; + const max = this.children.length; + const previousIndex = this._activeIndex; + this._activeIndex = operation === Operation.decrease ? + previousIndex - 1 < 0 ? max - 1 : previousIndex - 1 : + Math.min(previousIndex + 1, max) % max; - this.invalidate(); - } + this.invalidate(); } protected onElementCreated(element: HTMLElement, key: string) { @@ -317,7 +311,7 @@ export class Menu extends MenuBase { } protected onMenuFocus() { - this.setState({ active: true }); + !this.state.active && this.setState({ active: true }); } protected onMenuFocusOut() { @@ -328,10 +322,13 @@ export class Menu extends MenuBase { } }); } + else { + this.setState({ active: false }); + } } protected onMenuKeyDown(event: KeyboardEvent) { - const { orientation = this.getDefaultOrientation() } = this.properties; + const { label, orientation = this.getDefaultOrientation() } = this.properties; const keys = orientation === 'horizontal' ? horizontalKeys : verticalKeys; switch (event.keyCode) { @@ -343,25 +340,42 @@ export class Menu extends MenuBase { this.setState({ active: false }); break; case keys.ascend: + event.preventDefault(); event.stopPropagation(); this.exitMenu(); break; case keys.decrease: + event.preventDefault(); event.stopPropagation(); this.moveActiveIndex(Operation.decrease); break; case keys.increase: + event.preventDefault(); event.stopPropagation(); this.moveActiveIndex(Operation.increase); break; case keys.escape: - event.stopPropagation(); - this._isLabelActive = true; - this.moveActiveIndex(Operation.reset); + if (label) { + event.stopPropagation(); + this._isLabelActive = true; + this.toggleDisplay(false); + } break; } } + protected onMenuMouseDown(event: MouseEvent) { + let itemNode = event.target; + while (!itemNode.hasAttribute('data-dojo-index') && itemNode.parentElement) { + itemNode = itemNode.parentElement; + } + + const index = parseInt(itemNode.getAttribute('data-dojo-index') || '', 10); + if (!isNaN(index)) { + this._activeIndex = index; + } + } + protected onMenuMouseEnter() { const { disabled, @@ -396,12 +410,14 @@ export class Menu extends MenuBase { } protected renderChildren() { - const { hidden = this.getDefaultHidden() } = this.properties; + const { hidden = this.getDefaultHidden(), label } = this.properties; const activeIndex = this.state.active && !this._isLabelActive ? this._activeIndex : null; if (!hidden) { this.children.forEach((child: any, i) => { if (child && child.properties) { + child.properties.index = i; + child.properties.focusable = !label && i === this._activeIndex; child.properties.active = i === activeIndex; } }); @@ -427,6 +443,7 @@ export class Menu extends MenuBase { } else { this.setState({ active: false }); + this._activeIndex = 0; onRequestHide && onRequestHide(); } } diff --git a/src/menu/MenuItem.ts b/src/menu/MenuItem.ts index ab0c597df7..97e2c471ef 100644 --- a/src/menu/MenuItem.ts +++ b/src/menu/MenuItem.ts @@ -16,8 +16,10 @@ export type MenuItemType = 'item' | 'checkbox' | 'radio'; * @property controls The ID of an element that this input controls. * @property disabled Indicates whether the item is disabled. * @property expanded Indicates whether a widget controlled by `this` is expanded. + * @property focusable Determines whether the item can receive focus with tab key. * @property hasMenu Indicates whether the widget is used as the label for a menu. * @property hasPopup Indicates whether the widget has a drop down child. + * @property index Specifies the index of the item within a parent menu. * @property onClick Called when the widget is activated via a click or space key. * @property onKeyDown Called when keys are pressed while the widget has focus. * @property properties Additional properties for the widget's vnode. @@ -30,8 +32,10 @@ export interface MenuItemProperties extends ThemeableProperties { controls?: string; disabled?: boolean; expanded?: boolean; + focusable?: boolean; hasMenu?: boolean; hasPopup?: boolean; + index?: boolean; onClick?: (event: MouseEvent) => void; onKeyDown?: (event: KeyboardEvent) => void; properties?: VirtualDomProperties; @@ -53,12 +57,13 @@ const roleMap: { [key: string]: string } = { export class MenuItem extends MenuItemBase { render() { const { - active = false, controls, disabled = false, expanded = false, + focusable, hasPopup = false, hasMenu = false, + index, properties, selected = false, tag = 'span', @@ -73,15 +78,16 @@ export class MenuItem extends MenuItemBase { const itemProperties: { [key: string]: any; } = { 'aria-controls': controls, - 'aria-expanded': String(expanded), + 'aria-expanded': hasPopup ? String(expanded) : undefined, 'aria-haspopup': hasPopup ? 'true' : undefined, - 'aria-disabled': String(disabled), + 'aria-disabled': disabled ? 'true' : undefined, + 'data-dojo-index': typeof index === 'number' ? String(index) : undefined, classes, key: 'root', onclick: this.onClick, onkeydown: this.onKeyDown, role: roleMap[type], - tabIndex: active ? 0 : -1 + tabIndex: focusable ? 0 : -1 }; if (type === 'checkbox' || type === 'radio') { @@ -93,24 +99,28 @@ export class MenuItem extends MenuItemBase { protected onClick(event: MouseEvent) { const { disabled, onClick } = this.properties; + if (!disabled && onClick) { onClick(event); } } protected onElementUpdated(element: HTMLElement, key: string) { - if (key === 'root') { - this.properties.active && element.focus(); + const { active } = this.properties; + + if (key === 'root' && active) { + element.focus(); } } protected onKeyDown(event: KeyboardEvent) { const { disabled, onKeyDown } = this.properties; + if (!disabled) { + onKeyDown && onKeyDown(event); if (event.keyCode === SPACE_KEY) { ( event.target).click(); } - onKeyDown && onKeyDown(event); } } } diff --git a/src/menu/README.md b/src/menu/README.md index a56d64dc20..7baf57a769 100644 --- a/src/menu/README.md +++ b/src/menu/README.md @@ -144,21 +144,22 @@ w(Menu, {}, [ ### Keyboard Navigation -`Menu` follows the [WAI-ARIA Authoring Practices](https://www.w3.org/TR/wai-aria-practices-1.1/#menu) for menus to the extent that the various use cases are described. As such, `Menu` provides the following keyboard interactions: +`Menu` follows the [WAI-ARIA Authoring Practices](https://www.w3.org/TR/wai-aria-practices-1.1/#menu) for menus to the extent that the various use cases are described. As such, menus exhibit the following behavior: -**With `orientation="horizontal"`**: +- One `tab` press into a top-level menu, one `tab` press out of it. +- Arrow keys control navigation between items within a menu (see below). +- Pressing the escape key within a submenu closes the submenu. When the submenu is re-opened, focus is placed on the first item. +- Pressing the enter key triggers an item's action. If the item is used as the trigger for a submenu, then the submenu is toggled opened/closed. +- Pressing the space key triggers an item's action. If the item is used as the trigger for a submenu, then the submenu is toggled opened/closed. + +**Arrow key navigation with `orientation="horizontal"`**: - Pressing the left arrow key moves focus to the previous item. If the first item is currently selected, focus is moved to the last item. - Pressing the right arrow key moves focus to the next item. If the last item is currently selected, focus is moved to the first item. - Pressing the down arrow key when focus is on a trigger for a submenu moves focus into the submenu, placing the focus on the previously-focused item. If the submenu is hidden, it will be displayed. - Pressing the up arrow key when focus is on an item within a submenu moves focus back to the trigger item. The next time the submenu receives focus, the previously-focused item receives the focus. Pressing the up arrow key has no effect in other circumstances. -**With `orientation="vertical"` (the default)**: +**Arrow key navigation with `orientation="vertical"` (the default)**: - Pressing the up arrow key moves focus to the previous item. If the first item is currently selected, focus is moved to the last item. - Pressing the down arrow key moves focus to the next item. If the last item is currently selected, focus is moved to the first item. - Pressing the right arrow key when focus is on a trigger for a submenu moves focus into the submenu, placing the focus on the previously-focused item. If the submenu is hidden, it will be displayed. - Pressing the left arrow key when focus is on an item within a submenu moves focus back to the trigger item. The next time the submenu receives focus, the previously-focused item receives the focus. Pressing the left arrow key has no effect in other circumstances. - -**Common operations**: -- Pressing the escape key within a submenu closes the submenu. When the submenu is re-opened, focus is placed on the first item. -- Pressing the enter key triggers an item's action. If the item is used as the trigger for a submenu, then the submenu is toggled opened/closed. -- Pressing the space key triggers an item's action. If the item is used as the trigger for a submenu, then the submenu is toggled opened/closed. diff --git a/src/menu/example/index.ts b/src/menu/example/index.ts index 77c4abe494..adb9140f87 100644 --- a/src/menu/example/index.ts +++ b/src/menu/example/index.ts @@ -4,7 +4,7 @@ import { ProjectorMixin } from '@dojo/widget-core/mixins/Projector'; import { StatefulMixin } from '@dojo/widget-core/mixins/Stateful'; import { v, w } from '@dojo/widget-core/d'; import Menu, { Orientation, Role } from '../Menu'; -import MenuItem from '../MenuItem'; +import MenuItem, { MenuItemType } from '../MenuItem'; const AppBase = StatefulMixin(WidgetBase); @@ -192,7 +192,8 @@ export class App extends AppBase { } }, onClick: toggleSelected, - selected: this.state[`${key}Selected`] + selected: this.state[`${key}Selected`], + type: 'checkbox' }, [ name ]); })) ]); diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 9f15423143..3f68ac2100 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -448,10 +448,7 @@ registerSuite({ }); ( menu).onLabelKeyDown( { keyCode: 40 }); ( menu).onLabelKeyDown( {}); - const vnode: any = menu.__render__(); - const menuNode = vnode.children[1]; - assert.strictEqual(menuNode.properties.tabIndex, -1, 'tab index should be -1 when the submenu has focus'); assert.notOk(menu.properties.hidden, 'the submenu should not be hidden'); }, @@ -467,10 +464,7 @@ registerSuite({ }); ( menu).onLabelKeyDown( { keyCode: 39 }); ( menu).onLabelKeyDown( {}); - const vnode: any = menu.__render__(); - const menuNode = vnode.children[1]; - assert.strictEqual(menuNode.properties.tabIndex, -1, 'tab index should be -1 when the submenu has focus'); assert.notOk(menu.properties.hidden, 'the submenu should not be hidden'); } }, @@ -544,31 +538,32 @@ registerSuite({ ( menu).onMenuFocus(); ( menu).onMenuFocusOut(); - assert.isTrue(menu.state.active, 'menu should remain active'); + assert.isFalse(menu.state.active, 'menu should be inactive'); } }, onMenuKeyDown: (() => { + const preventDefault = () => {}; const stopPropagation = () => {}; function getExitAssertion(keyCode = 37, orientation: Orientation = 'vertical') { return function () { const menu = new Menu(); menu.setProperties({ orientation }); - ( menu).onMenuKeyDown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, preventDefault, stopPropagation }); sinon.spy(menu, 'invalidate'); assert.isFalse(( menu.invalidate).called, 'menu should not be invalidated'); menu.setProperties({ label: 'menu label', orientation }); - ( menu).onMenuKeyDown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, preventDefault, stopPropagation }); assert.isFalse(( menu.invalidate).called, 'menu should not be invalidated while hidden'); menu.setProperties({ label: 'menu label', hidden: false, orientation }); const child = new MenuItem(); child.setProperties({ active: true }); menu.setChildren([ child ]); - ( menu).onMenuKeyDown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, preventDefault, stopPropagation }); menu.__render__(); assert.isTrue(( menu.invalidate).called, 'menu should be invalidated'); @@ -585,13 +580,13 @@ registerSuite({ menu.setProperties({ label: 'menu label', hidden: false, orientation }); menu.setChildren( [ first, second ]); ( menu).onMenuFocus(); - ( menu).onMenuKeyDown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, preventDefault, stopPropagation }); ( menu).renderChildren(); assert.isTrue(second.properties.active, 'active status should cycle from the first to last child'); assert.notOk(first.properties.active, 'only one child should be active'); - ( menu).onMenuKeyDown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, preventDefault, stopPropagation }); ( menu).renderChildren(); assert.notOk(second.properties.active, 'previously-active child should be inactive'); @@ -608,13 +603,13 @@ registerSuite({ menu.setProperties({ label: 'menu label', hidden: false, orientation }); menu.setChildren( [ first, second ]); ( menu).onMenuFocus(); - ( menu).onMenuKeyDown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, preventDefault, stopPropagation }); ( menu).renderChildren(); assert.isTrue(second.properties.active, 'next child should be active'); assert.notOk(first.properties.active, 'only one child should be active'); - ( menu).onMenuKeyDown( { keyCode, stopPropagation }); + ( menu).onMenuKeyDown( { keyCode, preventDefault, stopPropagation }); ( menu).renderChildren(); assert.notOk(second.properties.active, 'previously-active child should be inactive'); @@ -649,24 +644,35 @@ registerSuite({ child.setProperties({ active: true }); menu.setChildren([ child ]); ( menu).onMenuKeyDown( { keyCode: 9, stopPropagation: () => {} }); - const vnode: any = menu.__render__(); + menu.__render__(); - assert.strictEqual(vnode.properties.tabIndex, 0, 'Menu should be focusable'); assert.notOk(child.properties.active, 'Child should be marked as inactive'); }, 'escape key'() { const menu = new Menu(); - const onRequestHide = sinon.spy(); - menu.setProperties({ - label: 'menu label', - onRequestHide - }); + let onRequestHide = sinon.spy(); + + menu.setProperties({ onRequestHide }); ( menu).onMenuKeyDown( { keyCode: 27, stopPropagation: () => {} }); - const vnode: any = menu.__render__(); + menu.__render__(); + + assert.isFalse(onRequestHide.called, 'top-level menu should not be hidden'); - assert.strictEqual(vnode.children[0].properties.tabIndex, 0, 'label should be focusable'); - assert.isTrue(onRequestHide.called, 'menu should be hidden.'); + const renderLabel = menu.renderLabel; + let label: any; + menu.renderLabel = function () { + label = renderLabel.call(menu); + return label; + }; + + onRequestHide = sinon.spy(); + menu.setProperties({ label: 'menu label', onRequestHide }); + ( menu).onMenuKeyDown( { keyCode: 27, stopPropagation: () => {} }); + menu.__render__(); + + assert.isTrue(label.properties.active, 'menu label should be active'); + assert.isTrue(onRequestHide.called, 'menu should be hidden'); } }, @@ -684,6 +690,63 @@ registerSuite({ }; })(), + onMenuMouseDown: { + 'item with `data-dojo-index` property'() { + const menu = new Menu(); + const children: any[] = '01234'.split('').map(i => { + return new MenuItem(); + }); + + menu.setChildren(children); + ( menu).onMenuMouseDown( { + target: { + hasAttribute: () => false, + parentElement: { + getAttribute: () => '2', + hasAttribute: () => true + } + } + }); + ( menu).onMenuFocus(); + ( menu).renderChildren(); + + children.forEach((child: MenuItem, i) => { + if (i === 2) { + assert.isTrue(child.properties.active); + } + else { + assert.isFalse(child.properties.active); + } + }); + }, + + 'item without `data-dojo-index` property'() { + const menu = new Menu(); + const children: any[] = '01234'.split('').map(i => { + return new MenuItem(); + }); + + menu.setChildren(children); + ( menu).onMenuMouseDown( { + target: { + hasAttribute: () => true, + getAttribute: () => null + } + }); + ( menu).onMenuFocus(); + ( menu).renderChildren(); + + children.forEach((child: MenuItem, i) => { + if (i === 0) { + assert.isTrue(child.properties.active); + } + else { + assert.isFalse(child.properties.active); + } + }); + } + }, + onMenuMouseEnter: { 'when `expandOnClick` is true'() { const menu = new Menu(); diff --git a/src/menu/tests/unit/MenuItem.ts b/src/menu/tests/unit/MenuItem.ts index 67079b4a98..397ad48ba8 100644 --- a/src/menu/tests/unit/MenuItem.ts +++ b/src/menu/tests/unit/MenuItem.ts @@ -52,9 +52,9 @@ registerSuite({ const properties = vnode.properties; assert.strictEqual(properties['aria-controls'], 'controls-base'); - assert.strictEqual(properties['aria-expanded'], 'false'); + assert.isUndefined(properties['aria-expanded']); assert.isUndefined(properties['aria-haspopup']); - assert.strictEqual(properties['aria-disabled'], 'false'); + assert.isUndefined(properties['aria-disabled']); } }, @@ -163,10 +163,13 @@ registerSuite({ expanded() { const item = new MenuItem(); - item.setProperties({ - expanded: true - }); - const vnode: any = item.__render__(); + item.setProperties({ expanded: true }); + let vnode: any = item.__render__(); + + assert.isUndefined(vnode.properties['aria-expanded'], '`expanded` should be ignored without `hasPopup`'); + + item.setProperties({ expanded: true, hasPopup: true }); + vnode = item.__render__(); assert.strictEqual(vnode.properties['aria-expanded'], 'true', '`expanded` should be assigned to the `aria-expanded` attribute'); }, From 3e113fe0a909d8f26419c2baccf345692143d1cc Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Wed, 5 Apr 2017 13:31:55 -0500 Subject: [PATCH 18/19] Key management update. - Prevent the default action when the down arrow key is pressed while the menu trigger is focused. - Add `parentOrientation` property to ensure that vertical submenus of horizontal menus are opened with down arrow key instead of the right arrow key. --- src/menu/Menu.ts | 60 +++++++++++++++++++++++-------------- src/menu/tests/unit/Menu.ts | 5 +++- 2 files changed, 41 insertions(+), 24 deletions(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index 431f0e4f48..8745e52295 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -32,6 +32,8 @@ export type Role = 'menu' | 'menubar'; * @property nested Indicates whether the menu is nested within another menu. * @property onRequestHide Called when the menu is displayed and the trigger is activated. * @property onRequestShow Called when the menu is hidden and the trigger is activated. + * @property orientation Determines whether the menu is rendered horizontally. + * @property parentOrientation Indicates the orientation of the menu's parent (if applicable). * @property role The value to use for the menu's `role` property. Defaults to 'menu'. */ export interface MenuProperties extends ThemeableProperties { @@ -49,6 +51,7 @@ export interface MenuProperties extends ThemeableProperties { onRequestHide?: () => void; onRequestShow?: () => void; orientation?: Orientation; + parentOrientation?: Orientation; role?: Role; } @@ -62,27 +65,17 @@ export const enum Operation { increase } -const commonKeys = { - enter: 13, - escape: 27, - space: 32, - tab: 9 +const enum Keys { + down = 40, + enter = 13, + escape = 27, + left = 37, + right = 39, + space = 32, + tab = 9, + up = 38 }; -const horizontalKeys = assign({ - ascend: 38, // up arrow - decrease: 37, // left arrow - descend: 40, // down arrow - increase: 39 // right arrow -}, commonKeys); - -const verticalKeys = assign({ - ascend: 37, // left arrow - decrease: 38, // up arrow - descend: 39, // right arrow - increase: 40 // down arrow -}, commonKeys); - export const MenuBase = StatefulMixin(ThemeableMixin(WidgetBase)); @theme(css) @@ -154,6 +147,23 @@ export class Menu extends MenuBase { } } + private _getKeys() { + const { orientation = this.getDefaultOrientation(), parentOrientation } = this.properties; + const isHorizontal = orientation === 'horizontal'; + + return assign({ + ascend: isHorizontal ? Keys.up : Keys.left, + decrease: isHorizontal ? Keys.left : Keys.up, + descend: isHorizontal || parentOrientation === 'horizontal' ? Keys.down : Keys.right, + increase: isHorizontal ? Keys.right : Keys.down + }, { + enter: Keys.enter, + escape: Keys.escape, + space: Keys.space, + tab: Keys.tab + }); + } + protected animate(element: HTMLElement) { const { hidden = this.getDefaultHidden() } = this.properties; @@ -295,16 +305,20 @@ export class Menu extends MenuBase { } protected onLabelKeyDown(event: KeyboardEvent) { - const { disabled, orientation = this.getDefaultOrientation() } = this.properties; - const keys = orientation === 'horizontal' ? horizontalKeys : verticalKeys; + const { disabled } = this.properties; if (!disabled) { + const keys = this._getKeys(); const key = event.keyCode; if (key === keys.enter) { this.toggleDisplay(); } else if (key === keys.descend) { + if (key === Keys.down) { + event.preventDefault(); + } + this.toggleDisplay(true); } } @@ -328,8 +342,8 @@ export class Menu extends MenuBase { } protected onMenuKeyDown(event: KeyboardEvent) { - const { label, orientation = this.getDefaultOrientation() } = this.properties; - const keys = orientation === 'horizontal' ? horizontalKeys : verticalKeys; + const { label } = this.properties; + const keys = this._getKeys(); switch (event.keyCode) { case keys.space: diff --git a/src/menu/tests/unit/Menu.ts b/src/menu/tests/unit/Menu.ts index 3f68ac2100..91d2ba3ec2 100644 --- a/src/menu/tests/unit/Menu.ts +++ b/src/menu/tests/unit/Menu.ts @@ -446,9 +446,12 @@ registerSuite({ label: 'Menu label', orientation: 'horizontal' }); - ( menu).onLabelKeyDown( { keyCode: 40 }); + + const preventDefault = sinon.spy(); + ( menu).onLabelKeyDown( { keyCode: 40, preventDefault }); ( menu).onLabelKeyDown( {}); + assert.isTrue(preventDefault.called, 'the default action should be prevented to prevent scrolling'); assert.notOk(menu.properties.hidden, 'the submenu should not be hidden'); }, From 1d2f2e7654f87660c5c886cdebf97c9f8c27abd5 Mon Sep 17 00:00:00 2001 From: Matt Wistrand Date: Wed, 5 Apr 2017 14:05:39 -0500 Subject: [PATCH 19/19] Simplify `_getKeys` method. --- src/menu/Menu.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/menu/Menu.ts b/src/menu/Menu.ts index 8745e52295..9e133b7aa9 100644 --- a/src/menu/Menu.ts +++ b/src/menu/Menu.ts @@ -1,4 +1,3 @@ -import { assign } from '@dojo/core/lang'; import { createTimer } from '@dojo/core/util'; import uuid from '@dojo/core/uuid'; import { Handle } from '@dojo/interfaces/core'; @@ -151,17 +150,16 @@ export class Menu extends MenuBase { const { orientation = this.getDefaultOrientation(), parentOrientation } = this.properties; const isHorizontal = orientation === 'horizontal'; - return assign({ + return { ascend: isHorizontal ? Keys.up : Keys.left, decrease: isHorizontal ? Keys.left : Keys.up, descend: isHorizontal || parentOrientation === 'horizontal' ? Keys.down : Keys.right, - increase: isHorizontal ? Keys.right : Keys.down - }, { enter: Keys.enter, escape: Keys.escape, + increase: isHorizontal ? Keys.right : Keys.down, space: Keys.space, tab: Keys.tab - }); + }; } protected animate(element: HTMLElement) {