X Tutup
Skip to content

Commit 7dde18b

Browse files
tboschtbosch
committed
fix(style_compiler): don’t touch urls in stylesheets and keep stylesheets with absolute urls in templates
We can’t resolve relative urls (e.g. for images) in the compiler as these urls are meant to be loaded in the browser (unless we would inline images as base64…). Also, keep `<link rel=“stylesheet”>` in templates that reference absolute urls with e.g. `http://`. This behavior was already present for `@import` rules within stylesheets. Closes #4740
1 parent 2be9fef commit 7dde18b

File tree

7 files changed

+126
-93
lines changed

7 files changed

+126
-93
lines changed

‎modules/angular2/src/core/compiler/style_compiler.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {StringWrapper, isBlank} from 'angular2/src/core/facade/lang';
66
import {PromiseWrapper, Promise} from 'angular2/src/core/facade/async';
77
import {ShadowCss} from 'angular2/src/core/compiler/shadow_css';
88
import {UrlResolver} from 'angular2/src/core/compiler/url_resolver';
9-
import {resolveStyleUrls} from './style_url_resolver';
9+
import {extractStyleUrls} from './style_url_resolver';
1010
import {
1111
escapeSingleQuoteString,
1212
IS_DART,
@@ -58,7 +58,7 @@ export class StyleCompiler {
5858
}
5959

6060
compileStylesheetCodeGen(stylesheetUrl: string, cssText: string): SourceModule[] {
61-
var styleWithImports = resolveStyleUrls(this._urlResolver, stylesheetUrl, cssText);
61+
var styleWithImports = extractStyleUrls(this._urlResolver, stylesheetUrl, cssText);
6262
return [
6363
this._styleModule(
6464
stylesheetUrl, false,
@@ -78,7 +78,7 @@ export class StyleCompiler {
7878
var result = this._styleCache.get(cacheKey);
7979
if (isBlank(result)) {
8080
result = this._xhr.get(absUrl).then((style) => {
81-
var styleWithImports = resolveStyleUrls(this._urlResolver, absUrl, style);
81+
var styleWithImports = extractStyleUrls(this._urlResolver, absUrl, style);
8282
return this._loadStyles([styleWithImports.style], styleWithImports.styleUrls,
8383
encapsulate);
8484
});

‎modules/angular2/src/core/compiler/style_url_resolver.ts‎

Lines changed: 24 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,60 +1,45 @@
11
// Some of the code comes from WebComponents.JS
22
// https://github.com/webcomponents/webcomponentsjs/blob/master/src/HTMLImports/path.js
33

4-
import {RegExp, RegExpWrapper, StringWrapper, isPresent} from 'angular2/src/core/facade/lang';
4+
import {
5+
RegExp,
6+
RegExpWrapper,
7+
StringWrapper,
8+
isPresent,
9+
isBlank
10+
} from 'angular2/src/core/facade/lang';
511
import {UrlResolver} from 'angular2/src/core/compiler/url_resolver';
612

7-
/**
8-
* Rewrites URLs by resolving '@import' and 'url()' URLs from the given base URL,
9-
* removes and returns the @import urls
10-
*/
11-
export function resolveStyleUrls(resolver: UrlResolver, baseUrl: string, cssText: string):
12-
StyleWithImports {
13-
var foundUrls = [];
14-
cssText = extractUrls(resolver, baseUrl, cssText, foundUrls);
15-
cssText = replaceUrls(resolver, baseUrl, cssText);
16-
return new StyleWithImports(cssText, foundUrls);
17-
}
18-
1913
export class StyleWithImports {
2014
constructor(public style: string, public styleUrls: string[]) {}
2115
}
2216

23-
function extractUrls(resolver: UrlResolver, baseUrl: string, cssText: string, foundUrls: string[]):
24-
string {
25-
return StringWrapper.replaceAllMapped(cssText, _cssImportRe, (m) => {
17+
export function isStyleUrlResolvable(url: string): boolean {
18+
if (isBlank(url) || url.length === 0) return false;
19+
var schemeMatch = RegExpWrapper.firstMatch(_urlWithSchemaRe, url);
20+
return isBlank(schemeMatch) || schemeMatch[1] == 'package';
21+
}
22+
23+
/**
24+
* Rewrites stylesheets by resolving and removing the @import urls that
25+
* are either relative or don't have a `package:` scheme
26+
*/
27+
export function extractStyleUrls(resolver: UrlResolver, baseUrl: string, cssText: string):
28+
StyleWithImports {
29+
var foundUrls = [];
30+
var modifiedCssText = StringWrapper.replaceAllMapped(cssText, _cssImportRe, (m) => {
2631
var url = isPresent(m[1]) ? m[1] : m[2];
27-
var schemeMatch = RegExpWrapper.firstMatch(_urlWithSchemaRe, url);
28-
if (isPresent(schemeMatch) && schemeMatch[1] != 'package') {
32+
if (!isStyleUrlResolvable(url)) {
2933
// Do not attempt to resolve non-package absolute URLs with URI scheme
3034
return m[0];
3135
}
3236
foundUrls.push(resolver.resolve(baseUrl, url));
3337
return '';
3438
});
39+
return new StyleWithImports(modifiedCssText, foundUrls);
3540
}
3641

37-
function replaceUrls(resolver: UrlResolver, baseUrl: string, cssText: string): string {
38-
return StringWrapper.replaceAllMapped(cssText, _cssUrlRe, (m) => {
39-
var pre = m[1];
40-
var originalUrl = m[2];
41-
if (RegExpWrapper.test(_dataUrlRe, originalUrl)) {
42-
// Do not attempt to resolve data: URLs
43-
return m[0];
44-
}
45-
var url = StringWrapper.replaceAll(originalUrl, _quoteRe, '');
46-
var post = m[3];
47-
48-
var resolvedUrl = resolver.resolve(baseUrl, url);
49-
50-
return pre + "'" + resolvedUrl + "'" + post;
51-
});
52-
}
53-
54-
var _cssUrlRe = /(url\()([^)]*)(\))/g;
5542
var _cssImportRe = /@import\s+(?:url\()?\s*(?:(?:['"]([^'"]*))|([^;\)\s]*))[^;]*;?/g;
56-
var _quoteRe = /['"]/g;
57-
var _dataUrlRe = /^['"]?data:/g;
5843
// TODO: can't use /^[^:/?#.]+:/g due to clang-format bug:
5944
// https://github.com/angular/angular/issues/4596
60-
var _urlWithSchemaRe = /^['"]?([a-zA-Z\-\+\.]+):/g;
45+
var _urlWithSchemaRe = /^['"]?([a-zA-Z\-\+\.]+):/g;

‎modules/angular2/src/core/compiler/template_normalizer.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,13 @@ import {
44
CompileTemplateMetadata
55
} from './directive_metadata';
66
import {isPresent, isBlank} from 'angular2/src/core/facade/lang';
7+
import {ListWrapper} from 'angular2/src/core/facade/collection';
78
import {BaseException} from 'angular2/src/core/facade/exceptions';
89
import {Promise, PromiseWrapper} from 'angular2/src/core/facade/async';
910

1011
import {XHR} from 'angular2/src/core/compiler/xhr';
1112
import {UrlResolver} from 'angular2/src/core/compiler/url_resolver';
12-
import {resolveStyleUrls} from './style_url_resolver';
13+
import {extractStyleUrls, isStyleUrlResolvable} from './style_url_resolver';
1314
import {Injectable} from 'angular2/src/core/di';
1415
import {ViewEncapsulation} from 'angular2/src/core/metadata/view';
1516

@@ -57,9 +58,9 @@ export class TemplateNormalizer {
5758
visitor.styleUrls.map(url => this._urlResolver.resolve(templateAbsUrl, url))
5859
.concat(templateMeta.styleUrls.map(
5960
url => this._urlResolver.resolve(directiveType.moduleUrl, url)));
60-
61+
allStyleAbsUrls = ListWrapper.filter(allStyleAbsUrls, isStyleUrlResolvable);
6162
var allResolvedStyles = allStyles.map(style => {
62-
var styleWithImports = resolveStyleUrls(this._urlResolver, templateAbsUrl, style);
63+
var styleWithImports = extractStyleUrls(this._urlResolver, templateAbsUrl, style);
6364
styleWithImports.styleUrls.forEach(styleUrl => allStyleAbsUrls.push(styleUrl));
6465
return styleWithImports.style;
6566
});

‎modules/angular2/src/core/compiler/template_parser.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ import {CssSelector, SelectorMatcher} from 'angular2/src/core/compiler/selector'
4040
import {ElementSchemaRegistry} from 'angular2/src/core/compiler/schema/element_schema_registry';
4141
import {preparseElement, PreparsedElement, PreparsedElementType} from './template_preparser';
4242

43+
import {isStyleUrlResolvable} from './style_url_resolver';
44+
4345
import {
4446
HtmlAstVisitor,
4547
HtmlAst,
@@ -165,10 +167,16 @@ class TemplateParseVisitor implements HtmlAstVisitor {
165167
var nodeName = element.name;
166168
var preparsedElement = preparseElement(element);
167169
if (preparsedElement.type === PreparsedElementType.SCRIPT ||
168-
preparsedElement.type === PreparsedElementType.STYLE ||
169-
preparsedElement.type === PreparsedElementType.STYLESHEET) {
170+
preparsedElement.type === PreparsedElementType.STYLE) {
170171
// Skipping <script> for security reasons
171-
// Skipping <style> and stylesheets as we already processed them
172+
// Skipping <style> as we already processed them
173+
// in the StyleCompiler
174+
return null;
175+
}
176+
if (preparsedElement.type === PreparsedElementType.STYLESHEET &&
177+
isStyleUrlResolvable(preparsedElement.hrefAttr)) {
178+
// Skipping stylesheets with either relative urls or package scheme as we already processed
179+
// them
172180
// in the StyleCompiler
173181
return null;
174182
}

‎modules/angular2/test/core/compiler/style_url_resolver_spec.ts‎

Lines changed: 33 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,59 +1,34 @@
11
import {describe, it, expect, beforeEach, ddescribe, iit, xit, el} from 'angular2/testing_internal';
2-
import {resolveStyleUrls} from 'angular2/src/core/compiler/style_url_resolver';
2+
import {
3+
extractStyleUrls,
4+
isStyleUrlResolvable
5+
} from 'angular2/src/core/compiler/style_url_resolver';
36

47
import {UrlResolver} from 'angular2/src/core/compiler/url_resolver';
58

69
export function main() {
7-
describe('StyleUrlResolver', () => {
10+
describe('extractStyleUrls', () => {
811
var urlResolver;
912

1013
beforeEach(() => { urlResolver = new UrlResolver(); });
1114

12-
it('should resolve "url()" urls', () => {
15+
it('should not resolve "url()" urls', () => {
1316
var css = `
1417
.foo {
1518
background-image: url("double.jpg");
1619
background-image: url('simple.jpg');
1720
background-image: url(noquote.jpg);
1821
}`;
19-
var expectedCss = `
20-
.foo {
21-
background-image: url('http://ng.io/double.jpg');
22-
background-image: url('http://ng.io/simple.jpg');
23-
background-image: url('http://ng.io/noquote.jpg');
24-
}`;
25-
26-
var resolvedCss = resolveStyleUrls(urlResolver, 'http://ng.io', css).style;
27-
expect(resolvedCss).toEqual(expectedCss);
28-
});
29-
30-
it('should not strip quotes from inlined SVG styles', () => {
31-
var css = `
32-
.selector {
33-
background:rgb(55,71,79) url('data:image/svg+xml;utf8,<?xml version="1.0"?>');
34-
background:rgb(55,71,79) url("data:image/svg+xml;utf8,<?xml version='1.0'?>");
35-
background:rgb(55,71,79) url("/some/data:image");
36-
}
37-
`;
38-
39-
var expectedCss = `
40-
.selector {
41-
background:rgb(55,71,79) url('data:image/svg+xml;utf8,<?xml version="1.0"?>');
42-
background:rgb(55,71,79) url("data:image/svg+xml;utf8,<?xml version='1.0'?>");
43-
background:rgb(55,71,79) url('http://ng.io/some/data:image');
44-
}
45-
`;
46-
47-
var resolvedCss = resolveStyleUrls(urlResolver, 'http://ng.io', css).style;
48-
expect(resolvedCss).toEqual(expectedCss);
22+
var resolvedCss = extractStyleUrls(urlResolver, 'http://ng.io', css).style;
23+
expect(resolvedCss).toEqual(css);
4924
});
5025

5126
it('should extract "@import" urls', () => {
5227
var css = `
5328
@import '1.css';
5429
@import "2.css";
5530
`;
56-
var styleWithImports = resolveStyleUrls(urlResolver, 'http://ng.io', css);
31+
var styleWithImports = extractStyleUrls(urlResolver, 'http://ng.io', css);
5732
expect(styleWithImports.style.trim()).toEqual('');
5833
expect(styleWithImports.styleUrls).toEqual(['http://ng.io/1.css', 'http://ng.io/2.css']);
5934
});
@@ -64,15 +39,15 @@ export function main() {
6439
@import url("4.css");
6540
@import url(5.css);
6641
`;
67-
var styleWithImports = resolveStyleUrls(urlResolver, 'http://ng.io', css);
42+
var styleWithImports = extractStyleUrls(urlResolver, 'http://ng.io', css);
6843
expect(styleWithImports.style.trim()).toEqual('');
6944
expect(styleWithImports.styleUrls)
7045
.toEqual(['http://ng.io/3.css', 'http://ng.io/4.css', 'http://ng.io/5.css']);
7146
});
7247

7348
it('should extract "@import urls and keep rules in the same line', () => {
7449
var css = `@import url('some.css');div {color: red};`;
75-
var styleWithImports = resolveStyleUrls(urlResolver, 'http://ng.io', css);
50+
var styleWithImports = extractStyleUrls(urlResolver, 'http://ng.io', css);
7651
expect(styleWithImports.style.trim()).toEqual('div {color: red};');
7752
expect(styleWithImports.styleUrls).toEqual(['http://ng.io/some.css']);
7853
});
@@ -82,27 +57,46 @@ export function main() {
8257
@import 'print1.css' print;
8358
@import url(print2.css) print;
8459
`;
85-
var styleWithImports = resolveStyleUrls(urlResolver, 'http://ng.io', css);
60+
var styleWithImports = extractStyleUrls(urlResolver, 'http://ng.io', css);
8661
expect(styleWithImports.style.trim()).toEqual('');
8762
expect(styleWithImports.styleUrls)
8863
.toEqual(['http://ng.io/print1.css', 'http://ng.io/print2.css']);
8964
});
9065

9166
it('should leave absolute non-package @import urls intact', () => {
9267
var css = `@import url('http://server.com/some.css');`;
93-
var styleWithImports = resolveStyleUrls(urlResolver, 'http://ng.io', css);
68+
var styleWithImports = extractStyleUrls(urlResolver, 'http://ng.io', css);
9469
expect(styleWithImports.style.trim()).toEqual(`@import url('http://server.com/some.css');`);
9570
expect(styleWithImports.styleUrls).toEqual([]);
9671
});
9772

9873
it('should resolve package @import urls', () => {
9974
var css = `@import url('package:a/b/some.css');`;
100-
var styleWithImports = resolveStyleUrls(new FakeUrlResolver(), 'http://ng.io', css);
75+
var styleWithImports = extractStyleUrls(new FakeUrlResolver(), 'http://ng.io', css);
10176
expect(styleWithImports.style.trim()).toEqual(``);
10277
expect(styleWithImports.styleUrls).toEqual(['fake_resolved_url']);
10378
});
10479

10580
});
81+
82+
describe('isStyleUrlResolvable', () => {
83+
it('should resolve relative urls',
84+
() => { expect(isStyleUrlResolvable('someUrl.css')).toBe(true); });
85+
86+
it('should resolve package: urls',
87+
() => { expect(isStyleUrlResolvable('package:someUrl.css')).toBe(true); });
88+
89+
it('should resolve asset: urls',
90+
() => { expect(isStyleUrlResolvable('package:someUrl.css')).toBe(true); });
91+
92+
it('should not resolve empty urls', () => {
93+
expect(isStyleUrlResolvable(null)).toBe(false);
94+
expect(isStyleUrlResolvable('')).toBe(false);
95+
});
96+
97+
it('should not resolve urls with other schema',
98+
() => { expect(isStyleUrlResolvable('http://otherurl')).toBe(false); });
99+
});
106100
}
107101

108102
/// The real thing behaves differently between Dart and JS for package URIs.

‎modules/angular2/test/core/compiler/template_normalizer_spec.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,16 @@ export function main() {
242242
expect(template.styleUrls).toEqual([]);
243243
}));
244244

245+
it('should ignore link elements with absolute urls but non package: scheme',
246+
inject([TemplateNormalizer], (normalizer: TemplateNormalizer) => {
247+
var template = normalizer.normalizeLoadedTemplate(
248+
dirType,
249+
new CompileTemplateMetadata({encapsulation: null, styles: [], styleUrls: []}),
250+
'<link href="http://some/external.css" rel="stylesheet"></link>',
251+
'package:some/module/');
252+
expect(template.styleUrls).toEqual([]);
253+
}));
254+
245255
it('should extract @import style urls into styleAbsUrl',
246256
inject([TemplateNormalizer], (normalizer: TemplateNormalizer) => {
247257
var template = normalizer.normalizeLoadedTemplate(
@@ -252,7 +262,7 @@ export function main() {
252262
expect(template.styleUrls).toEqual(['package:some/module/test.css']);
253263
}));
254264

255-
it('should resolve relative urls in inline styles',
265+
it('should not resolve relative urls in inline styles',
256266
inject([TemplateNormalizer], (normalizer: TemplateNormalizer) => {
257267
var template = normalizer.normalizeLoadedTemplate(
258268
dirType, new CompileTemplateMetadata({
@@ -261,8 +271,7 @@ export function main() {
261271
styleUrls: []
262272
}),
263273
'', 'package:some/module/id');
264-
expect(template.styles)
265-
.toEqual(['.foo{background-image: url(\'package:some/module/double.jpg\');']);
274+
expect(template.styles).toEqual(['.foo{background-image: url(\'double.jpg\');']);
266275
}));
267276

268277
it('should resolve relative style urls in styleUrls',

‎modules/angular2/test/core/compiler/template_parser_spec.ts‎

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -845,9 +845,45 @@ Property binding a not used by any directive on an embedded template in TestComp
845845

846846
});
847847

848-
it('should ignore <link rel="stylesheet"> elements but include them for source info', () => {
849-
expect(humanizeTemplateAsts(parse('<link rel="stylesheet"></link>a', [])))
850-
.toEqual([[TextAst, 'a', 'TestComp > #text(a):nth-child(1)']]);
848+
describe('<link rel="stylesheet">', () => {
849+
850+
it('should keep <link rel="stylesheet"> elements if they have an absolute non package: url',
851+
() => {
852+
expect(humanizeTemplateAsts(
853+
parse('<link rel="stylesheet" href="http://someurl"></link>a', [])))
854+
.toEqual([
855+
[ElementAst, 'link', 'TestComp > link:nth-child(0)'],
856+
[
857+
AttrAst,
858+
'href',
859+
'http://someurl',
860+
'TestComp > link:nth-child(0)[href=http://someurl]'
861+
],
862+
[AttrAst, 'rel', 'stylesheet', 'TestComp > link:nth-child(0)[rel=stylesheet]'],
863+
[TextAst, 'a', 'TestComp > #text(a):nth-child(1)']
864+
]);
865+
});
866+
867+
it('should keep <link rel="stylesheet"> elements if they have no uri', () => {
868+
expect(humanizeTemplateAsts(parse('<link rel="stylesheet"></link>a', [])))
869+
.toEqual([
870+
[ElementAst, 'link', 'TestComp > link:nth-child(0)'],
871+
[AttrAst, 'rel', 'stylesheet', 'TestComp > link:nth-child(0)[rel=stylesheet]'],
872+
[TextAst, 'a', 'TestComp > #text(a):nth-child(1)']
873+
]);
874+
});
875+
876+
it('should ignore <link rel="stylesheet"> elements if they have a relative uri', () => {
877+
expect(
878+
humanizeTemplateAsts(parse('<link rel="stylesheet" href="./other.css"></link>a', [])))
879+
.toEqual([[TextAst, 'a', 'TestComp > #text(a):nth-child(1)']]);
880+
});
881+
882+
it('should ignore <link rel="stylesheet"> elements if they have a package: uri', () => {
883+
expect(humanizeTemplateAsts(
884+
parse('<link rel="stylesheet" href="package:somePackage"></link>a', [])))
885+
.toEqual([[TextAst, 'a', 'TestComp > #text(a):nth-child(1)']]);
886+
});
851887

852888
});
853889

0 commit comments

Comments
 (0)
X Tutup