I worked on this during our Feb 14 devtools workweek in PDX. The goal is to show all authored styles in the rule-view panel of the inspector in the devtools, rather than only showing the styles actually parsed as we do today. This would allow to show other vendor-prefixed properties and invalid properties, and therefore help developer understand why some properties are not applied.

http://www.youtube.com/watch?v=Bve7RD5amWs Here's the demo I did during the workweek. The patches wrote for this are very much drafts that need a lot of polishing and testing.

First attempt at having authored styles in the rule-view. Here is a list of things done: - Added a new pref "devtools.inspector.authoredStyles" to toggle authored styles from the options panel (false by default, and refreshes the rule-view when toggled). - Changed the layout of the options panel to be on 3 columns. - Moved css-tokenizer and css-parsing-utils modules to toolkit (they were in browser/devtools before) so that they can be both used on the client and the server. - Added a css rule text body parser to css-parsing-utils (+ some unit tests). - Added a new method to the RuleStyleActor to retrieve authored styles. This method gets the full stylesheet text from the parent stylesheet actor (which caches it after it's retrieved once), and then parses the rule text using css-parsing-utils, and caches it too. - Added a new option to the styles actor getApplied method to ask for retrieval of the authored css text. - Changed the rule-view to, based on the pref, ask for the authored styles. I haven't yet written any ui tests nor ran the existing ones.

Comment on attachment 8405354 [details] [diff] [review] bug984880-ruleview-authored-styles v1.patch Review of attachment 8405354 [details] [diff] [review]: ----------------------------------------------------------------- Certain CSS causes this to blow up. Here are a few examples: 1) try inspecting the main body element on jsfiddle.net 2) inspect the body element of the result on jsfiddle.net/unxra/3/. 3) inspect the body element of the result on http://jsfiddle.net/unxra/11/. At first I was sure this was related to the input[type=\"submit\"] also, but it seems to fail in different ways depending on which lines of CSS get removed.

Yeah, I've seen it fail on google.com too. I've narrowed it down to `this.line = DOMUtils.getRuleLine(this.rawRule);` in the StyleRuleActor (styles.js) which doesn't return the correct line number when css is either inline or minified (I'm not sure which of the two or both is causing it yet). Maybe I should report this in bug 974686 where I asked for a getRuleEndLine and getRuleEndColumn, because there's clearly a problem. In fact, you can see the problem even in Aurora, go to google.com, open the inspector, you'll see that the stylesheet links are all "inline:11" and clicking on the link doesn't open the editor at the right place.

Inline stylesheet rules locations should now be fixed. It's a hack for now, because DOMUtils.getRuleLine(rawRule) returns the line number relative to whatever resource that rule is in, so if it's a <style> tag, then the line number is relative to the HTML document itself. My hack is, using the location of the first rule, calculating the offset. I'm going to file a bug to see if getRuleLine can be modified or if we can get a getRelativeRuleLine function, but in the meantime, this allows to progress.

v3

v3 = minor changes and fixed failing test. I want to start with a new patch that moves the whole `if(authored)` decision making on the server-side only, this way leaving the rule-view completely unchanged.

Second take on this bug. Keeps all the authoredText vs. cssText management on the server-side, therefore simplifies the patch, and reduces the nb of potential regressions in the rule-view, because the rule-view doesn't change.

Rebased

Fixes a couple of bugs I found and also removes the pref to simplify things a bit (I think it is much better to show authored styles, and not sure that we need to support un-authored once it is available).

We (as in "the web compat team") are very much looking forward to this :)

Brian asked me to upload my WIP patch, so here it is. My fork of the upstream CSS parser is here: https://github.com/tromey/parse-css There's still quite a bit left to do. At least: * Preserve comments in the tokenizer, and preserve whitespace better. * Make the tokenizer lazy. I have a branch for this in the github repository, but this requires some changes in the firefox callers as well. * Maybe change tokenizer locations to just be offsets into the input string. * Actually use the new parser utilities, replacing our current ad hoc code. (This requires a bit of investigation as I also want to see if the existing css-autocompleter can reuse the new parser as well.) * Make editing and round-tripping (through the style editor) work properly; including bug 715888 and bug 913661.

(In reply to Tom Tromey :tromey from comment #13) > Created attachment 8573447 [details] [diff] [review] > rebase patch, import new css parser, and simplify a bit > > Brian asked me to upload my WIP patch, so here it is. Thanks Tom, that looks good. > My fork of the upstream CSS parser is here: > > https://github.com/tromey/parse-css > > There's still quite a bit left to do. At least: > > * Preserve comments in the tokenizer, and preserve whitespace better. > > * Make the tokenizer lazy. I have a branch for this in the github > repository, but this requires some changes in the firefox callers as > well. Meaning? Making the tokenizer a generator function that yields tokens, right? If so, that's great! We need this. > * Maybe change tokenizer locations to just be offsets into the input > string. > > * Actually use the new parser utilities, replacing our current > ad hoc code. (This requires a bit of investigation as I also want > to see if the existing css-autocompleter can reuse the new parser > as well.) > > * Make editing and round-tripping (through the style editor) work properly; > including bug 715888 and bug 913661.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14) > > Brian asked me to upload my WIP patch, so here it is. > Thanks Tom, that looks good. Haha, it should, you wrote a lot of it :-) > > * Make the tokenizer lazy. I have a branch for this in the github > > repository, but this requires some changes in the firefox callers as > > well. > Meaning? Making the tokenizer a generator function that yields tokens, > right? If so, that's great! We need this. Yes, exactly. This was actually very easy. I'll bring it into my local tree this week for better testing. I'm also thinking now that output-parser.js should be refactored to use the tokenizer (and maybe parser; or maybe some new parsing bits I'll write). I believe the regexps there will fail in various "authored" cases; and even right now in some situations.

This is rebased and partially rewritten to use the new CSS tokenizer. It still has one completely bogus change (the commented out code), and it needs more testing. But it is as good as the earlier patch it obsoletes.

While I'm there, clean up css-parsing-utils.js to remove some eslint warnings.

After a discussion on irc, I removed the Loader.jsm changes and updated the requires to follow.

I went through the WIP patch and fixed up all the oddities. This patch works pretty well on the tests I've tried. It also fixes bug 715888. * Changes in the rule view still don't show up in the style editor. That's a separate bug, but worth noting here. * Comments in the rule still don't show up in the rule view. It's not clear to me if this is a requirement for this bug. * "As authored" works ok, but the color editor doesn't respect it. Related perhaps to bug 1171494. * I have to write more tests for this.

This fixes some (but not all -- I skipped anything requiring any thought) eslint warnings in files touched by the previous patch.

(In reply to Tom Tromey :tromey from comment #19) > * Comments in the rule still don't show up in the rule view. > It's not clear to me if this is a requirement for this bug. I'd say no - we should handle comments in a followup.

Testing today showed a regression on browser/devtools/styleinspector/test/browser_ruleview_add-property-and-reselect.js among other things. In this case, adding a property, switching to a different element, and then reselecting the element will cause the rule view to forget the new property. I have not investigated but it seems that this is a consequence of only returning the authored style -- anything not in the original text will not be available to the rule view.

I added some tests, and now the color editor respects the original style.

This approach has an issue that you can see this way: Apply the patch. Open the rule view and select some element. Add a new property that affects the element; e.g., I added "font-weight: bold" to something, changing the display of the text. Select a different element in the markup view. Now reselect the original element -- and notice that nothing in the rule view mentions the property that you added. This particular scenario could be addressed by dealing with round-tripping and synchronization with the style editor, see bug 878398. I am unsure if there is some scenario that can't be addressed this way. I had thought perhaps modifying an element's CSS from JS would cause problems, but this works properly (it shows up in the "element" rule).

Another difficulty here is that even if we sync the inspector and editor, we can end up in a situation where neither shows the truth. The scenario is: open the editor, make some changes, then close the toolbox. Now re-open the editor -- the changes won't be visible. (And if we make this proposed change to the rule view, they won't be visible there either; only in the computed view.) To make this work I think we'll need to stash the edited text somewhere that survives closing the toolbox.

(In reply to Tom Tromey :tromey from comment #26) > Another difficulty here is that even if we sync the inspector and editor, we > can end up in a situation where neither shows the truth. The scenario is: > open the editor, make some changes, then close the toolbox. Now re-open the > editor -- the changes won't be visible. (And if we make this proposed change > to the rule view, they won't be visible there either; only in the computed > view.) > > To make this work I think we'll need to stash the edited text somewhere that > survives > closing the toolbox. Right, we've discussed adding a "shared mutable resource cache" concept before that all tools could use, so that everyone's seeing the same version of a resource (such as a CSS file). But, that seems like work for bug 878398, so I would not hold up this bug because of this problem.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27) > (In reply to Tom Tromey :tromey from comment #26) > > Another difficulty here is that even if we sync the inspector and editor, we > > can end up in a situation where neither shows the truth. The scenario is: > > open the editor, make some changes, then close the toolbox. Now re-open the > > editor -- the changes won't be visible. (And if we make this proposed change > > to the rule view, they won't be visible there either; only in the computed > > view.) > > > > To make this work I think we'll need to stash the edited text somewhere that > > survives > > closing the toolbox. > > Right, we've discussed adding a "shared mutable resource cache" concept > before that all tools could use, so that everyone's seeing the same version > of a resource (such as a CSS file). Exactly. This is an important key to solving the styles-as-authored and inspector-editor sync problems. Also storing the changes made through devtools into a writable cache means that we'd be able to extract a diff of all changes made in devtools.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27) > Right, we've discussed adding a "shared mutable resource cache" concept > before that all tools could use, so that everyone's seeing the same version > of a resource (such as a CSS file). > > But, that seems like work for bug 878398, so I would not hold up this bug > because of this problem. I have been thinking that I would not want to land this patch, even if bug 878398 is fixed, until this deeper problem was addressed. It seems to me like this would introduce a pretty serious problem into the rule view -- namely that it would not reflect reality in some scenarios. I'm curious to know what you think about this. I also found bug 922146, which provides some of the infrastructure I'll need. I started rebasing the patch there yesterday. Another thing I don't know what to do about is modifications to style sheets via CSSOM. IIUC content can use document.styleSheets[0].deleteRule or the like to modify style sheets in place. I think (haven't tried it yet) it means that changes via the style editor will overwrite changes made by content, due to how it reloads whole style sheets. One idea would be to try to reflect these changes back into the cache. However, changes can be done when the tools aren't active. That idea could be salvaged by noting when a rule has been modified this way and treating it specially -- in the rule view, showing the rule's cssText (I suspect this already happens); and in the editor, replacing the source rule's text with the cssText. This seems a bit tricky to get right, but doable.

So far I mostly just have thoughts on the problems at hand. I'm not quite sure how best to solve these things. (In reply to Tom Tromey :tromey from comment #29) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27) > > > Right, we've discussed adding a "shared mutable resource cache" concept > > before that all tools could use, so that everyone's seeing the same version > > of a resource (such as a CSS file). > > > > But, that seems like work for bug 878398, so I would not hold up this bug > > because of this problem. > > I have been thinking that I would not want to land this patch, even if bug > 878398 > is fixed, until this deeper problem was addressed. It seems to me like this > would introduce a pretty serious problem into the rule view -- namely that it > would not reflect reality in some scenarios. > > I'm curious to know what you think about this. Hmm, I missed comment 25 where you said a change in rule view is lost if change from element A to B and back to A again. I agree that problem should be solved before landing this bug, as that would be a functionality loss. I was previously thinking only of comment 26, about the style editor: it is already the case today that if you make changes in the style editor, close toolbox, and reopen, then the changes are lost. So, that scenario is unchanged. These issues are related to the discussions at Whistler about a general resources tool, particularly because there are several "views" of CSS: 1. stylesheets as they are downloaded 2. the live CSS accessible via CSSOM But once you have made an edit in the rule view, if we hope to keep that in sync with the style editor, then we want something like: 3. composed view with edits merged back into the stylesheet text as the thing to display in Style Editor. This could require knowing which rule set (a block with selector plus several rules) in the style sheet text corresponds to which rule view item (which I think you started to hint at). Another confounding factor are CSS source maps. The style editor could be displaying the original source in Sass, LESS, Stylus, etc. Since the browser can't know about these languages, it could be even trickier to represent rule view edits in the style editor if it is displaying the "original source" view. In the general case, there may not be anything we can do, since 1 rule set in these preprocessors can produce N rule sets in CSS. If you then edit real CSS in the rule view of 1 of N rule sets, where are you supposed to represent that in the Style Editor? It's not very clear. But, if the basic concept of tracking rule sets between rule view and style editor works for normal CSS, maybe we can find ways to be clever here too. > Another thing I don't know what to do about is modifications to style sheets > via CSSOM. IIUC content can use document.styleSheets[0].deleteRule or the > like > to modify style sheets in place. It can also use document.styleSheets[0].cssRules[0].style to modify rules in place. My impression is that these abilities to modify the CSSOM from content JS are used sparingly. Most developers just write CSS (or use some preprocessor), and that's the end of it. It would be nice to notice such changes for correctness, but I am not sure that it's critical to have, especially to start with. > I think (haven't tried it yet) it means that changes via the style editor > will > overwrite changes made by content, due to how it reloads whole style sheets. Yes, that seems likely. In an ideal world, we can still access the "composed view" (3 above) even after closing and reopening the toolbox and can redisplay it in the style editor with rule edits still in place in the text. In past discussions with Patrick and others, we had imagined that a server-side resource cache could help with problems like this by: * acting as a central store across tools * can hold on to edits even after toolbox close, in case they would be lost otherwise What exactly does it store? Unclear. Does it just store file contents? Some kind of AST? Not sure. > One idea would be to try to reflect these changes back into the cache. > However, > changes can be done when the tools aren't active. > > That idea could be salvaged by noting when a rule has been modified this way > and treating > it specially -- in the rule view, showing the rule's cssText (I suspect this > already > happens); and in the editor, replacing the source rule's text with the > cssText. > This seems a bit tricky to get right, but doable. Yes, I agree the case of content performing editing actions while the toolbox is closed is hard to handle. Maybe the toolbox reads the current state on open, and re-creates the composed view at that time? Not sure if required data is already lost by that time. Anyway, this is quite a hard problem overall. I think the main thing for this bug is to ensure the rule view preserves edits when switching between elements. I am hopeful that is a simpler issue than the larger problem of syncing the style editor as well.

Just thinking out loud - ignore if it doesn't make sense: Developers are used to the difference between a "View source" feature and a "DOM inspector". We're not used to thinking about CSS in the same way, but under the hood we have the same issue: developers may need access to both the "raw" code and the interpreted output. Would it simplify the implementation to have two modes and make the distinction between these modes clearer?

(In reply to Hallvord R. M. Steen [:hallvors] from comment #31) > Just thinking out loud - ignore if it doesn't make sense: > > Developers are used to the difference between a "View source" feature and a > "DOM inspector". We're not used to thinking about CSS in the same way, but > under the hood we have the same issue: developers may need access to both > the "raw" code and the interpreted output. > > Would it simplify the implementation to have two modes and make the > distinction between these modes clearer? It's an interesting point. I think you could say we are already heading in that direction with this bug, where the Rule View would be "view source" and the Computed View is the "DOM inspector".

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #30) > I agree that problem should be solved before landing this bug, as that would > be a functionality loss. Thanks. > Another confounding factor are CSS source maps. The style editor could be > displaying the original source in Sass, LESS, Stylus, etc. Since the > browser can't know about these languages, it could be even trickier to > represent rule view edits in the style editor if it is displaying the > "original source" view. In the general case, there may not be anything we > can do, since 1 rule set in these preprocessors can produce N rule sets in > CSS. What seems to happen today is that if you are viewing the original sources, then edits in the source editor don't have any visible effect. But if you switch to the "non-original" view, edits do take effect. That seems to be pretty much the only approach that makes sense. The tools could be a bit more clear about what is going on -- like saying "hey, you are editing this, but it is a derived file" or "you are viewing the source file but the derived file has edits that aren't reflected here". > Yes, that seems likely. In an ideal world, we can still access the > "composed view" (3 above) even after closing and reopening the toolbox and > can redisplay it in the style editor with rule edits still in place in the > text. > > In past discussions with Patrick and others, we had imagined that a > server-side resource cache could help with problems like this by: > > * acting as a central store across tools > * can hold on to edits even after toolbox close, in case they would be lost > otherwise > > What exactly does it store? Unclear. Does it just store file contents? > Some kind of AST? Not sure. I spent yesterday thinking about a plan for this. The basic idea is to have a single representation of the "source text" that is shared by the tools. This is the resource cache idea. The first time a tool asks for the source text of a given style sheet, we compute it on the server. This is done by fetching the source as is done today; and then comparing it with the CSSOM style sheets to see whether any edits have been made. This comparison would be done by walking the style sheet and examining each rule. At the same time we'd parse the original source text. I think an edited or added rule will have a line number of 0 (still have to check this -- but if there are cases that don't work, we can also solve this by adding a dirty bit); this plus parsing is enough to also reason about deleted rules. Then I picture the store commenting out the obsoleted rules and inserting the changes from CSSOM. This would yield the "original" text for use by the tools. After this there are three sources of changes: 1. Style editor; this can edit the text directly. 2. Rule view. This can send edits to the store. This could let us fix bug 913661 (with some additional work involving possibly parsing commented-out declarations) 3. CSSOM changes by content. We can listen to these and have the store update the text. I'm tentatively picturing a react-like style where the store applies the edits and then posts an event; and the style editor and rule view update in response to these events. One difficulty is if content is editing the CSS while a tool is open. For example if you are editing in the style editor when a timeout changes the CSSOM -- what should happen? In this model we don't really need the resource cache to persist across tool closing. It would be nice if it did, and this could make use of that. But if it did not we still could at least show something sensible.

(In reply to Tom Tromey :tromey from comment #33) > At the same time we'd parse the original source text. I think an edited or > added rule will have a line number of 0 (still have to check this -- but if > there > are cases that don't work, we can also solve this by adding a dirty bit); It seems that setting a property directly doesn't reset the line number. I tried with: document.styleSheets[1].cssRules[0].style.backgroundColor = 'red' ... then reopened the inspector. It still had the line number referring to the .css file.

(In reply to Tom Tromey :tromey from comment #33) > One difficulty is if content is editing the CSS while a tool is open. For > example > if you are editing in the style editor when a timeout changes the CSSOM -- > what > should happen? Joe pointed out that we should just follow whatever the markup view does, since it has the same issue.

The plan described in comment 33 seems pretty good to me after a quick read. But I'll need to think about it more in details I guess. I completely agree that we first need to address the resource store thing, and make sure that the rule-view and the style-editor can read and write to it. Any change made in these tools should end up modifying our knowledge of the authored style in this store so that we can have a unique source of truth. And as you said, we'll need to listen to style changes done by content to also update the store. I'm wondering how exactly you plan to re-conciliate the fetched text with the actual style when content has inserted/deleted rules before the toolbox was opened. The plan mentions comparing the text with the CSSOM, I'm wondering if that's going to lead to problems that are very hard to solve. What if content shuffles rules around by using deleteRule and insertRule, or has several rules with the same selector. As Ryan said earlier, modifying styles this way doesn't happen in the wild too often, so we probably shouldn't spend too much time on it. (Related: this post on David Walsh's blog and a comment of mine: http://davidwalsh.name/add-rules-stylesheets#comment-501007 ) We also still have the potential issue that the resource may have changed on the server by the time we go and fetch it (when the toolbox is opened the first time and if the resource isn't cached). But that's already an issue today, also probably a very rare problem, and would require necko platform work to address I think, so it's probably fine if we leave it as is for now. I haven't actually looked at how the debugger does it, but I was once told that there was a platform API that returned the script source that's being executed, without having to go and fetch it again.

(In reply to Patrick Brosset [:pbrosset] [:pbro] [on PTO until August 10th] from comment #36) > I'm wondering how exactly you plan to re-conciliate the fetched text with > the actual style when content has inserted/deleted rules before the toolbox > was opened. The plan mentions comparing the text with the CSSOM, I'm > wondering if that's going to lead to problems that are very hard to solve. > What if content shuffles rules around by using deleteRule and insertRule, or > has several rules with the same selector. I think I will add a dirty bit to each rule so we can see if it has been modified. For any rule that is modified (or added), we'll simply comment it out from the authored text and insert the new text from the CSSOM. This is not ideal, since we'll lose some authored data -- e.g., for properties that weren't modified, we'll forget how colors were written. But, I think this is still ok because (1) it will reflect reality, (2) this doesn't happen much, and (3) the authored text will still be there in a comment in the style editor. > We also still have the potential issue that the resource may have changed on > the server by the time we go and fetch it (when the toolbox is opened the > first time and if the resource isn't cached). But that's already an issue > today, also probably a very rare problem, and would require necko platform > work to address I think, so it's probably fine if we leave it as is for now. Yeah, I was planning to address this separately at a later date. > I haven't actually looked at how the debugger does it, but I was once told > that there was a platform API that returned the script source that's being > executed, without having to go and fetch it again. I will look into this.

Overall, I think this plan sounds good to me as well. I'll stress once more (just like Patrick did) that edits to the CSSOM by content JS are quite rare, so we should not spend large amounts of time to support them if it ends up being hard to do so. Currently, the rule view receives rule actors that represent CSSOM rules (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still converse over the RDP in terms of rules, or would it pull the text from the server's resource cache and parse things as needed in the tool's front end? [1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/styles.js#512

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #38) > Currently, the rule view receives rule actors that represent CSSOM rules > (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still > converse over the RDP in terms of rules, or would it pull the text from the > server's resource cache and parse things as needed in the tool's front end? Good question. The patch currently attached to this bug tries to do the work on the server side. However, this can't fully work, I think, because the client side also needs some tweaks to deal with some scenarios. So I am not really sure what I will do. I have been looking into this a bit this week; though I don't really understand some of what I see, for example why we have two kinds of style sheet actor.

(In reply to Tom Tromey :tromey from comment #39) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #38) > > > Currently, the rule view receives rule actors that represent CSSOM rules > > (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still > > converse over the RDP in terms of rules, or would it pull the text from the > > server's resource cache and parse things as needed in the tool's front end? > > Good question. The patch currently attached to this bug tries to do the > work on the server side. However, this can't fully work, I think, because > the client side also needs some tweaks to deal with some scenarios. I'm hoping Patrick and/or Brian can help flesh out the details here. I haven't spent enough time in these tools to know the edge cases well enough. > So I am not really sure what I will do. I have been looking into this a bit > this week; though I don't really understand some of what I see, for example > why we have two kinds of style sheet actor. It's there for compatibility purposes[1]. IIRC, actors/styleeditor.js is the old one. It really needs a nice ASCII warning at the top saying "I am old! Go away!". The new one is actors/stylesheets.js. The only reason the old file is still in the tree is because protocol.js builds the front / client from the actor's method descriptions, so talking to old servers needs these to remain. We should eventually find a nicer path than just keep large chunks of code around that we never run... [1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/styleeditor-panel.js#62

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40) > > So I am not really sure what I will do. I have been looking into this a bit > > this week; though I don't really understand some of what I see, for example > > why we have two kinds of style sheet actor. > > It's there for compatibility purposes[1]. IIRC, actors/styleeditor.js is > the old one. It really needs a nice ASCII warning at the top saying "I am > old! Go away!". The new one is actors/stylesheets.js. > > The only reason the old file is still in the tree is because protocol.js > builds the front / client from the actor's method descriptions, so talking > to old servers needs these to remain. We should eventually find a nicer > path than just keep large chunks of code around that we never run... This is right - stylesheets.js is the new one and the only one that would need to be changed to support authored styles.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40) > (In reply to Tom Tromey :tromey from comment #39) > > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #38) > > > > > Currently, the rule view receives rule actors that represent CSSOM rules > > > (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still > > > converse over the RDP in terms of rules, or would it pull the text from the > > > server's resource cache and parse things as needed in the tool's front end? > > > > Good question. The patch currently attached to this bug tries to do the > > work on the server side. However, this can't fully work, I think, because > > the client side also needs some tweaks to deal with some scenarios. > > I'm hoping Patrick and/or Brian can help flesh out the details here. I > haven't spent enough time in these tools to know the edge cases well enough. I assumed we would need to coordinate some frontend changes with the server side changes to ship this feature. We will have to address backwards compatibility, but we should be able to do that if we plan for it ahead of time. Any specific scenarios you have already thought of regarding how the client side will need to change? I agree that most of the work should still be done on the server - otherwise it seems like there would be too many cases where we end up wanting to access the content to read CSSOM or get some information from the parser and we'd have a bunch of one-off calls to the server while needing to worry that everything is staying in sync.

(In reply to Brian Grinstead [:bgrins] from comment #42) > I assumed we would need to coordinate some frontend changes with the server > side changes to ship this feature. We will have to address backwards > compatibility, but we should be able to do that if we plan for it ahead of > time. Any specific scenarios you have already thought of regarding how the > client side will need to change? The one case I know of is here: https://bugzilla.mozilla.org/attachment.cgi?id=8632254&action=diff#a/browser/devtools/styleinspector/rule-view.js_sec3 I don't recall offhand why this is needed -- it was in Patrick's patch, and I know I investigated it myself as well (to assure myself it was still needed) but I don't remember the outcome. This may be sufficiently obscure that we don't need to worry about backward compatibility with old clients. But if we do, I suppose we could do something like add a new request to put the rule and/or page style actor into "authored mode". > Currently, the rule view receives rule actors that represent CSSOM rules > (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still > converse over the RDP in terms of rules, or would it pull the text from the > server's resource cache and parse things as needed in the tool's front end? After looking into this more yesterday, my tentative plan is to continue to use the rule actors -- but change StyleRuleActor to operate on the style sheet actor. I think to make this work properly I'll need to change StyleRuleActor.modifyProperties to accept more kinds of objects. This is needed because the current vocabulary here is insufficient to express all the kinds of edits we would like to do. I am not totally sure this approach has all the desired properties. For example when editing the style sheet, the line numbers of all the subsequent rules will change. So it would be good for the rule view to notice this and update all the links. So perhaps some additional event and refresh will be needed.

(In reply to Tom Tromey :tromey from comment #43) > (In reply to Brian Grinstead [:bgrins] from comment #42) > > > I assumed we would need to coordinate some frontend changes with the server > > side changes to ship this feature. We will have to address backwards > > compatibility, but we should be able to do that if we plan for it ahead of > > time. Any specific scenarios you have already thought of regarding how the > > client side will need to change? > > The one case I know of is here: > > https://bugzilla.mozilla.org/attachment.cgi?id=8632254&action=diff#a/browser/ > devtools/styleinspector/rule-view.js_sec3 > > I don't recall offhand why this is needed -- it was in Patrick's patch, and > I know > I investigated it myself as well (to assure myself it was still needed) but > I don't > remember the outcome. > > This may be sufficiently obscure that we don't need to worry about backward > compatibility > with old clients. But if we do, I suppose we could do something like add a > new request > to put the rule and/or page style actor into "authored mode". Looking at this change, I don't think that would break backwards compat (or would be trivial to fix if so). > > Currently, the rule view receives rule actors that represent CSSOM rules > > (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still > > converse over the RDP in terms of rules, or would it pull the text from the > > server's resource cache and parse things as needed in the tool's front end? > > After looking into this more yesterday, my tentative plan is to continue to > use > the rule actors -- but change StyleRuleActor to operate on the style sheet > actor. Can you elaborate on this part? FWIW the StyleRuleActor does have a way to reference a StyleSheetActor already (via this.pageStyle._sheetRef). > I think to make this work properly I'll need to change > StyleRuleActor.modifyProperties > to accept more kinds of objects. This is needed because the current > vocabulary here > is insufficient to express all the kinds of edits we would like to do. This is actually more along the lines of the backwards-compat issues that might take some more work to handle. The frontend will be passing along these new inputs, and we need to make sure that the client still passes along the older inputs when talking to an older server. Sometimes this could be handled directly in the Front with a custom method, or sometimes the frontend may use a trait to completely hide a feature. It depends on the case, but I'm sure we will be able to work something out as we discover the particulars here. > I am not totally sure this approach has all the desired properties. For > example when > editing the style sheet, the line numbers of all the subsequent rules will > change. > So it would be good for the rule view to notice this and update all the > links. So > perhaps some additional event and refresh will be needed. I can think of an easy workaround for this particular case since the editing will be happening on another toolbox tab - whenever the inspector tab becomes visible, just do a full rule view refresh. But in the general case, we could send an unsolicited event to the frontend if one of the rules on the selected node for the client will be modified for some reason.

(In reply to Brian Grinstead [:bgrins] from comment #44) > > After looking into this more yesterday, my tentative plan is to continue to > > use > > the rule actors -- but change StyleRuleActor to operate on the style sheet > > actor. > > Can you elaborate on this part? FWIW the StyleRuleActor does have a way to > reference a StyleSheetActor already (via this.pageStyle._sheetRef). Yeah. What I am thinking is that StyleRuleActor.modifyProperties will write changes back to the StyleSheetActor; and in particular will work by re-evaluating the entire style sheet rather than editing the various rules piecemeal. Currently, modifyProperties only supports "set" and "delete" operations for a property; and rule-view.js sends all off the rule's properties on any change. My plan here is to instead send just the change that is needed, in a way that is useful for rewriting the CSS text. This requires adding a few cases. For example, we will want to represent disabling a property by commenting out the property in the text; and conversely, try to parse CSS comment bodies as properties to extract disabled properties. > along the older inputs when talking to an older server. Sometimes this > could be handled directly in the Front with a custom method, or sometimes > the frontend may use a trait to completely hide a feature. jryans taught me about traits today on irc and I think a new trait on the rule actor will be just what is needed.

Here's my current patch series. It is still incomplete but I thought I'd attach it as I have some PTO this week, and Brian was curious about it yesterday. To see it in action, apply the series, then open the inspector. Disable some property. Then, switch to the style editor and you will see that the property has been commented out. The old series started by moving css-parsing-utils and friends to toolkit. It seemed to me that this wasn't really needed any more, so I dropped that bit. The CSS text rewriting is all done on the client. I think this is necessary because parsing CSS comments necessarily involves some heuristics, and having all the decision-making be on the client means that we can't have bugs where the client and server heuristics get out of sync. There are various FIXME comments scattered around wherever I think something still needs attention. I'll fix all of these before review. Some of the bigger problems are: * The RuleModificationList API is now a total mess and has to be cleaned up. This change also breaks layoutview/view.js. * Edits in the style editor aren't reflected in the rule view. This may require resurrecting the patch in bug 947119. * None of the CSSOM plans have been implemented. * In the rule view, the disabled checkbox and the strikethrough now have two meanings: they might mean that the property is commented out in the source text, or they might mean that the property value is disabled. Maybe we can separate the meanings here, so that an invalid property can have the strikethrough but the checkbox will still be checked. Or, maybe we can have more smarts in the CSS rewriter. * If you add a duplicate property in the rule view, it will appear to be disabled, leading to confusion and bugs. I think there's a FIXME for this one. * Other editing changes are needed; for example StyleRuleActor.modifySelector2 needs to work by editing the CSS text. * I haven't run the test suite yet.

A couple other things I've noticed. * Because getAppliedProps is now async, various things using it broke. So something has to change; I am thinking perhaps a new explicit method to fetch the StyleRuleActor's authored text. * Setting css properties of an element works but these aren't "as-authored". It seems that this could be done by setting and getting the text from the element's style attribute.

(In reply to Tom Tromey :tromey from comment #54) > * Setting css properties of an element works but these aren't "as-authored". > It seems that this could be done by setting and getting the text from > the element's style attribute. Never mind, I see this is computed, not stored. So I think we'll have to just let that one remain semi-computed.

Here's a new version of the series. It fixes many of the to-dos mentioned earlier and a number of other bugs besides. If you want to try this, apply the patch for bug 1191024 first; and maybe bug 1187409 as well (or else don't touch CSS variables). Highlights from the new to-do list: * CSSOM changes. These are my lowest priority given earlier comments. * Bug 947119 or equivalent. * We also need something vaguely similar so that changes to the authored text cause property events to be generated by the StyleRuleActor, so that the rule view can update the line numbers it displays. * Write tests. * Maybe make adding new properties to a rule try to respect existing indentation. * Not sure if stringifyRule needs an update. We could have it instead return the authored text if that is desirable. * Check whether the line numbers reported by StyleRuleActor and displayed in the rule view ought to be run through the source map; if they aren't already. This is a bit weird because edits necessarily only ever happen on the "used" source (not sure if we have a term for this) -- not the original source. * Fix up the evil hack in styles.js involving rewriteDeclarations; maybe by moving RuleModificationList out of toolkit. There are some oddities as well. * The comment-parsing heuristic can fail in some round-trip situations. It rejects invalid property names in comments; so commenting-out such an invalid property, then visiting a different element, and then coming back will result in the property being completely gone. This one can maybe be worked around by having the rule view always write comments in some stylized way to bypass the parsing. * Currently there is no protection against really messing up the style sheet, say by adding a property with an unbalanced "}" in the value. This same thing can come up when deciding to uncomment a badly-formed commented-out property. I think judicious quoting in weird values can handle this.

One more thing for the to-do list is that we still need some way to stash the possibly-modified "authored" text somewhere; otherwise closing the tools and then reopening them will result in the rule view showing something at odds with the observed styles.

Here are the editing corner cases I know of. 1. CSSOM. Visit a page with a style sheet, then edit the style sheet using CSSOM. Then open the rule view. With as-authored, the rule view will display things that don't correspond to the displayed styling. A simple way is to just delete a rule: document.styleSheets[0].deleteRule(0) 2. Edit a property and introduce a closing brace. Handled naively this will terminate the enclosing rule and lead to chaos. 3. Other tricky cases here may include editing a property to have an unterminated string or URL. If the original CSS text is on a single line this could cause all the remaining rules to disappear. The authored patch series also uses a heuristic to parse comment text to see if it looks like a proper declaration. This lets the user disable a property and still have round-tripping work. Comment parsing looks for declarations like "name: value;", where "name" must be a valid property name, and where the ":" and ";" must be present. Either of these two conditions can be exploited to show weird behavior in round-trips. If you have a style sheet like: body { whatever: red; } then this will show up in the rule view as "whatever: red" with a line through it. If you then disable this the text will read: body { /* whatever: red; */ } Now if you close and reopen the toolbox, the comment-parsing heuristic will not consider this as a property, because "whatever" is not a valid property name. The case for a missing ";" or ":" is similar. The heuristic could be relaxed, of course, though there's the danger of over-relaxing it and treated too many comments as possible properties. This would yield a weird user experience. Also the ";" rule has a scenario where a naive uncommenting would result in an error, namely: body { /* color: red */ border-width: 85px; } Here uncommenting also requires the insertion of a ";" to avoid introducing an error.

I remembered later that the semicolon-insertion problem can be seen even without the commenting heuristic. If you add a declaration after a declaration that does not have a ";", then one must be inserted. Similarly in a pathological case like body { something: url(xyz I think both the ")" and ";" must be inserted.

(In reply to Tom Tromey :tromey from comment #69) > Here are the editing corner cases I know of. > > 1. CSSOM. Visit a page with a style sheet, then edit the style sheet > using CSSOM. Then open the rule view. > With as-authored, the rule view will display things that don't correspond > to the displayed styling. > A simple way is to just delete a rule: > document.styleSheets[0].deleteRule(0) Canary: no change when the rule is deleted, clicking away and then back on the node the rule is applied to, the rule is no longer there. Webkit: similar to Canary. resources css text is not updated a all, rule is only removed if you click away then back again. > 2. Edit a property and introduce a closing brace. > Handled naively this will terminate the enclosing rule and lead to chaos. Canary discards any non-valid characters in property names or values, e.g. adding a } to a property name or value then hitting enter discards *all* changes. This is really good. Webkit completely freaks out, rules disappear, it's pretty bad. Probably a bug. > 3. Other tricky cases here may include editing a property to have > an unterminated string or URL. If the original CSS text is on a single > line > this could cause all the remaining rules to disappear. Webkit handles this really badly: - code: https://www.evernote.com/l/AAGs5igrD-VG9L3ID9_tOFQPOEmnTJGjRMA - ruleview: https://www.evernote.com/l/AAH7K8ju9mFKYrucgSehJVBEbmbAickwSQI Canary: same code, *all* the styles for body disappear: https://www.evernote.com/l/AAGOsLuwZ8lLyLl_j8qo7-yn-__ArdTaldI > The authored patch series also uses a heuristic to parse comment text to see > if it looks like a proper declaration. This lets the user disable a property > and still have round-tripping work. > > Comment parsing looks for declarations like "name: value;", where "name" must > be a valid property name, and where the ":" and ";" must be present. Either > of these > two conditions can be exploited to show weird behavior in round-trips. > > If you have a style sheet like: > > body { > whatever: red; > } > > then this will show up in the rule view as "whatever: red" with a line > through it. > If you then disable this the text will read: > > body { > /* whatever: red; */ > } > > Now if you close and reopen the toolbox, the comment-parsing heuristic will > not consider > this as a property, because "whatever" is not a valid property name. Canary: this is pretty weird, what they do is this: 1. if the above rule without comment is in the source, it’s struck out with a warning yellow triangle icon to the left of it. There is no way to disable the rule there, see https://www.evernote.com/l/AAFnA8EcCQNKoJYCe38lpj8ToSwp6RO5Rhc 2. if I manually comment out the property property / value in the sources pane, then look again at the rule in the elements pane, the property property / value is struck out, but instead of the yellow triangle there is a checkbox that you can then use to disable the rule, see https://www.evernote.com/l/AAG2BbSCJ4hLSpbAPaCYnbmTYRTYnOWWWCs Webkit: invalid lines are ‘struck out’ https://www.evernote.com/l/AAEmHSBq9YlO8ZjKJgRLrj2h3nivWxvOipA commented lines are visually shown as comments in the rule view, this is pretty nice: https://www.evernote.com/l/AAEmHSBq9YlO8ZjKJgRLrj2h3nivWxvOipA > The case for a missing ";" or ":" is similar. > > The heuristic could be relaxed, of course, though there's the danger of > over-relaxing it > and treated too many comments as possible properties. This would yield a > weird user > experience. > > Also the ";" rule has a scenario where a naive uncommenting would result in > an error, > namely: > > body { > /* color: red */ > border-width: 85px; > } > > Here uncommenting also requires the insertion of a ";" to avoid introducing > an error. Canary: in this case one of two things happens: 1. if the missing semi-colon is on the last line of the block, it is effectively ‘auto-inserted’ and tolerated as if the semi-colon existed. - this code: https://www.evernote.com/l/AAE2b3QwAVNCyLRWA5RE-p02vzC3N4vWoQo - produces this view: https://www.evernote.com/l/AAGJ47w0ZoRHBasAzRk9hrSg9wOHh-Q3048 2. If the line missing the semi-colon has additional property/value pairs in subsequent lines, they all get lumped into the same line and invalidated: - this code: https://www.evernote.com/l/AAHL8TwdEXFC7L5FeyYEjwBJjZdVp-UDspM - this view in sources: https://www.evernote.com/l/AAHrtOvFAAhMXrhbTYjyJBnP6T8YmpPck9E Webkit 1. if the missing semi-colon is the last line, the behaviour is the same as chrome's 2. if there is a missing semi-colon between lines, the rule view tries to parse out what the lines probably are, e.g. https://www.evernote.com/l/AAEmHSBq9YlO8ZjKJgRLrj2h3nivWxvOipA - I bet you could abuse this in strange ways. 3. a commented line https://www.evernote.com/l/AAGmOhMeFXtKKLZtiA0920kpJzOExFf5Ebw - the rule view gives us the styles comment look, which is quite nice 4. once we un-comment the line without a semi-colon: - https://www.evernote.com/l/AAEy-nSvTL5BbLB9te7xxUVpUxN7n1a9z2k 5. Webkit also does some attempts to do smart semi-colon insertion, see https://www.dropbox.com/s/4a4zjj7v6i76ur6/webkit-ruleview-rewriting.gif?dl=0 when I comment out the second line, the first line gains a semi-colon. I can then enable both lines,, and the first line works! Notice also the not-great re-formatting of the css code once I switch back to the resource pane. Conclusion: there's a mix of good approaches on both side and what looks like some pretty serious bugs. My key concern for the front-end of this work is that when we think there is a problem with the CSS, we let the user know visually. A specific example is how chrome behaves when missing a semi-colon between two lines - they dump everything into a single line ( which looks weird enough ) and then strike it out and put a yellow warning triangle icon to the left of it. This tells me as a developer 'something is not right here'. It's not smart enough to tell you what, but it's better than webkits bizarre mutli-line semi-colon insertion parsing trick.

(In reply to Tom Tromey :tromey from comment #70) > I remembered later that the semicolon-insertion problem can be seen even > without the commenting heuristic. If you add a declaration after a > declaration > that does not have a ";", then one must be inserted. > Similarly in a pathological case like body { something: url(xyz > I think both the ")" and ";" must be inserted. Again, I mostly like chrome's approach - you can easily shoot yourself in the foot in the sources panel but it's very difficult to do so in the ruleview, for property / value fields they seem to have strict requirements for what's valid input - it would be worth looking at their source for this stuff.

(In reply to Jeff Griffiths (:canuckistani) from comment #71) Thanks for doing this analysis. [ CSSOM ] > Canary: no change when the rule is deleted, clicking away and then back on > the node the rule is applied to, the rule is no longer there. > > Webkit: similar to Canary. resources css text is not updated a all, rule is > only removed if you click away then back again. I think our plan of making CSSOM the lowest priority makes sense in light of this. > > 2. Edit a property and introduce a closing brace. > > Handled naively this will terminate the enclosing rule and lead to chaos. > > Canary discards any non-valid characters in property names or values, e.g. > adding a } to a property name or value then hitting enter discards *all* > changes. This is really good. I filed bug 1195398 for this. > > 3. Other tricky cases here may include editing a property to have > > an unterminated string or URL. If the original CSS text is on a single > > line > > this could cause all the remaining rules to disappear. > > > Webkit handles this really badly: > - code: https://www.evernote.com/l/AAGs5igrD-VG9L3ID9_tOFQPOEmnTJGjRMA > - ruleview: https://www.evernote.com/l/AAH7K8ju9mFKYrucgSehJVBEbmbAickwSQI > > Canary: same code, *all* the styles for body disappear: > https://www.evernote.com/l/AAGOsLuwZ8lLyLl_j8qo7-yn-__ArdTaldI I wasn't clear enough here, sorry. There are two bad "unterminated" cases. One is when the user uses the rule view to edit a correct thing like: body { content: "hi"; } to be invalid, say by removing the trailing quote. I think bug 1195398 may cover this sufficiently. Another case is that CSS automatically supplies terminators at the end of a style sheet. So, I believe, this is valid: body { content: "hi ...[EOF here] In the latter situation the "bad edit" is if you then try to add a rule or property or something after the weird-but-ostensibly-valid property. It seems to me that it would be great if the editor went ahead and terminated the object (string or URL) for you; but at least giving some kind of error and not completely failing would be good. I think the worst case is an unterminated string-like URL: body { something: url("http://... here the editor would have to add a closing quote, closing paren, and semicolon; and if adding a new rule, also a closing brace.

Here is a new version of the patches. I've broken out all the "pretty much ready" infrastructure patches into separate bugs now, so they can be reviewed and perhaps landed piecemeal. Rather than fix bug 922146, I have gone the simpler route of having the style inspector notice changes made by the style editor. My reasoning is that we have lived without the full functionality thus far; this was simpler; and the series is big enough already. The patches are broken down for easier review, though in some cases they don't fully make sense in isolation. Before landing I would probably squash some or all of them. What remains today: * Write more tests * Make rule edits respect indentation in some reasonable way * Deal with the editing corner cases already identified in this bug * File a bug for CSSOM, to be dealt with later * There is a bug where adding a new rule causes some incorrect behavior in the rule view

The need for this patch isn't totally obvious. Previously, Rule._getTextProperties dropped non-inherited rules; but this would make edits to the authored text apply to the wrong locations, because the edits are basically done using the index of the property in the rule. So, now we keep all properties and mark some as "invisible"; this makes the indices always be correct.

This one can't really be separated from the initial "as authored" patch, as that changed RuleModificationList, which this patch uses. However it was a pain to rebase this earlier in the series; and I will probably just squash most of these patches at push time anyhow.

When the rule text is edited, in theory some other rule's line number can change. It's hard to observe this right now because the editor doesn't insert newlines. But, once we handle newlines and indentation in rewriteDeclarations, we'll need this patch as well.

Here's the rolled-up patch. You can apply it to a very recent fx-team -- you need the fix for bug 1195978.

(In reply to Tom Tromey :tromey from comment #84) > Created attachment 8649481 [details] [diff] [review] > rolled-up patch > > Here's the rolled-up patch. You can apply it to a very recent fx-team > -- you need the fix for bug 1195978. Sigh, I mean bug 1013814

Thanks for the rolled-up patch Tom. I've applied it locally and tested it a bit. First of all, great work, this is really awesome! Secondly, I've done a bit of QA, and found a few bugs that I'll list below. I don't think many should block landing the feature because that's something we didn't have before anyway. - This first one is probably the only one that should block landing though: editing the style-editor result in a refresh that scrolls back up and positions the carets at 0:0 It's particularly frustrating if I'm a slow typer as the editor might refresh before I'm done typing my code. I understand that the refresh is probably there for when you change the stylesheet from the rule-view, but it should not happen if you're making the change from the style-editor. Also, even if you make the change from the rule-view, the scroll and caret positions should be preserved. - Minor: declarations added in the rule-view aren't indented properly in the style-editor (but I think that's already part of your todo list). - I managed to get the following error when enabling/disabling and commenting/uncommenting a declaration in the rule-view and style-editor respectively: TypeError: decl.commentOffsets is undefined css-parsing-utils.js:251:1 But of course, I couldn't get a clear set of STR after that. - I found a weird edge case: Enter the following in the style-editor body { content: "te*/st"; } Go to the rule view and disable the property. This comments it out in the style-editor, and escapes the content string "test*\/st" which I think is fine. Then go back to the rule-view and enable the property again. This happens in the style-editor: body { content: "te*\/st" /*; */ } - Invalid properties in @keyframes rules aren't shown as invalid. @keyframes animation { 0% { -webkit-transform: translateX(-50%) rotate(0deg); transform: translateX(-50%) rotate(0deg); } } In this example, the -webkit-transform property has the expected warning sign next to it, but doesn't have the strikethrough text decoration that properties in other rules do. - With the same example, if I disable the -webkit-transform property in the rule-view, it ends up being commented out in the style-editor, but if I then re-enable it in the rule-view, it stays commented out in the style-editor and the following error is logged: TypeError: decl.commentOffsets is undefined css-parsing-utils.js:251:1 - In the style-editor, select any stylesheet, delete all its content, wait for a second -> the content is back! (but the corresponding rules are removed correctly from the page, I can see the style change as I expected it to). Ok, that's all for now. There are other problems (we discussed about on IRC) when the page uses the CSS Object Model to change styles, but we agreed to look at those later (for reference, if you file a bug for it, this page is a good example: https://bgrins.github.io/devtools-demos/inspector/authored-styles.html )

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #86) > - With the same example, if I disable the -webkit-transform property in the > rule-view, it ends up being commented out in the style-editor, but if I then > re-enable it in the rule-view, it stays commented out in the style-editor > and the following error is logged: > TypeError: decl.commentOffsets is undefined css-parsing-utils.js:251:1 This happens due to the comment parsing heuristic. I failed to anticipate that the re-parsing of the text in rewriteDeclarations would mess things up -- but it does. This requires some re-thinking. The fundamental issue for the heuristic is that we don't want to parse any old comment as a possible declaration; but at the same time users can write wildly invalid CSS that will show up as just the kind of text that we ought to reject. One idea might be to use a special syntax when writing our own comments. The parser could recognize this syntax and change the heuristic to accept otherwise invalid declarations. Another idea would be to keep track of all the property names we've seen, valid or not, in a given rule, and then use this list to inform the heuristic. One more approach could be to keep the results of parseDeclarations around, attached to the TextProperty objects. However, this would require tricky updating of these objects when editing, and I would much rather avoid even more hair in this area.

(In reply to Tom Tromey :tromey from comment #87) > One idea might be to use a special syntax when writing our own comments. > The parser could recognize this syntax and change the heuristic to accept > otherwise invalid declarations. This might be the best I think. Just to be clear, you mean ignoring comments unless they start with a special syntax like /*! property: value; */

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #88) > (In reply to Tom Tromey :tromey from comment #87) > > One idea might be to use a special syntax when writing our own comments. > > The parser could recognize this syntax and change the heuristic to accept > > otherwise invalid declarations. > This might be the best I think. Just to be clear, you mean ignoring comments > unless they start with a special syntax like /*! property: value; */ I was thinking of applying the heuristic to normal comments, and relaxing it for the specially-formatted comments. This way if the user has commented-out properties, we might still detect them; but this would still let us avoid bugs like the one you ran into here.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #86) > - In the style-editor, select any stylesheet, delete all its content, wait > for a second -> the content is back! This turned out to be a latent bug in stylesheets.js: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/stylesheets.js#596 This test fails when this.text === "". I've fixed this locally.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #86) > - Invalid properties in @keyframes rules aren't shown as invalid. > @keyframes animation { > 0% { > -webkit-transform: translateX(-50%) rotate(0deg); > transform: translateX(-50%) rotate(0deg); > } > } > In this example, the -webkit-transform property has the expected warning > sign next to it, but doesn't have the strikethrough text decoration that > properties in other rules do. I looked into this briefly. Keyframe rules are skipped here: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#334 Fixing this properly -- by letting the server decide whether a given property is valid or not -- is going to require a bit more work. I think at least a new actor method and some more round trips will be needed.

v2 of the rolled-up patch. This fixes a lot of bugs.

My current to-do list: * Add a new method to determine property name validity, then use this in rule-view to filter keyframe properties * A change in the rule view can make another rule's line number change. This should propagate back to the rule view. There's a patch for this but I have to investigate why it doesn't work. * Bug 1196431, generalizing the indentation detector * Write more tests. * Run all the tests and fix up problems.

(In reply to Tom Tromey :tromey from comment #92) > Created attachment 8651920 [details] [diff] [review] > rolled-up patch > > v2 of the rolled-up patch. This fixes a lot of bugs. I've done a quick round of tests with this new version. Here are some comments about it: - create a new rule in the rule-view, add some properties, switch to the style-editor: .primary.go{ color:red; background:yellow; margin:2em; } I think the code that writes rules could introduce some formatting spaces. Something like: .primary.go { color: red; background: yellow; margin: 2em; } - not related to this patch, filed bug 1198299, but mentioning it here still since this project involves refreshing the style-editor on rule-view changes: Open the inspector first (make sure the style-editor wasn't opened first), add a rule, switch to the style-editor: a new inline stylesheet is listed and shows the rule. Now, if you do the opposite: open the style-editor first, then add the rule, then back to the style-editor, the new style-sheet doesn't appear. - not sure if related: - go to https://www.mozilla.org/en-US/ - open the inspector and select <body> - notice that it has 2 font-size declarations - change the selector to body[class] and submit - on submit, somehow, the rule changes and one of the font-sizes disappears - in the style-editor, it's still there, and if you make any changes in the text there, then the font-size declaration re-appears in the rule-view.

Even more bug fixes. This one fixes all the bugs I know of, except for the keyframe bug mentioned in comment #86 and comment #91. FYI the font-size bug mentioned in comment #94 was caused by StyleRuleActor.modifySelector2 not causing the new rule to compute its authored text. (Oh, and I just saw I left some debugging goo in there, please ignore.)

This fixes all the known bugs. I found a simpler way to fix the keyframe problem. The next step for the series is just writing tests; and doing more testing generally.

Comment on attachment 8660063 [details] [diff] [review] fix latent bug StyleSheetActor._getText I think this one is pretty obviously a latent bug. See comment #86 and comment #90. No test for this one yet, but it is on my list.

Comment on attachment 8660078 [details] [diff] [review] special-case about:PreferenceStyleSheet The context for this patch won't make sense without the "as-authored" patch. But the basic idea is that we can't fetch the rule text for about:PreferenceStyleSheet due to the known problem with it mentioned in bug 935803 comment 37. So, update the new trait and getAuthoredCssText to recognize this. This case is triggered by one of the existing styleinspector tests.

Comment on attachment 8660075 [details] [diff] [review] preserve edited style sheets across toolbox closes This is basically an inspector-specific fix for bug 768229. It doesn't add a writeable cache as such, but instead just stores any modified style sheet text in a weak map. The map is keyed by document, so, I think, navigation should not cause erroneous reuse; and it is weak so that edits don't cause a permanent leak.

Comment on attachment 8660077 [details] [diff] [review] get correct default indentation for style edits This adds a method to the style sheet front that guesses the correct indentation for the style sheet. One oddity of this code is that it only ever tries to guess the indentation a single time. Other spots doing this seem to monitor the prefs. The failure mode would be something like clearing the whole style sheet in the editor, then editing in new text with a different indentation scheme, then adding a rule and property in the rule view, resulting in incorrect indentation. I considered this pretty obscure but it occurs to me now that I could have it watch the prefs and then unregister the pref observer in the destroy method. So, let me know. No test for this one yet.

Comment on attachment 8660063 [details] [diff] [review] fix latent bug StyleSheetActor._getText Review of attachment 8660063 [details] [diff] [review]: ----------------------------------------------------------------- The test for this would probably go in https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/mochitest unless if there's an existing test somewhere related to this that could be modified ::: toolkit/devtools/server/actors/stylesheets.js @@ +592,5 @@ > * @return {Promise} > * Promise that resolves with a string text of the stylesheet. > */ > _getText: function() { > + if (typeof (this.text) === "string") { Nit: no need to parenthesize here: typeof this.text === "string"

Comment on attachment 8660064 [details] [diff] [review] as-authored styles in the rule view This is the main patch. It has several odd bits. You may notice some Promise shenanigans in createProperty and setPropertyValue (oh, and I see now I introduced a couple of methods without documentation -- will fix, sorry). These are needed so that _applyingModifications is set by the time these methods return; many tests rely on this behavior. (And this is one area that may need a bit more work, due to some other tests.) noticeNewValue is a bit strange. It is related to value sanitization when a property value is incomplete. In this case, if we add a new property, we may need to fix up the previous property in order to keep the style sheet syntactically valid. This method lets us notice such changes (which are generated by the rewriter) and update the view. This includes some basic tests plus a bunch of test updates, mostly to account for as-authored colors. I recently found out about bug 965181, maybe this needs some synchronization with it. The actual switch to authored rewriting is buried in startModifyingProperties. Note that not all rules can be authored-edited, in particular the element rule cannot.

Comment on attachment 8660078 [details] [diff] [review] special-case about:PreferenceStyleSheet Review of attachment 8660078 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/styles.js @@ +1141,5 @@ > + // method. Special case about:PreferenceStyleSheet, as it is > + // generated on the fly and the URI is not registered with the > + // about: handler. > + // https://bugzilla.mozilla.org/show_bug.cgi?id=935803#c37 > + canSetRuleText: !!(this._parentSheet && I think it'd make sense to add a getter on the StyleRuleActor for this so we can have this weird workaround in one place. Something like this (maybe with a better name): get hasWritableParentSheet() { // Special case about:PreferenceStyleSheet, as it is generated on the // fly and the URI is not registered with the about: handler. // https://bugzilla.mozilla.org/show_bug.cgi?id=935803#c37 return this._parentSheet && this._parentSheet.href !== "about:PreferenceStyleSheet"; } Then that could be used in both places. @@ +1258,5 @@ > /** > * Return the authored form of a rule's text. This will include > * invalid and otherwise ignored properties. > */ > getAuthoredCssText: function*() { Should we also check this in setRuleText()? I'm guessing the frontend is well-behaved and doesn't call that but I think the intention between this check and the one in setRuleText is the same.

(In reply to Tom Tromey :tromey from comment #117) > Comment on attachment 8660064 [details] [diff] [review] > as-authored styles in the rule view > > This is the main patch. It has several odd bits. > > You may notice some Promise shenanigans in createProperty and > setPropertyValue Oops, and I forgot to update the "Wow" comment, haha. It turns out that some tests relied on enabling or disabling emitting the changed event, even if no change actually occurred. Hence the hack; but maybe it can be done in a better way.

Comment on attachment 8660064 [details] [diff] [review] as-authored styles in the rule view Review of attachment 8660064 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly good to me, but I'd like to see the new test use an HTML page instead, and wouldn't mind taking another look at the code changes in rule-view after you've read my comments. ::: browser/devtools/styleinspector/rule-view.js @@ +617,5 @@ > } > > + let deferred = promise.defer(); > + let modifications = this.style.startModifyingProperties(); > + modifications.getDefaultIndentation().then((indent) => { Shouldn't the modifications object deal with calling getDefaultIndentation at the right times on its own? Instead of requiring the rule-view to call it first, wait for it to resolve, and then calling createProperty passing the right indent. @@ +620,5 @@ > + let modifications = this.style.startModifyingProperties(); > + modifications.getDefaultIndentation().then((indent) => { > + modifications.createProperty(ind, name, value, priority, indent); > + this.internalApplyProperties(modifications).then(() => { > + deferred.resolve(null); this.internalApplyProperties(modifications).then(deferred.resolve); should work @@ +772,5 @@ > + let deferred = promise.defer(); > + let modifications = this.style.startModifyingProperties(); > + modifications.getDefaultIndentation().then((indent) => { > + modifications.setProperty(index, property.name, value, priority, indent); > + this.internalApplyProperties(modifications).then(() => { this.internalApplyProperties(modifications).then(deferred.resolve); should work @@ +810,5 @@ > * @param {Boolean} value > */ > setPropertyEnabled: function(property, value) { > + if (property.enabled === !!value) { > + // Wow. Wow! ::: browser/devtools/styleinspector/test/browser_computedview_cycle_color.js @@ +60,5 @@ > {type: "mousedown", shiftKey: true}, win); > is(valueNode.textContent, "red", > "Color displayed as a color name."); > > + // Back to "Authored" Shouldn't we keep the extra shift-click to make sure we cycle back to hex? ::: browser/devtools/styleinspector/test/browser_ruleview_authored.js @@ +14,5 @@ > + yield overrideTest(inspector, view); > + yield colorEditingTest(inspector, view); > +}); > + > +function* createTestContent(inspector, style) { This function makes use of CPOWs to access and modify content directly. We're trying to get rid of those. Could you please use new HTML test pages instead? This also would make the test cleaner/easier to read. You can even make 3 different HTML pages and 3 different tests out of this if that helps make the test simpler. ::: browser/devtools/styleinspector/test/browser_ruleview_cycle-color.js @@ +52,4 @@ > EventUtils.synthesizeMouseAtCenter(swatch, > {type: "mousedown", shiftKey: true}, win); > is(valueNode.textContent, "#F00", > + "Color displayed as an authored value."); Shouldn't we keep the extra shift-click to make sure we cycle back to hex? ::: toolkit/devtools/server/actors/styles.js @@ +1091,5 @@ > if (this.rawRule.parentStyleSheet) { > form.parentStyleSheet = this.pageStyle._sheetRef(this.rawRule.parentStyleSheet).actorID; > } > > + form.authoredText = this.authoredText; nit: can you insert a newline below this. Also, StyleRuleActors are becoming a bit harder to use. In order to work correctly, they not only need to be instantiated and returned like they used to, they also need to have their async getAuthoredCssText function called before form is called so that this.authoredText exists by the time form is called. It's unfortunate that protocol.js does not allow the initialize or form functions to return promises. This would avoid having to call rule.getAuthoredCssText() in getApplied, but instead each StyleRuleActor would call it itself before returning the form. We should definitely investigate making form async in protocol.js. But that's not related to your work here. So at least, could you add a comment before this line explaining that for this to work, someone has to call getAuthoredCssText before returning the actor to the client? @@ +1125,5 @@ > return form; > }, > > /** > + * Return the authored form of a rule's text. This will include This not only return the authored text, but also sets it on this. So the comment should at least say this. @@ +1128,5 @@ > /** > + * Return the authored form of a rule's text. This will include > + * invalid and otherwise ignored properties. > + */ > + getAuthoredCssText: function*() { Making this a generator function makes it possible for tasks to use yield with it, but it makes it impossible for non-task functions to call it. To maximize the possibility of being able to call this from several places, I would do either one of these 2 things: - make it a normal function, remove the yield before parentStyleSheet.getText() and instead append .then() after it. - or wrap this generator in a task too. In either case, please document this function as returning a promise. @@ +1140,5 @@ > + } > + > + let parentStyleSheet = > + this.pageStyle._sheetRef(this.rawRule.parentStyleSheet); > + let {str: cssText} = yield parentStyleSheet.getText(); My earlier point about how having an async form function would simplify the code is irrelevant if getText was sync, because this line is the only place that introduces something async in the process. I think we've already discussed how having to fetch the text again (potentially from the server) was a bad thing. Ultimately, it'd be great to get the text synchronously as it was downloaded when the page was rendered. Maybe that's what we need to work on after this project is done.

Comment on attachment 8660075 [details] [diff] [review] preserve edited style sheets across toolbox closes Review of attachment 8660075 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by review. I like this. Simple, and solves a big problem we've had. I'm happy to R+ this (but Brian may want to take a look too), but I just have a question (see below). ::: toolkit/devtools/server/actors/stylesheets.js @@ +908,5 @@ > + if (!styleMap) { > + styleMap = new Map(); > + modifiedStyleSheets.set(this.document, styleMap); > + } > + let key = this.href ? this.href : this.ownerNode; nit: Maybe move the inner map key construction to a helper method so it can be used both from update and _getText. Are we sure that multiple href-less stylesheets can't share the same ownerNode? What about @import-ed stylesheets?

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #121) > > + let key = this.href ? this.href : this.ownerNode; > > nit: Maybe move the inner map key construction to a helper method so it can > be used both from update and _getText. > Are we sure that multiple href-less stylesheets can't share the same > ownerNode? > What about @import-ed stylesheets? I am not sure; but it occurs to me that we can circumvent all these questions by making the key just be the style sheet itself.

Comment on attachment 8660069 [details] [diff] [review] add "invisible" attribute to TextProp As-authored editing relies on the index of a property in textProps being the same as its index in the result of parseDeclarations. This is how the rewriter knows what text to preserve and what text to rewrite. However, the rule view was dropping non-inherited properties from textProps. This patch changes this code to keep all properties, but to mark some properties as "invisible" -- meaning they don't show up in the UI, but do stay in textProps. This ensures the indices are consistent.

Comment on attachment 8660074 [details] [diff] [review] update rule link after changes are made With as-authored, when a rule is edited, it might affect the line number of some other rule. In particular, adding a property can insert a newline, and deleting a property can delete a newline. This patch adds a new event to the StyleRuleActor that is emitted when the line or column changes. It also updates the rule view to change the UI to reflect line number changes. This removes caching from Rule.title. This didn't seem very important to me; and caching means that we would also need to do cache invalidation. No test yet, that will come a bit later.

This version uses the style sheet itself as the key.

Comment on attachment 8660809 [details] [diff] [review] preserve edited style sheets across toolbox closes The earlier version of this was r+ by Patrick, but he wondered if you had any comments, so setting f?. This version of the patch associates the text with the style sheet objects themselves. This avoids the potential problems that Patrick pointed out in comment 121.

Comment on attachment 8660067 [details] [diff] [review] fix up rule actor to notice update events When a style sheet is changed, the rule actor needs to find out about it. This way, it can propagate changes to the rule view; and so keep the rule view and the style editor in sync. Changes done via setRuleText are known not to change the number or ordering of rules in the style sheet. And, it is more efficient in the rule view if a minor property change does not require a full UI refresh. So, we call out these kinds of changes specially, by tagging updates as either being "general" or "rule preserving". This adds a FIXME comment in _styleApplied. This is removed in a later patch. This adds a parameter to the style-applied event. I was unsure whether this required additional compatibility work.

Comment on attachment 8660809 [details] [diff] [review] preserve edited style sheets across toolbox closes Review of attachment 8660809 [details] [diff] [review]: ----------------------------------------------------------------- I have a question regarding inline style sheets. Will the call to DOMUtils.parseStyleSheet update the actual textContent of the DOM element? If so, I believe we won't want to set the cache in that case. Because if we set the cache value from a style editor update, the style editor would become out of sync if the DOM element's textContent was changed directly from the script. And if parseStyleSheet is updating the textContent anyway, no big deal to just re-fetch it from the DOM node every time. Otherwise, are we OK with ignoring changes due to setting textContent with JS on an inline sheet? That seems like an edge case so it'd be fine with me, but if so can you encode that into the test in case we decide we want to change that behavior later? Something kind of like: yield ContentTask.spawn(gBrowser.selectedBrowser, {}, function* () { let style = content.document.querySelector("style"); style.textContent = "body { color: orange; }"; }); Then close and reopen toolbox again to and confirm that the text is the 'expected' (outdated) text.

(In reply to Brian Grinstead [:bgrins] from comment #129) > I have a question regarding inline style sheets. Will the call to > DOMUtils.parseStyleSheet update the actual textContent of the DOM element? > If so, I believe we won't want to set the cache in that case. It's not obvious when looking at this patch in isolation, but inline style sheets never wind up here, because the canSetRuleText trait of a StyleRuleActor for such a sheet will be false. StyleRuleActor can only wind up calling StyleSheetActor.update when canSetRuleText is true.

(In reply to Tom Tromey :tromey from comment #130) > (In reply to Brian Grinstead [:bgrins] from comment #129) > > > I have a question regarding inline style sheets. Will the call to > > DOMUtils.parseStyleSheet update the actual textContent of the DOM element? > > If so, I believe we won't want to set the cache in that case. > > It's not obvious when looking at this patch in isolation, but inline style > sheets > never wind up here, because the canSetRuleText trait of a StyleRuleActor > for such a sheet will be false. StyleRuleActor can only wind up > calling StyleSheetActor.update when canSetRuleText is true. In that case I think the test isn't exercising this functionality as we'd hope (since sync.html uses an inline style sheet). If that's right, can you expand the test case to handle both an inline and external stylesheet (and possibly even an @imported sheet)? You could add in a link from sync.html to import.css which would handle both external and @import cases.

Fix review nit; wrote test case.

(In reply to Brian Grinstead [:bgrins] from comment #131) > (In reply to Tom Tromey :tromey from comment #130) > > (In reply to Brian Grinstead [:bgrins] from comment #129) > > > > > I have a question regarding inline style sheets. Will the call to > > > DOMUtils.parseStyleSheet update the actual textContent of the DOM element? > > > If so, I believe we won't want to set the cache in that case. For some reason I thought you were referring to an element's style here. I don't think I realized we could update a style sheet's textContent. I will look into that; my concern is whether it can be done without running into the "@import problem" all over again.

Comment on attachment 8660897 [details] [diff] [review] fix latent bug StyleSheetActor._getText This was already r+, but now I've written the test, and thought you might want to review it.

Comment on attachment 8660067 [details] [diff] [review] fix up rule actor to notice update events Review of attachment 8660067 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/styles.js @@ +1018,5 @@ > this.column = DOMUtils.getRuleColumn(this.rawRule); > + this._parentSheet = this.rawRule.parentStyleSheet; > + this._getRuleIndex(); > + let sheetActor = this.pageStyle._sheetRef(this._parentSheet); > + this._styleApplied = this._styleApplied.bind(this); Can you move this function binding to before the 'if', even if it's not useful in the 'else' branch, it's still boring boilerplate code that we don't want to have to read here. As a reader, it's easier to quickly read past it at the beginning of the initialize method. nit: maybe rename to '_onStyleApplied'. @@ +1019,5 @@ > + this._parentSheet = this.rawRule.parentStyleSheet; > + this._getRuleIndex(); > + let sheetActor = this.pageStyle._sheetRef(this._parentSheet); > + this._styleApplied = this._styleApplied.bind(this); > + sheetActor.on("style-applied", this._styleApplied); It occurred to me that there's nothing in the PageStyleActor that cleans up StyleRuleActors. The parent PageStyleActor actor just creates instances of StyleRuleActors when needed, storing them in a map (this.refMap), but never destroys them. I haven't actually check how protocol.js deals with this, but I think a parent actor's lifetime determine its child actors' lifetimes, so we might be fine. However, there's no destroy method implemented on StyleRuleActor, so this listener isn't being removed. So please add a destroy method on this actor and store sheetActor on 'this' so you can do: this.sheetActor.off("style-applied", this._styleApplied); And add some console.log to it to verify that the actors chain destruction in protocol.js does call it. @@ +1135,5 @@ > /** > + * Compute the index of this actor's raw rule in its parent style > + * sheet. > + */ > + _getRuleIndex: function() { The name of this method implies a return value is expected. Instead this sets the rule index on 'this'. Maybe change this to a getter: get ruleIndex() {...} so you can simply use this.ruleIndex wherever you need to. (optionally, if looping through rules is costly everytime, the getter could store the index on this._ruleIndex and early return it when it's already set). @@ +1158,5 @@ > + // so stop listening to events now. > + let sheetActor = this.pageStyle._sheetRef(this._parentSheet); > + sheetActor.off("style-applied", this._styleApplied); > + } else if (this._ruleIndex >= 0) { > + // The sheet was updated by this actor, in a way that preserves "by this actor"? You mean by the stylesheet actor? ::: toolkit/devtools/server/actors/stylesheets.js @@ +43,5 @@ > +// The possible kinds of style-applied events. > +// UPDATE_PRESERVING_RULES means that the update is guaranteed to > +// preserve the number and order of rules on the style sheet. > +// UPDATE_GENERAL covers any other kind of change to the style sheet. > +let UPDATE_PRESERVING_RULES = 0; You should use const instead of let here. @@ +45,5 @@ > +// preserve the number and order of rules on the style sheet. > +// UPDATE_GENERAL covers any other kind of change to the style sheet. > +let UPDATE_PRESERVING_RULES = 0; > +exports.UPDATE_PRESERVING_RULES = UPDATE_PRESERVING_RULES; > +let UPDATE_GENERAL = 1; ditto @@ +394,5 @@ > value: Arg(1, "json") > }, > "style-applied" : { > + type: "styleApplied", > + kind: Arg(0, "number") protocol.js events end up being sent to the fronts as protocol packets, and as such, I'm pretty sure their structures get verified. But you should test this to be sure. Using the devtools "Connect" screen you can test the following 2 scenarios: - use aurora and connect to your local build (although we don't really support this), - use your local build and connect to aurora (make sure you use the gcli 'listen <port>' command on the debuggee to be able to connect to it). @@ +886,2 @@ > */ > + update: method(function(text, transition, kind = UPDATE_GENERAL) { This change might also introduce compatibility issues. I see you haven't changed the request definition object though, so you're not expecting fronts to change the way they call update, so this might be fine. Still, could you test this by connecting to an older debuggee (connect to aurora for instance).

Comment on attachment 8660069 [details] [diff] [review] add "invisible" attribute to TextProp Review of attachment 8660069 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, just an important comment about the way the new test waits for changes to be handled, but no extra reviews required for this. ::: browser/devtools/styleeditor/test/browser_styleeditor_sync.js @@ +3,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ */ > +"use strict"; > + > +// Test that changes in the style inspector are synchronized into the > +// style editor. I'm fine if you do this in another patch (or even in another bug in the interest of landing the patches here quickly), but you should add another similar test that tests the opposite direction but also one more that tests that synchronization works even when the style-editor has been opened before. @@ +29,5 @@ > + > + // Disable the "font-size" property. > + let propEditor = ruleEditor.rule.textProps[0].editor; > + propEditor.enable.click(); > + yield ruleEditor.rule._applyingModifications; See bug 1166774, we want to stop using this promise to make sure changes were handled in the style-inspector. This should work: let onRuleViewChanged = view.once("ruleview-changed"); propEditor.enable.click(); yield onRuleViewChanged; But make sure there are no pending protocol requests when the test ends (there should be an error displayed in the terminal when this happens anyway). @@ +38,5 @@ > + // situation. > + ruleEditor = getRuleViewRuleEditor(view, 3); > + propEditor = ruleEditor.rule.textProps[1].editor; > + propEditor.enable.click(); > + yield ruleEditor.rule._applyingModifications; ditto. ::: browser/devtools/styleinspector/rule-view.js @@ +851,5 @@ > let store = this.elementStyle.store; > let props = parseDeclarations(this.style.authoredText, true); > for (let prop of props) { > let name = prop.name; > + let invisible = this.inherited && !domUtils.isInheritedProperty(name); It took me a second or two to remember why some properties were hidden, and then I remembered that we only showed inherited properties inside inherited rules. So maybe a comment above this line would be appreciated by future readers of this code. @@ +1068,5 @@ > + * See whether this rule has any non-invisible properties. > + * @return {Boolean} true if there is any visible property, or false > + * if all properties are invisible > + */ > + anyProperties: function() { nit: maybe rename to something like 'hasAtLeastOneVisibleProperty'. (or something shorter, but I believe the 'has' prefix does help). @@ +1094,5 @@ > * Whether the property is enabled. > + * @param {Boolean} invisible > + * Whether the property is invisible. An invisible property > + * does not show up in the UI; these are needed so that the > + * index of a property in textProps is the same as the index s/textProps/Rule.textProps

Comment on attachment 8660897 [details] [diff] [review] fix latent bug StyleSheetActor._getText Review of attachment 8660897 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/tests/mochitest/chrome.ini @@ +51,5 @@ > [test_framerate_06.html] > skip-if = buildapp == 'mulet' > [test_getProcess.html] > skip-if = buildapp == 'mulet' > +[test_getTextEmpty.html] All test names in this ini file follow this high level pattern: test_<component/module/actor>-<test-case-name>.html So I suggest renaming it: test_stylesheets-getTextEmpty.html ::: toolkit/devtools/server/tests/mochitest/test_getTextEmpty.html @@ +34,5 @@ > + send: function() { }, > + }, > + window: window, > + }; > + let actor = new StyleSheetActor(document.styleSheets[1], phonyParentActor); Tests in this directory normally act as clients to the debugger server. They start the server, and instantiate fronts and test their methods. Here you're instantiating the actor directly. This test looks like it would be better off as an xpcshell test, unless it has DOM dependencies you can't mock. In which case I'd advise doing like the other tests in this directory and instantiate a StyleSheetFront instead, and test the public getText method instead of _getText. Having said this, this test works and does test the right thing, so maybe we should just keep it this way. Brian might have ideas.

Comment on attachment 8660809 [details] [diff] [review] preserve edited style sheets across toolbox closes Review of attachment 8660809 [details] [diff] [review]: ----------------------------------------------------------------- Clearing feedback as per Comment 133