984880 - [rule view] Display styles as authored (including invalid CSS properties and unsupported stuff)

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

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.

Comment 1

[Patrick Brosset <:pbro>]

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.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug984880-ruleview-authored-styles v1.patch

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 3

[Brian Grinstead [:bgrins]]

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.

Comment 4

[Patrick Brosset <:pbro>]

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.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug984880-ruleview-authored-styles v2.patch

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.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: bug984880-ruleview-authored-styles v3.patch

v3

Comment 7

[Patrick Brosset <:pbro>]

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.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug984880-ruleview-authored-styles-SERVERSIDE v1.patch

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.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: authored-styles-rebased.patch

Rebased

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: shared-styles-WIP.patch

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

Comment 12

[Hallvord R. M. Steen [:hallvors]]

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

Comment 13

[Tom Tromey :tromey]

Attachment: rebase patch, import new css parser, and simplify a bit

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.

Comment 14

[Patrick Brosset <:pbro>]

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

Comment 15

[Tom Tromey :tromey]

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

Comment 16

[Tom Tromey :tromey]

Attachment: as-authored styles in rule view

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.

Comment 17

[Tom Tromey :tromey]

Attachment: trivia for eslint

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

Comment 18

[Tom Tromey :tromey]

Attachment: as-authored styles in rule view

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

Comment 19

[Tom Tromey :tromey]

Attachment: as-authored styles in rule view

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.

Comment 20

[Tom Tromey :tromey]

Attachment: trivia for eslint

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

Comment 21

[Brian Grinstead [:bgrins]]

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

Comment 22

[Tom Tromey :tromey]

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.

Comment 23

[Tom Tromey :tromey]

Attachment: as-authored styles in rule view

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

Comment 24

[Tom Tromey :tromey]

Attachment: trivia for eslint

Comment 25

[Tom Tromey :tromey]

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

Comment 26

[Tom Tromey :tromey]

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.

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 28

[Patrick Brosset <:pbro>]

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

Comment 29

[Tom Tromey :tromey]

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

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 31

[Hallvord R. M. Steen [:hallvors]]

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?

Comment 32

[Brian Grinstead [:bgrins]]

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

Comment 33

[Tom Tromey :tromey]

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

Comment 34

[Tom Tromey :tromey]

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

Comment 35

[Tom Tromey :tromey]

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

Comment 36

[Patrick Brosset <:pbro>]

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.

Comment 37

[Tom Tromey :tromey]

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

Comment 38

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 39

[Tom Tromey :tromey]

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

Comment 40

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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

Comment 41

[Brian Grinstead [:bgrins]]

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

Comment 42

[Brian Grinstead [:bgrins]]

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

Comment 43

[Tom Tromey :tromey]

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

Comment 44

[Brian Grinstead [:bgrins]]

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

Comment 45

[Tom Tromey :tromey]

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

Comment 46

[Tom Tromey :tromey]

Attachment: update RuleModificationList comments

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.

Comment 47

[Tom Tromey :tromey]

Attachment: fix eslint nits in test_parseDeclarations

Comment 48

[Tom Tromey :tromey]

Attachment: make css-color preserve original text; add classifyColor

Comment 49

[Tom Tromey :tromey]

Attachment: make parseDeclarations handle comments; add rewriteDeclarations

Comment 50

[Tom Tromey :tromey]

Attachment: add getRuleText

Comment 51

[Tom Tromey :tromey]

Attachment: as-authored styles in the rule view

Comment 52

[Tom Tromey :tromey]

Attachment: update style editor when stylesheet contents change

Comment 53

[Tom Tromey :tromey]

Attachment: fix up rule actor to notice update events

Comment 54

[Tom Tromey :tromey]

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.

Comment 55

[Tom Tromey :tromey]

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

Comment 56

[Tom Tromey :tromey]

Attachment: update RuleModificationList comments

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.

Comment 57

[Tom Tromey :tromey]

Attachment: fix eslint nits in test_parseDeclarations

Comment 58

[Tom Tromey :tromey]

Attachment: make css-color preserve original text; add classifyColor

Comment 59

[Tom Tromey :tromey]

Attachment: make parseDeclarations handle comments; add rewriteDeclarations

Comment 60

[Tom Tromey :tromey]

Attachment: add getRuleText

Comment 61

[Tom Tromey :tromey]

Attachment: as-authored styles in the rule view

Comment 62

[Tom Tromey :tromey]

Attachment: update style editor when stylesheet contents change

Comment 63

[Tom Tromey :tromey]

Attachment: fix up rule actor to notice update events

Comment 64

[Tom Tromey :tromey]

Attachment: edit selector in authored CSS text

Comment 65

[Tom Tromey :tromey]

Attachment: make addNewRule edit authored CSS text

Comment 66

[Tom Tromey :tromey]

Attachment: add "invisible" attribute to TextProp

Comment 67

[Tom Tromey :tromey]

Attachment: make edits to keyframe rules affect authored CSS text

Comment 68

[Tom Tromey :tromey]

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.

Comment 69

[Tom Tromey :tromey]

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.

Comment 70

[Tom Tromey :tromey]

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.

Comment 71

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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

Comment 72

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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

Comment 73

[Tom Tromey :tromey]

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

Comment 74

[Tom Tromey :tromey]

Attachment: as-authored styles in the rule view

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

Comment 75

[Tom Tromey :tromey]

Attachment: update style editor when stylesheet contents change

Comment 76

[Tom Tromey :tromey]

Attachment: fix up rule actor to notice update events

Comment 77

[Tom Tromey :tromey]

Attachment: add "invisible" attribute to TextProp

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.

Comment 78

[Tom Tromey :tromey]

Attachment: make edits to keyframe rules affect authored CSS text

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.

Comment 79

[Tom Tromey :tromey]

Attachment: edit selector in authored CSS text

Comment 80

[Tom Tromey :tromey]

Attachment: make addNewRule edit authored CSS text

Comment 81

[Tom Tromey :tromey]

Attachment: update rule link after changes are made

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.

Comment 82

[Tom Tromey :tromey]

Attachment: preserve edited style sheets across toolbox closes

Comment 83

[Tom Tromey :tromey]

Attachment: update inspector after edits in style editor

Comment 84

[Tom Tromey :tromey]

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

Comment 85

[Tom Tromey :tromey]

(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

Comment 86

[Patrick Brosset <:pbro>]

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 )

Comment 87

[Tom Tromey :tromey]

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

Comment 88

[Patrick Brosset <:pbro>]

(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; */

Comment 89

[Tom Tromey :tromey]

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

Comment 90

[Tom Tromey :tromey]

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

Comment 91

[Tom Tromey :tromey]

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

Comment 92

[Tom Tromey :tromey]

Attachment: rolled-up patch

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

Comment 93

[Tom Tromey :tromey]

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.

Comment 94

[Patrick Brosset <:pbro>]

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

Comment 96

[Tom Tromey :tromey]

Attachment: rolled-up patch

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

Comment 97

[Tom Tromey :tromey]

Attachment: rolled-up patch

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 98

[Tom Tromey :tromey]

Attachment: fix latent bug StyleSheetActor._getText

Comment 99

[Tom Tromey :tromey]

Attachment: as-authored styles in the rule view

Comment 100

[Tom Tromey :tromey]

Attachment: update style editor when stylesheet contents change

Comment 101

[Tom Tromey :tromey]

Attachment: fix up rule actor to notice update events

Comment 102

[Tom Tromey :tromey]

Attachment: add "invisible" attribute to TextProp

Comment 103

[Tom Tromey :tromey]

Attachment: make edits to keyframe rules affect authored CSS text

Comment 104

[Tom Tromey :tromey]

Attachment: edit selector in authored CSS text

Comment 105

[Tom Tromey :tromey]

Attachment: make addNewRule edit authored CSS text

Comment 106

[Tom Tromey :tromey]

Attachment: update rule link after changes are made

Comment 107

[Tom Tromey :tromey]

Attachment: preserve edited style sheets across toolbox closes

Comment 108

[Tom Tromey :tromey]

Attachment: update inspector after edits in style editor

Comment 109

[Tom Tromey :tromey]

Attachment: get correct default indentation for style edits

Comment 110

[Tom Tromey :tromey]

Attachment: special-case about:PreferenceStyleSheet

Comment 111

[Tom Tromey :tromey]

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 112

[Tom Tromey :tromey]

Attachment: rolled-up patch v5

Comment 113

[Tom Tromey :tromey]

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 114

[Tom Tromey :tromey]

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 115

[Tom Tromey :tromey]

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 116

[Brian Grinstead [:bgrins]]

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 117

[Tom Tromey :tromey]

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 118

[Brian Grinstead [:bgrins]]

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.

Comment 119

[Tom Tromey :tromey]

(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 120

[Patrick Brosset <:pbro>]

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 121

[Patrick Brosset <:pbro>]

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?

Comment 122

[Tom Tromey :tromey]

(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 123

[Tom Tromey :tromey]

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 124

[Tom Tromey :tromey]

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.

Comment 125

[Tom Tromey :tromey]

Attachment: preserve edited style sheets across toolbox closes

This version uses the style sheet itself as the key.

Comment 126

[Tom Tromey :tromey]

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

Comment 127

[Tom Tromey :tromey]

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 128

[Tom Tromey :tromey]

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 129

[Brian Grinstead [:bgrins]]

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.

Comment 130

[Tom Tromey :tromey]

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

Comment 131

[Brian Grinstead [:bgrins]]

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

Comment 132

[Tom Tromey :tromey]

Attachment: fix latent bug StyleSheetActor._getText

Fix review nit; wrote test case.

Comment 133

[Tom Tromey :tromey]

(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 134

[Tom Tromey :tromey]

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 135

[Patrick Brosset <:pbro>]

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 136

[Patrick Brosset <:pbro>]

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 137

[Patrick Brosset <:pbro>]

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 138

[Brian Grinstead [:bgrins]]

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

Comment 139

[Brian Grinstead [:bgrins]]

Comment on attachment 8660809 [details] [diff] [review]
preserve edited style sheets across toolbox closes

Review of attachment 8660809 [details] [diff] [review]:
-----------------------------------------------------------------

As mentioned in the second part of  Comment 129, I'd like to see the test case expanded.  We can figure out how to better handle the inline sheets in Bug 1205380

Comment 140

[Brian Grinstead [:bgrins]]

Comment on attachment 8660077 [details] [diff] [review]
get correct default indentation for style edits

Review of attachment 8660077 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/stylesheets.js
@@ +25,5 @@
>  
> +const {
> +  getIndentationFromPrefs,
> +  getIndentationFromString
> +} = require("devtools/toolkit/shared/indentation");

It's weird that we have this module but it's never been used before this (except for it's test).

@@ +1038,5 @@
> +      let {indentUnit, indentWithTabs} = prefIndent;
> +      return promise.resolve(indentWithTabs ? "\t" : " ".repeat(indentUnit));
> +    }
> +
> +    if (this._guessedIndentation) {

I'm not sure I get the failure mode Comment 115.  Specifically, what does it have to do with prefs?  It looks like getIndentationFromPrefs() is fetching the prefs again every time you call it, and getIndentationFromString() should also work every time (given that the string is up to date).

@@ +1043,5 @@
> +      return promise.resolve(this._guessedIndentation);
> +    }
> +
> +    return Task.spawn(function*() {
> +      let longStr = yield this.getText();

It seems like it'd be less RDP traffic to add a new method for detecting indentation from the text, although I'd like to make sure I'm understanding the failure case before suggesting any changes

Comment 141

[Tom Tromey :tromey]

(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? 

I filed bug 1205380 for this.
I think having parseStyleSheet update the text would be handy, but I don't
know if it is possible.

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

This turned out not to be really possible.  Assigning to textContent discards
the current attached style sheet and makes a new one.  So, closing and reopening
the toolbox actually does pick up the change.  It is a pre-existing
bug that the style editor doesn't notice this; I filed bug 1205408 for this.

I did add a test to check that textContent doesn't update as a sort of placeholder
for bug 1205380.

Comment 142

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #140)

> > +} = require("devtools/toolkit/shared/indentation");
> 
> It's weird that we have this module but it's never been used before this
> (except for it's test).

This was an earlier refactoring to prepare the way.
See bug 1196431.
It's used in one other place, see browser/devtools/sourceeditor/editor.js

> @@ +1038,5 @@
> > +      let {indentUnit, indentWithTabs} = prefIndent;
> > +      return promise.resolve(indentWithTabs ? "\t" : " ".repeat(indentUnit));
> > +    }
> > +
> > +    if (this._guessedIndentation) {
> 
> I'm not sure I get the failure mode Comment 115.  Specifically, what does it
> have to do with prefs?  It looks like getIndentationFromPrefs() is fetching
> the prefs again every time you call it, and getIndentationFromString()
> should also work every time (given that the string is up to date).

The new method caches the guessed indentation on the StyleSheetFront.
Once the indentation is guessed from the source text, it is never guessed again.
This can be incorrect if the source text is radically edited.

> It seems like it'd be less RDP traffic to add a new method for detecting
> indentation from the text, although I'd like to make sure I'm understanding
> the failure case before suggesting any changes

It's certainly doable, but it seemed better to me to keep this kind of heuristic
on the client side.

Comment 143

[Tom Tromey :tromey]

Attachment: special-case about:PreferenceStyleSheet

Comment 144

[Tom Tromey :tromey]

Comment on attachment 8662025 [details] [diff] [review]
special-case about:PreferenceStyleSheet

Updated per review.

You can't see it in this context since I put the infrastructure
into an earlier patch, where it made the most sense, but now
form, getAuthoredCssText, and setRuleText all use this property.

Comment 145

[Tom Tromey :tromey]

Attachment: add "invisible" attribute to TextProp

Updated per review.

Comment 146

[Tom Tromey :tromey]

Attachment: preserve edited style sheets across toolbox closes

Comment 147

[Tom Tromey :tromey]

Comment on attachment 8662038 [details] [diff] [review]
preserve edited style sheets across toolbox closes

Patrick gave this r+ earlier; but re-requesting review from you
since I've since made the asked-for test change.  Well, at least
in part, see comment #141.

Comment 148

[Tom Tromey :tromey]

Attachment: make edits to keyframe rules affect authored CSS text

Updated for the canSetRuleText property.

Comment 149

[Tom Tromey :tromey]

Comment on attachment 8662043 [details] [diff] [review]
make edits to keyframe rules affect authored CSS text

Testing revealed that one could not edit a property in a keyframe
rule.  This happened because a couple of methods failed to check
for KEYFRAME_RULE.

Comment 150

[Tom Tromey :tromey]

Comment on attachment 8660066 [details] [diff] [review]
update style editor when stylesheet contents change

This changes the style editor to notice when edits are done by
the style inspector.

No test yet, I will add one though.

Comment 151

[Tom Tromey :tromey]

Attachment: update style editor when stylesheet contents change

It turns out the test landed in the wrong patch.

Comment 152

[Tom Tromey :tromey]

Comment on attachment 8662052 [details] [diff] [review]
update style editor when stylesheet contents change

Sorry for the noise; I realized that I had written the test for this,
but it got attached to the wrong patch.

This patch changes the style editor so that it updates in response to
edits from the style inspector.

Comment 153

[Tom Tromey :tromey]

Attachment: edit selector in authored CSS text

A test case that was in this patch belonged elsewhere.

Comment 154

[Tom Tromey :tromey]

Comment on attachment 8660072 [details] [diff] [review]
make addNewRule edit authored CSS text

This makes it so that addNewRule can edit the style sheet when requested.

In this case I wasn't sure about the protocol compatibility story.
I will try this out per your earlier comments along these lines.

I haven't written the test for this one yet.

Comment 155

[Tom Tromey :tromey]

Comment on attachment 8660076 [details] [diff] [review]
update inspector after edits in style editor

This arranges for edits in the style editor to be reflected back
in the rule view.

Comment 156

[Tom Tromey :tromey]

Comment on attachment 8662053 [details] [diff] [review]
edit selector in authored CSS text

This changes the style rule actor so that editing the selector
causes the changes to be written back to the style sheet.

Again here I was not sure about the protocol compatibility.
I will try that out soon.

No test yet.

Comment 157

[Patrick Brosset <:pbro>]

Comment on attachment 8662052 [details] [diff] [review]
update style editor when stylesheet contents change

Review of attachment 8662052 [details] [diff] [review]:
-----------------------------------------------------------------

I have a few comments about this patch, but I'll R+ it anyway. The overall logic seems fine to me.
You have a whole lot of patches to get reviews for and a lot of work to do still on this project, so in the interest of landing something, my comment about putting the fetchSource and onStyleApplied code in common can be addressed in a follow-up bug if you want to.

::: browser/devtools/styleeditor/StyleSheetEditor.jsm
@@ +327,5 @@
> +  _onStyleApplied: function() {
> +    if (this._isUpdating) {
> +      // We just applied an edit in the editor, so we can drop this
> +      // notification.
> +      this._isUpdating = false;

Why set this._isUpdating to false here? The user might still be typing and updates might still be going on. Shouldn't this just be an early return?

@@ +329,5 @@
> +      // We just applied an edit in the editor, so we can drop this
> +      // notification.
> +      this._isUpdating = false;
> +    } else if (this.sourceEditor) {
> +      Task.spawn(function*() {

The Task.spawn int he middle of the function there tend to make it harder to read. Why not:

_onStyleApplied: Task.async(function*() {
  if (this._isUpdating || !this.sourceEditor) {
    return;
  }
  
  let longStr = yield this.styleSheet.getText();
  ...
})

@@ +331,5 @@
> +      this._isUpdating = false;
> +    } else if (this.sourceEditor) {
> +      Task.spawn(function*() {
> +        let longStr = yield this.styleSheet.getText();
> +        let newText = yield longStr.string();

I see that these lines (and some of the following ones) are repeated in fetchSource. Can you somehow put them in common in a third method so that there's only one code path for updating the text of a stylesheet?

@@ +520,5 @@
>      if (this.styleSheet.disabled) {
>        return;  // TODO: do we want to do this?
>      }
>  
> +    if (this._justSetText) {

When can this happen? Is it because the setTimeout used in updateStyleSheet can execute its callback just after the style got updated from the server?
If so, then I guess _onStyleApplied should first clear the timeout if there's one.

@@ +541,1 @@
>      this.styleSheet.update(this._state.text, transitionsEnabled)

Shouldn't you be reseting this._isUpdating in the .then handler here?

Comment 158

[Patrick Brosset <:pbro>]

Comment on attachment 8660076 [details] [diff] [review]
update inspector after edits in style editor

Review of attachment 8660076 [details] [diff] [review]:
-----------------------------------------------------------------

R+ assuming we agree that filtering on the front-end would be better done in a later bug (because too involved at this stage). But if you believe you can do this easily and therefore have something with better performance, then please cancel the R flag.

::: toolkit/devtools/server/actors/styles.js
@@ +169,5 @@
> +
> +  destroy: function() {
> +    for (let sheet of this._watchedSheets) {
> +      sheet.off("style-applied", this._styleApplied);
> +    }

Can you also clear the set here?

@@ +202,5 @@
> +   * Called when a style sheet is updated.
> +   */
> +  _styleApplied: function(kind) {
> +    if (kind === UPDATE_GENERAL) {
> +      events.emit(this, "stylesheet-updated");

Do we want to be careful about UI update performance here? As in, here you're sending a generic event without saying which stylesheet just got updated.
I think you could send the stylesheet actor along with this event, which would be useful for the style-inspector to detect whether it should refresh or not, depending on which node is selected.
I'm fine starting with this approach if filtering turns out to be hard on the client-side, but I'd suggest adding the parameter to the event right now, even if we don't use it in the style-inspector yet (for easier compatibility in the future).

Comment 159

[Patrick Brosset <:pbro>]

Comment on attachment 8660072 [details] [diff] [review]
make addNewRule edit authored CSS text

Review of attachment 8660072 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/styles.js
@@ +937,5 @@
> +    if (editAuthored) {
> +      let sheetActor = this._sheetRef(sheet);
> +      let {str: authoredText} = yield sheetActor.getText();
> +      authoredText += "\n" + selector + " {\n" + "}";
> +      yield sheetActor.update(authoredText, false);

I have a question about this: shouldn't we *just* update the authored text on the sheetActor and let the update process unfold from there? 
Updating the text should make the sheetActor emit an event, which the PageStyleActor listens to to, in turn, send an event to the client which will use it to refresh itself with the new rules.

@@ +1001,5 @@
>    }),
>  
>    addNewRule: protocol.custom(function(node, pseudoClasses) {
> +    let addPromise;
> +    if (this._form.traits && this._form.traits.authoredStyles) {

Nice use of an actor trait directly from the front to handle backward compatibility! I like it.

Comment 160

[Patrick Brosset <:pbro>]

Comment on attachment 8662053 [details] [diff] [review]
edit selector in authored CSS text

Review of attachment 8662053 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/styles.js
@@ +1330,2 @@
>  
> +    // We know the selector modification is ok, so if the client asked

What do you mean by "we know the selector modification is ok" ?

@@ +1447,3 @@
>  
> +    if (editAuthored) {
> +      selectorPromise = selectorPromise.then(Task.async(function*(newCssRule) {

Wrapping the handler in a Task.async call seems a bit overkill to me here, I usually use it when the code path would be hard to read otherwise (i.e. when several yields in if/for/while are needed). Also note that bind(this) isn't needed.
Maybe this would be clearer:

selectorPromise = selectorPromise.then(newCssRule => {
  return (newCssRule
          ? this.pageStyle._styleRef(newCssRule).getAuthoredCssText()
          : promise.resolve(null)).then(() => newCssRule);
});

@@ +1448,5 @@
> +    if (editAuthored) {
> +      selectorPromise = selectorPromise.then(Task.async(function*(newCssRule) {
> +        if (newCssRule) {
> +          let style = this.pageStyle._styleRef(newCssRule);
> +          yield style.getAuthoredCssText();

As commented earlier (comment 120) for another patch, getAuthoredCssText is something users of a styleRuleActor have to call unfortunately, so it would make sense to add a comment here saying why we need to call this now.

@@ +1919,5 @@
> + * @return {array} An array with two elements: [startOffset, endOffset].
> + *                 The elements mark the bounds in |initialText| of
> + *                 the CSS rule's selector.
> + */
> +function getSelectorOffsets(initialText, line, column) {

Please move this to css-parsing-utils.js, it looks enough like a useful helper function that I think it makes sense to have it in that shared module.

Comment 161

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #157)

> ::: browser/devtools/styleeditor/StyleSheetEditor.jsm
> @@ +327,5 @@
> > +  _onStyleApplied: function() {
> > +    if (this._isUpdating) {
> > +      // We just applied an edit in the editor, so we can drop this
> > +      // notification.
> > +      this._isUpdating = false;
> 
> Why set this._isUpdating to false here? The user might still be typing and
> updates might still be going on. Shouldn't this just be an early return?

The idea of this code is that an edit in the style editor should not cause
the style editor's text to be reset.  The style editor calls |update| on the
style sheet to make the user's typing take effect.  The style sheet emits
a style-applied event, which ends up in _onStyleApplied.  This flag is how
we know the edit originated from the style editor.

> > +    if (this._justSetText) {
> 
> When can this happen? Is it because the setTimeout used in updateStyleSheet
> can execute its callback just after the style got updated from the server?
> If so, then I guess _onStyleApplied should first clear the timeout if
> there's one.

This prevents an event cycle from the other origin: if the style inspector
calls |update| on the style sheet, then the style sheet emits style-applied,
ending up in _onStyleApplied.  In this case we want to set the text of the
editor.  However, setting the text causes an event which leads us into
|_updateStyleSheet| -- but we don't want to parse the style sheet yet again;
instead we want to ignore the event.

> 
> @@ +541,1 @@
> >      this.styleSheet.update(this._state.text, transitionsEnabled)
> 
> Shouldn't you be reseting this._isUpdating in the .then handler here?

It's handled via the event system; I think it has to be because the goal is
to filter events to avoid cycles.

Comment 162

[Brian Grinstead [:bgrins]]

Comment on attachment 8660897 [details] [diff] [review]
fix latent bug StyleSheetActor._getText

Review of attachment 8660897 [details] [diff] [review]:
-----------------------------------------------------------------

I can take one more look at this after the suggested test changes.  Let me know if they are unclear.  Also, can you update the commit message to be more specific?

::: 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]

Ditto

::: toolkit/devtools/server/tests/mochitest/test_getTextEmpty.html
@@ +34,5 @@
> +        send: function() { },
> +      },
> +      window: window,
> +    };
> +    let actor = new StyleSheetActor(document.styleSheets[1], phonyParentActor);

I had suggested a mochitest because mocking out the rawSheet object for the StyleSheetActor (including CSSRuleList) seemed nasty.

Sure, I think if we connected to the server and then created a StyleSheetFront that would be best for this case.  Tom, unfortunately there isn't yet an example of that for this object in this dir yet but it should be similar to test_inspector-anonymous.html, using attachURL and then creating a StyleSheetFront.

Comment 163

[Tom Tromey :tromey]

Attachment: handle cached empty string in StyleSheetActor._getText

This moves the test to .../server/tests/browser.  This turned out to
be much simpler as there are already tests of the stylesheet-related
actors there, so the infrastructure all exists.

Comment 164

[Tom Tromey :tromey]

Attachment: update style editor when stylesheet contents change

Updated per review; but also see comment #161.

Comment 165

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #160)

> > +    // We know the selector modification is ok, so if the client asked
> 
> What do you mean by "we know the selector modification is ok" ?

Earlier code already applied the selector, which would have resulted in
an exception if the selector was malformed.

> Maybe this would be clearer:
> 
> selectorPromise = selectorPromise.then(newCssRule => {
>   return (newCssRule
>           ? this.pageStyle._styleRef(newCssRule).getAuthoredCssText()
>           : promise.resolve(null)).then(() => newCssRule);
> });

Thanks, I didn't realize that returning a promise from a promise had
this behavior.

> > +function getSelectorOffsets(initialText, line, column) {
> 
> Please move this to css-parsing-utils.js, it looks enough like a useful
> helper function that I think it makes sense to have it in that shared module.

IIUC css-parsing-utils is only available on the client, because it is in
browser/devtools, not toolkit/devtools.

Comment 166

[Tom Tromey :tromey]

Attachment: edit selector in authored CSS text

Updated per review; but also see comment #165.

Comment 167

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #120)

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

I ended up keeping it in .js but changing it to avoid CPOWs.
I can redo it as html if you really prefer; but I chose to keep it as js
because colorEditingTest had a couple of cases, and it seemed like 4 html
files was too many.

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

I made this change; it requires some changes to the rule rewriter patch, see
bug 1195356.

> this.internalApplyProperties(modifications).then(deferred.resolve);
> 
> should work

This is all different again.

> > +      // Wow.
> 
> Wow!

Yeah, oops, left that in.
It turns out some tests depend on seeing a change event even when setPropertyEnabled
has nothing to do.  This was a hack to work around that.
I will try to think of a better way to accomplish this.

> ::: 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?

This particular one does -- it just hoists authored up to the top.

> ::: 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?

I think I removed this because nextColorUnit skips a color unit if the resulting
string does not change.  But I can add it back.

Comment 168

[Tom Tromey :tromey]

Attachment: as-authored styles in the rule view

Updated per review.

Comment 169

[Tom Tromey :tromey]

Comment on attachment 8663118 [details] [diff] [review]
as-authored styles in the rule view

Also see comment #167.

Comment 170

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #135)
> Comment on attachment 8660067 [details] [diff] [review]
> fix up rule actor to notice update events

> @@ +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?

No, in this case, kind is UPDATE_PRESERVING_RULES and the update
was triggered by the StyleRuleActor.


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

These both work.
> @@ +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).

This works as well.

Comment 171

[Tom Tromey :tromey]

Attachment: fix up rule actor to notice update events

Updated per review.  Also see comment #170.

Comment 172

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #159)

> I have a question about this: shouldn't we *just* update the authored text
> on the sheetActor and let the update process unfold from there? 
> Updating the text should make the sheetActor emit an event, which the
> PageStyleActor listens to to, in turn, send an event to the client which
> will use it to refresh itself with the new rules.

Good idea.
I've implemented this.

> @@ +1001,5 @@
> >    }),
> >  
> >    addNewRule: protocol.custom(function(node, pseudoClasses) {
> > +    let addPromise;
> > +    if (this._form.traits && this._form.traits.authoredStyles) {
> 
> Nice use of an actor trait directly from the front to handle backward
> compatibility! I like it.

Sadly the benefits of this are obscured by the event-handling change mentioned
above.

Comment 173

[Tom Tromey :tromey]

Attachment: make addNewRule edit authored CSS text

Updated per review; see also comment #172.

Comment 174

[Tom Tromey :tromey]

Attachment: update inspector after edits in style editor

Updated per review.  This version puts the stylesheet on the event but
doesn't try to make the rule-view itself any smarter.

Comment 175

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #135)
> Comment on attachment 8660067 [details] [diff] [review]
> fix up rule actor to notice update events

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

I made this change, but now I have reverted it.

The subtlety here is that making this a getter means that it cannot be
computed in some places, particular _onStyleApplied.  But, we want to use
it there to fetch the correct new rule from the updated stylesheet.

Instead I've renamed this to _computeRuleIndex and call it once during
initialization.

Comment 176

[Brian Grinstead [:bgrins]]

Comment on attachment 8662025 [details] [diff] [review]
special-case about:PreferenceStyleSheet

Review of attachment 8662025 [details] [diff] [review]:
-----------------------------------------------------------------

> You can't see it in this context since I put the infrastructure
> into an earlier patch, where it made the most sense, but now
> form, getAuthoredCssText, and setRuleText all use this property.

Alright, if they are all using this property it seems fine to me

Comment 177

[Brian Grinstead [:bgrins]]

(In reply to Tom Tromey :tromey from comment #142)
> (In reply to Brian Grinstead [:bgrins] from comment #140)
> 
> > > +} = require("devtools/toolkit/shared/indentation");
> > 
> > It's weird that we have this module but it's never been used before this
> > (except for it's test).
> 
> This was an earlier refactoring to prepare the way.
> See bug 1196431.
> It's used in one other place, see browser/devtools/sourceeditor/editor.js
> 
> > @@ +1038,5 @@
> > > +      let {indentUnit, indentWithTabs} = prefIndent;
> > > +      return promise.resolve(indentWithTabs ? "\t" : " ".repeat(indentUnit));
> > > +    }
> > > +
> > > +    if (this._guessedIndentation) {
> > 
> > I'm not sure I get the failure mode Comment 115.  Specifically, what does it
> > have to do with prefs?  It looks like getIndentationFromPrefs() is fetching
> > the prefs again every time you call it, and getIndentationFromString()
> > should also work every time (given that the string is up to date).
> 
> The new method caches the guessed indentation on the StyleSheetFront.
> Once the indentation is guessed from the source text, it is never guessed
> again.
> This can be incorrect if the source text is radically edited.
> 
> > It seems like it'd be less RDP traffic to add a new method for detecting
> > indentation from the text, although I'd like to make sure I'm understanding
> > the failure case before suggesting any changes
> 
> It's certainly doable, but it seemed better to me to keep this kind of
> heuristic
> on the client side.

I'm guessing the reason you aren't invalidating _guessedIndentation when the text is updated is for performance reasons? Could we create a custom 'update' method on the front that re-guesses and caches the indentation based on the text passed into it so we don't have a perf problem?  Relatedly, is the call to 'update' the only case when the style sheet text changes?

Comment 178

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c1b86d65f17

Comment 179

[Patrick Brosset <:pbro>]

Comment on attachment 8663796 [details] [diff] [review]
make addNewRule edit authored CSS text

Review of attachment 8663796 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleinspector/rule-view.js
@@ +1654,5 @@
>      if (!client.traits.addNewRule) {
>        return;
>      }
>  
> +    if (this.pageStyle.authoredStyles) {

This method just doubled in size with this new if, and as a result, it's kind of hard to follow what's going on.
This path is the new way we deal with adding rules now, right, so let's revert the condition and do something like this:

let supportsAuthoredStyles = this.pageStyle.authoredStyles;
if (!supportsAuthoredStyles) {
  // We're talking to an old server, bail out and use an earlier implementation.
  this._onAddNewRuleNonAuthored();
  return;
}

... and then the new code ...

This helps because the old code in this method is now only 2 lines long.

@@ +1661,5 @@
> +      // be updated.  So, we wait for this update and for the rule
> +      // creation request to complete, and then focus the new rule's
> +      // selector.
> +      let eventPromise = promise.defer();
> +      this.once("ruleview-refreshed", () => {

`once` already returns a promise if no callback is given:
let eventPromise = this.once("ruleview-refreshed");
So there's no need to create a new one.

::: toolkit/devtools/server/actors/styles.js
@@ +994,5 @@
>    get walker() {
>      return this.inspector.walker;
>    },
>  
> +  get authoredStyles() {

A better name could be supportsAuthoredStyles, so that it's not confused with a getter to access the style itself.

Comment 180

[Patrick Brosset <:pbro>]

Comment on attachment 8663118 [details] [diff] [review]
as-authored styles in the rule view

Review of attachment 8663118 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. Only a few minor comments, and a suggestion to rewrite the promise-based code in applyProperties

::: browser/devtools/styleinspector/rule-view.js
@@ +723,5 @@
> +    let modificationsPromise = promise.defer();
> +
> +    // If there is already a pending modification, we have to wait
> +    // until it settles before applying the next modification.
> +    let previousApplication = promise.resolve(this._applyingModifications);

Not sure I understand what is being achieved by wrapping this._applyingModifications in a promise.resolve() ?
this._applyingModifications is already a promise, you could just do
this._applyingModifications.then(() => ...) ?

@@ +747,1 @@
>      }).then(null, promiseWarn);

.catch(promiseWarn) instead of .then(null, promiseWarn)

@@ +747,4 @@
>      }).then(null, promiseWarn);
>  
> +    this._applyingModifications = resultPromise;
> +    return resultPromise;

I have the impression this method can be rewritten by chaining all promises together instead of creating intermediary objects, but I might be wrong, how about this:

applyProperties: function(modifier) {
  let onResult = this._applyingModifications.then(() => {
    let modifications = this.style.startModifyingProperties();
    modifier(modifications);
    let onApply;
    if (this.style.canSetRuleText) {
      onApply = this._applyPropertiesAuthored(modifications);
    } else {
      onApply = this._applyPropertiesNoAuthored(modifications);
    }
    return onApply;
  }).then(() => {
    this.elementStyle.markOverriddenAll();

    if (onResult === this._applyingModifications) {
       this._applyingModifications = null;
      this.elementStyle._changed();
    }
  }).catch(promiseWarn);

  this._applyingModifications = onResult;
  return onResult;
},

@@ +1219,5 @@
> +
> +  /**
> +   * See whether this property's name is known.
> +   *
> +   * @return {Boolean} true if the property name is valid, false otherwise.

s/is valid/is known

::: browser/devtools/styleinspector/test/browser_ruleview_authored.js
@@ +12,5 @@
> +  yield colorEditingTest();
> +});
> +
> +function* createTestContent(style) {
> +  let content = "<style type=\"text/css\">\n" +

Just a suggestion: getting rid of \" and \n and + in multi-line double-quoted strings can be done by using template strings:

let content = `<style type="text/css">
                 ${style}
               </style>
               <div id="testid" class="testclass">Styled Node</div>`;

::: toolkit/devtools/server/actors/styles.js
@@ +1124,5 @@
>      if (this.rawRule.parentStyleSheet) {
>        form.parentStyleSheet = this.pageStyle._sheetRef(this.rawRule.parentStyleSheet).actorID;
>      }
>  
> +    // One tricky thing here is that other methods in hits actor must

s/in hits actor/in this actor

@@ +1174,5 @@
> +   */
> +  getAuthoredCssText: function() {
> +    if (!this.canSetRuleText ||
> +        this.type !== Ci.nsIDOMCSSRule.STYLE_RULE) {
> +      return "";

It's better if the function consistently returns a promise, so here:
return promise.resolve("");

@@ +1178,5 @@
> +      return "";
> +    }
> +
> +    if (typeof this.authoredText === "string") {
> +      return this.authoredText;

It's better if the function consistently returns a promise, so here:
return promise.resolve(this.authoredText);

Comment 181

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #180)
> Comment on attachment 8663118 [details] [diff] [review]
> as-authored styles in the rule view

> > +    let previousApplication = promise.resolve(this._applyingModifications);
> 
> Not sure I understand what is being achieved by wrapping
> this._applyingModifications in a promise.resolve() ?
> this._applyingModifications is already a promise, you could just do
> this._applyingModifications.then(() => ...) ?

It can be null.

> @@ +747,1 @@
> >      }).then(null, promiseWarn);
> 
> .catch(promiseWarn) instead of .then(null, promiseWarn)
> 
> @@ +747,4 @@
> >      }).then(null, promiseWarn);
> >  
> > +    this._applyingModifications = resultPromise;
> > +    return resultPromise;
> 
> I have the impression this method can be rewritten by chaining all promises
> together instead of creating intermediary objects, but I might be wrong, how
> about this:

Yeah -- in another review you pointed out that a promise that resolves to a promise
chains.  I've updated this code to do that.

Comment 182

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #177)

> I'm guessing the reason you aren't invalidating _guessedIndentation when the
> text is updated is for performance reasons?

Yeah; it didn't seem worth it to send the whole text back over for each update.

But it occurs to me -- I changed the rewriter to lazily guess the indentation.
So maybe querying whenever it is needed would be ok now, since it shouldn't
really be that often.

I'll go ahead and make this change.

> Could we create a custom
> 'update' method on the front that re-guesses and caches the indentation
> based on the text passed into it so we don't have a perf problem? 

You mean notice locally applied changes and go from there?

I think a change coming from the style inspector should never change the
guessed indentation; or if it did, it would mean the guess was wrong in the
first place.

> Relatedly, is the call to 'update' the only case when the style sheet text
> changes?

Yes.

Comment 183

[Tom Tromey :tromey]

Attachment: as-authored styles in the rule view

Rebased for the big renaming.  Updated per reviews.  Many bugs fixed.
I put most of the test suite changes in this patch as well.

Comment 184

[Tom Tromey :tromey]

Attachment: update style editor when stylesheet contents change

Comment 185

[Tom Tromey :tromey]

Attachment: fix up rule actor to notice update events

Comment 186

[Tom Tromey :tromey]

Attachment: add "invisible" attribute to TextProp

Comment 187

[Tom Tromey :tromey]

Attachment: make edits to keyframe rules affect authored CSS text

Comment 188

[Tom Tromey :tromey]

Attachment: edit selector in authored CSS text

Comment 189

[Tom Tromey :tromey]

Attachment: make addNewRule edit authored CSS text

Comment 190

[Tom Tromey :tromey]

Attachment: update rule link after changes are made

Comment 191

[Tom Tromey :tromey]

Attachment: preserve edited style sheets across toolbox closes

Comment 192

[Tom Tromey :tromey]

Attachment: update inspector after edits in style editor

Comment 193

[Tom Tromey :tromey]

Attachment: get correct default indentation for style edits

Comment 194

[Tom Tromey :tromey]

Attachment: special-case about:PreferenceStyleSheet

Comment 195

[Tom Tromey :tromey]

Comment on attachment 8667283 [details] [diff] [review]
as-authored styles in the rule view

I think you should probably take another look at this one.
I put most of the test case updates into this patch.

One thing worth noting is browser_ruleview_add-property_01.js.
I changed the test here because I think the current behavior
is wrong -- it seems to me that an invalid value should be struck
through.

Comment 196

[Tom Tromey :tromey]

Comment on attachment 8667285 [details] [diff] [review]
update style editor when stylesheet contents change

See comment #157 and comment #161 for history.
This version of the patch adds a new test that you asked for
(in some other comment, I can dig it up if you want).

Comment 197

[Tom Tromey :tromey]

Comment on attachment 8667290 [details] [diff] [review]
edit selector in authored CSS text

I added a test and fixed another bug, namely the "mozilla.org" part
of comment #94.

Also the highlighter change seems possibly suspect.

See comment #165 for some more history.

Comment 198

[Tom Tromey :tromey]

Comment on attachment 8667291 [details] [diff] [review]
make addNewRule edit authored CSS text

Added a test case.

Comment 199

[Tom Tromey :tromey]

Comment on attachment 8667292 [details] [diff] [review]
update rule link after changes are made

Added a test case; and changed the front to clear the location cache
when a property change event is emitted.

Comment 200

[Tom Tromey :tromey]

Comment on attachment 8667293 [details] [diff] [review]
preserve edited style sheets across toolbox closes

This was r+ by Patrick but see comment #127, comment #129, comment #141.

Comment 201

[Tom Tromey :tromey]

Comment on attachment 8667295 [details] [diff] [review]
get correct default indentation for style edits

Added a test case.

Comment 202

[Tom Tromey :tromey]

Comment on attachment 8667294 [details] [diff] [review]
update inspector after edits in style editor

This was r+ but I have since added a test case.

Also, this test case can be slightly modified to point out a pre-existing bug.
If the test case instead has an empty style sheet, and then we edit a rule
into it, the new rule will not show up in the rule view, even though it applies.

This happens because the rule view only listens for events from style sheets
that already have some rule that applies to the current element.

See bug 922146 for more information on this.

Comment 203

[Patrick Brosset <:pbro>]

Comment on attachment 8667283 [details] [diff] [review]
as-authored styles in the rule view

Review of attachment 8667283 [details] [diff] [review]:
-----------------------------------------------------------------

The code changes since the last patch look good to me.

Comment 204

[Patrick Brosset <:pbro>]

Comment on attachment 8667290 [details] [diff] [review]
edit selector in authored CSS text

Review of attachment 8667290 [details] [diff] [review]:
-----------------------------------------------------------------

My previous review comments have been addressed, but I have a comment about the new test, the description doesn't seem to match what the test does.

::: devtools/client/styleinspector/test/browser_ruleview_mark_overridden_06.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +// Tests that the rule view marks overridden rules correctly based on the
> +// specificity of the rule.

This comment doesn't seem to be in line with the test. The test modifies an existing rule's selector text, right? And there's only one rule, so there's nothing related to specificity, right? This just tests that the properties are displayed correctly even after the selector was changed.

@@ +25,5 @@
> +});
> +
> +function* testMarkOverridden(inspector, view) {
> +  let elementStyle = view._elementStyle;
> +  let idRule = elementStyle.rules[1];

s/idRule/rule
The rule's selector is just 'div', not #testid

@@ +60,5 @@
> +  prop = idRule.textProps[1];
> +  is(prop.name, "background-color",
> +    "Second property should be background-color");
> +  is(prop.value, "chartreuse", "First property value should be chartreuse");
> +  ok(!prop.overridden, "prop should not be overridden.");

Can you extract the last 10 lines of code in a helper function, e.g. checkProperties(rule) ?
They are repeated twice in the test, and that makes it harder to read the flow of the test.

Comment 205

[Tom Tromey :tromey]

Attachment: edit selector in authored CSS text

Updated per review.

Comment 206

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be52f1496991

Comment 207

[Brian Grinstead [:bgrins]]

Comment on attachment 8667292 [details] [diff] [review]
update rule link after changes are made

Review of attachment 8667292 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/styleinspector/rule-view.js
@@ +251,5 @@
>  
>          // We're done with the previous list of rules.
> +        for (let r of this._refreshRules) {
> +          if (r && r.editor) {
> +            r.editor.destroy();

It looks like items from _refreshRules can be pushed into the this.rules array in maybeAddRule.  So couldn't there be a case where this is the same reference as an item already in this.rules, and we are prematurely destroying the editor?

@@ +2750,5 @@
> +   * Event handler called when a property changes on the
> +   * StyleRuleActor.
> +   */
> +  _propertyChanged: function(name) {
> +    if (name === "line") {

Don't care about column changes?

::: devtools/client/styleinspector/test/browser_ruleview_lineNumbers.js
@@ +40,5 @@
> +  info("Entering a value and bluring the field to expect a rule change");
> +  editor.input.value = "23px";
> +  let onBlur = once(editor.input, "blur");
> +  editor.input.blur();
> +  yield onBlur;

Is yielding on onBlur necessary?  Seems the editor event handlers would fire synchronously due to call to editor.input.blur()

@@ +47,5 @@
> +  yield onPropertyChange;
> +
> +  let newBodyTitle = getRuleViewLinkTextByIndex(view, 2);
> +
> +  isnot(newBodyTitle, value, "title of body rule has changed");

What do you think about being more specific with the line number assertions here?  instead of checking to just make sure that newBodyTitle != value what if we did something like this:

`ok(getRuleViewLinkTextByIndex(view, 2).endsWith(":100"), "initial line number is correct");` instead of `let value = getRuleViewLinkTextByIndex(view, 2);`

`ok(getRuleViewLinkTextByIndex(view, 2).endsWith(":101"), "changed line number is correct");` instead of ` isnot(newBodyTitle, value, "title of body rule has changed");`

True, the test will fail if the test case line numbers change, but it's easier to follow exactly what is being asserted.

Comment 208

[Brian Grinstead [:bgrins]]

Comment on attachment 8667293 [details] [diff] [review]
preserve edited style sheets across toolbox closes

Review of attachment 8667293 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.  Is there a test for this somewhere?

Comment 209

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #208)
> Comment on attachment 8667293 [details] [diff] [review]
> preserve edited style sheets across toolbox closes
> 
> Review of attachment 8667293 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.  Is there a test for this somewhere?

Thanks for noticing this.
There was a test but somehow it got dropped.
I'll resurrect it shortly.

Comment 210

[Tom Tromey :tromey]

Attachment: preserve edited style sheets across toolbox closes

Resurrect the missing test.

Comment 211

[Tom Tromey :tromey]

Comment on attachment 8668078 [details] [diff] [review]
preserve edited style sheets across toolbox closes

Now with the missing test - sorry about that!

Comment 212

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #207)

> It looks like items from _refreshRules can be pushed into the this.rules
> array in maybeAddRule.  So couldn't there be a case where this is the same
> reference as an item already in this.rules, and we are prematurely
> destroying the editor?

Thanks, I've fixed this.
I also changed _refreshRules to a parameter, not a property of "this",
because there's no reason for it to be a property.

> @@ +2750,5 @@
> > +   * Event handler called when a property changes on the
> > +   * StyleRuleActor.
> > +   */
> > +  _propertyChanged: function(name) {
> > +    if (name === "line") {
> 
> Don't care about column changes?

I don't think so, because column numbers aren't displayed.

> Is yielding on onBlur necessary?  Seems the editor event handlers would fire
> synchronously due to call to editor.input.blur()

It seemed to work ok when I removed it, so it's gone.
I just copied this from some other test.

> What do you think about being more specific with the line number assertions
> here?  instead of checking to just make sure that newBodyTitle != value what
> if we did something like this:

Good idea, I did this.

Comment 213

[Tom Tromey :tromey]

Attachment: update rule link after changes are made

Updated per review.

Comment 214

[Tom Tromey :tromey]

Attachment: update rule link after changes are made

Fix a silly typo.

Comment 215

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48b9ae9b1226

Comment 216

[Tom Tromey :tromey]

Attachment: as-authored styles in the rule view

Updated for the try push results.

I missed test_styles-modify.html earlier; it just needed the obvious
update for the API changes to RuleModificationList.

The other changes are just trivial updates for e10s; mostly fallout
from the buglet fixed in the last update to the rewriter; see
bug 1195356.

Comment 217

[Brian Grinstead [:bgrins]]

Comment on attachment 8667295 [details] [diff] [review]
get correct default indentation for style edits

Review of attachment 8667295 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/styleeditor/test/browser_styleeditor_guessIndentation.js
@@ +1,1 @@
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */

Does this test really belong in the styleinspector folder?  It looks like the only styleeditor specific part is at the very bottom, just checking to see if the text was updated with the right indentation.  I guess it doesn't matter too much either way.  But if we ever go through and simplify all the boilerplate needed for testing the inplace editor stuff we'd be more likely to catch it if it was with the other rule view tests.

@@ +6,5 @@
> +
> +// Tests that we can guess indentation from a style sheet, not just a
> +// rule.
> +
> +Services.scriptloader.loadSubScript("chrome://mochitests/content/browser/devtools/client/styleinspector/test/head.js", this);

This could use a comment explaining exactly what globals are needed from the styleinspector head.js file.  Alternatively, I'm not sure if this would work, but this wouldn't require a comment:

let styleInspectorHead = {};
Services.scriptloader.loadSubScript("chrome://mochitests/content/browser/devtools/client/styleinspector/test/head.js", styleInspectorHead);

....

let {inspector, view} = yield styleInspectorHead.openRuleView();

Comment 218

[Brian Grinstead [:bgrins]]

Comment on attachment 8668473 [details] [diff] [review]
update rule link after changes are made

Review of attachment 8668473 [details] [diff] [review]:
-----------------------------------------------------------------

We discussed this on IRC, just leaving it here for posterity

::: devtools/client/styleinspector/rule-view.js
@@ +235,5 @@
>          }
>  
>          // Store the current list of rules (if any) during the population
>          // process.  They will be reused if possible.
> +        let refreshRules = this.rules;

Nit: refreshRules -> existingRules

@@ +283,5 @@
>     *
>     * @param {Object} options
>     *        Options for creating the Rule, see the Rule constructor.
> +   * @param {Array} refreshRules
> +   *        Rules to reuse if possible

Can you put a comment that this array reference is modified by the function?

::: devtools/server/actors/styles.js
@@ +1235,5 @@
> +   * @param {any} value
> +   *        The new value
> +   */
> +  _notifyPropertyChanged: function(property, value) {
> +    events.emit(this, "property-change", property, value);

I think a single location-change event that passes along the line and column would be a bit simpler

Comment 219

[Tom Tromey :tromey]

Attachment: update rule link after changes are made

Updated per review.

Comment 220

[Tom Tromey :tromey]

Attachment: get correct default indentation for style edits

Updated per review.

Comment 221

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #217)

> Does this test really belong in the styleinspector folder? 

I moved it.

> > +Services.scriptloader.loadSubScript("chrome://mochitests/content/browser/devtools/client/styleinspector/test/head.js", this);
> 
> This could use a comment explaining exactly what globals are needed from the
> styleinspector head.js file.  Alternatively, I'm not sure if this would
> work, but this wouldn't require a comment:
> 
> let styleInspectorHead = {};
> Services.scriptloader.loadSubScript("chrome://mochitests/content/browser/
> devtools/client/styleinspector/test/head.js", styleInspectorHead);

This can't be done because head.js requires some globals which aren't found
when importing this way.

I'll add a comment.

Comment 222

[Tom Tromey :tromey]

Attachment: as-authored styles in the rule view

Now that bug 979574 landed, this patch required a small update to the
test case introduced there.

Comment 223

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba33036452e4

Comment 224

[Tom Tromey :tromey]

Attachment: as-authored styles in the rule view

The squashed patch.  This is what I sent through try yesterday.  I'm
squashing this for checkin because the patches here aren't really
independent; anything that was I put into a separate bug already.

Comment 225

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/40f709d9c088

Comment 226

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40f709d9c088

Comment 227

[Patrick Brosset <:pbro>]

Awesome work Tom!

Comment 232

[Mihai Boldan, Desktop QA [:mboldan]]

I've performed Exploratory Testing around this feature on Firefox Nightly 44.0a1 (2015-10-26) across platforms [1] and managed to confirm the following:
- the layout of the options panel is displayed on 3 columns;
- the Style Editor and the Inspector > Rules are synchronized;
- the changes made in Style Editor are not lost if closing the toolbox.

Are there some other test that need to be performed around this issue?

[1] Windows 10 x86, Ubuntu 14.04 x86, Mac OS X 10.10.5

Comment 233

[Tom Tromey :tromey]

(In reply to Mihai Boldan, QA [:mboldan] from comment #232)
> I've performed Exploratory Testing around this feature on Firefox Nightly
> 44.0a1 (2015-10-26) across platforms [1] and managed to confirm the
> following:
> - the layout of the options panel is displayed on 3 columns;
> - the Style Editor and the Inspector > Rules are synchronized;
> - the changes made in Style Editor are not lost if closing the toolbox.
> 
> Are there some other test that need to be performed around this issue?

It would also be good to verify that styles are preserved in an "authored" way.
Previously the tool sometimes displayed values in a different way than was
original written.  See bug 752881, bug 715888.

Comment 234

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: screenshot

After performing the additional testing suggested in Comment 233 with latest 44.0a1 (2015-10-27), across platforms, I noticed that the styles are correctly preserved in an 'authored' way. But, on various websites, the length unit 'px' is displayed even if the length is '0' - E.g. on the http://edition.cnn.com/ website (see the attached screenshot). Any ideas?

Comment 235

[Tom Tromey :tromey]

(In reply to Mihai Boldan, QA [:mboldan] from comment #234)
> Created attachment 8679938 [details]
> screenshot
> 
> After performing the additional testing suggested in Comment 233 with latest
> 44.0a1 (2015-10-27), across platforms, I noticed that the styles are
> correctly preserved in an 'authored' way. But, on various websites, the
> length unit 'px' is displayed even if the length is '0' - E.g. on the
> http://edition.cnn.com/ website (see the attached screenshot). Any ideas?

The screenshot seems ok to me, in that I don't see a discrepancy between
the style attribute and the rule view.  But maybe there's some other way to know
that the length was specified as 0?

In any case, style attributes are special, since they don't come from a style sheet.
That is, they aren't handled in an "authored" way, since IIUC the information simply
isn't available.  Sorry that wasn't more clear!

Comment 236

[Mihai Boldan, Desktop QA [:mboldan]]

Thanks for the clarification. According to Comment 235, I'm marking this issue as Verified - Fixed.

Comment 237

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This is a large improvement, we should mention it with a relnote.  :tromey, could you set firefox-relnote to ? and complete the comment?

Comment 238

[Tom Tromey :tromey]

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature.
[Suggested wording]:
The rule view now displays styles using their authored text,
and edits in the rule view are now linked to the style editor.
[Links (documentation, blog post, etc)]:

Comment 241

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Beta44 release notes.

Comment 242

[Will Bamberg [:wbamberg]]

I've updated https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Examine_CSS_rules and https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Edit_rules for this. It seemed like only a little edit for such a large feature.

Let me know if there's anything else I should say here.

Comment 243

[Patrick Brosset <:pbro>]

(In reply to Will Bamberg [:wbamberg] from comment #242)
> I've updated
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_CSS#Examine_CSS_rules and
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_CSS#Edit_rules for this. It seemed like only a little edit
> for such a large feature.
Yeah I know, but that was expected. I think those doc edits look fine.