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

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

@@ -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,
@@ -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,
@@ -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);
         });

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

@@ -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[]):
+export function isStyleUrlResolvable(url: string): boolean {
-    string {
+  if (isBlank(url) || url.length === 0) return false;
-  return 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') {
+  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];
+    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;

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

@@ -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';
 
@@ -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;
     });

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

@@ -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,
@@ -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.STYLE) {
-        preparsedElement.type === PreparsedElementType.STYLESHEET) {
       // 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;
     }

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

@@ -1,51 +1,26 @@
 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 = `
+      var resolvedCss = extractStyleUrls(urlResolver, 'http://ng.io', css).style;
-      .foo {
+      expect(resolvedCss).toEqual(css);
-        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);
     });
 
     it('should extract "@import" urls', () => {
@@ -53,7 +28,7 @@ export function main() {
       @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']);
     });
@@ -64,7 +39,7 @@ 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']);
@@ -72,7 +47,7 @@ export function main() {
 
     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']);
     });
@@ -82,7 +57,7 @@ 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']);
@@ -90,19 +65,38 @@ export function main() {
 
     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.

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

@@ -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(
@@ -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({
@@ -261,8 +271,7 @@ export function main() {
                  styleUrls: []
                }),
                '', 'package:some/module/id');
-           expect(template.styles)
+           expect(template.styles).toEqual(['.foo{background-image: url(\'double.jpg\');']);
-               .toEqual(['.foo{background-image: url(\'package:some/module/double.jpg\');']);
          }));
 
       it('should resolve relative style urls in styleUrls',

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

@@ -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', () => {
+      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)']]);
+        });
 
       });