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(ivy): handling className as an input properly #33188

Closed
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
4 changes: 4 additions & 0 deletions packages/compiler/src/render3/view/styling_builder.ts
Expand Up @@ -201,6 +201,10 @@ export class StylingBuilder {
const entry:
BoundStylingEntry = {name: property, value, sourceSpan, hasOverrideFlag, unit: null};
if (isMapBased) {
if (this._classMapInput) {
throw new Error(
'[class] and [className] bindings cannot be used on the same element simultaneously');
}
this._classMapInput = entry;
} else {
(this._singleClassInputs = this._singleClassInputs || []).push(entry);
Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/render3/instructions/element.ts
Expand Up @@ -19,7 +19,7 @@ import {assertNodeType} from '../node_assert';
import {appendChild} from '../node_manipulation';
import {decreaseElementDepthCount, getElementDepthCount, getIsParent, getLView, getNamespace, getPreviousOrParentTNode, getSelectedIndex, increaseElementDepthCount, setIsNotParent, setPreviousOrParentTNode} from '../state';
import {setUpAttributes} from '../util/attrs_utils';
import {getInitialStylingValue, hasClassInput, hasStyleInput} from '../util/styling_utils';
import {getInitialStylingValue, hasClassInput, hasStyleInput, selectClassBasedInputName} from '../util/styling_utils';
import {getNativeByTNode, getTNode} from '../util/view_utils';

import {createDirectivesInstances, elementCreate, executeContentQueries, getOrCreateTNode, renderInitialStyling, resolveDirectives, saveResolvedLocalsInData, setInputsForProperty} from './shared';
Expand Down Expand Up @@ -132,7 +132,8 @@ export function ɵɵelementEnd(): void {
}

if (hasClassInput(tNode)) {
setDirectiveStylingInput(tNode.classes, lView, tNode.inputs !['class']);
const inputName: string = selectClassBasedInputName(tNode.inputs !);
setDirectiveStylingInput(tNode.classes, lView, tNode.inputs ![inputName]);
}

if (hasStyleInput(tNode)) {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/render3/instructions/shared.ts
Expand Up @@ -849,7 +849,7 @@ function initializeInputAndOutputAliases(tView: TView, tNode: TNode): void {
}

if (inputsStore !== null) {
if (inputsStore.hasOwnProperty('class')) {
if (inputsStore.hasOwnProperty('class') || inputsStore.hasOwnProperty('className')) {
tNode.flags |= TNodeFlags.hasClassInput;
}
if (inputsStore.hasOwnProperty('style')) {
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/render3/instructions/styling.ts
Expand Up @@ -19,7 +19,7 @@ import {activateStylingMapFeature} from '../styling/map_based_bindings';
import {attachStylingDebugObject} from '../styling/styling_debug';
import {NO_CHANGE} from '../tokens';
import {renderStringify} from '../util/misc_utils';
import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, hasClassInput, hasStyleInput, hasValueChanged, isContextLocked, isHostStylingActive, isStylingContext, normalizeIntoStylingMap, patchConfig, setValue, stylingMapToString} from '../util/styling_utils';
import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, hasClassInput, hasStyleInput, hasValueChanged, isContextLocked, isHostStylingActive, isStylingContext, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils';
import {getNativeByTNode, getTNode} from '../util/view_utils';


Expand Down Expand Up @@ -402,7 +402,7 @@ function updateDirectiveInputValue(
// directive input(s) in the event that it is falsy during the
// first update pass.
if (newValue || isContextLocked(context, false)) {
const inputName = isClassBased ? 'class' : 'style';
const inputName: string = isClassBased ? selectClassBasedInputName(tNode.inputs !) : 'style';
const inputs = tNode.inputs ![inputName] !;
const initialValue = getInitialStylingValue(context);
const value = normalizeStylingDirectiveInputValue(initialValue, newValue, isClassBased);
Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/render3/util/styling_utils.ts
Expand Up @@ -5,7 +5,7 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {TNode, TNodeFlags} from '../interfaces/node';
import {PropertyAliases, TNode, TNodeFlags} from '../interfaces/node';
import {LStylingData, StylingMapArray, StylingMapArrayIndex, TStylingConfig, TStylingContext, TStylingContextIndex, TStylingContextPropConfigFlags} from '../interfaces/styling';
import {NO_CHANGE} from '../tokens';

Expand Down Expand Up @@ -433,3 +433,9 @@ export function normalizeIntoStylingMap(

return stylingMapArr;
}

// TODO (matsko|AndrewKushnir): refactor this once we figure out how to generate separate
// `input('class') + classMap()` instructions.
export function selectClassBasedInputName(inputs: PropertyAliases): string {
return inputs.hasOwnProperty('class') ? 'class' : 'className';
}
68 changes: 68 additions & 0 deletions packages/core/test/acceptance/styling_spec.ts
Expand Up @@ -551,6 +551,74 @@ describe('styling', () => {
expect(capturedMyClassBindingCount).toEqual(1);
});

it('should write to a `className` input binding', () => {
@Component({
selector: 'comp',
template: `{{className}}`,
})
class Comp {
@Input() className: string = '';
}
@Component({
template: `<comp [className]="'my-className'"></comp>`,

Choose a reason for hiding this comment

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

could we have another test for the case of a static class (<comp class="static" [className]="'my-className'"></comp>)? It is not clear for me if this situation will be dissolved as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added extra test to make sure that the use-case that you've mentioned is covered. The only difference is that in Ivy, writing to the className input binding has a side-effect of applying styling (i.e. static class value is combined with className binding value). Thank you.

})
class App {
}

TestBed.configureTestingModule({declarations: [Comp, App]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
expect(fixture.debugElement.nativeElement.firstChild.innerHTML).toBe('my-className');
});

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

@Component({
template: `<comp class="static" [className]="'my-className'"></comp>`,
})
class App {
}

TestBed.configureTestingModule({declarations: [Comp, App]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
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 @@ -584,6 +584,9 @@
{
"name": "saveResolvedLocalsInData"
},
{
"name": "selectClassBasedInputName"
},
{
"name": "selectIndexInternal"
},
Expand Down
3 changes: 3 additions & 0 deletions packages/core/test/bundling/todo/bundle.golden_symbols.json
Expand Up @@ -1181,6 +1181,9 @@
{
"name": "searchTokensOnInjector"
},
{
"name": "selectClassBasedInputName"
},
{
"name": "selectIndexInternal"
},
Expand Down