Skip to content

Commit

Permalink
fix(checkbox, radio): ripple error on focus event (#3869)
Browse files Browse the repository at this point in the history 
* The checkbox and radio disable the ripples using a `*ngFor` check. This causes the `ViewChildren` instance to turn null. The reference is required for the focus indicators.

* If ripples are disabled there will be still ripples on click. This is due to the fact that label elements redirect focus to the underlying input element. The `focusOrigin` is therefore `program` and a focus ripple will show up.

* Tests had to be adjusted because the `[md-ripple]` element won't be removed anymore. Therefore the tests now confirm that no ripples are showing up.

Fixes #3856.
  • Loading branch information
@devversion @jelbourn
devversion authored and jelbourn committed Apr 11, 2017
1 parent 7f65f31 commit e22b55e
Show file tree
Hide file tree
Showing 7 changed files with 98 additions and 95 deletions.
4 changes: 2 additions & 2 deletions src/lib/checkbox/checkbox.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@
[indeterminate]="indeterminate"
[attr.aria-label]="ariaLabel"
[attr.aria-labelledby]="ariaLabelledby"
(blur)="_onInputBlur()"
(change)="_onInteractionEvent($event)"
(click)="_onInputClick($event)">
<div md-ripple *ngIf="!_isRippleDisabled()" class="mat-checkbox-ripple"
<div md-ripple class="mat-checkbox-ripple"
[mdRippleTrigger]="label"
[mdRippleDisabled]="_isRippleDisabled()"
[mdRippleCentered]="true"></div>
<div class="mat-checkbox-frame"></div>
<div class="mat-checkbox-background">
Expand Down
52 changes: 23 additions & 29 deletions src/lib/checkbox/checkbox.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,23 +359,6 @@ describe('MdCheckbox', () => {
.toBe(0, 'Expected no ripple after element is blurred.');
}));

it('should show a ripple when focused programmatically', fakeAsync(() => {
expect(fixture.nativeElement.querySelectorAll('.mat-ripple-element').length)
.toBe(0, 'Expected no ripples to be present.');

dispatchFakeEvent(inputElement, 'focus');
tick(RIPPLE_FADE_IN_DURATION);

expect(fixture.nativeElement.querySelectorAll('.mat-ripple-element').length)
.toBe(1, 'Expected focus ripple to be present.');

dispatchFakeEvent(checkboxInstance._inputElement.nativeElement, 'blur');
tick(RIPPLE_FADE_OUT_DURATION);

expect(fixture.nativeElement.querySelectorAll('.mat-ripple-element').length)
.toBe(0, 'Expected focus ripple to be removed.');
}));

describe('ripple elements', () => {

it('should show ripples on label mousedown', () => {
Expand All @@ -387,30 +370,41 @@ describe('MdCheckbox', () => {
expect(checkboxNativeElement.querySelectorAll('.mat-ripple-element').length).toBe(1);
});

it('should not have a ripple when disabled', () => {
let rippleElement = checkboxNativeElement.querySelector('[md-ripple]');
expect(rippleElement).toBeTruthy('Expected an enabled checkbox to have a ripple');

it('should not show ripples when disabled', () => {
testComponent.isDisabled = true;
fixture.detectChanges();

rippleElement = checkboxNativeElement.querySelector('[md-ripple]');
expect(rippleElement).toBeFalsy('Expected a disabled checkbox not to have a ripple');
dispatchFakeEvent(labelElement, 'mousedown');
dispatchFakeEvent(labelElement, 'mouseup');

expect(checkboxNativeElement.querySelectorAll('.mat-ripple-element').length).toBe(0);

testComponent.isDisabled = false;
fixture.detectChanges();

dispatchFakeEvent(labelElement, 'mousedown');
dispatchFakeEvent(labelElement, 'mouseup');

expect(checkboxNativeElement.querySelectorAll('.mat-ripple-element').length).toBe(1);
});

it('should remove ripple if mdRippleDisabled input is set', async(() => {
it('should remove ripple if mdRippleDisabled input is set', () => {
testComponent.disableRipple = true;
fixture.detectChanges();

expect(checkboxNativeElement.querySelectorAll('[md-ripple]').length)
.toBe(0, 'Expect no [md-ripple] in checkbox');
dispatchFakeEvent(labelElement, 'mousedown');
dispatchFakeEvent(labelElement, 'mouseup');

expect(checkboxNativeElement.querySelectorAll('.mat-ripple-element').length).toBe(0);

testComponent.disableRipple = false;
fixture.detectChanges();

expect(checkboxNativeElement.querySelectorAll('[md-ripple]').length)
.toBe(1, 'Expect [md-ripple] in checkbox');
}));
dispatchFakeEvent(labelElement, 'mousedown');
dispatchFakeEvent(labelElement, 'mouseup');

expect(checkboxNativeElement.querySelectorAll('.mat-ripple-element').length).toBe(1);
});
});

describe('color behaviour', () => {
Expand Down
41 changes: 19 additions & 22 deletions src/lib/checkbox/checkbox.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ import {
} from '@angular/core';
import {NG_VALUE_ACCESSOR, ControlValueAccessor} from '@angular/forms';
import {coerceBooleanProperty} from '../core/coercion/boolean-property';
import {Subscription} from 'rxjs/Subscription';
import {
MdRipple,
RippleRef,
FocusOriginMonitor,
FocusOrigin,
} from '../core';


Expand Down Expand Up @@ -183,10 +183,7 @@ export class MdCheckbox implements ControlValueAccessor, AfterViewInit, OnDestro
private _controlValueAccessorChangeFn: (value: any) => void = (value) => {};

/** Reference to the focused state ripple. */
private _focusedRipple: RippleRef;

/** Reference to the focus origin monitor subscription. */
private _focusedSubscription: Subscription;
private _focusRipple: RippleRef;

constructor(private _renderer: Renderer,
private _elementRef: ElementRef,
Expand All @@ -196,13 +193,9 @@ export class MdCheckbox implements ControlValueAccessor, AfterViewInit, OnDestro
}

ngAfterViewInit() {
this._focusedSubscription = this._focusOriginMonitor
this._focusOriginMonitor
.monitor(this._inputElement.nativeElement, this._renderer, false)
.subscribe(focusOrigin => {
if (!this._focusedRipple && (focusOrigin === 'keyboard' || focusOrigin === 'program')) {
this._focusedRipple = this._ripple.launch(0, 0, { persistent: true, centered: true });
}
});
.subscribe(focusOrigin => this._onInputFocusChange(focusOrigin));
}

ngOnDestroy() {
Expand Down Expand Up @@ -343,10 +336,14 @@ export class MdCheckbox implements ControlValueAccessor, AfterViewInit, OnDestro
this.change.emit(event);
}

/** Informs the component when we lose focus in order to style accordingly */
_onInputBlur() {
this._removeFocusedRipple();
this.onTouched();
/** Function is called whenever the focus changes for the input element. */
private _onInputFocusChange(focusOrigin: FocusOrigin) {
if (!this._focusRipple && focusOrigin === 'keyboard') {
this._focusRipple = this._ripple.launch(0, 0, {persistent: true, centered: true});
} else if (!focusOrigin) {
this._removeFocusRipple();
this.onTouched();
}
}

/** Toggles the `checked` state of the checkbox. */
Expand All @@ -371,7 +368,7 @@ export class MdCheckbox implements ControlValueAccessor, AfterViewInit, OnDestro
// Preventing bubbling for the second event will solve that issue.
event.stopPropagation();

this._removeFocusedRipple();
this._removeFocusRipple();

if (!this.disabled) {
this.toggle();
Expand All @@ -387,7 +384,7 @@ export class MdCheckbox implements ControlValueAccessor, AfterViewInit, OnDestro

/** Focuses the checkbox. */
focus(): void {
this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, this._renderer, 'program');
this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, this._renderer, 'keyboard');
}

_onInteractionEvent(event: Event) {
Expand Down Expand Up @@ -429,11 +426,11 @@ export class MdCheckbox implements ControlValueAccessor, AfterViewInit, OnDestro
return `mat-checkbox-anim-${animSuffix}`;
}

/** Fades out the focused state ripple. */
private _removeFocusedRipple(): void {
if (this._focusedRipple) {
this._focusedRipple.fadeOut();
this._focusedRipple = null;
/** Fades out the focus state ripple. */
private _removeFocusRipple(): void {
if (this._focusRipple) {
this._focusRipple.fadeOut();
this._focusRipple = null;
}
}
}
4 changes: 2 additions & 2 deletions src/lib/radio/radio.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
<div class="mat-radio-container">
<div class="mat-radio-outer-circle"></div>
<div class="mat-radio-inner-circle"></div>
<div md-ripple *ngIf="!_isRippleDisabled()" class="mat-radio-ripple"
<div md-ripple class="mat-radio-ripple"
[mdRippleTrigger]="label"
[mdRippleDisabled]="_isRippleDisabled()"
[mdRippleCentered]="true"></div>
</div>

Expand All @@ -18,7 +19,6 @@
[attr.aria-label]="ariaLabel"
[attr.aria-labelledby]="ariaLabelledby"
(change)="_onInputChange($event)"
(blur)="_onInputBlur()"
(click)="_onInputClick($event)">

<!-- The label content for radio control. -->
Expand Down
48 changes: 31 additions & 17 deletions src/lib/radio/radio.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,33 +228,47 @@ describe('MdRadio', () => {
expect(radioInstances.every(radio => !radio.checked)).toBe(true);
});

it('should not have a ripple on disabled radio buttons', () => {
let rippleElement = radioNativeElements[0].querySelector('[md-ripple]');
expect(rippleElement).toBeTruthy('Expected an enabled radio button to have a ripple');

it('should not show ripples on disabled radio buttons', () => {
radioInstances[0].disabled = true;
fixture.detectChanges();

rippleElement = radioNativeElements[0].querySelector('[md-ripple]');
expect(rippleElement).toBeFalsy('Expected a disabled radio button not to have a ripple');
dispatchFakeEvent(radioLabelElements[0], 'mousedown');
dispatchFakeEvent(radioLabelElements[0], 'mouseup');

expect(radioNativeElements[0].querySelectorAll('.mat-ripple-element').length)
.toBe(0, 'Expected a disabled radio button to not show ripples');

radioInstances[0].disabled = false;
fixture.detectChanges();

dispatchFakeEvent(radioLabelElements[0], 'mousedown');
dispatchFakeEvent(radioLabelElements[0], 'mouseup');

expect(radioNativeElements[0].querySelectorAll('.mat-ripple-element').length)
.toBe(1, 'Expected an enabled radio button to show ripples');
});

it('should remove ripple if mdRippleDisabled input is set', async(() => {
it('should not show ripples if mdRippleDisabled input is set', () => {
testComponent.disableRipple = true;
fixture.detectChanges();
for (let radioNativeElement of radioNativeElements)
{
expect(radioNativeElement.querySelectorAll('[md-ripple]').length)
.toBe(1, 'Expect [md-ripple] in radio buttons');

for (let radioLabel of radioLabelElements) {
dispatchFakeEvent(radioLabel, 'mousedown');
dispatchFakeEvent(radioLabel, 'mouseup');

expect(radioLabel.querySelectorAll('.mat-ripple-element').length).toBe(0);
}

testComponent.disableRipple = true;
testComponent.disableRipple = false;
fixture.detectChanges();
for (let radioNativeElement of radioNativeElements)
{
expect(radioNativeElement.querySelectorAll('[md-ripple]').length)
.toBe(0, 'Expect no [md-ripple] in radio buttons');

for (let radioLabel of radioLabelElements) {
dispatchFakeEvent(radioLabel, 'mousedown');
dispatchFakeEvent(radioLabel, 'mouseup');

expect(radioLabel.querySelectorAll('.mat-ripple-element').length).toBe(1);
}
}));
});

it(`should update the group's selected radio to null when unchecking that radio
programmatically`, () => {
Expand Down
42 changes: 20 additions & 22 deletions src/lib/radio/radio.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ import {
UniqueSelectionDispatcher,
MdRipple,
FocusOriginMonitor,
FocusOrigin,
} from '../core';
import {coerceBooleanProperty} from '../core/coercion/boolean-property';
import {Subscription} from 'rxjs/Subscription';


/**
Expand Down Expand Up @@ -405,11 +405,8 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
/** The child ripple instance. */
@ViewChild(MdRipple) _ripple: MdRipple;

/** Stream of focus event from the focus origin monitor. */
private _focusOriginMonitorSubscription: Subscription;

/** Reference to the current focus ripple. */
private _focusedRippleRef: RippleRef;
private _focusRipple: RippleRef;

/** The native `<input type=radio>` element */
@ViewChild('input') _inputElement: ElementRef;
Expand Down Expand Up @@ -446,13 +443,9 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
}

ngAfterViewInit() {
this._focusOriginMonitorSubscription = this._focusOriginMonitor
this._focusOriginMonitor
.monitor(this._inputElement.nativeElement, this._renderer, false)
.subscribe(focusOrigin => {
if (focusOrigin === 'keyboard' && !this._focusedRippleRef) {
this._focusedRippleRef = this._ripple.launch(0, 0, { persistent: true, centered: true });
}
});
.subscribe(focusOrigin => this._onInputFocusChange(focusOrigin));
}

ngOnDestroy() {
Expand All @@ -471,17 +464,6 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
return this.disableRipple || this.disabled;
}

_onInputBlur() {
if (this._focusedRippleRef) {
this._focusedRippleRef.fadeOut();
this._focusedRippleRef = null;
}

if (this.radioGroup) {
this.radioGroup._touch();
}
}

_onInputClick(event: Event) {
// We have to stop propagation for click events on the visual hidden input element.
// By default, when a user clicks on a label element, a generated click event will be
Expand Down Expand Up @@ -516,4 +498,20 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
}
}

/** Function is called whenever the focus changes for the input element. */
private _onInputFocusChange(focusOrigin: FocusOrigin) {
if (!this._focusRipple && focusOrigin === 'keyboard') {
this._focusRipple = this._ripple.launch(0, 0, {persistent: true, centered: true});
} else if (!focusOrigin) {
if (this.radioGroup) {
this.radioGroup._touch();
}

if (this._focusRipple) {
this._focusRipple.fadeOut();
this._focusRipple = null;
}
}
}

}
2 changes: 1 addition & 1 deletion src/lib/slide-toggle/slide-toggle.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ export class MdSlideToggle implements OnDestroy, AfterContentInit, ControlValueA

/** Focuses the slide-toggle. */
focus() {
this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, this._renderer, 'program');
this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, this._renderer, 'keyboard');
}

/** Whether the slide-toggle is checked. */
Expand Down

0 comments on commit e22b55e

Please sign in to comment.