Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix Live CSS with an iframe element #8144

Merged
merged 10 commits into from
Aug 6, 2014
Merged

Fix Live CSS with an iframe element #8144

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b496f08
Fix Live CSS with an iframe element
Jun 17, 2014
db16092
Added unit test, fixed NetworkAgent
Jun 19, 2014
7546767
Add another unit test, Rename simple2.* test files
Jun 19, 2014
f99d852
Fix RemoteAgent and ScriptAgent
Jun 19, 2014
0b39d2e
Fix Live CSS bug
Jul 10, 2014
d81e380
Fix unit tests
Jul 15, 2014
a0fabf5
Merge remote-tracking branch 'upstream/master' into live-css-iframe
Aug 5, 2014
43e9ce6
Fix JSDoc, unify clearCSSForDocument with reloadCSSForDocument
Aug 5, 2014
f3206d3
Fix JSDoc, use getOnlyValue instead of _.keys stuff
Aug 6, 2014
1893696
Code review changes
Aug 6, 2014
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
91 changes: 52 additions & 39 deletions src/LiveDevelopment/Agents/CSSAgent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,15 @@ define(function CSSAgent(require, exports, module) {

require("thirdparty/path-utils/path-utils.min");

var _ = require("thirdparty/lodash");

var Inspector = require("LiveDevelopment/Inspector/Inspector");

/**
* Stylesheet urls
* Stylesheet details
* @type {Object.<string, CSS.CSSStyleSheetHeader>}
*/
var _urlToStyle = {};

/**
* Map of stylesheet ids to urls
* @type {Object.<string, string>}
*/
var _styleSheetIdToUrl;
var _styleSheetDetails = {};

/**
* Is getAllStyleSheets() API defined? - This is undefined until we test for API
Expand All @@ -73,18 +69,26 @@ define(function CSSAgent(require, exports, module) {
* @param {frame: Frame} res
*/
function _onFrameNavigated(event, res) {
// Clear maps when navigating to a new page
_urlToStyle = {};
_styleSheetIdToUrl = {};
// Clear maps when navigating to a new page, but not if an iframe was loaded
if (!res.frame.parentId) {
_styleSheetDetails = {};
}
}

/**
* Get a style sheet for a url
* @param {string} url
* @return {CSS.CSSStyleSheetHeader}
* @return {Array} Array of CSSStyleSheetHeaders
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be:

     * @return {Array.<CSSStyleSheetHeader>}

*/
function styleForURL(url) {
return _urlToStyle[_canonicalize(url)];
var styleSheetId, styles = {};
url = _canonicalize(url);
for (styleSheetId in _styleSheetDetails) {
if (_styleSheetDetails[styleSheetId].canonicalizedURL === url) {
styles[styleSheetId] = _styleSheetDetails[styleSheetId];
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this ever return more than 1 stylesheet for a url? If so, what's the use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the big problem we had.
Let's say you've got this code:

<html>
  <head>
    <link rel="stylesheet" href="css/style.css" />
  </head>
  <body>
    <iframe>
      <html>
        <head>
          <link rel="stylesheet" href="css/style.css" />
        </head>
        <body>
          Foo.
        <body>
      </html>
    </iframe>
  </body>
</html>

In that case, you've got the exact same stylesheet used in two different documents on one page, and what this means is that the browser/inspector has two different stylesheet objects with the same URL.

return styles;
}

/**
Expand All @@ -93,13 +97,11 @@ define(function CSSAgent(require, exports, module) {
* @deprecated
*/
function getStylesheetURLs() {
var urls = [], url;
for (url in _urlToStyle) {
if (_urlToStyle.hasOwnProperty(url)) {
urls.push(url);
}
var styleSheetId, urls = [];
for (styleSheetId in _styleSheetDetails) {
urls[_styleSheetDetails[styleSheetId].canonicalizedURL] = true;
}
return urls;
return _.keys(urls);
}

/**
Expand All @@ -108,9 +110,15 @@ define(function CSSAgent(require, exports, module) {
* @return {jQuery.Promise}
*/
function reloadCSSForDocument(doc) {
var style = styleForURL(doc.url);
console.assert(style, "Style Sheet for document not loaded: " + doc.url);
return Inspector.CSS.setStyleSheetText(style.styleSheetId, doc.getText());
var styles = styleForURL(doc.url),
styleSheetId,
deferreds = [];
console.assert(styles, "Style Sheet for document not loaded: " + doc.url);
for (styleSheetId in styles) {
deferreds.push(Inspector.CSS.setStyleSheetText(styles[styleSheetId].styleSheetId, doc.getText()));
}
// return master deferred
return $.when.apply($, deferreds);
}

/**
Expand All @@ -119,9 +127,15 @@ define(function CSSAgent(require, exports, module) {
* @return {jQuery.Promise}
*/
function clearCSSForDocument(doc) {
var style = styleForURL(doc.url);
console.assert(style, "Style Sheet for document not loaded: " + doc.url);
return Inspector.CSS.setStyleSheetText(style.styleSheetId, "");
var styles = styleForURL(doc.url),
styleSheetId,
deferreds = [];
console.assert(styles, "Style Sheet for document not loaded: " + doc.url);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

styleForURL() now returns an Object, so styles will never be null. I think you can test for _.keys(styles).length.

for (styleSheetId in styles) {
deferreds.push(Inspector.CSS.setStyleSheetText(styles[styleSheetId].styleSheetId, ""));
}
// return master deferred
return $.when.apply($, deferreds);
}

/**
Expand All @@ -130,16 +144,20 @@ define(function CSSAgent(require, exports, module) {
* @param {header: CSSStyleSheetHeader}
*/
function _styleSheetAdded(event, res) {
var url = _canonicalize(res.header.sourceURL),
existing = _urlToStyle[url];
var url = _canonicalize(res.header.sourceURL),
existing = styleForURL(res.header.sourceURL),
styleSheetId = res.header.styleSheetId;

// detect duplicates
if (existing && existing.styleSheetId === res.header.styleSheetId) {
existing = _.some(existing, function (styleSheet) {
return styleSheet && styleSheet.styleSheetId === styleSheetId;
});
if (existing) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's confusing that you're re-using the existing var which starts as an array and is then converted to a boolean. Use 2 different vars.

return;
}

_urlToStyle[url] = res.header;
_styleSheetIdToUrl[res.header.styleSheetId] = url;
_styleSheetDetails[styleSheetId] = res.header;
_styleSheetDetails[styleSheetId].canonicalizedURL = url; // canonicalized URL

$(exports).triggerHandler("styleSheetAdded", [url, res.header]);
}
Expand All @@ -150,16 +168,11 @@ define(function CSSAgent(require, exports, module) {
* @param {styleSheetId: StyleSheetId}
*/
function _styleSheetRemoved(event, res) {
var url = _styleSheetIdToUrl[res.styleSheetId],
header = url && _urlToStyle[url];

if (url) {
delete _urlToStyle[url];
}
var header = _styleSheetDetails[res.styleSheetId];

delete _styleSheetIdToUrl[res.styleSheetId];
delete _styleSheetDetails[res.styleSheetId];

$(exports).triggerHandler("styleSheetRemoved", [url, header]);
$(exports).triggerHandler("styleSheetRemoved", [header.canonicalizedURL, header]);
}

/**
Expand Down Expand Up @@ -233,4 +246,4 @@ define(function CSSAgent(require, exports, module) {
exports.clearCSSForDocument = clearCSSForDocument;
exports.load = load;
exports.unload = unload;
});
});
7 changes: 5 additions & 2 deletions src/LiveDevelopment/Agents/NetworkAgent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,10 @@ define(function NetworkAgent(require, exports, module) {
// WebInspector Event: Page.frameNavigated
function _onFrameNavigated(event, res) {
// res = {frame}
_reset();
// Clear log when navigating to a new page, but not if an iframe was loaded
if (!res.frame.parentId) {
_reset();
}
_logURL(res.frame.url);
}

Expand Down Expand Up @@ -101,4 +104,4 @@ define(function NetworkAgent(require, exports, module) {
exports.enable = enable;
exports.load = load;
exports.unload = unload;
});
});
7 changes: 6 additions & 1 deletion src/LiveDevelopment/Agents/RemoteAgent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,12 @@ define(function RemoteAgent(require, exports, module) {

// WebInspector Event: Page.frameNavigated
function _onFrameNavigated(event, res) {
// res = {timestamp}
// res = {frame}
// Re-inject RemoteFunctions when navigating to a new page, but not if an iframe was loaded
if (res.frame.parentId) {
return;
}

_stopKeepAliveInterval();

// inject RemoteFunctions
Expand Down
8 changes: 5 additions & 3 deletions src/LiveDevelopment/Agents/ScriptAgent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,10 @@ define(function ScriptAgent(require, exports, module) {
* @param {frame: Frame} res
*/
function _onFrameNavigated(event, res) {
// Clear maps when navigating to a new page
_reset();
// Clear maps when navigating to a new page, but not if an iframe was loaded
if (!res.frame.parentId) {
_reset();
}
}

/** Initialize the agent */
Expand Down Expand Up @@ -171,4 +173,4 @@ define(function ScriptAgent(require, exports, module) {
exports.scriptForURL = scriptForURL;
exports.load = load;
exports.unload = unload;
});
});
7 changes: 4 additions & 3 deletions src/LiveDevelopment/Documents/CSSDocument.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ define(function CSSDocumentModule(require, exports, module) {
*/
CSSDocument.prototype.getSourceFromBrowser = function getSourceFromBrowser() {
var deferred = new $.Deferred(),
styleSheetId = this._getStyleSheetHeader().styleSheetId,
styleSheetHeader = this._getStyleSheetHeader(),
styleSheetId = styleSheetHeader[_.keys(styleSheetHeader)[0]].styleSheetId,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether there's a nicer method to determinate the first index of an object?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know this code well enough to know what it's trying to do, but just wanted to point out that there's no concept of "first" (or "index" for that matter) in associative arrays -- the keys are unordered. I'm guessing this code is assuming only one key exists, and it just wants its value, but it doesn't know what the one key is...

If that's the case, you could do it more efficiently with a utiltiy like this:

function getOnlyValue(obj) {
    for (var key in obj) {
        if (_.has(obj, key)) return obj[key];
    }
}

Or, to fail fast when the single-key assumption is wrong:

function getOnlyValue(obj) {
    var foundKey;
    for (var key in obj) {
        if (_.has(obj, key)) {
            if (foundKey) throw new Error("Object has multiple keys: " + key + ", " + foundKey);
            foundKey = key;
        }
    }
    return obj[foundKey];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it can have multiple values (that's what this PR is all about - we assumed it can only have one value before), but they should all be the same, so it doesn't matter much which one to use. And as they are enumerated (but not beginning with 0), the first one should be ok...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SAplayer When you say "they are enumerated (but not beginning with 0)," do you mean the keys are all numbers, but they're non-sequential?

If so, still bear in mind that there's no guarantee on the order of Object.keys() -- the lowest number is not necessarily first in the array. If your code relies on always getting the lowest index/key, you should .sort() the keys array to ensure that'll always actually happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's how the array is structured.
I don't need to have the first entry, AFAIK every entry should have the same value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's safe accessing [0]. I think @peterflynn 's first suggestion should work (with a change to make jslint happy):

    function getOnlyValue(obj) {
        for (var key in obj) {
            if (_.has(obj, key)) {
                return obj[key];
            }
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcelgerber You still haven't fixed this line.

inspectorPromise = Inspector.CSS.getStyleSheetText(styleSheetId);

inspectorPromise.then(function (res) {
Expand Down Expand Up @@ -252,12 +253,12 @@ define(function CSSDocumentModule(require, exports, module) {
Inspector.CSS.getMatchedStylesForNode(node.nodeId, function onGetMatchesStyles(res) {
// res = {matchedCSSRules, pseudoElements, inherited}
var codeMirror = this.editor._codeMirror,
styleSheetId = this._getStyleSheetHeader().styleSheetId;
styleSheetIds = this._getStyleSheetHeader();

var i, rule, from, to;
for (i in res.matchedCSSRules) {
rule = res.matchedCSSRules[i];
if (rule.ruleId && rule.ruleId.styleSheetId === styleSheetId) {
if (rule.ruleId && styleSheetIds && styleSheetIds[rule.ruleId.styleSheetId]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

styleSheetIds is an Object, so it doesn't need to be checked for null.

from = codeMirror.posFromIndex(rule.selectorRange.start);
to = codeMirror.posFromIndex(rule.style.range.end);
this._highlight.push(codeMirror.markText(from, to, { className: "highlight" }));
Expand Down
9 changes: 9 additions & 0 deletions test/spec/LiveDevelopment-test-files/iframe.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#testId {
font-size: 20px;
}

.testClass {
background-color:rgba(102,102,204,.5);
color:rgba(255,255,255,1);
}

2 changes: 1 addition & 1 deletion ...c/LiveDevelopment-test-files/simple2.html → ...ec/LiveDevelopment-test-files/iframe.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<meta charset="UTF-8">
<title>Simple Test</title>
<link rel="stylesheet" href="simpleShared.css">
<link rel="stylesheet" href="simple2.css">
<link rel="stylesheet" href="iframe.css">
</head>

<body>
Expand Down
14 changes: 14 additions & 0 deletions test/spec/LiveDevelopment-test-files/simple1iframe.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<!doctype html>
<html>
<head>
<meta charset="UTF-8">
<title>Simple Test</title>
<link rel="stylesheet" href="simpleShared.css">
<link rel="stylesheet" href="simple1.css">
</head>

<body>
<p id="testId" class="testClass">iframes are cool.</p>
<iframe src="iframe.html"></iframe>
</body>
</html>
8 changes: 0 additions & 8 deletions test/spec/LiveDevelopment-test-files/simple2.css
View file
Open in desktop

This file was deleted.

10 changes: 9 additions & 1 deletion test/spec/LiveDevelopment-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ define(function (require, exports, module) {
HighlightAgentModule.load();

// module spies
spyOn(CSSAgentModule, "styleForURL").andReturn("");
spyOn(CSSAgentModule, "styleForURL").andReturn([]);
spyOn(CSSAgentModule, "reloadCSSForDocument").andCallFake(function () { return new $.Deferred().resolve(); });
spyOn(HighlightAgentModule, "redraw").andCallFake(function () {});
spyOn(HighlightAgentModule, "rule").andCallFake(function () {});
Expand Down Expand Up @@ -626,6 +626,10 @@ define(function (require, exports, module) {
doOneTest("simple1Query.html", "simple1.css");
});

it("should push changes after loading an iframe", function () {
doOneTest("simple1iframe.html", "simple1.css");
});

it("should push in memory css changes made before the session starts", function () {
var localText,
browserText;
Expand Down Expand Up @@ -751,6 +755,10 @@ define(function (require, exports, module) {
expect(updatedNode.value).toBe("Live Preview in Brackets is awesome!");
});
});

it("should push changes to the iframes' css file", function () {
doOneTest("simple1iframe.html", "iframe.css");
});
});

describe("HTML Editing", function () {
Expand Down