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.

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

Jump to bottom

refactor(core): remove style sanitization code for [style]/[style.prop] bindings #36965

Closed
wants to merge 3 commits into from
Closed

refactor(core): remove style sanitization code for [style]/[style.prop] bindings #36965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
07f35cb
refactor(core): remove style sanitization code for `[style]`/`[style.…
matsko May 6, 2020
b9b2fb1
fixup! refactor(core): remove style sanitization code for `[style]`/`…
matsko May 7, 2020
73a7f92
fixup! refactor(core): remove style sanitization code for `[style]`/`…
matsko May 12, 2020
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: 0 additions & 4 deletions goldens/public-api/core/core.d.ts
View file
Open in desktop
Expand Up @@ -723,8 +723,6 @@ export declare function ɵɵcontentQuery<T>(directiveIndex: number, predicate: T

export declare function ɵɵCopyDefinitionFeature(definition: ɵDirectiveDef<any> | ɵComponentDef<any>): void;

export declare const ɵɵdefaultStyleSanitizer: StyleSanitizeFn;

export declare function ɵɵdefineComponent<T>(componentDefinition: {
type: Type<T>;
selectors?: ɵCssSelectorList;
Expand Down Expand Up @@ -1050,8 +1048,6 @@ export declare function ɵɵstylePropInterpolate8(prop: string, prefix: string,

export declare function ɵɵstylePropInterpolateV(prop: string, values: any[], valueSuffix?: string | null): typeof ɵɵstylePropInterpolateV;

export declare function ɵɵstyleSanitizer(sanitizer: StyleSanitizeFn | null): void;

export declare function ɵɵtemplate(index: number, templateFn: ComponentTemplate<any> | null, decls: number, vars: number, tagName?: string | null, attrsIndex?: number | null, localRefsIndex?: number | null, localRefExtractor?: LocalRefExtractor): void;

export declare function ɵɵtemplateRefExtractor(tNode: TNode, currentView: ɵangular_packages_core_core_bo): TemplateRef<unknown> | null;
Expand Down
4 changes: 2 additions & 2 deletions goldens/size-tracking/aio-payloads.json
View file
Open in desktop
Expand Up @@ -12,8 +12,8 @@
"master": {
"uncompressed": {
"runtime-es2015": 2987,
"main-es2015": 453518,
"polyfills-es2015": 52195
"main-es2015": 452717,
"polyfills-es2015": 52628
}
}
},
Expand Down
126 changes: 11 additions & 115 deletions packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts
View file
Open in desktop
Expand Up @@ -555,7 +555,7 @@ describe('compiler compliance: styling', () => {
$r3$.ɵɵelement(0, "div");
}
if (rf & 2) {
$r3$.ɵɵstyleProp("background-image", ctx.myImage, $r3$.ɵɵdefaultStyleSanitizer);
$r3$.ɵɵstyleProp("background-image", ctx.myImage);
}
},
encapsulation: 2
Expand Down Expand Up @@ -1151,15 +1151,15 @@ describe('compiler compliance: styling', () => {
app: {
'spec.ts': `
import {Component, NgModule, HostBinding} from '@angular/core';

@Component({
selector: 'my-component',
template: \`
<div class="A{{p1}}B"></div>
<div class="A{{p1}}B{{p2}}C"></div>
<div class="A{{p1}}B{{p2}}C{{p3}}D"></div>
<div class="A{{p1}}B{{p2}}C{{p3}}D{{p4}}E"></div>
<div class="A{{p1}}B{{p2}}C{{p3}}D{{p4}}E{{p5}}F"></div>
<div class="A{{p1}}B{{p2}}C"></div>
<div class="A{{p1}}B{{p2}}C{{p3}}D"></div>
<div class="A{{p1}}B{{p2}}C{{p3}}D{{p4}}E"></div>
<div class="A{{p1}}B{{p2}}C{{p3}}D{{p4}}E{{p5}}F"></div>
<div class="A{{p1}}B{{p2}}C{{p3}}D{{p4}}E{{p5}}F{{p6}}G"></div>
<div class="A{{p1}}B{{p2}}C{{p3}}D{{p4}}E{{p5}}F{{p6}}G{{p7}}H"></div>
<div class="A{{p1}}B{{p2}}C{{p3}}D{{p4}}E{{p5}}F{{p6}}G{{p7}}H{{p8}}I"></div>
Expand All @@ -1178,7 +1178,7 @@ describe('compiler compliance: styling', () => {
p8 = 100;
p9 = 100;
}

@NgModule({declarations: [MyComponent]})
export class MyModule {}
`
Expand Down Expand Up @@ -1221,7 +1221,7 @@ describe('compiler compliance: styling', () => {
app: {
'spec.ts': `
import {Component, NgModule, HostBinding} from '@angular/core';

@Component({
selector: 'my-component',
template: \`
Expand All @@ -1248,7 +1248,7 @@ describe('compiler compliance: styling', () => {
p8 = 100;
p9 = 100;
}

@NgModule({declarations: [MyComponent]})
export class MyModule {}
`
Expand Down Expand Up @@ -1554,8 +1554,8 @@ describe('compiler compliance: styling', () => {
const template = `
…
if (rf & 2) {
$r3$.ɵɵstylePropInterpolate1("background", "url(", ctx.myUrl1, ")", $r3$.ɵɵdefaultStyleSanitizer);
$r3$.ɵɵstylePropInterpolate2("border-image", "url(", ctx.myUrl2, ") ", ctx.myRepeat, " auto", $r3$.ɵɵdefaultStyleSanitizer);
$r3$.ɵɵstylePropInterpolate1("background", "url(", ctx.myUrl1, ")");
$r3$.ɵɵstylePropInterpolate2("border-image", "url(", ctx.myUrl2, ") ", ctx.myRepeat, " auto");
$r3$.ɵɵstylePropInterpolate3("box-shadow", "", ctx.myBoxX, " ", ctx.myBoxY, " ", ctx.myBoxWidth, " black");
}
…
Expand Down Expand Up @@ -2036,110 +2036,6 @@ describe('compiler compliance: styling', () => {
});

describe('new styling refactor', () => {
it('should generate a `styleSanitizer` instruction when one or more sanitizable style properties are statically detected',
() => {
const files = {
app: {
'spec.ts': `
import {Component, NgModule} from '@angular/core';

@Component({
selector: 'my-app',
template: \`
<div [style.background-image]="bgExp"></div>
\`
})
export class MyAppComp {
bgExp = '';
}
`
}
};

const template = `
template: function MyAppComp_Template(rf, ctx) {
…
if (rf & 2) {
$r3$.ɵɵstyleProp("background-image", ctx.bgExp, $r3$.ɵɵdefaultStyleSanitizer);
}
…
}
`;

const result = compile(files, angularFiles);
expectEmit(result.source, template, 'Incorrect template');
});

it('should generate a `styleSanitizer` instruction when a `styleMap` instruction is used',
() => {
const files = {
app: {
'spec.ts': `
import {Component, NgModule} from '@angular/core';

@Component({
selector: 'my-app',
template: \`
<div [style]="mapExp"></div>
\`
})
export class MyAppComp {
mapExp = {};
}
`
}
};

const template = `
template: function MyAppComp_Template(rf, ctx) {
…
if (rf & 2) {
$r3$.ɵɵstyleMap(ctx.mapExp);
}
…
}
`;

const result = compile(files, angularFiles);
expectEmit(result.source, template, 'Incorrect template');
});

it('shouldn\'t generate a `styleSanitizer` instruction when class-based instructions are used',
() => {
const files = {
app: {
'spec.ts': `
import {Component, NgModule} from '@angular/core';

@Component({
selector: 'my-app',
template: \`
<div [class]="mapExp" [class.name]="nameExp"></div>
\`
})
export class MyAppComp {
mapExp = {};
nameExp = true;
}
`
}
};

const template = `
template: function MyAppComp_Template(rf, ctx) {
…
if (rf & 2) {
$r3$.ɵɵclassMap(ctx.mapExp);
$r3$.ɵɵclassProp("name", ctx.nameExp);
}
…
}
`;

const result = compile(files, angularFiles);
expectEmit(result.source, template, 'Incorrect template');
});

it('should generate the correct amount of host bindings when styling is present', () => {
const files = {
app: {
Expand Down
2 changes: 0 additions & 2 deletions packages/compiler/src/render3/r3_identifiers.ts
View file
Open in desktop
Expand Up @@ -315,8 +315,6 @@ export class Identifiers {
// sanitization-related functions
static sanitizeHtml: o.ExternalReference = {name: 'ɵɵsanitizeHtml', moduleName: CORE};
static sanitizeStyle: o.ExternalReference = {name: 'ɵɵsanitizeStyle', moduleName: CORE};
static defaultStyleSanitizer:
o.ExternalReference = {name: 'ɵɵdefaultStyleSanitizer', moduleName: CORE};
static sanitizeResourceUrl:
o.ExternalReference = {name: 'ɵɵsanitizeResourceUrl', moduleName: CORE};
static sanitizeScript: o.ExternalReference = {name: 'ɵɵsanitizeScript', moduleName: CORE};
Expand Down
66 changes: 17 additions & 49 deletions packages/compiler/src/render3/view/styling_builder.ts
View file
Open in desktop
Expand Up @@ -5,7 +5,6 @@
* 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 {ConstantPool} from '../../constant_pool';
import {AttributeMarker} from '../../core';
import {AST, ASTWithSource, BindingPipe, BindingType, Interpolation} from '../../expression_parser/ast';
import * as o from '../../output/output_ast';
Expand Down Expand Up @@ -92,9 +91,8 @@ export interface StylingInstructionCall {
interface BoundStylingEntry {
hasOverrideFlag: boolean;
name: string|null;
unit: string|null;
suffix: string|null;
sourceSpan: ParseSourceSpan;
sanitize: boolean;
value: AST;
}

Expand Down Expand Up @@ -217,20 +215,15 @@ export class StylingBuilder {

registerStyleInput(
name: string, isMapBased: boolean, value: AST, sourceSpan: ParseSourceSpan,
unit?: string|null): BoundStylingEntry|null {
suffix?: string|null): BoundStylingEntry|null {
if (isEmptyExpression(value)) {
return null;
}
name = normalizePropName(name);
const {property, hasOverrideFlag, unit: bindingUnit} = parseProperty(name);
const entry: BoundStylingEntry = {
name: property,
sanitize: property ? isStyleSanitizable(property) : true,
unit: unit || bindingUnit,
value,
sourceSpan,
hasOverrideFlag
};
const {property, hasOverrideFlag, suffix: bindingSuffix} = parseProperty(name);
suffix = typeof suffix === 'string' && suffix.length !== 0 ? suffix : bindingSuffix;
const entry:
BoundStylingEntry = {name: property, suffix: suffix, value, sourceSpan, hasOverrideFlag};
if (isMapBased) {
this._styleMapInput = entry;
} else {
Expand All @@ -250,8 +243,8 @@ export class StylingBuilder {
return null;
}
const {property, hasOverrideFlag} = parseProperty(name);
const entry: BoundStylingEntry =
{name: property, value, sourceSpan, sanitize: false, hasOverrideFlag, unit: null};
const entry:
BoundStylingEntry = {name: property, value, sourceSpan, hasOverrideFlag, suffix: null};
if (isMapBased) {
if (this._classMapInput) {
throw new Error(
Expand Down Expand Up @@ -430,7 +423,7 @@ export class StylingBuilder {
allocateBindingSlots: totalBindingSlotsRequired,
supportsInterpolation: !!getInterpolationExpressionFn,
params: (convertFn: (value: any) => o.Expression | o.Expression[]) => {
// params => stylingProp(propName, value, suffix|sanitizer)
// params => stylingProp(propName, value, suffix)
const params: o.Expression[] = [];
params.push(o.literal(input.name));

Expand All @@ -441,16 +434,10 @@ export class StylingBuilder {
params.push(convertResult);
}

// [style.prop] bindings may use suffix values (e.g. px, em, etc...) and they
// can also use a sanitizer. Sanitization occurs for url-based entries. Having
// the suffix value and a sanitizer together into the instruction doesn't make
// any sense (url-based entries cannot be sanitized).
if (!isClassBased) {
if (input.unit) {
params.push(o.literal(input.unit));
} else if (input.sanitize) {
params.push(o.importExpr(R3.defaultStyleSanitizer));
}
// [style.prop] bindings may use suffix values (e.g. px, em, etc...), therefore,
// if that is detected then we need to pass that in as an optional param.
if (!isClassBased && input.suffix !== null) {
params.push(o.literal(input.suffix));
}

return params;
Expand Down Expand Up @@ -517,43 +504,24 @@ function registerIntoMap(map: Map<string, number>, key: string) {
}
}

function isStyleSanitizable(prop: string): boolean {
// Note that browsers support both the dash case and
// camel case property names when setting through JS.
return prop === 'background-image' || prop === 'backgroundImage' || prop === 'background' ||
prop === 'border-image' || prop === 'borderImage' || prop === 'border-image-source' ||
prop === 'borderImageSource' || prop === 'filter' || prop === 'list-style' ||
prop === 'listStyle' || prop === 'list-style-image' || prop === 'listStyleImage' ||
prop === 'clip-path' || prop === 'clipPath';
}

/**
* Simple helper function to either provide the constant literal that will house the value
* here or a null value if the provided values are empty.
*/
function getConstantLiteralFromArray(
constantPool: ConstantPool, values: o.Expression[]): o.Expression {
return values.length ? constantPool.getConstLiteral(o.literalArr(values), true) : o.NULL_EXPR;
}

export function parseProperty(name: string):
{property: string, unit: string, hasOverrideFlag: boolean} {
{property: string, suffix: string|null, hasOverrideFlag: boolean} {
let hasOverrideFlag = false;
const overrideIndex = name.indexOf(IMPORTANT_FLAG);
if (overrideIndex !== -1) {
name = overrideIndex > 0 ? name.substring(0, overrideIndex) : '';
hasOverrideFlag = true;
}

let unit = '';
let suffix: string|null = null;
let property = name;
const unitIndex = name.lastIndexOf('.');
if (unitIndex > 0) {
unit = name.substr(unitIndex + 1);
suffix = name.substr(unitIndex + 1);
property = name.substring(0, unitIndex);
}

return {property, unit, hasOverrideFlag};
return {property, suffix, hasOverrideFlag};
}

/**
Expand Down
2 changes: 0 additions & 2 deletions packages/core/src/core_render3_private_export.ts
View file
Open in desktop
Expand Up @@ -227,7 +227,6 @@ export {
ɵɵstylePropInterpolate7,
ɵɵstylePropInterpolate8,
ɵɵstylePropInterpolateV,
ɵɵstyleSanitizer,
ɵɵtemplate,
ɵɵtemplateRefExtractor,
ɵɵtext,
Expand Down Expand Up @@ -286,7 +285,6 @@ export {
bypassSanitizationTrustUrl as ɵbypassSanitizationTrustUrl,
} from './sanitization/bypass';
export {
ɵɵdefaultStyleSanitizer,
ɵɵsanitizeHtml,
ɵɵsanitizeResourceUrl,
ɵɵsanitizeScript,
Expand Down
1 change: 0 additions & 1 deletion packages/core/src/render3/index.ts
View file
Open in desktop
Expand Up @@ -113,7 +113,6 @@ export {
ɵɵstylePropInterpolate8,
ɵɵstylePropInterpolateV,

ɵɵstyleSanitizer,
ɵɵtemplate,

ɵɵtext,
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/render3/instructions/attribute.ts
View file
Open in desktop
Expand Up @@ -7,7 +7,7 @@
*/
import {bindingUpdated} from '../bindings';
import {SanitizerFn} from '../interfaces/sanitization';
import {getLView, getSelectedIndex, getSelectedTNode, getTView, nextBindingIndex} from '../state';
import {getLView, getSelectedTNode, getTView, nextBindingIndex} from '../state';
import {elementAttributeInternal, storePropertyBindingMetadata} from './shared';


Expand Down