X Tutup
Skip to content

fix(style_compiler): don’t touch urls in stylesheets and keep stylesh… #4740

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

Closed
tbosch wants to merge 1 commit into from
Closed

fix(style_compiler): don’t touch urls in stylesheets and keep stylesh… #4740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions modules/angular2/src/core/compiler/style_compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {StringWrapper, isBlank} from 'angular2/src/core/facade/lang';
import {PromiseWrapper, Promise} from 'angular2/src/core/facade/async';
import {ShadowCss} from 'angular2/src/core/compiler/shadow_css';
import {UrlResolver} from 'angular2/src/core/compiler/url_resolver';
import {resolveStyleUrls} from './style_url_resolver';
import {extractStyleUrls} from './style_url_resolver';
import {
escapeSingleQuoteString,
IS_DART,
Expand Down Expand Up @@ -58,7 +58,7 @@ export class StyleCompiler {
}

compileStylesheetCodeGen(stylesheetUrl: string, cssText: string): SourceModule[] {
var styleWithImports = resolveStyleUrls(this._urlResolver, stylesheetUrl, cssText);
var styleWithImports = extractStyleUrls(this._urlResolver, stylesheetUrl, cssText);
return [
this._styleModule(
stylesheetUrl, false,
Expand All @@ -78,7 +78,7 @@ export class StyleCompiler {
var result = this._styleCache.get(cacheKey);
if (isBlank(result)) {
result = this._xhr.get(absUrl).then((style) => {
var styleWithImports = resolveStyleUrls(this._urlResolver, absUrl, style);
var styleWithImports = extractStyleUrls(this._urlResolver, absUrl, style);
return this._loadStyles([styleWithImports.style], styleWithImports.styleUrls,
encapsulate);
});
Expand Down
63 changes: 24 additions & 39 deletions modules/angular2/src/core/compiler/style_url_resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,60 +1,45 @@
// Some of the code comes from WebComponents.JS
// https://github.com/webcomponents/webcomponentsjs/blob/master/src/HTMLImports/path.js

import {RegExp, RegExpWrapper, StringWrapper, isPresent} from 'angular2/src/core/facade/lang';
import {
RegExp,
RegExpWrapper,
StringWrapper,
isPresent,
isBlank
} from 'angular2/src/core/facade/lang';
import {UrlResolver} from 'angular2/src/core/compiler/url_resolver';

/**
* Rewrites URLs by resolving '@import' and 'url()' URLs from the given base URL,
* removes and returns the @import urls
*/
export function resolveStyleUrls(resolver: UrlResolver, baseUrl: string, cssText: string):
StyleWithImports {
var foundUrls = [];
cssText = extractUrls(resolver, baseUrl, cssText, foundUrls);
cssText = replaceUrls(resolver, baseUrl, cssText);
return new StyleWithImports(cssText, foundUrls);
}

export class StyleWithImports {
constructor(public style: string, public styleUrls: string[]) {}
}

function extractUrls(resolver: UrlResolver, baseUrl: string, cssText: string, foundUrls: string[]):
string {
return StringWrapper.replaceAllMapped(cssText, _cssImportRe, (m) => {
export function isStyleUrlResolvable(url: string): boolean {
if (isBlank(url) || url.length === 0) return false;
var schemeMatch = RegExpWrapper.firstMatch(_urlWithSchemaRe, url);
return isBlank(schemeMatch) || schemeMatch[1] == 'package';
}

/**
* Rewrites stylesheets by resolving and removing the @import urls that
* are either relative or don't have a `package:` scheme
*/
export function extractStyleUrls(resolver: UrlResolver, baseUrl: string, cssText: string):
StyleWithImports {
var foundUrls = [];
var modifiedCssText = StringWrapper.replaceAllMapped(cssText, _cssImportRe, (m) => {
var url = isPresent(m[1]) ? m[1] : m[2];
var schemeMatch = RegExpWrapper.firstMatch(_urlWithSchemaRe, url);
if (isPresent(schemeMatch) && schemeMatch[1] != 'package') {
if (!isStyleUrlResolvable(url)) {
// Do not attempt to resolve non-package absolute URLs with URI scheme
return m[0];
}
foundUrls.push(resolver.resolve(baseUrl, url));
return '';
});
return new StyleWithImports(modifiedCssText, foundUrls);
}

function replaceUrls(resolver: UrlResolver, baseUrl: string, cssText: string): string {
return StringWrapper.replaceAllMapped(cssText, _cssUrlRe, (m) => {
var pre = m[1];
var originalUrl = m[2];
if (RegExpWrapper.test(_dataUrlRe, originalUrl)) {
// Do not attempt to resolve data: URLs
return m[0];
}
var url = StringWrapper.replaceAll(originalUrl, _quoteRe, '');
var post = m[3];

var resolvedUrl = resolver.resolve(baseUrl, url);

return pre + "'" + resolvedUrl + "'" + post;
});
}

var _cssUrlRe = /(url\()([^)]*)(\))/g;
var _cssImportRe = /@import\s+(?:url\()?\s*(?:(?:['"]([^'"]*))|([^;\)\s]*))[^;]*;?/g;
var _quoteRe = /['"]/g;
var _dataUrlRe = /^['"]?data:/g;
// TODO: can't use /^[^:/?#.]+:/g due to clang-format bug:
// https://github.com/angular/angular/issues/4596
var _urlWithSchemaRe = /^['"]?([a-zA-Z\-\+\.]+):/g;
var _urlWithSchemaRe = /^['"]?([a-zA-Z\-\+\.]+):/g;
7 changes: 4 additions & 3 deletions modules/angular2/src/core/compiler/template_normalizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@ import {
CompileTemplateMetadata
} from './directive_metadata';
import {isPresent, isBlank} from 'angular2/src/core/facade/lang';
import {ListWrapper} from 'angular2/src/core/facade/collection';
import {BaseException} from 'angular2/src/core/facade/exceptions';
import {Promise, PromiseWrapper} from 'angular2/src/core/facade/async';

import {XHR} from 'angular2/src/core/compiler/xhr';
import {UrlResolver} from 'angular2/src/core/compiler/url_resolver';
import {resolveStyleUrls} from './style_url_resolver';
import {extractStyleUrls, isStyleUrlResolvable} from './style_url_resolver';
import {Injectable} from 'angular2/src/core/di';
import {ViewEncapsulation} from 'angular2/src/core/metadata/view';

Expand Down Expand Up @@ -57,9 +58,9 @@ export class TemplateNormalizer {
visitor.styleUrls.map(url => this._urlResolver.resolve(templateAbsUrl, url))
.concat(templateMeta.styleUrls.map(
url => this._urlResolver.resolve(directiveType.moduleUrl, url)));

allStyleAbsUrls = ListWrapper.filter(allStyleAbsUrls, isStyleUrlResolvable);
var allResolvedStyles = allStyles.map(style => {
var styleWithImports = resolveStyleUrls(this._urlResolver, templateAbsUrl, style);
var styleWithImports = extractStyleUrls(this._urlResolver, templateAbsUrl, style);
styleWithImports.styleUrls.forEach(styleUrl => allStyleAbsUrls.push(styleUrl));
return styleWithImports.style;
});
Expand Down
14 changes: 11 additions & 3 deletions modules/angular2/src/core/compiler/template_parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ import {CssSelector, SelectorMatcher} from 'angular2/src/core/compiler/selector'
import {ElementSchemaRegistry} from 'angular2/src/core/compiler/schema/element_schema_registry';
import {preparseElement, PreparsedElement, PreparsedElementType} from './template_preparser';

import {isStyleUrlResolvable} from './style_url_resolver';

import {
HtmlAstVisitor,
HtmlAst,
Expand Down Expand Up @@ -165,10 +167,16 @@ class TemplateParseVisitor implements HtmlAstVisitor {
var nodeName = element.name;
var preparsedElement = preparseElement(element);
if (preparsedElement.type === PreparsedElementType.SCRIPT ||
preparsedElement.type === PreparsedElementType.STYLE ||
preparsedElement.type === PreparsedElementType.STYLESHEET) {
preparsedElement.type === PreparsedElementType.STYLE) {
// Skipping <script> for security reasons
// Skipping <style> and stylesheets as we already processed them
// Skipping <style> as we already processed them
// in the StyleCompiler
return null;
}
if (preparsedElement.type === PreparsedElementType.STYLESHEET &&
isStyleUrlResolvable(preparsedElement.hrefAttr)) {
// Skipping stylesheets with either relative urls or package scheme as we already processed
// them
// in the StyleCompiler
return null;
}
Expand Down
72 changes: 33 additions & 39 deletions modules/angular2/test/core/compiler/style_url_resolver_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,59 +1,34 @@
import {describe, it, expect, beforeEach, ddescribe, iit, xit, el} from 'angular2/testing_internal';
import {resolveStyleUrls} from 'angular2/src/core/compiler/style_url_resolver';
import {
extractStyleUrls,
isStyleUrlResolvable
} from 'angular2/src/core/compiler/style_url_resolver';

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

export function main() {
describe('StyleUrlResolver', () => {
describe('extractStyleUrls', () => {
var urlResolver;

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

it('should resolve "url()" urls', () => {
it('should not resolve "url()" urls', () => {
var css = `
.foo {
background-image: url("double.jpg");
background-image: url('simple.jpg');
background-image: url(noquote.jpg);
}`;
var expectedCss = `
.foo {
background-image: url('http://ng.io/double.jpg');
background-image: url('http://ng.io/simple.jpg');
background-image: url('http://ng.io/noquote.jpg');
}`;

var resolvedCss = resolveStyleUrls(urlResolver, 'http://ng.io', css).style;
expect(resolvedCss).toEqual(expectedCss);
});

it('should not strip quotes from inlined SVG styles', () => {
var css = `
.selector {
background:rgb(55,71,79) url('data:image/svg+xml;utf8,<?xml version="1.0"?>');
background:rgb(55,71,79) url("data:image/svg+xml;utf8,<?xml version='1.0'?>");
background:rgb(55,71,79) url("/some/data:image");
}
`;

var expectedCss = `
.selector {
background:rgb(55,71,79) url('data:image/svg+xml;utf8,<?xml version="1.0"?>');
background:rgb(55,71,79) url("data:image/svg+xml;utf8,<?xml version='1.0'?>");
background:rgb(55,71,79) url('http://ng.io/some/data:image');
}
`;

var resolvedCss = resolveStyleUrls(urlResolver, 'http://ng.io', css).style;
expect(resolvedCss).toEqual(expectedCss);
var resolvedCss = extractStyleUrls(urlResolver, 'http://ng.io', css).style;
expect(resolvedCss).toEqual(css);
});

it('should extract "@import" urls', () => {
var css = `
@import '1.css';
@import "2.css";
`;
var styleWithImports = resolveStyleUrls(urlResolver, 'http://ng.io', css);
var styleWithImports = extractStyleUrls(urlResolver, 'http://ng.io', css);
expect(styleWithImports.style.trim()).toEqual('');
expect(styleWithImports.styleUrls).toEqual(['http://ng.io/1.css', 'http://ng.io/2.css']);
});
Expand All @@ -64,15 +39,15 @@ export function main() {
@import url("4.css");
@import url(5.css);
`;
var styleWithImports = resolveStyleUrls(urlResolver, 'http://ng.io', css);
var styleWithImports = extractStyleUrls(urlResolver, 'http://ng.io', css);
expect(styleWithImports.style.trim()).toEqual('');
expect(styleWithImports.styleUrls)
.toEqual(['http://ng.io/3.css', 'http://ng.io/4.css', 'http://ng.io/5.css']);
});

it('should extract "@import urls and keep rules in the same line', () => {
var css = `@import url('some.css');div {color: red};`;
var styleWithImports = resolveStyleUrls(urlResolver, 'http://ng.io', css);
var styleWithImports = extractStyleUrls(urlResolver, 'http://ng.io', css);
expect(styleWithImports.style.trim()).toEqual('div {color: red};');
expect(styleWithImports.styleUrls).toEqual(['http://ng.io/some.css']);
});
Expand All @@ -82,27 +57,46 @@ export function main() {
@import 'print1.css' print;
@import url(print2.css) print;
`;
var styleWithImports = resolveStyleUrls(urlResolver, 'http://ng.io', css);
var styleWithImports = extractStyleUrls(urlResolver, 'http://ng.io', css);
expect(styleWithImports.style.trim()).toEqual('');
expect(styleWithImports.styleUrls)
.toEqual(['http://ng.io/print1.css', 'http://ng.io/print2.css']);
});

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

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

});

describe('isStyleUrlResolvable', () => {
it('should resolve relative urls',
() => { expect(isStyleUrlResolvable('someUrl.css')).toBe(true); });

it('should resolve package: urls',
() => { expect(isStyleUrlResolvable('package:someUrl.css')).toBe(true); });

it('should resolve asset: urls',
() => { expect(isStyleUrlResolvable('package:someUrl.css')).toBe(true); });

it('should not resolve empty urls', () => {
expect(isStyleUrlResolvable(null)).toBe(false);
expect(isStyleUrlResolvable('')).toBe(false);
});

it('should not resolve urls with other schema',
() => { expect(isStyleUrlResolvable('http://otherurl')).toBe(false); });
});
}

/// The real thing behaves differently between Dart and JS for package URIs.
Expand Down
15 changes: 12 additions & 3 deletions modules/angular2/test/core/compiler/template_normalizer_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,16 @@ export function main() {
expect(template.styleUrls).toEqual([]);
}));

it('should ignore link elements with absolute urls but non package: scheme',
inject([TemplateNormalizer], (normalizer: TemplateNormalizer) => {
var template = normalizer.normalizeLoadedTemplate(
dirType,
new CompileTemplateMetadata({encapsulation: null, styles: [], styleUrls: []}),
'<link href="http://some/external.css" rel="stylesheet"></link>',
'package:some/module/');
expect(template.styleUrls).toEqual([]);
}));

it('should extract @import style urls into styleAbsUrl',
inject([TemplateNormalizer], (normalizer: TemplateNormalizer) => {
var template = normalizer.normalizeLoadedTemplate(
Expand All @@ -252,7 +262,7 @@ export function main() {
expect(template.styleUrls).toEqual(['package:some/module/test.css']);
}));

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

it('should resolve relative style urls in styleUrls',
Expand Down
42 changes: 39 additions & 3 deletions modules/angular2/test/core/compiler/template_parser_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,9 +845,45 @@ Property binding a not used by any directive on an embedded template in TestComp

});

it('should ignore <link rel="stylesheet"> elements but include them for source info', () => {
expect(humanizeTemplateAsts(parse('<link rel="stylesheet"></link>a', [])))
.toEqual([[TextAst, 'a', 'TestComp > #text(a):nth-child(1)']]);
describe('<link rel="stylesheet">', () => {

it('should keep <link rel="stylesheet"> elements if they have an absolute non package: url',
() => {
expect(humanizeTemplateAsts(
parse('<link rel="stylesheet" href="http://someurl"></link>a', [])))
.toEqual([
[ElementAst, 'link', 'TestComp > link:nth-child(0)'],
[
AttrAst,
'href',
'http://someurl',
'TestComp > link:nth-child(0)[href=http://someurl]'
],
[AttrAst, 'rel', 'stylesheet', 'TestComp > link:nth-child(0)[rel=stylesheet]'],
[TextAst, 'a', 'TestComp > #text(a):nth-child(1)']
]);
});

it('should keep <link rel="stylesheet"> elements if they have no uri', () => {
expect(humanizeTemplateAsts(parse('<link rel="stylesheet"></link>a', [])))
.toEqual([
[ElementAst, 'link', 'TestComp > link:nth-child(0)'],
[AttrAst, 'rel', 'stylesheet', 'TestComp > link:nth-child(0)[rel=stylesheet]'],
[TextAst, 'a', 'TestComp > #text(a):nth-child(1)']
]);
});

it('should ignore <link rel="stylesheet"> elements if they have a relative uri', () => {
expect(
humanizeTemplateAsts(parse('<link rel="stylesheet" href="./other.css"></link>a', [])))
.toEqual([[TextAst, 'a', 'TestComp > #text(a):nth-child(1)']]);
});

it('should ignore <link rel="stylesheet"> elements if they have a package: uri', () => {
expect(humanizeTemplateAsts(
parse('<link rel="stylesheet" href="package:somePackage"></link>a', [])))
.toEqual([[TextAst, 'a', 'TestComp > #text(a):nth-child(1)']]);
});

});

Expand Down
X Tutup