Fix Live CSS with an iframe element #8144

Merged
merged 10 commits into from Aug 6, 2014

Projects

None yet

4 participants

@MarcelGerber
Member

For #7935 and #7785

Bugs to fix:

  • Only updating the iframe if the same CSS file is used for both iframe and page
  • Changes to the iframe's HTML will only take effect on save (out of scope)
@njx
Member
njx commented Jun 18, 2014

Triaged - we definitely want this if it works!

Whoever reviews: please double-check (probably by just putting in a breakpoint) that the code is properly hit for a normal navigation, and run the live dev unit tests.

@MarcelGerber
Member

Unit test added (I may add another one).
For the unit tests to run properly, I had to do the same change to NetworkAgent.
Btw, DOMAgent already has this check (https://github.com/adobe/brackets/blob/master/src/LiveDevelopment/Agents/DOMAgent.js#L212).
LiveDevelopment as well (https://github.com/adobe/brackets/blob/master/src/LiveDevelopment/LiveDevelopment.js#L868)

We should propably take a look at these agents' _onFrameNavigated functions as well:

  • RemoteAgent
  • ScriptAgent
@MarcelGerber
Member

Ready for review now.
Includes 2 unit tests (the second one is passing on master, too) and 3 fixes to other agents.

@redmunds redmunds self-assigned this Jun 20, 2014
@redmunds
Contributor

@SAPlayer This is definitely an improvement.

I played around with your test page, and the one thing that I saw that wasn't working like I expected was when I edited simpleShared.css -- only the iframe rendering is updated in browser, but that may be a fact of life.

I also saw some messages in console that need to be investigated:

GET http://127.0.0.1:9222/json net::ERR_CONNECTION_REFUSED Inspector.js:266
Uncaught TypeError: Cannot call method 'promise' of null LiveDevelopment.js:1287

I don't think this is ready to merge for Release 0.41, but should be able to get it in soon.

@MarcelGerber
Member

Ah, I see.
Changes to the iframe's HTMLDoc aren't pushed either, but these aren't too common scenarios imo.
An iframe will be used to display cross-origin content most of the time.

@MarcelGerber
Member

@redmunds I found the issue causing the CSS problem:
In CSSAgent, we've got the two variables _urlToStyle and _styleSheetIdToUrl (both arrays). While the ID is unique, the URL isn't as the stylesheet can be used two times. So in such cases, _urlToStyle is missing some entries.
The only solution I could come up with is to rewrite the whole agent, which would be quite tough and would include API-breaking changes.
Can you think of another solution?

@MarcelGerber
Member

@redmunds I just managed to fix the Live CSS issue by doing the major changes described above - will investigate the other two now (but it looks like bug # 3 is intermediate...)

@MarcelGerber MarcelGerber and 2 others commented on an outdated diff Jul 15, 2014
src/LiveDevelopment/Documents/CSSDocument.js
@@ -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,
@MarcelGerber
MarcelGerber Jul 15, 2014 Member

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

@peterflynn
peterflynn Jul 15, 2014 Member

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];
}
@MarcelGerber
MarcelGerber Jul 16, 2014 Member

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...

@peterflynn
peterflynn Jul 16, 2014 Member

@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.

@MarcelGerber
MarcelGerber Jul 16, 2014 Member

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.

@redmunds
redmunds Aug 5, 2014 Contributor

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];
            }
        }
    }
@redmunds
redmunds Aug 6, 2014 Contributor

@MarcelGerber You still haven't fixed this line.

@MarcelGerber
Member

@redmunds It looks like our whole DOMAgent, or maybe even the Inspector, isn't made for iframe updates, or would at least need some major changes. I guess it's not worth it - what's your opinion?

@redmunds
Contributor

There's work being done to totally change how Live Preview works, so it's definitely not worth putting a lot of effort into this.

@MarcelGerber
Member

Fine. As stated above, it isn't even common.
Is there some branch/Trello card to watch for the state of the new architecture? (is it
RESEARCH: Live Development w/ Open Protocol?)

When implementing this, we should be aware of what to pay attention on.

@redmunds
Contributor

Yes, that's the Trello Card.

@redmunds
Contributor

@SAPlayer Is this PR worth merging, or should it be closed?

@MarcelGerber
Member

It will improve the experience quite a lot, so yes, it's worth merging.
But it needs much testing as well I guess.

@redmunds redmunds commented on an outdated diff Aug 5, 2014
src/LiveDevelopment/Agents/CSSAgent.js
}
/**
* Get a style sheet for a url
* @param {string} url
- * @return {CSS.CSSStyleSheetHeader}
+ * @return {Array} Array of CSSStyleSheetHeaders
@redmunds
redmunds Aug 5, 2014 Contributor

This should be:

     * @return {Array.<CSSStyleSheetHeader>}
@redmunds redmunds commented on the diff Aug 5, 2014
src/LiveDevelopment/Agents/CSSAgent.js
*/
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];
+ }
+ }
@redmunds
redmunds Aug 5, 2014 Contributor

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

@MarcelGerber
MarcelGerber Aug 5, 2014 Member

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.

@redmunds redmunds commented on an outdated diff Aug 5, 2014
src/LiveDevelopment/Agents/CSSAgent.js
@@ -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);
@redmunds
redmunds Aug 5, 2014 Contributor

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

@redmunds redmunds commented on an outdated diff Aug 5, 2014
src/LiveDevelopment/Agents/CSSAgent.js
// detect duplicates
- if (existing && existing.styleSheetId === res.header.styleSheetId) {
+ existing = _.some(existing, function (styleSheet) {
+ return styleSheet && styleSheet.styleSheetId === styleSheetId;
+ });
+ if (existing) {
@redmunds
redmunds Aug 5, 2014 Contributor

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.

@redmunds redmunds commented on an outdated diff Aug 5, 2014
src/LiveDevelopment/Documents/CSSDocument.js
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]) {
@redmunds
redmunds Aug 5, 2014 Contributor

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

@MarcelGerber
Member

@redmunds Changes pushed. I've also unified clearCSSForDocument with reloadCSSForDocument

@MarcelGerber MarcelGerber and 1 other commented on an outdated diff Aug 5, 2014
src/LiveDevelopment/Agents/CSSAgent.js
}
/**
* Reload a CSS style sheet from a document
* @param {Document} document
+ * @param {?string=doc.getText()} new content of every stylesheet
@MarcelGerber
MarcelGerber Aug 5, 2014 Member

I actually wonder if it's a valid JSDoc.

@redmunds
redmunds Aug 5, 2014 Contributor
     * @param {?string=doc.getText()} new content of every stylesheet

Not valid. Where do you see that?

@MarcelGerber
MarcelGerber Aug 5, 2014 Member
* @param {?string} new content of every stylesheet. Defaults to doc.getText() if omitted

Is this valid then?

@redmunds
redmunds Aug 5, 2014 Contributor

If it's optional, it should be the following. Also, param name was missing.

* @param {string=} newContent new content of every stylesheet. Defaults to doc.getText() if omitted

FYI: https://developers.google.com/closure/compiler/docs/js-for-compiler

@redmunds
Contributor
redmunds commented Aug 5, 2014

I've been playing around with the simple1iframe.html test file trying to see the difference between this branch and master. The only difference I see is that changing the bg color in simpleShared.css changes only the iframe in master, but entire page in your branch. Can you post a couple recipe that illustrates this fixing #7935 and #7785 ?

@MarcelGerber
Member
  1. Open simple1iframe.html and simple1.css
  2. Start a Live Preview on the HTML file
  3. Switch over to the CSS file and edit the color to green

Result: The page doesn't change
Expected: The topmost text should get colored green

Take a look at the DevTools console: Multiple error messages logged


So basically, the outer HTML won't reflect any CSS changes any more (at least unless you press Ctrl-Shift-R).
And as the normal iframe workflow is like having an iframe linking to an external URL (like YouTube, Twitter), the whole Live CSS updating won't work anymore.

@redmunds
Contributor
redmunds commented Aug 6, 2014

Thanks for the recipe. I think I didn't see that because I was switching to iframe.html between those 2 files.

@MarcelGerber
Member

@redmunds Changes pushed again.

@redmunds redmunds commented on an outdated diff Aug 6, 2014
src/LiveDevelopment/Documents/CSSDocument.js
@@ -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];
+ }
+ }
+ }
@redmunds
redmunds Aug 6, 2014 Contributor

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.).

@redmunds redmunds and 1 other commented on an outdated diff Aug 6, 2014
src/LiveDevelopment/Documents/CSSDocument.js
var deferred = new $.Deferred(),
- styleSheetId = this._getStyleSheetHeader().styleSheetId,
+ styleSheetHeader = this._getStyleSheetHeader(),
+ styleSheetId = getOnlyValue(styleSheetHeader).styleSheetId,
@redmunds
redmunds Aug 6, 2014 Contributor

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

@redmunds
redmunds Aug 6, 2014 Contributor

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
@MarcelGerber
MarcelGerber Aug 6, 2014 Member

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.

@redmunds
redmunds Aug 6, 2014 Contributor

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

@MarcelGerber
MarcelGerber Aug 6, 2014 Member

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

@redmunds
redmunds Aug 6, 2014 Contributor

I'm no longer seeing the error.

@redmunds redmunds commented on the diff Aug 6, 2014
src/LiveDevelopment/Agents/CSSAgent.js
@@ -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, "");
@redmunds
redmunds Aug 6, 2014 Contributor

Nice refactoring!

@redmunds
Contributor
redmunds commented Aug 6, 2014

Done with review. A couple more comments.

@MarcelGerber
Member

@redmunds Changes pushed once again.

@redmunds
Contributor
redmunds commented Aug 6, 2014

Thanks! Merging.

@redmunds redmunds merged commit e5e1f46 into adobe:master Aug 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@MarcelGerber MarcelGerber deleted the MarcelGerber:live-css-iframe branch Aug 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment