From 99af567357dd5c8b74c58a160b704f204ac51054 Mon Sep 17 00:00:00 2001 From: Joy Zhong Date: Tue, 4 Jun 2019 15:47:11 -0400 Subject: [PATCH] fix(menu): Remove code to focus on first/last element on TAB/SHIFT+TAB. (#4786) Instead, TAB should close the menu and respect default browser tab order. BREAKING CHANGE: The following adapter methods were removed: isFirstElementFocused, isLastElementFocused, focusFirstElement, focusLastElement. The following functionality to handle TAB on menusurface has been removed: "If TAB and last element is focused => Focus on first element", "If SHIFT + TAB and first element is focused => Focus on last element" --- packages/mdc-menu-surface/README.md | 4 - packages/mdc-menu-surface/adapter.ts | 8 -- packages/mdc-menu-surface/component.ts | 13 --- packages/mdc-menu-surface/foundation.ts | 16 +--- .../mdc-menu-surface/mdc-menu-surface.test.js | 84 ------------------- .../menu-surface.foundation.test.js | 53 +----------- 6 files changed, 4 insertions(+), 174 deletions(-) diff --git a/packages/mdc-menu-surface/README.md b/packages/mdc-menu-surface/README.md index 4523ab98db3..1a9412fe3dc 100644 --- a/packages/mdc-menu-surface/README.md +++ b/packages/mdc-menu-surface/README.md @@ -206,10 +206,6 @@ Method Signature | Description `isFocused() => boolean` | Returns a boolean value indicating whether the root element of the menu surface is focused. `saveFocus() => void` | Stores the currently focused element on the document, for restoring with `restoreFocus`. `restoreFocus() => void` | Restores the previously saved focus state, by making the previously focused element the active focus again. -`isFirstElementFocused() => boolean` | Returns a boolean value indicating if the first focusable element of the menu-surface is focused. -`isLastElementFocused() => boolean` | Returns a boolean value indicating if the last focusable element of the menu-surface is focused. -`focusFirstElement() => void` | Focuses the first focusable element of the menu-surface. -`focusLastElement() => void` | Focuses the last focusable element of the menu-surface. `getInnerDimensions() => MDCMenuDimensions` | Returns an object with the items container width and height. `getAnchorDimensions() => ClientRect \| null` | Returns an object with the dimensions and position of the anchor. `getBodyDimensions() => MDCMenuDimensions` | Returns an object with width and height of the body, in pixels. diff --git a/packages/mdc-menu-surface/adapter.ts b/packages/mdc-menu-surface/adapter.ts index 4a1aa6b8c69..04a4e9489ba 100644 --- a/packages/mdc-menu-surface/adapter.ts +++ b/packages/mdc-menu-surface/adapter.ts @@ -38,8 +38,6 @@ export interface MDCMenuSurfaceAdapter { isElementInContainer(el: Element): boolean; isFocused(): boolean; - isFirstElementFocused(): boolean; - isLastElementFocused(): boolean; isRtl(): boolean; getInnerDimensions(): MDCMenuDimensions; @@ -57,12 +55,6 @@ export interface MDCMenuSurfaceAdapter { /** Restores focus to the element that was focused before the menu surface was opened. */ restoreFocus(): void; - /** Focuses the first focusable element in the menu-surface. */ - focusFirstElement(): void; - - /** Focuses the first focusable element in the menu-surface. */ - focusLastElement(): void; - /** Emits an event when the menu surface is closed. */ notifyClose(): void; diff --git a/packages/mdc-menu-surface/component.ts b/packages/mdc-menu-surface/component.ts index d3a316f46fa..afea00eb4ab 100644 --- a/packages/mdc-menu-surface/component.ts +++ b/packages/mdc-menu-surface/component.ts @@ -43,8 +43,6 @@ export class MDCMenuSurface extends MDCComponent { protected root_!: HTMLElement; // assigned in MDCComponent constructor private previousFocus_?: HTMLElement | SVGElement | null; - private firstFocusableElement_?: HTMLElement | SVGElement; - private lastFocusableElement_?: HTMLElement | SVGElement; private handleKeydown_!: SpecificEventListener<'keydown'>; // assigned in initialSyncWithDOM() private handleBodyClick_!: SpecificEventListener<'click'>; // assigned in initialSyncWithDOM() @@ -84,9 +82,6 @@ export class MDCMenuSurface extends MDCComponent { set open(value: boolean) { if (value) { - const focusableElements = this.root_.querySelectorAll(strings.FOCUSABLE_ELEMENTS); - this.firstFocusableElement_ = focusableElements[0]; - this.lastFocusableElement_ = focusableElements[focusableElements.length - 1]; this.foundation_.open(); } else { this.foundation_.close(); @@ -173,14 +168,6 @@ export class MDCMenuSurface extends MDCComponent { } } }, - isFirstElementFocused: () => - this.firstFocusableElement_ ? this.firstFocusableElement_ === document.activeElement : false, - isLastElementFocused: () => - this.lastFocusableElement_ ? this.lastFocusableElement_ === document.activeElement : false, - focusFirstElement: () => - this.firstFocusableElement_ && this.firstFocusableElement_.focus && this.firstFocusableElement_.focus(), - focusLastElement: () => - this.lastFocusableElement_ && this.lastFocusableElement_.focus && this.lastFocusableElement_.focus(), getInnerDimensions: () => { return {width: this.root_.offsetWidth, height: this.root_.offsetHeight}; diff --git a/packages/mdc-menu-surface/foundation.ts b/packages/mdc-menu-surface/foundation.ts index 6319c6597f3..564b19f7b02 100644 --- a/packages/mdc-menu-surface/foundation.ts +++ b/packages/mdc-menu-surface/foundation.ts @@ -65,8 +65,6 @@ export class MDCMenuSurfaceFoundation extends MDCFoundation false, isFocused: () => false, - isFirstElementFocused: () => false, - isLastElementFocused: () => false, isRtl: () => false, getInnerDimensions: () => ({height: 0, width: 0}), @@ -80,8 +78,6 @@ export class MDCMenuSurfaceFoundation extends MDCFoundation undefined, restoreFocus: () => undefined, - focusFirstElement: () => undefined, - focusLastElement: () => undefined, notifyClose: () => undefined, notifyOpen: () => undefined, @@ -233,21 +229,11 @@ export class MDCMenuSurfaceFoundation extends MDCFoundation { document.body.removeChild(root); }); -test('adapter#isFirstElementFocused returns true if the first element is focused', () => { - const {root, component} = setupTest({open: true}); - const item = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[0]; - document.body.appendChild(root); - component.open = true; - - assert.isFalse(component.getDefaultFoundation().adapter_.isFirstElementFocused()); - item.focus(); - assert.isTrue(component.getDefaultFoundation().adapter_.isFirstElementFocused()); - component.open = false; - - document.body.removeChild(root); -}); - -test('adapter#isLastElementFocused returns true if the last element is focused', () => { - const {root, component} = setupTest({open: true}); - const item = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[1]; - document.body.appendChild(root); - component.open = true; - - assert.isFalse(component.getDefaultFoundation().adapter_.isLastElementFocused()); - item.focus(); - assert.isTrue(component.getDefaultFoundation().adapter_.isLastElementFocused()); - - component.open = false; - document.body.removeChild(root); -}); - -test('adapter#focusFirstElement focuses the first menu surface element', () => { - const {root, component} = setupTest({open: true}); - const item = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[0]; - document.body.appendChild(root); - component.open = true; - - component.getDefaultFoundation().adapter_.focusFirstElement(); - assert.equal(document.activeElement, item); - component.open = false; - document.body.removeChild(root); -}); - -test('adapter#focusLastElement focuses the last menu surface element', () => { - const {root, component} = setupTest({open: true}); - const item = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[1]; - document.body.appendChild(root); - component.open = true; - - component.getDefaultFoundation().adapter_.focusLastElement(); - assert.equal(document.activeElement, item); - component.open = false; - document.body.removeChild(root); -}); - test('adapter#hasAnchor returns true if the menu surface has an anchor', () => { const anchor = bel`
`; const {root, component} = setupTest({open: true}); @@ -509,35 +457,3 @@ test('adapter#setMaxHeight sets the maxHeight style on the menu surface element' component.getDefaultFoundation().adapter_.setMaxHeight('100px'); assert.equal(root.style.maxHeight, '100px'); }); - -test('Pressing Shift+Tab on first element focuses the last menu surface element', () => { - const root = getFixture(true); - document.body.appendChild(root); - const firstItem = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[0]; - const lastItem = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[1]; - const component = new MDCMenuSurface(root); - component.open = true; - - firstItem.focus(); - component.getDefaultFoundation().handleKeydown({key: 'Tab', shiftKey: true, preventDefault: () => {}}); - assert.equal(document.activeElement, lastItem); - - component.open = false; - document.body.removeChild(root); -}); - -test('Pressing Tab on last element focuses the first menu surface element', () => { - const root = getFixture(true); - document.body.appendChild(root); - const firstItem = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[0]; - const lastItem = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[1]; - const component = new MDCMenuSurface(root); - component.open = true; - - lastItem.focus(); - component.getDefaultFoundation().handleKeydown({key: 'Tab', shiftKey: false, preventDefault: () => {}}); - assert.equal(document.activeElement, firstItem); - - component.open = false; - document.body.removeChild(root); -}); diff --git a/test/unit/mdc-menu-surface/menu-surface.foundation.test.js b/test/unit/mdc-menu-surface/menu-surface.foundation.test.js index d050b441366..ae44fcd01f2 100644 --- a/test/unit/mdc-menu-surface/menu-surface.foundation.test.js +++ b/test/unit/mdc-menu-surface/menu-surface.foundation.test.js @@ -111,9 +111,9 @@ test('exports Corner', () => { test('defaultAdapter returns a complete adapter implementation', () => { verifyDefaultAdapter(MDCMenuSurfaceFoundation, [ 'addClass', 'removeClass', 'hasClass', 'hasAnchor', 'notifyClose', 'notifyOpen', 'isElementInContainer', - 'isRtl', 'setTransformOrigin', 'isFocused', 'saveFocus', 'restoreFocus', 'isFirstElementFocused', - 'isLastElementFocused', 'focusFirstElement', 'focusLastElement', 'getInnerDimensions', 'getAnchorDimensions', - 'getWindowDimensions', 'getBodyDimensions', 'getWindowScroll', 'setPosition', 'setMaxHeight', + 'isRtl', 'setTransformOrigin', 'isFocused', 'saveFocus', 'restoreFocus', 'getInnerDimensions', + 'getAnchorDimensions', 'getWindowDimensions', 'getBodyDimensions', 'getWindowScroll', 'setPosition', + 'setMaxHeight', ]); }); @@ -717,53 +717,6 @@ test('#handleKeydown with Escape key closes the menu surface and sends close eve td.verify(mockAdapter.notifyClose()); }); -test('#handleKeydown with Tab key on the last element, it moves to the first', () => { - const {foundation, mockAdapter} = setupTest(); - const clock = installClock(); - const target = {}; - const event = {target, key: 'Tab', preventDefault: () => {}}; - - td.when(mockAdapter.isLastElementFocused()).thenReturn(true); - - foundation.init(); - foundation.handleKeydown(event); - clock.tick(numbers.SELECTED_TRIGGER_DELAY); - clock.runToFrame(); - td.verify(mockAdapter.focusFirstElement()); -}); - -test('#handleKeydown with Shift+Tab keys on the first element, it moves to the last', () => { - const {foundation, mockAdapter} = setupTest(); - const clock = installClock(); - const target = {}; - const event = {target, key: 'Tab', shiftKey: true, preventDefault: () => {}}; - - td.when(mockAdapter.isFirstElementFocused()).thenReturn(true); - - foundation.init(); - foundation.handleKeydown(event); - clock.tick(numbers.SELECTED_TRIGGER_DELAY); - clock.runToFrame(); - td.verify(mockAdapter.focusLastElement(), {times: 1}); -}); - -test('#handleKeydown with Shift+Tab on middle element does not cause focus to wrap around to first/last', () => { - const {foundation, mockAdapter} = setupTest(); - const clock = installClock(); - const target = {}; - const event = {target, key: 'Tab', shiftKey: true, preventDefault: () => {}}; - - td.when(mockAdapter.isFirstElementFocused()).thenReturn(false); - td.when(mockAdapter.isLastElementFocused()).thenReturn(false); - - foundation.init(); - foundation.handleKeydown(event); - clock.tick(numbers.SELECTED_TRIGGER_DELAY); - clock.runToFrame(); - td.verify(mockAdapter.focusFirstElement(), {times: 0}); - td.verify(mockAdapter.focusLastElement(), {times: 0}); -}); - test('#handleKeydown on any other key, do not prevent default on the event', () => { const {foundation} = setupTest(); const clock = installClock();