Skip to content

Commit

Permalink
fix(select): revert 2fed2c1 that delegates to list for single selecti…
Browse files Browse the repository at this point in the history 
…on logic

PiperOrigin-RevId: 318345783
  • Loading branch information
@allan-chen @copybara-github
allan-chen authored and copybara-github committed Jun 25, 2020
1 parent 2fed2c1 commit 38197b4
Show file tree
Hide file tree
Showing 8 changed files with 102 additions and 171 deletions.
3 changes: 1 addition & 2 deletions packages/mdc-list/component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,7 @@ export class MDCList extends MDCComponent<MDCListFoundation> {
return toggleEl!.checked;
},
isFocusInsideList: () => {
return this.root !== document.activeElement &&
this.root.contains(document.activeElement);
return this.root.contains(document.activeElement);
},
isRootFocused: () => document.activeElement === this.root,
listItemAtIndexHasClass: (index, className) =>
Expand Down
56 changes: 18 additions & 38 deletions packages/mdc-list/foundation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@
* THE SOFTWARE.
*/

// TODO(b/152410470): Remove trailing underscores from private properties
// tslint:disable:strip-private-property-underscore

import {MDCFoundation} from '@material/base/foundation';
import {normalizeKey} from '@material/dom/keyboard';

Expand Down Expand Up @@ -178,7 +175,6 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
handleFocusIn(_: FocusEvent, listItemIndex: number) {
if (listItemIndex >= 0) {
this.focusedItemIndex = listItemIndex;
this.adapter.setAttributeForElementIndex(listItemIndex, 'tabindex', '0');
this.adapter.setTabIndexForListItemChildren(listItemIndex, '0');
}
}
Expand All @@ -188,7 +184,6 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
*/
handleFocusOut(_: FocusEvent, listItemIndex: number) {
if (listItemIndex >= 0) {
this.adapter.setAttributeForElementIndex(listItemIndex, 'tabindex', '-1');
this.adapter.setTabIndexForListItemChildren(listItemIndex, '-1');
}

Expand All @@ -198,7 +193,7 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
*/
setTimeout(() => {
if (!this.adapter.isFocusInsideList()) {
this.setTabindexToFirstSelectedOrFocusedItem();
this.setTabindexToFirstSelectedItem_();
}
}, 0);
}
Expand Down Expand Up @@ -230,7 +225,7 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
const handleKeydownOpts: typeahead.HandleKeydownOpts = {
event,
focusItemAtIndex: (index) => {
this.focusItemAtIndex(index);
this.focusItemAtIndex(index)
},
focusedItemIndex: -1,
isTargetListItem: isRootListItem,
Expand Down Expand Up @@ -316,6 +311,9 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
return;
}

this.setTabindexAtIndex_(index);
this.focusedItemIndex = index;

if (this.adapter.listItemAtIndexHasClass(
index, cssClasses.LIST_ITEM_DISABLED_CLASS)) {
return;
Expand Down Expand Up @@ -411,12 +409,8 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
this.adapter.removeClassForElementIndex(
this.selectedIndex_ as number, selectedClassName);
}

this.adapter.addClassForElementIndex(index, selectedClassName);
this.setAriaForSingleSelectionAtIndex_(index);
this.setTabindexAtIndex_(index);
if (index !== numbers.UNSET_INDEX) {
this.adapter.addClassForElementIndex(index, selectedClassName);
}

this.selectedIndex_ = index;
}
Expand All @@ -439,12 +433,9 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
this.selectedIndex_ as number, ariaAttribute, 'false');
}

if (index !== numbers.UNSET_INDEX) {
const ariaAttributeValue =
isAriaCurrent ? this.ariaCurrentAttrValue_ : 'true';
this.adapter.setAttributeForElementIndex(
index, ariaAttribute, ariaAttributeValue as string);
}
const ariaAttributeValue = isAriaCurrent ? this.ariaCurrentAttrValue_ : 'true';
this.adapter.setAttributeForElementIndex(
index, ariaAttribute, ariaAttributeValue as string);
}

/**
Expand Down Expand Up @@ -481,27 +472,15 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {

private setTabindexAtIndex_(index: number) {
if (this.focusedItemIndex === numbers.UNSET_INDEX && index !== 0) {
// If some list item was selected set first list item's tabindex to -1.
// Generally, tabindex is set to 0 on first list item of list that has no
// preselected items.
// If no list item was selected set first list item's tabindex to -1.
// Generally, tabindex is set to 0 on first list item of list that has no preselected items.
this.adapter.setAttributeForElementIndex(0, 'tabindex', '-1');
} else if (this.focusedItemIndex >= 0 && this.focusedItemIndex !== index) {
this.adapter.setAttributeForElementIndex(
this.focusedItemIndex, 'tabindex', '-1');
}

// Set the previous selection's tabindex to -1. We need this because
// in selection menus that are not visible, programmatically setting an
// option will not change focus but will change where tabindex should be 0.
if (!(this.selectedIndex_ instanceof Array) &&
this.selectedIndex_ !== index) {
this.adapter.setAttributeForElementIndex(
this.selectedIndex_, 'tabindex', '-1');
}

if (index !== numbers.UNSET_INDEX) {
this.adapter.setAttributeForElementIndex(index, 'tabindex', '0');
}
this.adapter.setAttributeForElementIndex(index, 'tabindex', '0');
}

/**
Expand All @@ -511,8 +490,9 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
return this.isSingleSelectionList_ || this.isCheckboxList_ || this.isRadioList_;
}

private setTabindexToFirstSelectedOrFocusedItem() {
let targetIndex = this.focusedItemIndex >= 0 ? this.focusedItemIndex : 0;
private setTabindexToFirstSelectedItem_() {
let targetIndex = 0;

if (this.isSelectableList_()) {
if (typeof this.selectedIndex_ === 'number' && this.selectedIndex_ !== numbers.UNSET_INDEX) {
targetIndex = this.selectedIndex_;
Expand All @@ -539,8 +519,7 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
if (this.isCheckboxList_) {
throw new Error('MDCListFoundation: Expected array of index for checkbox based list but got number: ' + index);
}
return this.isIndexInRange_(index) ||
this.isSingleSelectionList_ && index === numbers.UNSET_INDEX;
return this.isIndexInRange_(index);
} else {
return false;
}
Expand Down Expand Up @@ -587,6 +566,7 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
}

private focusItemAtIndex(index: number) {
this.setTabindexAtIndex_(index);
this.adapter.focusItemAtIndex(index);
this.focusedItemIndex = index;
}
Expand All @@ -608,7 +588,7 @@ export class MDCListFoundation extends MDCFoundation<MDCListAdapter> {
nextChar: string, startingIndex?: number, skipFocus = false) {
const opts: typeahead.TypeaheadMatchItemOpts = {
focusItemAtIndex: (index) => {
this.focusItemAtIndex(index);
this.focusItemAtIndex(index)
},
focusedItemIndex: startingIndex ? startingIndex : this.focusedItemIndex,
nextChar,
Expand Down
37 changes: 10 additions & 27 deletions packages/mdc-list/test/foundation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,11 @@ describe('MDCListFoundation', () => {
.toHaveBeenCalledWith(2, 'tabindex', '0');
});

it('#handleFocusOut sets tabindex=0 to previously focused item when focus' +
'leaves list that has no selection',
it('#handleFocusOut sets tabindex=0 to first item when focus leaves single selection list that has no ' +
'selection',
() => {
const {foundation, mockAdapter} = setupTest();

foundation['focusedItemIndex'] = 3;
foundation.setSingleSelection(true);
mockAdapter.getListItemCount.and.returnValue(4);
mockAdapter.isFocusInsideList.and.returnValue(false);
Expand All @@ -213,7 +212,7 @@ describe('MDCListFoundation', () => {
foundation.handleFocusOut(event, 3);
jasmine.clock().tick(1);
expect(mockAdapter.setAttributeForElementIndex)
.toHaveBeenCalledWith(3, 'tabindex', '0');
.toHaveBeenCalledWith(0, 'tabindex', '0');
});

it('#handleFocusOut does not set tabindex=0 to selected list item when focus moves to next list item.',
Expand Down Expand Up @@ -690,8 +689,7 @@ describe('MDCListFoundation', () => {
expect(preventDefault).toHaveBeenCalledTimes(1);
expect(mockAdapter.setAttributeForElementIndex)
.toHaveBeenCalledWith(0, strings.ARIA_SELECTED, 'true');
expect(mockAdapter.setAttributeForElementIndex)
.toHaveBeenCalledWith(0, 'tabindex', '0');
expect(mockAdapter.setAttributeForElementIndex).toHaveBeenCalledTimes(1);
});

it('#handleKeydown does not select the list item when' +
Expand Down Expand Up @@ -733,9 +731,7 @@ describe('MDCListFoundation', () => {
expect(preventDefault).toHaveBeenCalledTimes(1);
expect(mockAdapter.setAttributeForElementIndex)
.toHaveBeenCalledWith(0, strings.ARIA_SELECTED, 'true');
expect(mockAdapter.setAttributeForElementIndex)
.toHaveBeenCalledWith(0, 'tabindex', '0');
;
expect(mockAdapter.setAttributeForElementIndex).toHaveBeenCalledTimes(1);
});

it('#handleKeydown space key when singleSelection=true does not select an element is isRootListItem=false',
Expand Down Expand Up @@ -797,8 +793,7 @@ describe('MDCListFoundation', () => {
expect(preventDefault).toHaveBeenCalledTimes(2);
expect(mockAdapter.setAttributeForElementIndex)
.toHaveBeenCalledWith(0, strings.ARIA_SELECTED, 'true');
expect(mockAdapter.setAttributeForElementIndex)
.not.toHaveBeenCalledWith(0, strings.ARIA_SELECTED, 'false');
expect(mockAdapter.setAttributeForElementIndex).toHaveBeenCalledTimes(1);
});

it('#handleKeydown space key is triggered 2x when singleSelection is true on second ' +
Expand All @@ -820,8 +815,7 @@ describe('MDCListFoundation', () => {
expect(preventDefault).toHaveBeenCalledTimes(2);
expect(mockAdapter.setAttributeForElementIndex)
.toHaveBeenCalledWith(1, strings.ARIA_SELECTED, 'true');
expect(mockAdapter.setAttributeForElementIndex)
.toHaveBeenCalledWith(0, 'tabindex', '-1');
expect(mockAdapter.setAttributeForElementIndex).toHaveBeenCalledTimes(1);
});

it('#handleKeydown bail out early if event origin doesnt have a mdc-list-item ancestor from the current list',
Expand Down Expand Up @@ -997,7 +991,7 @@ describe('MDCListFoundation', () => {
(args: any) => JSON.stringify(args) ==
JSON.stringify([0, 'tabindex', '0']))
.length)
.toEqual(1);
.toEqual(2);
});

it('#handleClick when toggleCheckbox=false does not change the checkbox state',
Expand Down Expand Up @@ -1183,13 +1177,7 @@ describe('MDCListFoundation', () => {
.not.toHaveBeenCalledWith(2, strings.ARIA_CURRENT, 'false');
expect(mockAdapter.setAttributeForElementIndex)
.toHaveBeenCalledWith(2, strings.ARIA_CURRENT, 'page');

expect(mockAdapter.setAttributeForElementIndex.calls.allArgs()
.filter(
(args: any) => JSON.stringify(args) ==
JSON.stringify([2, strings.ARIA_CURRENT, 'page']))
.length)
.toEqual(1);
expect(mockAdapter.setAttributeForElementIndex).toHaveBeenCalledTimes(1);
});

it('#setSelectedIndex should set aria-selected as default option in the absence of aria-selected on pre-selected ' +
Expand All @@ -1206,12 +1194,7 @@ describe('MDCListFoundation', () => {
.not.toHaveBeenCalledWith(2, jasmine.any(String), 'false');
expect(mockAdapter.setAttributeForElementIndex)
.toHaveBeenCalledWith(2, strings.ARIA_SELECTED, 'true');
expect(mockAdapter.setAttributeForElementIndex.calls.allArgs()
.filter(
(args: any) => JSON.stringify(args) ==
JSON.stringify([2, strings.ARIA_SELECTED, 'true']))
.length)
.toEqual(1);
expect(mockAdapter.setAttributeForElementIndex).toHaveBeenCalledTimes(1);
});

it('#setSelectedIndex sets aria-current="false" to previously selected index and sets aria-current without any token' +
Expand Down
39 changes: 1 addition & 38 deletions packages/mdc-menu/component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,16 @@
* THE SOFTWARE.
*/

// TODO(b/152410470): Remove trailing underscores from private properties
// tslint:disable:strip-private-property-underscore

import {MDCComponent} from '@material/base/component';
import {CustomEventListener, SpecificEventListener} from '@material/base/types';
import {closest} from '@material/dom/ponyfill';
import {MDCList, MDCListFactory} from '@material/list/component';
import {numbers as listConstants} from '@material/list/constants';
import {MDCListFoundation} from '@material/list/foundation';
import {MDCListActionEvent, MDCListIndex} from '@material/list/types';
import {MDCListActionEvent} from '@material/list/types';
import {MDCMenuSurface, MDCMenuSurfaceFactory} from '@material/menu-surface/component';
import {Corner} from '@material/menu-surface/constants';
import {MDCMenuSurfaceFoundation} from '@material/menu-surface/foundation';
import {MDCMenuDistance} from '@material/menu-surface/types';

import {MDCMenuAdapter} from './adapter';
import {cssClasses, DefaultFocusState, strings} from './constants';
import {MDCMenuFoundation} from './foundation';
Expand Down Expand Up @@ -177,38 +172,6 @@ export class MDCMenu extends MDCComponent<MDCMenuFoundation> {
return this.list_ ? this.list_.listElements : [];
}

/**
* Turns on/off the underlying list's single selection mode. Used mainly
* by select menu.
*
* @param singleSelection Whether to enable single selection mode.
*/
set singleSelection(singleSelection: boolean) {
if (this.list_) {
this.list_.singleSelection = singleSelection;
}
}

/**
* Retrieves the selected index. Only applicable to select menus.
* @return The selected index, which is a number for single selection and
* radio lists, and an array of numbers for checkbox lists.
*/
get selectedIndex(): MDCListIndex {
return this.list_ ? this.list_.selectedIndex : listConstants.UNSET_INDEX;
}

/**
* Sets the selected index of the list. Only applicable to select menus.
* @param index The selected index, which is a number for single selection and
* radio lists, and an array of numbers for checkbox lists.
*/
set selectedIndex(index: MDCListIndex) {
if (this.list_) {
this.list_.selectedIndex = index;
}
}

set quickOpen(quickOpen: boolean) {
this.menuSurface_.quickOpen = quickOpen;
}
Expand Down
10 changes: 0 additions & 10 deletions packages/mdc-select/adapter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,16 +202,6 @@ export interface MDCSelectAdapter {
*/
getMenuItemAttr(menuItem: Element, attr: string): string | null;

/**
* Returns the selected index.
*/
getSelectedIndex(): number;

/**
* Sets the selected index in the menu.
*/
setSelectedIndex(index: number): void;

/**
* Adds the class name on the menu item at the given index.
*/
Expand Down
8 changes: 0 additions & 8 deletions packages/mdc-select/component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,6 @@ export class MDCSelect extends MDCComponent<MDCSelectFoundation> {
this.menuElement = this.root.querySelector(strings.MENU_SELECTOR)!;
this.menu = menuFactory(this.menuElement);
this.menu.hasTypeahead = true;
this.menu.singleSelection = true;
}

private createRipple(): MDCRipple {
Expand Down Expand Up @@ -380,13 +379,6 @@ export class MDCSelect extends MDCComponent<MDCSelectFoundation> {
setMenuWrapFocus: (wrapFocus: boolean) => {
this.menu.wrapFocus = wrapFocus;
},
getSelectedIndex: () => {
const index = this.menu.selectedIndex;
return index instanceof Array ? index[0] : index;
},
setSelectedIndex: (index: number) => {
this.menu.selectedIndex = index;
},
setAttributeAtIndex:
(index: number, attributeName: string, attributeValue: string) => {
this.menu.items[index].setAttribute(attributeName, attributeValue);
Expand Down
Loading

0 comments on commit 38197b4

Please sign in to comment.