Skip to content

fix(select): unable to set a tabindex #3479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 10, 2017
Merged

fix(select): unable to set a tabindex #3479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
934be6f
fix(select): unable to set a tabindex
crisbeto Mar 7, 2017
94be467
fix: allow use of `tabindex`
crisbeto Mar 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 32 additions & 3 deletions src/lib/select/select.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ describe('MdSelect', () => {
SelectWithErrorSibling,
ThrowsErrorOnInit,
BasicSelectOnPush,
BasicSelectOnPushPreselected
BasicSelectOnPushPreselected,
SelectWithPlainTabindex
],
providers: [
{provide: OverlayContainer, useFactory: () => {
Expand Down Expand Up @@ -1081,10 +1082,28 @@ describe('MdSelect', () => {
expect(select.getAttribute('aria-label')).toEqual('Food');
});

it('should set the tabindex of the select to 0', () => {
it('should set the tabindex of the select to 0 by default', () => {
expect(select.getAttribute('tabindex')).toEqual('0');
});

it('should be able to override the tabindex', () => {
fixture.componentInstance.tabIndexOverride = 3;
fixture.detectChanges();

expect(select.getAttribute('tabindex')).toBe('3');
});

it('should be able to set the tabindex via the native attribute', () => {
fixture.destroy();

const plainTabindexFixture = TestBed.createComponent(SelectWithPlainTabindex);

plainTabindexFixture.detectChanges();
select = plainTabindexFixture.debugElement.query(By.css('md-select')).nativeElement;

expect(select.getAttribute('tabindex')).toBe('5');
});

it('should set aria-required for required selects', () => {
expect(select.getAttribute('aria-required'))
.toEqual('false', `Expected aria-required attr to be false for normal selects.`);
Expand Down Expand Up @@ -1583,7 +1602,8 @@ describe('MdSelect', () => {
selector: 'basic-select',
template: `
<div [style.height.px]="heightAbove"></div>
<md-select placeholder="Food" [formControl]="control" [required]="isRequired">
<md-select placeholder="Food" [formControl]="control" [required]="isRequired"
[tabIndex]="tabIndexOverride">
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to use tabindex instead of tabIndex. That way users can set tabindex as they normally would without having to learn a different API for the select.

<md-select tabindex="6">
</md-select>

Copy link
Member Author

@crisbeto crisbeto Mar 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I went this route to be consistent with the other components (e.g. the checkbox). IIRC we named it tabIndex, because the property on the native DOM element is called tabIndex.

Copy link
Member

Choose a reason for hiding this comment

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

I have been asking in #3368 about it - I thought about supporting both (for convenience).

Copy link
Contributor

@kara kara Mar 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Chatted with Jeremy about this, and it seems like it makes sense to support both. Since the HTML property is tabIndex, you should bind to tabIndex. But people should still be able to use tabindex="5" without binding the way they normally would. We can achieve this with another @Attribute binding for tabindex.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added @kara, can you take another look?

<md-option *ngFor="let food of foods" [value]="food.value" [disabled]="food.disabled">
{{ food.viewValue }}
</md-option>
Expand All @@ -1606,6 +1626,7 @@ class BasicSelect {
isRequired: boolean;
heightAbove = 0;
heightBelow = 0;
tabIndexOverride: number;

@ViewChild(MdSelect) select: MdSelect;
@ViewChildren(MdOption) options: QueryList<MdOption>;
Expand Down Expand Up @@ -1864,6 +1885,14 @@ class MultiSelect {
@ViewChildren(MdOption) options: QueryList<MdOption>;
}

@Component({
selector: 'select-with-plain-tabindex',
template: `
<md-select tabindex="5"></md-select>
`
})
class SelectWithPlainTabindex { }


class FakeViewportRuler {
getViewportRect() {
Expand Down
26 changes: 18 additions & 8 deletions src/lib/select/select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
ViewEncapsulation,
ViewChild,
ChangeDetectorRef,
Attribute,
} from '@angular/core';
import {MdOption, MdOptionSelectionChange} from '../core/option/option';
import {ENTER, SPACE} from '../core/keyboard/keycodes';
Expand Down Expand Up @@ -99,7 +100,7 @@ export type MdSelectFloatPlaceholderType = 'always' | 'never' | 'auto';
encapsulation: ViewEncapsulation.None,
host: {
'role': 'listbox',
'[attr.tabindex]': '_getTabIndex()',
'[attr.tabindex]': 'tabIndex',
'[attr.aria-label]': 'placeholder',
'[attr.aria-required]': 'required.toString()',
'[attr.aria-disabled]': 'disabled.toString()',
Expand Down Expand Up @@ -151,6 +152,9 @@ export class MdSelect implements AfterContentInit, ControlValueAccessor, OnDestr
/** The animation state of the placeholder. */
private _placeholderState = '';

/** Tab index for the element. */
private _tabIndex: number;

/**
* The width of the trigger. Must be saved to set the min width of the overlay panel
* and the width of the selected value.
Expand Down Expand Up @@ -266,6 +270,15 @@ export class MdSelect implements AfterContentInit, ControlValueAccessor, OnDestr
}
private _floatPlaceholder: MdSelectFloatPlaceholderType = 'auto';

/** Tab index for the select element. */
@Input()
get tabIndex(): number { return this._disabled ? -1 : this._tabIndex; }
set tabIndex(value: number) {
if (typeof value !== 'undefined') {
this._tabIndex = value;
}
}

/** Combined stream of all of the child options' change events. */
get optionSelectionChanges(): Observable<MdOptionSelectionChange> {
return Observable.merge(...this.options.map(option => option.onSelectionChange));
Expand All @@ -282,10 +295,13 @@ export class MdSelect implements AfterContentInit, ControlValueAccessor, OnDestr

constructor(private _element: ElementRef, private _renderer: Renderer,
private _viewportRuler: ViewportRuler, private _changeDetectorRef: ChangeDetectorRef,
@Optional() private _dir: Dir, @Self() @Optional() public _control: NgControl) {
@Optional() private _dir: Dir, @Self() @Optional() public _control: NgControl,
@Attribute('tabindex') tabIndex: string) {
if (this._control) {
this._control.valueAccessor = this;
}

this._tabIndex = parseInt(tabIndex) || 0;
}

ngAfterContentInit() {
Expand Down Expand Up @@ -452,12 +468,6 @@ export class MdSelect implements AfterContentInit, ControlValueAccessor, OnDestr
}
}

/** Returns the correct tabindex for the select depending on disabled state. */
_getTabIndex() {
return this.disabled ? '-1' : '0';
}


/**
* Sets the scroll position of the scroll container. This must be called after
* the overlay pane is attached or the scroll container element will not yet be
Expand Down