Skip to content
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

fix(core): treat [class] and [className] as unrelated bindings #35668

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/compiler/src/render3/view/styling_builder.ts
Expand Up @@ -202,8 +202,7 @@ export class StylingBuilder {
let binding: BoundStylingEntry|null = null;
const prefix = name.substring(0, 6);
const isStyle = name === 'style' || prefix === 'style.' || prefix === 'style!';
const isClass = !isStyle &&
(name === 'class' || name === 'className' || prefix === 'class.' || prefix === 'class!');
const isClass = !isStyle && (name === 'class' || prefix === 'class.' || prefix === 'class!');
kara marked this conversation as resolved.
Show resolved Hide resolved
if (isStyle || isClass) {
const isMapBased = name.charAt(5) !== '.'; // style.prop or class.prop makes this a no
const property = name.substr(isMapBased ? 5 : 6); // the dot explains why there's a +1
Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/render3/instructions/property.ts
Expand Up @@ -53,6 +53,5 @@ export function setDirectiveInputsWhichShadowsStyling(
const inputs = tNode.inputs !;
const property = isClassBased ? 'class' : 'style';
// We support both 'class' and `className` hence the fallback.
const stylingInputs = inputs[property] || (isClassBased && inputs['className']);
setInputsForProperty(tView, lView, stylingInputs, property, value);
setInputsForProperty(tView, lView, inputs[property], property, value);
}
2 changes: 1 addition & 1 deletion packages/core/src/render3/instructions/shared.ts
Expand Up @@ -907,7 +907,7 @@ function initializeInputAndOutputAliases(tView: TView, tNode: TNode): void {
}

if (inputsStore !== null) {
if (inputsStore.hasOwnProperty('class') || inputsStore.hasOwnProperty('className')) {
if (inputsStore.hasOwnProperty('class')) {
tNode.flags |= TNodeFlags.hasClassInput;
}
if (inputsStore.hasOwnProperty('style')) {
Expand Down
83 changes: 47 additions & 36 deletions packages/core/test/acceptance/styling_spec.ts
Expand Up @@ -1129,17 +1129,17 @@ describe('styling', () => {
});

onlyInIvy('only ivy combines static and dynamic class-related attr values')
.it('should write to a `className` input binding, when static `class` is present', () => {
.it('should write combined class attribute and class binding to the class input', () => {
@Component({
selector: 'comp',
template: `{{className}}`,
})
class Comp {
@Input() className: string = '';
@Input('class') className: string = '';
}

@Component({
template: `<comp class="static" [className]="'my-className'"></comp>`,
template: `<comp class="static" [class]="'my-className'"></comp>`,
})
class App {
}
Expand All @@ -1150,32 +1150,6 @@ describe('styling', () => {
expect(fixture.debugElement.nativeElement.firstChild.innerHTML).toBe('static my-className');
});

onlyInIvy('in Ivy [class] and [className] bindings on the same element are not allowed')
.it('should throw an error in case [class] and [className] bindings are used on the same element',
() => {
@Component({
selector: 'comp',
template: `{{class}} - {{className}}`,
})
class Comp {
@Input() class: string = '';
@Input() className: string = '';
}
@Component({
template: `<comp [class]="'my-class'" [className]="'className'"></comp>`,
})
class App {
}

TestBed.configureTestingModule({declarations: [Comp, App]});
expect(() => {
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
})
.toThrowError(
'[class] and [className] bindings cannot be used on the same element simultaneously');
});

onlyInIvy('only ivy persists static class/style attrs with their binding counterparts')
.it('should write to a `class` input binding if there is a static class value and there is a binding value',
() => {
Expand Down Expand Up @@ -3460,13 +3434,12 @@ describe('styling', () => {
class MyApp {
// When the first view in the list gets CD-ed, everything works.
// When the second view gets CD-ed, the styling has already created the data structures
// in the `TView`. As a result when
// `[class.foo]` runs it already knows that `[class]` is a duplicate and hence it
// should check with it. While the resolution is happening it reads the value of the
// `[class]`, however `[class]` has not yet executed and therefore it does not have
// normalized value in its `LView`. The result is that the assertions fails as it
// expects an
// `KeyValueArray`.
// in the `TView`. As a result when `[class.foo]` runs it already knows that `[class]`
// is a duplicate and hence it can overwrite the `[class.foo]` binding. While the
// styling resolution is happening the algorithm reads the value of the `[class]`
// (because it overwrites `[class.foo]`), however `[class]` has not yet executed and
// therefore it does not have normalized value in its `LView`. The result is that the
// assertions fails as it expects an `KeyValueArray`.
}

TestBed.configureTestingModule({declarations: [MyApp, MyCmp, HostStylingsDir]});
Expand All @@ -3476,6 +3449,44 @@ describe('styling', () => {
expectClass(cmp1).toEqual({foo: true, bar: true});
expectClass(cmp2).toEqual({foo: true, bar: true});
});

it('should not bind [class] to @Input("className")', () => {
@Component({
selector: 'my-cmp',
template: `className = {{className}}`,
})
class MyCmp {
@Input()
className: string = 'unbound';
}
@Component({template: `<my-cmp [class]="'bound'"></my-cmp>`})
class MyApp {
}

TestBed.configureTestingModule({declarations: [MyApp, MyCmp]});
const fixture = TestBed.createComponent(MyApp);
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toEqual('className = unbound');
});
mhevery marked this conversation as resolved.
Show resolved Hide resolved

it('should not bind class to @Input("className")', () => {
@Component({
selector: 'my-cmp',
template: `className = {{className}}`,
})
class MyCmp {
@Input()
className: string = 'unbound';
}
@Component({template: `<my-cmp class="bound"></my-cmp>`})
class MyApp {
}

TestBed.configureTestingModule({declarations: [MyApp, MyCmp]});
const fixture = TestBed.createComponent(MyApp);
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toEqual('className = unbound');
});
});
});

Expand Down