Skip to content

fix(checkbox, slide-toggle): no margin if content is projected #12973

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
49 changes: 47 additions & 2 deletions src/lib/checkbox/checkbox.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import {MutationObserverFactory} from '@angular/cdk/observers';
describe('MatCheckbox', () => {
let fixture: ComponentFixture<any>;

function createComponent<T>(componentType: Type<T>): ComponentFixture<T> {
function createComponent<T>(componentType: Type<T>, extraDeclarations: Type<any>[] = []) {
TestBed.configureTestingModule({
imports: [MatCheckboxModule, FormsModule, ReactiveFormsModule],
declarations: [componentType],
declarations: [componentType, ...extraDeclarations],
}).compileComponents();

return TestBed.createComponent<T>(componentType);
Expand Down Expand Up @@ -1104,7 +1104,37 @@ describe('MatCheckbox', () => {
fixture.detectChanges();
expect(checkboxInnerContainer.querySelector('input')!.hasAttribute('value')).toBe(false);
});
});

describe('label margin', () => {
it('should properly update margin if label content is projected', () => {
const mutationCallbacks: Function[] = [];

TestBed.configureTestingModule({
providers: [
{provide: MutationObserverFactory, useValue: {
create: (callback: Function) => {
mutationCallbacks.push(callback);
return {observe: () => {}, disconnect: () => {}};
}
}}
]
});

fixture = createComponent(CheckboxWithProjectedLabel, [TextBindingComponent]);
fixture.detectChanges();

const checkboxInnerContainer = fixture.debugElement
.query(By.css('.mat-checkbox-inner-container')).nativeElement;

// Do not run the change detection for the fixture manually because we want to verify
// that the checkbox properly toggles the margin class even if the observe content output
// fires outside of the zone.
mutationCallbacks.forEach(callback => callback());

expect(checkboxInnerContainer.classList).not
.toContain('mat-checkbox-inner-container-no-side-margin');
});
});
});

Expand Down Expand Up @@ -1238,3 +1268,18 @@ class CheckboxWithoutLabel {
template: `<mat-checkbox tabindex="5"></mat-checkbox>`
})
class CheckboxWithTabindexAttr {}

/** Test component that uses another component for its label. */
@Component({
template: `<mat-checkbox><some-text></some-text></mat-checkbox>`
})
class CheckboxWithProjectedLabel {}

/** Component that renders some text through a binding. */
@Component({
selector: 'some-text',
template: '<span>{{text}}</span>'
})
class TextBindingComponent {
text: string = 'Some text';
}
10 changes: 6 additions & 4 deletions src/lib/checkbox/checkbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -271,10 +271,12 @@ export class MatCheckbox extends _MatCheckboxMixinBase implements ControlValueAc

/** Method being called whenever the label text changes. */
_onLabelTextChange() {
// This method is getting called whenever the label of the checkbox changes.
// Since the checkbox uses the OnPush strategy we need to notify it about the change
// that has been recognized by the cdkObserveContent directive.
this._changeDetectorRef.markForCheck();
// Since the event of the `cdkObserveContent` directive runs outside of the zone, the checkbox
// component will be only marked for check, but no actual change detection runs automatically.
// Instead of going back into the zone in order to trigger a change detection which causes
// *all* components to be checked (if explicitly marked or not using OnPush), we only trigger
// an explicit change detection for the checkbox view and it's children.
this._changeDetectorRef.detectChanges();
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test for this, even if that test is doing getComputedStyle?

}

// Implemented as part of ControlValueAccessor.
Expand Down
44 changes: 40 additions & 4 deletions src/lib/slide-toggle/slide-toggle.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ describe('MatSlideToggle without forms', () => {
declarations: [
SlideToggleBasic,
SlideToggleWithTabindexAttr,
SlideToggleWithoutLabel
SlideToggleWithoutLabel,
SlideToggleProjectedLabel,
TextBindingComponent,
],
providers: [
{provide: HAMMER_GESTURE_CONFIG, useFactory: () => gestureConfig = new TestGestureConfig()},
Expand Down Expand Up @@ -657,7 +659,6 @@ describe('MatSlideToggle without forms', () => {
describe('without label', () => {
let fixture: ComponentFixture<SlideToggleWithoutLabel>;
let testComponent: SlideToggleWithoutLabel;
let slideToggleElement: HTMLElement;
let slideToggleBarElement: HTMLElement;

beforeEach(() => {
Expand All @@ -666,7 +667,6 @@ describe('MatSlideToggle without forms', () => {
const slideToggleDebugEl = fixture.debugElement.query(By.directive(MatSlideToggle));

testComponent = fixture.componentInstance;
slideToggleElement = slideToggleDebugEl.nativeElement;
slideToggleBarElement = slideToggleDebugEl
.query(By.css('.mat-slide-toggle-bar')).nativeElement;
});
Expand Down Expand Up @@ -697,10 +697,33 @@ describe('MatSlideToggle without forms', () => {
flushMutationObserver();
fixture.detectChanges();

expect(slideToggleElement.classList)
expect(slideToggleBarElement.classList)
.not.toContain('mat-slide-toggle-bar-no-side-margin');
}));
});

describe('label margin', () => {
let fixture: ComponentFixture<SlideToggleProjectedLabel>;
let slideToggleBarElement: HTMLElement;

beforeEach(() => {
fixture = TestBed.createComponent(SlideToggleProjectedLabel);
slideToggleBarElement = fixture.debugElement
.query(By.css('.mat-slide-toggle-bar')).nativeElement;

fixture.detectChanges();
});

it('should properly update margin if label content is projected', () => {
// Do not run the change detection for the fixture manually because we want to verify
// that the slide-toggle properly toggles the margin class even if the observe content
// output fires outside of the zone.
flushMutationObserver();

expect(slideToggleBarElement.classList).not
.toContain('mat-slide-toggle-bar-no-side-margin');
});
});
});

describe('MatSlideToggle with forms', () => {
Expand Down Expand Up @@ -1087,3 +1110,16 @@ class SlideToggleWithModelAndChangeEvent {
checked: boolean;
onChange: () => void = () => {};
}

@Component({
template: `<mat-slide-toggle><some-text></some-text></mat-slide-toggle>`
})
class SlideToggleProjectedLabel {}

@Component({
selector: 'some-text',
template: `<span>{{text}}</span>`
})
class TextBindingComponent {
text: string = 'Some text';
}
10 changes: 6 additions & 4 deletions src/lib/slide-toggle/slide-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -358,9 +358,11 @@ export class MatSlideToggle extends _MatSlideToggleMixinBase implements OnDestro

/** Method being called whenever the label text changes. */
_onLabelTextChange() {
// This method is getting called whenever the label of the slide-toggle changes.
// Since the slide-toggle uses the OnPush strategy we need to notify it about the change
// that has been recognized by the cdkObserveContent directive.
this._changeDetectorRef.markForCheck();
// Since the event of the `cdkObserveContent` directive runs outside of the zone, the
// slide-toggle component will be only marked for check, but no actual change detection runs
// automatically. Instead of going back into the zone in order to trigger a change detection
// which causes *all* components to be checked (if explicitly marked or not using OnPush),
// we only trigger an explicit change detection for the slide-toggle view and it's children.
this._changeDetectorRef.detectChanges();
}
}