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 9 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
94 changes: 54 additions & 40 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.<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,24 +97,36 @@ 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);
}

/**
* Reload a CSS style sheet from a document
* @param {Document} document
* @param {string=} newContent new content of every stylesheet. Defaults to doc.getText() if omitted
* @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());
function reloadCSSForDocument(doc, newContent) {
var styles = styleForURL(doc.url),
styleSheetId,
deferreds = [];

if (newContent === undefined) {
newContent = doc.getText();
}
for (styleSheetId in styles) {
deferreds.push(Inspector.CSS.setStyleSheetText(styles[styleSheetId].styleSheetId, newContent));
}
if (!deferreds.length) {
console.error("Style Sheet for document not loaded: " + doc.url);
return new $.Deferred().reject().promise();
}
// return master deferred
return $.when.apply($, deferreds);
}

/**
Expand All @@ -119,9 +135,7 @@ 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, "");
return reloadCSSForDocument(doc, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice refactoring!

}

/**
Expand All @@ -130,16 +144,21 @@ 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,
duplicate;

// detect duplicates
if (existing && existing.styleSheetId === res.header.styleSheetId) {
duplicate = _.some(existing, function (styleSheet) {
return styleSheet && styleSheet.styleSheetId === styleSheetId;
});
if (duplicate) {
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 +169,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 +247,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;
});
});
18 changes: 14 additions & 4 deletions src/LiveDevelopment/Documents/CSSDocument.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,18 @@ define(function CSSDocumentModule(require, exports, module) {
* @return {jQuery.promise} Promise resolved with the text content of this CSS document
*/
CSSDocument.prototype.getSourceFromBrowser = function getSourceFromBrowser() {
function getOnlyValue(obj) {
var key;
for (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.

I didn't noticed this before, but this function should explicitly return null; at the end in case it falls out of the for loop (instead of implicitly returning undefined.).


var deferred = new $.Deferred(),
styleSheetId = this._getStyleSheetHeader().styleSheetId,
styleSheetHeader = this._getStyleSheetHeader(),
styleSheetId = getOnlyValue(styleSheetHeader).styleSheetId,
Copy link
Contributor

Choose a reason for hiding this comment

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

getOnlyValue() can return undefined or null so this can cause an exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I am getting the following exceptions just launching Live Preview in this recipe:

Assertion failed: Attempted to call remote method without objectId set. RemoteAgent.js:58
_call RemoteAgent.js:58
Some arguments of method 'Runtime.callFunctionOn' can't be processed
Parameter 'objectId' with type 'String' was not found. 
Object

Object
 ErrorNotification.js:117
window.console.error ErrorNotification.js:117

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 don't see that error, but it seems like it's a race condition. The _objectId is maintained internally by RemoteAgent, and the only change is a return that only fires for iframes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if that error is related to this code, but I still think this potential exception should be prevented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was it an intermittent one or do you still see it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm no longer seeing the error.

inspectorPromise = Inspector.CSS.getStyleSheetText(styleSheetId);

inspectorPromise.then(function (res) {
Expand Down Expand Up @@ -252,12 +262,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[rule.ruleId.styleSheetId]) {
from = codeMirror.posFromIndex(rule.selectorRange.start);
to = codeMirror.posFromIndex(rule.style.range.end);
this._highlight.push(codeMirror.markText(from, to, { className: "highlight" }));
Expand All @@ -268,4 +278,4 @@ define(function CSSDocumentModule(require, exports, module) {

// Export the class
module.exports = CSSDocument;
});
});
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