Merge pull request #12705 from adobe/marcel/ld-css-relative-url

zaggino · web-flow · commit 452911af5805 · 2016-08-26T08:20:28.000+10:00
MultiBrowser Live Preview: Fix cases where url()s in CSS weren't resolved correctly
diff --git a/src/LiveDevelopment/MultiBrowserImpl/documents/LiveCSSDocument.js b/src/LiveDevelopment/MultiBrowserImpl/documents/LiveCSSDocument.js
@@ -21,6 +21,8 @@
  *
  */
 
+/*jslint regexp: true */
+
 /**
  * LiveCSSDocument manages a single CSS source document
  *
@@ -45,7 +47,8 @@ define(function LiveCSSDocumentModule(require, exports, module) {
     var _               = require("thirdparty/lodash"),
         CSSUtils        = require("language/CSSUtils"),
         EventDispatcher = require("utils/EventDispatcher"),
-        LiveDocument    = require("LiveDevelopment/MultiBrowserImpl/documents/LiveDocument");
+        LiveDocument    = require("LiveDevelopment/MultiBrowserImpl/documents/LiveDocument"),
+        PathUtils       = require("thirdparty/path-utils/path-utils");
 
     /**
      * @constructor
@@ -93,14 +96,31 @@ define(function LiveCSSDocumentModule(require, exports, module) {
      * When the user edits the file, update the stylesheet in the browser and redraw highlights.
      */
     LiveCSSDocument.prototype._updateBrowser = function () {
-        var i;
+        var i,
+            docUrl = this.doc.url;
+
+        // Determines whether an url() line contains a relative or absolute URL, and makes
+        // the URL absolute to the CSS file if it is relative
+        function makeUrlsRelativeToCss(match, quotationMark, url) {
+            if (PathUtils.isRelativeUrl(url)) {
+                var absUrl = PathUtils.makeUrlAbsolute(url, docUrl);
+                return "url(" + quotationMark + absUrl + quotationMark + ")";
+            }
+            return match;
+        }
+
         for (i = 0; i < this.roots.length; i++) {
-            if (this.doc.url !== this.roots[i].toString()) {
+            if (docUrl !== this.roots[i].toString()) {
                 // if it's not directly included through <link>,
                 // reload the original doc
                 this.trigger("updateDoc", this.roots[i]);
             } else {
-                this.protocol.setStylesheetText(this.doc.url, this.doc.getText());
+                var docText = this.doc.getText();
+
+                // Replace all occurrences of url() where the URL is relative to the CSS file with
+                // an absolute URL so it is relative to the CSS file, not the HTML file (see #11936)
+                docText = docText.replace(/\burl\(\s*(["']?)([^)\n]+)\1\s*\)/ig, makeUrlsRelativeToCss);
+                this.protocol.setStylesheetText(docUrl, docText);
             }
         }
         this.redrawHighlights();
diff --git a/test/spec/LiveDevelopment-MultiBrowser-test-files/index.html b/test/spec/LiveDevelopment-MultiBrowser-test-files/index.html
@@ -4,6 +4,10 @@
         <link rel="stylesheet" href="sub/test.css" />
     </head>
     <body>
-        <h1>Hello</h1>
+        <div class="main">
+            <h1>Hello</h1>
+        </div>
+        <div class="sub">
+        </div>
     </body>
 </html>
diff --git a/test/spec/LiveDevelopment-MultiBrowser-test-files/sub/icon_chevron.png b/test/spec/LiveDevelopment-MultiBrowser-test-files/sub/icon_chevron.png
diff --git a/test/spec/LiveDevelopment-MultiBrowser-test-files/sub/test.css b/test/spec/LiveDevelopment-MultiBrowser-test-files/sub/test.css
@@ -1,3 +1,7 @@
 body { background-color: red;}
 
+.main { background: url(icon_chevron.png); }
+
+.sub { background: url(file:///fake.png); }
+
 h1 { color: blue; }
diff --git a/test/spec/LiveDevelopmentMultiBrowser-test.js b/test/spec/LiveDevelopmentMultiBrowser-test.js
@@ -269,6 +269,9 @@ define(function (require, exports, module) {
                 runs(function () {
                     liveDoc.getSourceFromBrowser().done(function (text) {
                         browserText = text;
+                        // In LiveDocument._updateBrowser, we replace relative url()s with an absolute equivalent
+                        // Strip the leading https://127.0.0.1:port part so we can compare browser and editor text
+                        browserText = browserText.replace(/url\('http:\/\/127\.0\.0\.1:\d+\/import1\.css'\);/, "url('import1.css');");
                     }).always(function () {
                         doneSyncing = true;
                     });
@@ -280,6 +283,51 @@ define(function (require, exports, module) {
                 });
             });
 
+            it("should make CSS-relative URLs absolute", function () {
+                var localText,
+                    browserText,
+                    liveDoc,
+                    curDoc;
+
+                runs(function () {
+                    waitsForDone(SpecRunnerUtils.openProjectFiles(["index.html"]), "SpecRunnerUtils.openProjectFiles simple1.html", 1000);
+                });
+
+                waitsForLiveDevelopmentToOpen();
+
+                runs(function () {
+                    waitsForDone(SpecRunnerUtils.openProjectFiles(["sub/test.css"]), "SpecRunnerUtils.openProjectFiles simple1.css", 1000);
+                });
+                runs(function () {
+                    curDoc =  DocumentManager.getCurrentDocument();
+                    localText = curDoc.getText();
+                    localText += "\n .testClass { background-color:#090; }\n";
+                    curDoc.setText(localText);
+                });
+                runs(function () {
+                    liveDoc = LiveDevelopment.getLiveDocForPath(testFolder + "/sub/test.css");
+                });
+                var doneSyncing = false;
+                runs(function () {
+                    liveDoc.getSourceFromBrowser().done(function (text) {
+                        browserText = text;
+                    }).always(function () {
+                        doneSyncing = true;
+                    });
+                });
+                waitsFor(function () { return doneSyncing; }, "Browser to sync changes", 5000);
+
+                runs(function () {
+                    // Drop the port from 127.0.0.1:port so it's easier to work with
+                    browserText = browserText.replace(/127\.0\.0\.1:\d+/, "127.0.0.1");
+
+                    // expect relative URL to have been made absolute
+                    expect(browserText).toContain(".main { background: url(https://127.0.0.1/sub/icon_chevron.png); }");
+                    // expect absolute URL to stay unchanged
+                    expect(browserText).toContain(".sub { background: url(file:///fake.png); }");
+                });
+            });
+
             xit("should push in memory css changes made before the session starts", function () {
                 var localText,
                     browserText;
