Closed Bug 984880 Opened 11 years ago Closed 9 years ago

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

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox44 verified, relnote-firefox 44+)

VERIFIED FIXED
Firefox 44
Tracking Status
firefox44 --- verified
relnote-firefox --- 44+

People

(Reporter: pbro, Assigned: tromey)

References

(Blocks 5 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 97 obsolete files)

(deleted), patch
tromey
: review+
Details | Diff | Splinter Review
(deleted), image/png
Details
I worked on this during our Feb 14 devtools workweek in PDX. The goal is to show all authored styles in the rule-view panel of the inspector in the devtools, rather than only showing the styles actually parsed as we do today. This would allow to show other vendor-prefixed properties and invalid properties, and therefore help developer understand why some properties are not applied.
http://www.youtube.com/watch?v=Bve7RD5amWs Here's the demo I did during the workweek. The patches wrote for this are very much drafts that need a lot of polishing and testing.
Blocks: 878398
Attached patch bug984880-ruleview-authored-styles v1.patch (obsolete) (deleted) — Splinter Review
First attempt at having authored styles in the rule-view. Here is a list of things done: - Added a new pref "devtools.inspector.authoredStyles" to toggle authored styles from the options panel (false by default, and refreshes the rule-view when toggled). - Changed the layout of the options panel to be on 3 columns. - Moved css-tokenizer and css-parsing-utils modules to toolkit (they were in browser/devtools before) so that they can be both used on the client and the server. - Added a css rule text body parser to css-parsing-utils (+ some unit tests). - Added a new method to the RuleStyleActor to retrieve authored styles. This method gets the full stylesheet text from the parent stylesheet actor (which caches it after it's retrieved once), and then parses the rule text using css-parsing-utils, and caches it too. - Added a new option to the styles actor getApplied method to ask for retrieval of the authored css text. - Changed the rule-view to, based on the pref, ask for the authored styles. I haven't yet written any ui tests nor ran the existing ones.
Comment on attachment 8405354 [details] [diff] [review] bug984880-ruleview-authored-styles v1.patch Review of attachment 8405354 [details] [diff] [review]: ----------------------------------------------------------------- Certain CSS causes this to blow up. Here are a few examples: 1) try inspecting the main body element on jsfiddle.net 2) inspect the body element of the result on jsfiddle.net/unxra/3/. 3) inspect the body element of the result on http://jsfiddle.net/unxra/11/. At first I was sure this was related to the input[type=\"submit\"] also, but it seems to fail in different ways depending on which lines of CSS get removed.
Yeah, I've seen it fail on google.com too. I've narrowed it down to `this.line = DOMUtils.getRuleLine(this.rawRule);` in the StyleRuleActor (styles.js) which doesn't return the correct line number when css is either inline or minified (I'm not sure which of the two or both is causing it yet). Maybe I should report this in bug 974686 where I asked for a getRuleEndLine and getRuleEndColumn, because there's clearly a problem. In fact, you can see the problem even in Aurora, go to google.com, open the inspector, you'll see that the stylesheet links are all "inline:11" and clicking on the link doesn't open the editor at the right place.
Depends on: 974686
Attached patch bug984880-ruleview-authored-styles v2.patch (obsolete) (deleted) — Splinter Review
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.
Attachment #8405354 - Attachment is obsolete: true
Attached patch bug984880-ruleview-authored-styles v3.patch (obsolete) (deleted) — Splinter Review
v3
Attachment #8406745 - Attachment is obsolete: true
v3 = minor changes and fixed failing test. I want to start with a new patch that moves the whole `if(authored)` decision making on the server-side only, this way leaving the rule-view completely unchanged.
Second take on this bug. Keeps all the authoredText vs. cssText management on the server-side, therefore simplifies the patch, and reduces the nb of potential regressions in the rule-view, because the rule-view doesn't change.
Attached patch authored-styles-rebased.patch (obsolete) (deleted) — Splinter Review
Rebased
Attachment #8406854 - Attachment is obsolete: true
Attachment #8406891 - Attachment is obsolete: true
Attached patch shared-styles-WIP.patch (obsolete) (deleted) — Splinter Review
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).
Attachment #8475412 - Attachment is obsolete: true
We (as in "the web compat team") are very much looking forward to this :)
Summary: [rule view] Display styles as authored → [rule view] Display styles as authored (including invalid CSS properties and unsupported stuff)
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.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Blocks: 913661
(In reply to Tom Tromey :tromey from comment #13) > Created attachment 8573447 [details] [diff] [review] > rebase patch, import new css parser, and simplify a bit > > Brian asked me to upload my WIP patch, so here it is. Thanks Tom, that looks good. > My fork of the upstream CSS parser is here: > > https://github.com/tromey/parse-css > > There's still quite a bit left to do. At least: > > * Preserve comments in the tokenizer, and preserve whitespace better. > > * Make the tokenizer lazy. I have a branch for this in the github > repository, but this requires some changes in the firefox callers as > well. Meaning? Making the tokenizer a generator function that yields tokens, right? If so, that's great! We need this. > * Maybe change tokenizer locations to just be offsets into the input > string. > > * Actually use the new parser utilities, replacing our current > ad hoc code. (This requires a bit of investigation as I also want > to see if the existing css-autocompleter can reuse the new parser > as well.) > > * Make editing and round-tripping (through the style editor) work properly; > including bug 715888 and bug 913661.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14) > > Brian asked me to upload my WIP patch, so here it is. > Thanks Tom, that looks good. Haha, it should, you wrote a lot of it :-) > > * Make the tokenizer lazy. I have a branch for this in the github > > repository, but this requires some changes in the firefox callers as > > well. > Meaning? Making the tokenizer a generator function that yields tokens, > right? If so, that's great! We need this. Yes, exactly. This was actually very easy. I'll bring it into my local tree this week for better testing. I'm also thinking now that output-parser.js should be refactored to use the tokenizer (and maybe parser; or maybe some new parsing bits I'll write). I believe the regexps there will fail in various "authored" cases; and even right now in some situations.
Depends on: 1152033
Depends on: 1153305
Attached patch as-authored styles in rule view (obsolete) (deleted) — Splinter Review
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.
Attachment #8573447 - Attachment is obsolete: true
Attached patch trivia for eslint (obsolete) (deleted) — Splinter Review
While I'm there, clean up css-parsing-utils.js to remove some eslint warnings.
Attached patch as-authored styles in rule view (obsolete) (deleted) — Splinter Review
After a discussion on irc, I removed the Loader.jsm changes and updated the requires to follow.
Attachment #8616037 - Attachment is obsolete: true
Attached patch as-authored styles in rule view (obsolete) (deleted) — Splinter Review
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.
Attachment #8475579 - Attachment is obsolete: true
Attachment #8616041 - Attachment is obsolete: true
Attachment #8616110 - Attachment is obsolete: true
Attached patch trivia for eslint (obsolete) (deleted) — Splinter Review
This fixes some (but not all -- I skipped anything requiring any thought) eslint warnings in files touched by the previous patch.
(In reply to Tom Tromey :tromey from comment #19) > * Comments in the rule still don't show up in the rule view. > It's not clear to me if this is a requirement for this bug. I'd say no - we should handle comments in a followup.
Testing today showed a regression on browser/devtools/styleinspector/test/browser_ruleview_add-property-and-reselect.js among other things. In this case, adding a property, switching to a different element, and then reselecting the element will cause the rule view to forget the new property. I have not investigated but it seems that this is a consequence of only returning the authored style -- anything not in the original text will not be available to the rule view.
Attached patch as-authored styles in rule view (obsolete) (deleted) — Splinter Review
I added some tests, and now the color editor respects the original style.
Attachment #8630124 - Attachment is obsolete: true
Attachment #8630125 - Attachment is obsolete: true
Attached patch trivia for eslint (obsolete) (deleted) — Splinter Review
This approach has an issue that you can see this way: Apply the patch. Open the rule view and select some element. Add a new property that affects the element; e.g., I added "font-weight: bold" to something, changing the display of the text. Select a different element in the markup view. Now reselect the original element -- and notice that nothing in the rule view mentions the property that you added. This particular scenario could be addressed by dealing with round-tripping and synchronization with the style editor, see bug 878398. I am unsure if there is some scenario that can't be addressed this way. I had thought perhaps modifying an element's CSS from JS would cause problems, but this works properly (it shows up in the "element" rule).
Another difficulty here is that even if we sync the inspector and editor, we can end up in a situation where neither shows the truth. The scenario is: open the editor, make some changes, then close the toolbox. Now re-open the editor -- the changes won't be visible. (And if we make this proposed change to the rule view, they won't be visible there either; only in the computed view.) To make this work I think we'll need to stash the edited text somewhere that survives closing the toolbox.
(In reply to Tom Tromey :tromey from comment #26) > Another difficulty here is that even if we sync the inspector and editor, we > can end up in a situation where neither shows the truth. The scenario is: > open the editor, make some changes, then close the toolbox. Now re-open the > editor -- the changes won't be visible. (And if we make this proposed change > to the rule view, they won't be visible there either; only in the computed > view.) > > To make this work I think we'll need to stash the edited text somewhere that > survives > closing the toolbox. Right, we've discussed adding a "shared mutable resource cache" concept before that all tools could use, so that everyone's seeing the same version of a resource (such as a CSS file). But, that seems like work for bug 878398, so I would not hold up this bug because of this problem.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27) > (In reply to Tom Tromey :tromey from comment #26) > > Another difficulty here is that even if we sync the inspector and editor, we > > can end up in a situation where neither shows the truth. The scenario is: > > open the editor, make some changes, then close the toolbox. Now re-open the > > editor -- the changes won't be visible. (And if we make this proposed change > > to the rule view, they won't be visible there either; only in the computed > > view.) > > > > To make this work I think we'll need to stash the edited text somewhere that > > survives > > closing the toolbox. > > Right, we've discussed adding a "shared mutable resource cache" concept > before that all tools could use, so that everyone's seeing the same version > of a resource (such as a CSS file). Exactly. This is an important key to solving the styles-as-authored and inspector-editor sync problems. Also storing the changes made through devtools into a writable cache means that we'd be able to extract a diff of all changes made in devtools.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27) > Right, we've discussed adding a "shared mutable resource cache" concept > before that all tools could use, so that everyone's seeing the same version > of a resource (such as a CSS file). > > But, that seems like work for bug 878398, so I would not hold up this bug > because of this problem. I have been thinking that I would not want to land this patch, even if bug 878398 is fixed, until this deeper problem was addressed. It seems to me like this would introduce a pretty serious problem into the rule view -- namely that it would not reflect reality in some scenarios. I'm curious to know what you think about this. I also found bug 922146, which provides some of the infrastructure I'll need. I started rebasing the patch there yesterday. Another thing I don't know what to do about is modifications to style sheets via CSSOM. IIUC content can use document.styleSheets[0].deleteRule or the like to modify style sheets in place. I think (haven't tried it yet) it means that changes via the style editor will overwrite changes made by content, due to how it reloads whole style sheets. One idea would be to try to reflect these changes back into the cache. However, changes can be done when the tools aren't active. That idea could be salvaged by noting when a rule has been modified this way and treating it specially -- in the rule view, showing the rule's cssText (I suspect this already happens); and in the editor, replacing the source rule's text with the cssText. This seems a bit tricky to get right, but doable.
So far I mostly just have thoughts on the problems at hand. I'm not quite sure how best to solve these things. (In reply to Tom Tromey :tromey from comment #29) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27) > > > Right, we've discussed adding a "shared mutable resource cache" concept > > before that all tools could use, so that everyone's seeing the same version > > of a resource (such as a CSS file). > > > > But, that seems like work for bug 878398, so I would not hold up this bug > > because of this problem. > > I have been thinking that I would not want to land this patch, even if bug > 878398 > is fixed, until this deeper problem was addressed. It seems to me like this > would introduce a pretty serious problem into the rule view -- namely that it > would not reflect reality in some scenarios. > > I'm curious to know what you think about this. Hmm, I missed comment 25 where you said a change in rule view is lost if change from element A to B and back to A again. I agree that problem should be solved before landing this bug, as that would be a functionality loss. I was previously thinking only of comment 26, about the style editor: it is already the case today that if you make changes in the style editor, close toolbox, and reopen, then the changes are lost. So, that scenario is unchanged. These issues are related to the discussions at Whistler about a general resources tool, particularly because there are several "views" of CSS: 1. stylesheets as they are downloaded 2. the live CSS accessible via CSSOM But once you have made an edit in the rule view, if we hope to keep that in sync with the style editor, then we want something like: 3. composed view with edits merged back into the stylesheet text as the thing to display in Style Editor. This could require knowing which rule set (a block with selector plus several rules) in the style sheet text corresponds to which rule view item (which I think you started to hint at). Another confounding factor are CSS source maps. The style editor could be displaying the original source in Sass, LESS, Stylus, etc. Since the browser can't know about these languages, it could be even trickier to represent rule view edits in the style editor if it is displaying the "original source" view. In the general case, there may not be anything we can do, since 1 rule set in these preprocessors can produce N rule sets in CSS. If you then edit real CSS in the rule view of 1 of N rule sets, where are you supposed to represent that in the Style Editor? It's not very clear. But, if the basic concept of tracking rule sets between rule view and style editor works for normal CSS, maybe we can find ways to be clever here too. > Another thing I don't know what to do about is modifications to style sheets > via CSSOM. IIUC content can use document.styleSheets[0].deleteRule or the > like > to modify style sheets in place. It can also use document.styleSheets[0].cssRules[0].style to modify rules in place. My impression is that these abilities to modify the CSSOM from content JS are used sparingly. Most developers just write CSS (or use some preprocessor), and that's the end of it. It would be nice to notice such changes for correctness, but I am not sure that it's critical to have, especially to start with. > I think (haven't tried it yet) it means that changes via the style editor > will > overwrite changes made by content, due to how it reloads whole style sheets. Yes, that seems likely. In an ideal world, we can still access the "composed view" (3 above) even after closing and reopening the toolbox and can redisplay it in the style editor with rule edits still in place in the text. In past discussions with Patrick and others, we had imagined that a server-side resource cache could help with problems like this by: * acting as a central store across tools * can hold on to edits even after toolbox close, in case they would be lost otherwise What exactly does it store? Unclear. Does it just store file contents? Some kind of AST? Not sure. > One idea would be to try to reflect these changes back into the cache. > However, > changes can be done when the tools aren't active. > > That idea could be salvaged by noting when a rule has been modified this way > and treating > it specially -- in the rule view, showing the rule's cssText (I suspect this > already > happens); and in the editor, replacing the source rule's text with the > cssText. > This seems a bit tricky to get right, but doable. Yes, I agree the case of content performing editing actions while the toolbox is closed is hard to handle. Maybe the toolbox reads the current state on open, and re-creates the composed view at that time? Not sure if required data is already lost by that time. Anyway, this is quite a hard problem overall. I think the main thing for this bug is to ensure the rule view preserves edits when switching between elements. I am hopeful that is a simpler issue than the larger problem of syncing the style editor as well.
Just thinking out loud - ignore if it doesn't make sense: Developers are used to the difference between a "View source" feature and a "DOM inspector". We're not used to thinking about CSS in the same way, but under the hood we have the same issue: developers may need access to both the "raw" code and the interpreted output. Would it simplify the implementation to have two modes and make the distinction between these modes clearer?
(In reply to Hallvord R. M. Steen [:hallvors] from comment #31) > Just thinking out loud - ignore if it doesn't make sense: > > Developers are used to the difference between a "View source" feature and a > "DOM inspector". We're not used to thinking about CSS in the same way, but > under the hood we have the same issue: developers may need access to both > the "raw" code and the interpreted output. > > Would it simplify the implementation to have two modes and make the > distinction between these modes clearer? It's an interesting point. I think you could say we are already heading in that direction with this bug, where the Rule View would be "view source" and the Computed View is the "DOM inspector".
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #30) > I agree that problem should be solved before landing this bug, as that would > be a functionality loss. Thanks. > Another confounding factor are CSS source maps. The style editor could be > displaying the original source in Sass, LESS, Stylus, etc. Since the > browser can't know about these languages, it could be even trickier to > represent rule view edits in the style editor if it is displaying the > "original source" view. In the general case, there may not be anything we > can do, since 1 rule set in these preprocessors can produce N rule sets in > CSS. What seems to happen today is that if you are viewing the original sources, then edits in the source editor don't have any visible effect. But if you switch to the "non-original" view, edits do take effect. That seems to be pretty much the only approach that makes sense. The tools could be a bit more clear about what is going on -- like saying "hey, you are editing this, but it is a derived file" or "you are viewing the source file but the derived file has edits that aren't reflected here". > Yes, that seems likely. In an ideal world, we can still access the > "composed view" (3 above) even after closing and reopening the toolbox and > can redisplay it in the style editor with rule edits still in place in the > text. > > In past discussions with Patrick and others, we had imagined that a > server-side resource cache could help with problems like this by: > > * acting as a central store across tools > * can hold on to edits even after toolbox close, in case they would be lost > otherwise > > What exactly does it store? Unclear. Does it just store file contents? > Some kind of AST? Not sure. I spent yesterday thinking about a plan for this. The basic idea is to have a single representation of the "source text" that is shared by the tools. This is the resource cache idea. The first time a tool asks for the source text of a given style sheet, we compute it on the server. This is done by fetching the source as is done today; and then comparing it with the CSSOM style sheets to see whether any edits have been made. This comparison would be done by walking the style sheet and examining each rule. At the same time we'd parse the original source text. I think an edited or added rule will have a line number of 0 (still have to check this -- but if there are cases that don't work, we can also solve this by adding a dirty bit); this plus parsing is enough to also reason about deleted rules. Then I picture the store commenting out the obsoleted rules and inserting the changes from CSSOM. This would yield the "original" text for use by the tools. After this there are three sources of changes: 1. Style editor; this can edit the text directly. 2. Rule view. This can send edits to the store. This could let us fix bug 913661 (with some additional work involving possibly parsing commented-out declarations) 3. CSSOM changes by content. We can listen to these and have the store update the text. I'm tentatively picturing a react-like style where the store applies the edits and then posts an event; and the style editor and rule view update in response to these events. One difficulty is if content is editing the CSS while a tool is open. For example if you are editing in the style editor when a timeout changes the CSSOM -- what should happen? In this model we don't really need the resource cache to persist across tool closing. It would be nice if it did, and this could make use of that. But if it did not we still could at least show something sensible.
(In reply to Tom Tromey :tromey from comment #33) > At the same time we'd parse the original source text. I think an edited or > added rule will have a line number of 0 (still have to check this -- but if > there > are cases that don't work, we can also solve this by adding a dirty bit); It seems that setting a property directly doesn't reset the line number. I tried with: document.styleSheets[1].cssRules[0].style.backgroundColor = 'red' ... then reopened the inspector. It still had the line number referring to the .css file.
(In reply to Tom Tromey :tromey from comment #33) > One difficulty is if content is editing the CSS while a tool is open. For > example > if you are editing in the style editor when a timeout changes the CSSOM -- > what > should happen? Joe pointed out that we should just follow whatever the markup view does, since it has the same issue.
The plan described in comment 33 seems pretty good to me after a quick read. But I'll need to think about it more in details I guess. I completely agree that we first need to address the resource store thing, and make sure that the rule-view and the style-editor can read and write to it. Any change made in these tools should end up modifying our knowledge of the authored style in this store so that we can have a unique source of truth. And as you said, we'll need to listen to style changes done by content to also update the store. I'm wondering how exactly you plan to re-conciliate the fetched text with the actual style when content has inserted/deleted rules before the toolbox was opened. The plan mentions comparing the text with the CSSOM, I'm wondering if that's going to lead to problems that are very hard to solve. What if content shuffles rules around by using deleteRule and insertRule, or has several rules with the same selector. As Ryan said earlier, modifying styles this way doesn't happen in the wild too often, so we probably shouldn't spend too much time on it. (Related: this post on David Walsh's blog and a comment of mine: http://davidwalsh.name/add-rules-stylesheets#comment-501007 ) We also still have the potential issue that the resource may have changed on the server by the time we go and fetch it (when the toolbox is opened the first time and if the resource isn't cached). But that's already an issue today, also probably a very rare problem, and would require necko platform work to address I think, so it's probably fine if we leave it as is for now. I haven't actually looked at how the debugger does it, but I was once told that there was a platform API that returned the script source that's being executed, without having to go and fetch it again.
(In reply to Patrick Brosset [:pbrosset] [:pbro] [on PTO until August 10th] from comment #36) > I'm wondering how exactly you plan to re-conciliate the fetched text with > the actual style when content has inserted/deleted rules before the toolbox > was opened. The plan mentions comparing the text with the CSSOM, I'm > wondering if that's going to lead to problems that are very hard to solve. > What if content shuffles rules around by using deleteRule and insertRule, or > has several rules with the same selector. I think I will add a dirty bit to each rule so we can see if it has been modified. For any rule that is modified (or added), we'll simply comment it out from the authored text and insert the new text from the CSSOM. This is not ideal, since we'll lose some authored data -- e.g., for properties that weren't modified, we'll forget how colors were written. But, I think this is still ok because (1) it will reflect reality, (2) this doesn't happen much, and (3) the authored text will still be there in a comment in the style editor. > We also still have the potential issue that the resource may have changed on > the server by the time we go and fetch it (when the toolbox is opened the > first time and if the resource isn't cached). But that's already an issue > today, also probably a very rare problem, and would require necko platform > work to address I think, so it's probably fine if we leave it as is for now. Yeah, I was planning to address this separately at a later date. > I haven't actually looked at how the debugger does it, but I was once told > that there was a platform API that returned the script source that's being > executed, without having to go and fetch it again. I will look into this.
Overall, I think this plan sounds good to me as well. I'll stress once more (just like Patrick did) that edits to the CSSOM by content JS are quite rare, so we should not spend large amounts of time to support them if it ends up being hard to do so. Currently, the rule view receives rule actors that represent CSSOM rules (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still converse over the RDP in terms of rules, or would it pull the text from the server's resource cache and parse things as needed in the tool's front end? [1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/styles.js#512
Flags: needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #38) > Currently, the rule view receives rule actors that represent CSSOM rules > (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still > converse over the RDP in terms of rules, or would it pull the text from the > server's resource cache and parse things as needed in the tool's front end? Good question. The patch currently attached to this bug tries to do the work on the server side. However, this can't fully work, I think, because the client side also needs some tweaks to deal with some scenarios. So I am not really sure what I will do. I have been looking into this a bit this week; though I don't really understand some of what I see, for example why we have two kinds of style sheet actor.
(In reply to Tom Tromey :tromey from comment #39) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #38) > > > Currently, the rule view receives rule actors that represent CSSOM rules > > (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still > > converse over the RDP in terms of rules, or would it pull the text from the > > server's resource cache and parse things as needed in the tool's front end? > > Good question. The patch currently attached to this bug tries to do the > work on the server side. However, this can't fully work, I think, because > the client side also needs some tweaks to deal with some scenarios. I'm hoping Patrick and/or Brian can help flesh out the details here. I haven't spent enough time in these tools to know the edge cases well enough. > So I am not really sure what I will do. I have been looking into this a bit > this week; though I don't really understand some of what I see, for example > why we have two kinds of style sheet actor. It's there for compatibility purposes[1]. IIRC, actors/styleeditor.js is the old one. It really needs a nice ASCII warning at the top saying "I am old! Go away!". The new one is actors/stylesheets.js. The only reason the old file is still in the tree is because protocol.js builds the front / client from the actor's method descriptions, so talking to old servers needs these to remain. We should eventually find a nicer path than just keep large chunks of code around that we never run... [1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/styleeditor-panel.js#62
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40) > > So I am not really sure what I will do. I have been looking into this a bit > > this week; though I don't really understand some of what I see, for example > > why we have two kinds of style sheet actor. > > It's there for compatibility purposes[1]. IIRC, actors/styleeditor.js is > the old one. It really needs a nice ASCII warning at the top saying "I am > old! Go away!". The new one is actors/stylesheets.js. > > The only reason the old file is still in the tree is because protocol.js > builds the front / client from the actor's method descriptions, so talking > to old servers needs these to remain. We should eventually find a nicer > path than just keep large chunks of code around that we never run... This is right - stylesheets.js is the new one and the only one that would need to be changed to support authored styles.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40) > (In reply to Tom Tromey :tromey from comment #39) > > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #38) > > > > > Currently, the rule view receives rule actors that represent CSSOM rules > > > (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still > > > converse over the RDP in terms of rules, or would it pull the text from the > > > server's resource cache and parse things as needed in the tool's front end? > > > > Good question. The patch currently attached to this bug tries to do the > > work on the server side. However, this can't fully work, I think, because > > the client side also needs some tweaks to deal with some scenarios. > > I'm hoping Patrick and/or Brian can help flesh out the details here. I > haven't spent enough time in these tools to know the edge cases well enough. I assumed we would need to coordinate some frontend changes with the server side changes to ship this feature. We will have to address backwards compatibility, but we should be able to do that if we plan for it ahead of time. Any specific scenarios you have already thought of regarding how the client side will need to change? I agree that most of the work should still be done on the server - otherwise it seems like there would be too many cases where we end up wanting to access the content to read CSSOM or get some information from the parser and we'd have a bunch of one-off calls to the server while needing to worry that everything is staying in sync.
(In reply to Brian Grinstead [:bgrins] from comment #42) > I assumed we would need to coordinate some frontend changes with the server > side changes to ship this feature. We will have to address backwards > compatibility, but we should be able to do that if we plan for it ahead of > time. Any specific scenarios you have already thought of regarding how the > client side will need to change? The one case I know of is here: https://bugzilla.mozilla.org/attachment.cgi?id=8632254&action=diff#a/browser/devtools/styleinspector/rule-view.js_sec3 I don't recall offhand why this is needed -- it was in Patrick's patch, and I know I investigated it myself as well (to assure myself it was still needed) but I don't remember the outcome. This may be sufficiently obscure that we don't need to worry about backward compatibility with old clients. But if we do, I suppose we could do something like add a new request to put the rule and/or page style actor into "authored mode". > Currently, the rule view receives rule actors that represent CSSOM rules > (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still > converse over the RDP in terms of rules, or would it pull the text from the > server's resource cache and parse things as needed in the tool's front end? After looking into this more yesterday, my tentative plan is to continue to use the rule actors -- but change StyleRuleActor to operate on the style sheet actor. I think to make this work properly I'll need to change StyleRuleActor.modifyProperties to accept more kinds of objects. This is needed because the current vocabulary here is insufficient to express all the kinds of edits we would like to do. I am not totally sure this approach has all the desired properties. For example when editing the style sheet, the line numbers of all the subsequent rules will change. So it would be good for the rule view to notice this and update all the links. So perhaps some additional event and refresh will be needed.
(In reply to Tom Tromey :tromey from comment #43) > (In reply to Brian Grinstead [:bgrins] from comment #42) > > > I assumed we would need to coordinate some frontend changes with the server > > side changes to ship this feature. We will have to address backwards > > compatibility, but we should be able to do that if we plan for it ahead of > > time. Any specific scenarios you have already thought of regarding how the > > client side will need to change? > > The one case I know of is here: > > https://bugzilla.mozilla.org/attachment.cgi?id=8632254&action=diff#a/browser/ > devtools/styleinspector/rule-view.js_sec3 > > I don't recall offhand why this is needed -- it was in Patrick's patch, and > I know > I investigated it myself as well (to assure myself it was still needed) but > I don't > remember the outcome. > > This may be sufficiently obscure that we don't need to worry about backward > compatibility > with old clients. But if we do, I suppose we could do something like add a > new request > to put the rule and/or page style actor into "authored mode". Looking at this change, I don't think that would break backwards compat (or would be trivial to fix if so). > > Currently, the rule view receives rule actors that represent CSSOM rules > > (AIUI) (like in getApplied[1]) to drive its UI. Would the rule view still > > converse over the RDP in terms of rules, or would it pull the text from the > > server's resource cache and parse things as needed in the tool's front end? > > After looking into this more yesterday, my tentative plan is to continue to > use > the rule actors -- but change StyleRuleActor to operate on the style sheet > actor. Can you elaborate on this part? FWIW the StyleRuleActor does have a way to reference a StyleSheetActor already (via this.pageStyle._sheetRef). > I think to make this work properly I'll need to change > StyleRuleActor.modifyProperties > to accept more kinds of objects. This is needed because the current > vocabulary here > is insufficient to express all the kinds of edits we would like to do. This is actually more along the lines of the backwards-compat issues that might take some more work to handle. The frontend will be passing along these new inputs, and we need to make sure that the client still passes along the older inputs when talking to an older server. Sometimes this could be handled directly in the Front with a custom method, or sometimes the frontend may use a trait to completely hide a feature. It depends on the case, but I'm sure we will be able to work something out as we discover the particulars here. > I am not totally sure this approach has all the desired properties. For > example when > editing the style sheet, the line numbers of all the subsequent rules will > change. > So it would be good for the rule view to notice this and update all the > links. So > perhaps some additional event and refresh will be needed. I can think of an easy workaround for this particular case since the editing will be happening on another toolbox tab - whenever the inspector tab becomes visible, just do a full rule view refresh. But in the general case, we could send an unsolicited event to the frontend if one of the rules on the selected node for the client will be modified for some reason.
(In reply to Brian Grinstead [:bgrins] from comment #44) > > After looking into this more yesterday, my tentative plan is to continue to > > use > > the rule actors -- but change StyleRuleActor to operate on the style sheet > > actor. > > Can you elaborate on this part? FWIW the StyleRuleActor does have a way to > reference a StyleSheetActor already (via this.pageStyle._sheetRef). Yeah. What I am thinking is that StyleRuleActor.modifyProperties will write changes back to the StyleSheetActor; and in particular will work by re-evaluating the entire style sheet rather than editing the various rules piecemeal. Currently, modifyProperties only supports "set" and "delete" operations for a property; and rule-view.js sends all off the rule's properties on any change. My plan here is to instead send just the change that is needed, in a way that is useful for rewriting the CSS text. This requires adding a few cases. For example, we will want to represent disabling a property by commenting out the property in the text; and conversely, try to parse CSS comment bodies as properties to extract disabled properties. > along the older inputs when talking to an older server. Sometimes this > could be handled directly in the Front with a custom method, or sometimes > the frontend may use a trait to completely hide a feature. jryans taught me about traits today on irc and I think a new trait on the rule actor will be just what is needed.
Depends on: 1187409
Attached patch update RuleModificationList comments (obsolete) (deleted) — Splinter Review
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.
Attachment #8632254 - Attachment is obsolete: true
Attachment #8632255 - Attachment is obsolete: true
Attached patch fix eslint nits in test_parseDeclarations (obsolete) (deleted) — Splinter Review
Attached patch add getRuleText (obsolete) (deleted) — Splinter Review
Attached patch as-authored styles in the rule view (obsolete) (deleted) — Splinter Review
Attached patch fix up rule actor to notice update events (obsolete) (deleted) — Splinter Review
A couple other things I've noticed. * Because getAppliedProps is now async, various things using it broke. So something has to change; I am thinking perhaps a new explicit method to fetch the StyleRuleActor's authored text. * Setting css properties of an element works but these aren't "as-authored". It seems that this could be done by setting and getting the text from the element's style attribute.
(In reply to Tom Tromey :tromey from comment #54) > * Setting css properties of an element works but these aren't "as-authored". > It seems that this could be done by setting and getting the text from > the element's style attribute. Never mind, I see this is computed, not stored. So I think we'll have to just let that one remain semi-computed.
Depends on: 1191024
Attached patch update RuleModificationList comments (obsolete) (deleted) — Splinter Review
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.
Attachment #8640568 - Attachment is obsolete: true
Attachment #8640569 - Attachment is obsolete: true
Attachment #8640570 - Attachment is obsolete: true
Attachment #8640571 - Attachment is obsolete: true
Attachment #8640572 - Attachment is obsolete: true
Attachment #8640573 - Attachment is obsolete: true
Attachment #8640574 - Attachment is obsolete: true
Attachment #8640575 - Attachment is obsolete: true
Attached patch fix eslint nits in test_parseDeclarations (obsolete) (deleted) — Splinter Review
Attached patch add getRuleText (obsolete) (deleted) — Splinter Review
Attached patch as-authored styles in the rule view (obsolete) (deleted) — Splinter Review
Attached patch fix up rule actor to notice update events (obsolete) (deleted) — Splinter Review
Attached patch edit selector in authored CSS text (obsolete) (deleted) — Splinter Review
Attached patch make addNewRule edit authored CSS text (obsolete) (deleted) — Splinter Review
Attached patch add "invisible" attribute to TextProp (obsolete) (deleted) — Splinter Review
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.
Blocks: 1190188
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.
Flags: needinfo?(jgriffiths)
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.
Depends on: 1013814
Blocks: 1194146
(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.
Flags: needinfo?(jgriffiths)
(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.
Depends on: 1195343
Depends on: 1195349
Depends on: 1195356
Depends on: 1195357
Depends on: 1195361
Depends on: 1195398
(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.
Attached patch as-authored styles in the rule view (obsolete) (deleted) — Splinter Review
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
Attachment #8644597 - Attachment is obsolete: true
Attachment #8644598 - Attachment is obsolete: true
Attachment #8644600 - Attachment is obsolete: true
Attachment #8644601 - Attachment is obsolete: true
Attachment #8644602 - Attachment is obsolete: true
Attachment #8644603 - Attachment is obsolete: true
Attachment #8644604 - Attachment is obsolete: true
Attachment #8644605 - Attachment is obsolete: true
Attachment #8644606 - Attachment is obsolete: true
Attachment #8644607 - Attachment is obsolete: true
Attachment #8644608 - Attachment is obsolete: true
Attachment #8644609 - Attachment is obsolete: true
Attached patch fix up rule actor to notice update events (obsolete) (deleted) — Splinter Review
Attached patch add "invisible" attribute to TextProp (obsolete) (deleted) — Splinter Review
The need for this patch isn't totally obvious. Previously, Rule._getTextProperties dropped non-inherited rules; but this would make edits to the authored text apply to the wrong locations, because the edits are basically done using the index of the property in the rule. So, now we keep all properties and mark some as "invisible"; this makes the indices always be correct.
This one can't really be separated from the initial "as authored" patch, as that changed RuleModificationList, which this patch uses. However it was a pain to rebase this earlier in the series; and I will probably just squash most of these patches at push time anyhow.
Attached patch edit selector in authored CSS text (obsolete) (deleted) — Splinter Review
Attached patch make addNewRule edit authored CSS text (obsolete) (deleted) — Splinter Review
Attached patch update rule link after changes are made (obsolete) (deleted) — Splinter Review
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.
Attached patch update inspector after edits in style editor (obsolete) (deleted) — Splinter Review
Attached patch rolled-up patch (obsolete) (deleted) — Splinter Review
Here's the rolled-up patch. You can apply it to a very recent fx-team -- you need the fix for bug 1195978.
Depends on: 1195978
(In reply to Tom Tromey :tromey from comment #84) > Created attachment 8649481 [details] [diff] [review] > rolled-up patch > > Here's the rolled-up patch. You can apply it to a very recent fx-team > -- you need the fix for bug 1195978. Sigh, I mean bug 1013814
Thanks for the rolled-up patch Tom. I've applied it locally and tested it a bit. First of all, great work, this is really awesome! Secondly, I've done a bit of QA, and found a few bugs that I'll list below. I don't think many should block landing the feature because that's something we didn't have before anyway. - This first one is probably the only one that should block landing though: editing the style-editor result in a refresh that scrolls back up and positions the carets at 0:0 It's particularly frustrating if I'm a slow typer as the editor might refresh before I'm done typing my code. I understand that the refresh is probably there for when you change the stylesheet from the rule-view, but it should not happen if you're making the change from the style-editor. Also, even if you make the change from the rule-view, the scroll and caret positions should be preserved. - Minor: declarations added in the rule-view aren't indented properly in the style-editor (but I think that's already part of your todo list). - I managed to get the following error when enabling/disabling and commenting/uncommenting a declaration in the rule-view and style-editor respectively: TypeError: decl.commentOffsets is undefined css-parsing-utils.js:251:1 But of course, I couldn't get a clear set of STR after that. - I found a weird edge case: Enter the following in the style-editor body { content: "te*/st"; } Go to the rule view and disable the property. This comments it out in the style-editor, and escapes the content string "test*\/st" which I think is fine. Then go back to the rule-view and enable the property again. This happens in the style-editor: body { content: "te*\/st" /*; */ } - Invalid properties in @keyframes rules aren't shown as invalid. @keyframes animation { 0% { -webkit-transform: translateX(-50%) rotate(0deg); transform: translateX(-50%) rotate(0deg); } } In this example, the -webkit-transform property has the expected warning sign next to it, but doesn't have the strikethrough text decoration that properties in other rules do. - With the same example, if I disable the -webkit-transform property in the rule-view, it ends up being commented out in the style-editor, but if I then re-enable it in the rule-view, it stays commented out in the style-editor and the following error is logged: TypeError: decl.commentOffsets is undefined css-parsing-utils.js:251:1 - In the style-editor, select any stylesheet, delete all its content, wait for a second -> the content is back! (but the corresponding rules are removed correctly from the page, I can see the style change as I expected it to). Ok, that's all for now. There are other problems (we discussed about on IRC) when the page uses the CSS Object Model to change styles, but we agreed to look at those later (for reference, if you file a bug for it, this page is a good example: https://bgrins.github.io/devtools-demos/inspector/authored-styles.html )
Blocks: 1196250
Depends on: 1196431
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #86) > - With the same example, if I disable the -webkit-transform property in the > rule-view, it ends up being commented out in the style-editor, but if I then > re-enable it in the rule-view, it stays commented out in the style-editor > and the following error is logged: > TypeError: decl.commentOffsets is undefined css-parsing-utils.js:251:1 This happens due to the comment parsing heuristic. I failed to anticipate that the re-parsing of the text in rewriteDeclarations would mess things up -- but it does. This requires some re-thinking. The fundamental issue for the heuristic is that we don't want to parse any old comment as a possible declaration; but at the same time users can write wildly invalid CSS that will show up as just the kind of text that we ought to reject. One idea might be to use a special syntax when writing our own comments. The parser could recognize this syntax and change the heuristic to accept otherwise invalid declarations. Another idea would be to keep track of all the property names we've seen, valid or not, in a given rule, and then use this list to inform the heuristic. One more approach could be to keep the results of parseDeclarations around, attached to the TextProperty objects. However, this would require tricky updating of these objects when editing, and I would much rather avoid even more hair in this area.
(In reply to Tom Tromey :tromey from comment #87) > One idea might be to use a special syntax when writing our own comments. > The parser could recognize this syntax and change the heuristic to accept > otherwise invalid declarations. This might be the best I think. Just to be clear, you mean ignoring comments unless they start with a special syntax like /*! property: value; */
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #88) > (In reply to Tom Tromey :tromey from comment #87) > > One idea might be to use a special syntax when writing our own comments. > > The parser could recognize this syntax and change the heuristic to accept > > otherwise invalid declarations. > This might be the best I think. Just to be clear, you mean ignoring comments > unless they start with a special syntax like /*! property: value; */ I was thinking of applying the heuristic to normal comments, and relaxing it for the specially-formatted comments. This way if the user has commented-out properties, we might still detect them; but this would still let us avoid bugs like the one you ran into here.
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #86) > - In the style-editor, select any stylesheet, delete all its content, wait > for a second -> the content is back! This turned out to be a latent bug in stylesheets.js: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/stylesheets.js#596 This test fails when this.text === "". I've fixed this locally.
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #86) > - Invalid properties in @keyframes rules aren't shown as invalid. > @keyframes animation { > 0% { > -webkit-transform: translateX(-50%) rotate(0deg); > transform: translateX(-50%) rotate(0deg); > } > } > In this example, the -webkit-transform property has the expected warning > sign next to it, but doesn't have the strikethrough text decoration that > properties in other rules do. I looked into this briefly. Keyframe rules are skipped here: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#334 Fixing this properly -- by letting the server decide whether a given property is valid or not -- is going to require a bit more work. I think at least a new actor method and some more round trips will be needed.
Attached patch rolled-up patch (obsolete) (deleted) — Splinter Review
v2 of the rolled-up patch. This fixes a lot of bugs.
Attachment #8649481 - Attachment is obsolete: true
Depends on: 1197967
My current to-do list: * Add a new method to determine property name validity, then use this in rule-view to filter keyframe properties * A change in the rule view can make another rule's line number change. This should propagate back to the rule view. There's a patch for this but I have to investigate why it doesn't work. * Bug 1196431, generalizing the indentation detector * Write more tests. * Run all the tests and fix up problems.
(In reply to Tom Tromey :tromey from comment #92) > Created attachment 8651920 [details] [diff] [review] > rolled-up patch > > v2 of the rolled-up patch. This fixes a lot of bugs. I've done a quick round of tests with this new version. Here are some comments about it: - create a new rule in the rule-view, add some properties, switch to the style-editor: .primary.go{ color:red; background:yellow; margin:2em; } I think the code that writes rules could introduce some formatting spaces. Something like: .primary.go { color: red; background: yellow; margin: 2em; } - not related to this patch, filed bug 1198299, but mentioning it here still since this project involves refreshing the style-editor on rule-view changes: Open the inspector first (make sure the style-editor wasn't opened first), add a rule, switch to the style-editor: a new inline stylesheet is listed and shows the rule. Now, if you do the opposite: open the style-editor first, then add the rule, then back to the style-editor, the new style-sheet doesn't appear. - not sure if related: - go to https://www.mozilla.org/en-US/ - open the inspector and select <body> - notice that it has 2 font-size declarations - change the selector to body[class] and submit - on submit, somehow, the rule changes and one of the font-sizes disappears - in the style-editor, it's still there, and if you make any changes in the text there, then the font-size declaration re-appears in the rule-view.
Attached patch rolled-up patch (obsolete) (deleted) — Splinter Review
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.)
Attachment #8651920 - Attachment is obsolete: true
Blocks: 1195995
Attached patch rolled-up patch (obsolete) (deleted) — Splinter Review
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.
Attachment #8653711 - Attachment is obsolete: true
Depends on: 1202095
Attached patch fix latent bug StyleSheetActor._getText (obsolete) (deleted) — Splinter Review
Attachment #8648931 - Attachment is obsolete: true
Attachment #8648932 - Attachment is obsolete: true
Attachment #8648933 - Attachment is obsolete: true
Attachment #8648937 - Attachment is obsolete: true
Attachment #8648938 - Attachment is obsolete: true
Attachment #8648939 - Attachment is obsolete: true
Attachment #8648941 - Attachment is obsolete: true
Attachment #8648943 - Attachment is obsolete: true
Attachment #8648944 - Attachment is obsolete: true
Attachment #8648946 - Attachment is obsolete: true
Attached patch as-authored styles in the rule view (obsolete) (deleted) — Splinter Review
Attached patch fix up rule actor to notice update events (obsolete) (deleted) — Splinter Review
Attached patch add "invisible" attribute to TextProp (obsolete) (deleted) — Splinter Review
Attached patch edit selector in authored CSS text (obsolete) (deleted) — Splinter Review
Attached patch make addNewRule edit authored CSS text (obsolete) (deleted) — Splinter Review
Attached patch update rule link after changes are made (obsolete) (deleted) — Splinter Review
Attached patch update inspector after edits in style editor (obsolete) (deleted) — Splinter Review
Attached patch get correct default indentation for style edits (obsolete) (deleted) — Splinter Review
Attached patch special-case about:PreferenceStyleSheet (obsolete) (deleted) — Splinter Review
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.
Attachment #8660063 - Flags: review?(bgrinstead)
Attached patch rolled-up patch v5 (obsolete) (deleted) — Splinter Review
Attachment #8654357 - Attachment is obsolete: true
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.
Attachment #8660078 - Flags: review?(bgrinstead)
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.
Attachment #8660075 - Flags: review?(bgrinstead)
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.
Attachment #8660077 - Flags: review?(bgrinstead)
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"
Attachment #8660063 - Flags: review?(bgrinstead) → review+
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.
Attachment #8660064 - Flags: review?(pbrosset)
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.
Attachment #8660078 - Flags: review?(bgrinstead)
(In reply to Tom Tromey :tromey from comment #117) > Comment on attachment 8660064 [details] [diff] [review] > as-authored styles in the rule view > > This is the main patch. It has several odd bits. > > You may notice some Promise shenanigans in createProperty and > setPropertyValue Oops, and I forgot to update the "Wow" comment, haha. It turns out that some tests relied on enabling or disabling emitting the changed event, even if no change actually occurred. Hence the hack; but maybe it can be done in a better way.
Comment on attachment 8660064 [details] [diff] [review] as-authored styles in the rule view Review of attachment 8660064 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly good to me, but I'd like to see the new test use an HTML page instead, and wouldn't mind taking another look at the code changes in rule-view after you've read my comments. ::: browser/devtools/styleinspector/rule-view.js @@ +617,5 @@ > } > > + let deferred = promise.defer(); > + let modifications = this.style.startModifyingProperties(); > + modifications.getDefaultIndentation().then((indent) => { Shouldn't the modifications object deal with calling getDefaultIndentation at the right times on its own? Instead of requiring the rule-view to call it first, wait for it to resolve, and then calling createProperty passing the right indent. @@ +620,5 @@ > + let modifications = this.style.startModifyingProperties(); > + modifications.getDefaultIndentation().then((indent) => { > + modifications.createProperty(ind, name, value, priority, indent); > + this.internalApplyProperties(modifications).then(() => { > + deferred.resolve(null); this.internalApplyProperties(modifications).then(deferred.resolve); should work @@ +772,5 @@ > + let deferred = promise.defer(); > + let modifications = this.style.startModifyingProperties(); > + modifications.getDefaultIndentation().then((indent) => { > + modifications.setProperty(index, property.name, value, priority, indent); > + this.internalApplyProperties(modifications).then(() => { this.internalApplyProperties(modifications).then(deferred.resolve); should work @@ +810,5 @@ > * @param {Boolean} value > */ > setPropertyEnabled: function(property, value) { > + if (property.enabled === !!value) { > + // Wow. Wow! ::: browser/devtools/styleinspector/test/browser_computedview_cycle_color.js @@ +60,5 @@ > {type: "mousedown", shiftKey: true}, win); > is(valueNode.textContent, "red", > "Color displayed as a color name."); > > + // Back to "Authored" Shouldn't we keep the extra shift-click to make sure we cycle back to hex? ::: browser/devtools/styleinspector/test/browser_ruleview_authored.js @@ +14,5 @@ > + yield overrideTest(inspector, view); > + yield colorEditingTest(inspector, view); > +}); > + > +function* createTestContent(inspector, style) { This function makes use of CPOWs to access and modify content directly. We're trying to get rid of those. Could you please use new HTML test pages instead? This also would make the test cleaner/easier to read. You can even make 3 different HTML pages and 3 different tests out of this if that helps make the test simpler. ::: browser/devtools/styleinspector/test/browser_ruleview_cycle-color.js @@ +52,4 @@ > EventUtils.synthesizeMouseAtCenter(swatch, > {type: "mousedown", shiftKey: true}, win); > is(valueNode.textContent, "#F00", > + "Color displayed as an authored value."); Shouldn't we keep the extra shift-click to make sure we cycle back to hex? ::: toolkit/devtools/server/actors/styles.js @@ +1091,5 @@ > if (this.rawRule.parentStyleSheet) { > form.parentStyleSheet = this.pageStyle._sheetRef(this.rawRule.parentStyleSheet).actorID; > } > > + form.authoredText = this.authoredText; nit: can you insert a newline below this. Also, StyleRuleActors are becoming a bit harder to use. In order to work correctly, they not only need to be instantiated and returned like they used to, they also need to have their async getAuthoredCssText function called before form is called so that this.authoredText exists by the time form is called. It's unfortunate that protocol.js does not allow the initialize or form functions to return promises. This would avoid having to call rule.getAuthoredCssText() in getApplied, but instead each StyleRuleActor would call it itself before returning the form. We should definitely investigate making form async in protocol.js. But that's not related to your work here. So at least, could you add a comment before this line explaining that for this to work, someone has to call getAuthoredCssText before returning the actor to the client? @@ +1125,5 @@ > return form; > }, > > /** > + * Return the authored form of a rule's text. This will include This not only return the authored text, but also sets it on this. So the comment should at least say this. @@ +1128,5 @@ > /** > + * Return the authored form of a rule's text. This will include > + * invalid and otherwise ignored properties. > + */ > + getAuthoredCssText: function*() { Making this a generator function makes it possible for tasks to use yield with it, but it makes it impossible for non-task functions to call it. To maximize the possibility of being able to call this from several places, I would do either one of these 2 things: - make it a normal function, remove the yield before parentStyleSheet.getText() and instead append .then() after it. - or wrap this generator in a task too. In either case, please document this function as returning a promise. @@ +1140,5 @@ > + } > + > + let parentStyleSheet = > + this.pageStyle._sheetRef(this.rawRule.parentStyleSheet); > + let {str: cssText} = yield parentStyleSheet.getText(); My earlier point about how having an async form function would simplify the code is irrelevant if getText was sync, because this line is the only place that introduces something async in the process. I think we've already discussed how having to fetch the text again (potentially from the server) was a bad thing. Ultimately, it'd be great to get the text synchronously as it was downloaded when the page was rendered. Maybe that's what we need to work on after this project is done.
Attachment #8660064 - Flags: review?(pbrosset)
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?
Attachment #8660075 - Flags: review?(bgrinstead) → review+
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #121) > > + let key = this.href ? this.href : this.ownerNode; > > nit: Maybe move the inner map key construction to a helper method so it can > be used both from update and _getText. > Are we sure that multiple href-less stylesheets can't share the same > ownerNode? > What about @import-ed stylesheets? I am not sure; but it occurs to me that we can circumvent all these questions by making the key just be the style sheet itself.
Comment on attachment 8660069 [details] [diff] [review] add "invisible" attribute to TextProp As-authored editing relies on the index of a property in textProps being the same as its index in the result of parseDeclarations. This is how the rewriter knows what text to preserve and what text to rewrite. However, the rule view was dropping non-inherited properties from textProps. This patch changes this code to keep all properties, but to mark some properties as "invisible" -- meaning they don't show up in the UI, but do stay in textProps. This ensures the indices are consistent.
Attachment #8660069 - Flags: review?(pbrosset)
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.
Attachment #8660074 - Flags: review?(bgrinstead)
This version uses the style sheet itself as the key.
Attachment #8660075 - Attachment is obsolete: true
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.
Attachment #8660809 - Flags: feedback?
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.
Attachment #8660067 - Flags: review?(pbrosset)
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.
Attachment #8660809 - Flags: feedback?
(In reply to Brian Grinstead [:bgrins] from comment #129) > I have a question regarding inline style sheets. Will the call to > DOMUtils.parseStyleSheet update the actual textContent of the DOM element? > If so, I believe we won't want to set the cache in that case. It's not obvious when looking at this patch in isolation, but inline style sheets never wind up here, because the canSetRuleText trait of a StyleRuleActor for such a sheet will be false. StyleRuleActor can only wind up calling StyleSheetActor.update when canSetRuleText is true.
(In reply to Tom Tromey :tromey from comment #130) > (In reply to Brian Grinstead [:bgrins] from comment #129) > > > I have a question regarding inline style sheets. Will the call to > > DOMUtils.parseStyleSheet update the actual textContent of the DOM element? > > If so, I believe we won't want to set the cache in that case. > > It's not obvious when looking at this patch in isolation, but inline style > sheets > never wind up here, because the canSetRuleText trait of a StyleRuleActor > for such a sheet will be false. StyleRuleActor can only wind up > calling StyleSheetActor.update when canSetRuleText is true. In that case I think the test isn't exercising this functionality as we'd hope (since sync.html uses an inline style sheet). If that's right, can you expand the test case to handle both an inline and external stylesheet (and possibly even an @imported sheet)? You could add in a link from sync.html to import.css which would handle both external and @import cases.
Attached patch fix latent bug StyleSheetActor._getText (obsolete) (deleted) — Splinter Review
Fix review nit; wrote test case.
Attachment #8660063 - Attachment is obsolete: true
(In reply to Brian Grinstead [:bgrins] from comment #131) > (In reply to Tom Tromey :tromey from comment #130) > > (In reply to Brian Grinstead [:bgrins] from comment #129) > > > > > I have a question regarding inline style sheets. Will the call to > > > DOMUtils.parseStyleSheet update the actual textContent of the DOM element? > > > If so, I believe we won't want to set the cache in that case. For some reason I thought you were referring to an element's style here. I don't think I realized we could update a style sheet's textContent. I will look into that; my concern is whether it can be done without running into the "@import problem" all over again.
Comment on attachment 8660897 [details] [diff] [review] fix latent bug StyleSheetActor._getText This was already r+, but now I've written the test, and thought you might want to review it.
Attachment #8660897 - Flags: review?(bgrinstead)
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).
Attachment #8660067 - Flags: review?(pbrosset)
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
Attachment #8660069 - Flags: review?(pbrosset) → review+
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.
Attachment #8660897 - Flags: feedback+
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
Attachment #8660809 - Flags: feedback?(bgrinstead)
Blocks: 1205380
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 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
(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.
(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.
Attached patch special-case about:PreferenceStyleSheet (obsolete) (deleted) — Splinter Review
Attachment #8660078 - Attachment is obsolete: true
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.
Attachment #8662025 - Flags: review?(bgrinstead)
Attached patch add "invisible" attribute to TextProp (obsolete) (deleted) — Splinter Review
Updated per review.
Attachment #8660069 - Attachment is obsolete: true
Attachment #8662028 - Flags: review+
Attachment #8660809 - Attachment is obsolete: true
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.
Attachment #8662038 - Flags: review?(bgrinstead)
Updated for the canSetRuleText property.
Attachment #8660070 - Attachment is obsolete: true
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.
Attachment #8662043 - Flags: review?(pbrosset)
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.
Attachment #8660066 - Flags: review?(pbrosset)
It turns out the test landed in the wrong patch.
Attachment #8660066 - Attachment is obsolete: true
Attachment #8660066 - Flags: review?(pbrosset)
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.
Attachment #8662052 - Flags: review?(pbrosset)
Attached patch edit selector in authored CSS text (obsolete) (deleted) — Splinter Review
A test case that was in this patch belonged elsewhere.
Attachment #8660071 - Attachment is obsolete: true
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.
Attachment #8660072 - Flags: review?(pbrosset)
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.
Attachment #8660076 - Flags: review?(pbrosset)
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.
Attachment #8662053 - Flags: review?(pbrosset)
Attachment #8662043 - Flags: review?(pbrosset) → review+
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?
Attachment #8662052 - Flags: review?(pbrosset) → review+
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).
Attachment #8660076 - Flags: review?(pbrosset) → review+
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.
Attachment #8660072 - Flags: review?(pbrosset)
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.
Attachment #8662053 - Flags: review?(pbrosset) → review+
(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.
Blocks: 1205868
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.
Attachment #8660897 - Flags: review?(bgrinstead)
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.
Attachment #8660897 - Attachment is obsolete: true
Attachment #8662973 - Flags: review?(bgrinstead)
Updated per review; but also see comment #161.
Attachment #8662052 - Attachment is obsolete: true
Attachment #8663023 - Flags: review+
(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.
Attached patch edit selector in authored CSS text (obsolete) (deleted) — Splinter Review
Updated per review; but also see comment #165.
Attachment #8662053 - Attachment is obsolete: true
Attachment #8663066 - Flags: review+
(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.
Attached patch as-authored styles in the rule view (obsolete) (deleted) — Splinter Review
Updated per review.
Attachment #8660064 - Attachment is obsolete: true
Comment on attachment 8663118 [details] [diff] [review] as-authored styles in the rule view Also see comment #167.
Attachment #8663118 - Flags: review?(pbrosset)
(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.
Attached patch fix up rule actor to notice update events (obsolete) (deleted) — Splinter Review
Updated per review. Also see comment #170.
Attachment #8660067 - Attachment is obsolete: true
Attachment #8663741 - Flags: review?(pbrosset)
(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.
Attached patch make addNewRule edit authored CSS text (obsolete) (deleted) — Splinter Review
Updated per review; see also comment #172.
Attachment #8660072 - Attachment is obsolete: true
Attachment #8663796 - Flags: review?(pbrosset)
Attached patch update inspector after edits in style editor (obsolete) (deleted) — Splinter Review
Updated per review. This version puts the stylesheet on the event but doesn't try to make the rule-view itself any smarter.
Attachment #8660076 - Attachment is obsolete: true
Blocks: 1207209
Blocks: 752881
(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.
Blocks: 965181
Blocks: 1124210
Attachment #8662973 - Flags: review?(bgrinstead) → review+
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
Attachment #8662025 - Flags: review?(bgrinstead) → review+
(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?
Flags: needinfo?(ttromey)
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.
Attachment #8663796 - Flags: review?(pbrosset) → review+
Depends on: 1209077
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);
Attachment #8663118 - Flags: review?(pbrosset) → review+
(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.
Flags: needinfo?(ttromey)
(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.
Attached patch as-authored styles in the rule view (obsolete) (deleted) — Splinter Review
Rebased for the big renaming. Updated per reviews. Many bugs fixed. I put most of the test suite changes in this patch as well.
Attachment #8660074 - Attachment is obsolete: true
Attachment #8660077 - Attachment is obsolete: true
Attachment #8660086 - Attachment is obsolete: true
Attachment #8662025 - Attachment is obsolete: true
Attachment #8662028 - Attachment is obsolete: true
Attachment #8662038 - Attachment is obsolete: true
Attachment #8662043 - Attachment is obsolete: true
Attachment #8662973 - Attachment is obsolete: true
Attachment #8663023 - Attachment is obsolete: true
Attachment #8663066 - Attachment is obsolete: true
Attachment #8663118 - Attachment is obsolete: true
Attachment #8663741 - Attachment is obsolete: true
Attachment #8663796 - Attachment is obsolete: true
Attachment #8663824 - Attachment is obsolete: true
Attachment #8660074 - Flags: review?(bgrinstead)
Attachment #8660077 - Flags: review?(bgrinstead)
Attachment #8662038 - Flags: review?(bgrinstead)
Attachment #8663741 - Flags: review?(pbrosset)
Attached patch fix up rule actor to notice update events (obsolete) (deleted) — Splinter Review
Attached patch add "invisible" attribute to TextProp (obsolete) (deleted) — Splinter Review
Attached patch edit selector in authored CSS text (obsolete) (deleted) — Splinter Review
Attached patch make addNewRule edit authored CSS text (obsolete) (deleted) — Splinter Review
Attached patch update rule link after changes are made (obsolete) (deleted) — Splinter Review
Attached patch update inspector after edits in style editor (obsolete) (deleted) — Splinter Review
Attached patch get correct default indentation for style edits (obsolete) (deleted) — Splinter Review
Attached patch special-case about:PreferenceStyleSheet (obsolete) (deleted) — Splinter Review
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.
Attachment #8667283 - Flags: review?(pbrosset)
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).
Attachment #8667285 - Flags: review?(pbrosset)
Attachment #8667287 - Flags: review?(pbrosset)
Attachment #8667288 - Flags: review+
Attachment #8667289 - Flags: review+
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.
Attachment #8667290 - Flags: review?(pbrosset)
Comment on attachment 8667291 [details] [diff] [review] make addNewRule edit authored CSS text Added a test case.
Attachment #8667291 - Flags: review?(pbrosset)
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.
Attachment #8667292 - Flags: review?(bgrinstead)
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.
Attachment #8667293 - Flags: review?(bgrinstead)
Comment on attachment 8667295 [details] [diff] [review] get correct default indentation for style edits Added a test case.
Attachment #8667295 - Flags: review?(bgrinstead)
Attachment #8667296 - Flags: review+
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.
Attachment #8667294 - Flags: review?(pbrosset)
Blocks: 896503
Blocks: 979574
Blocks: 1171494
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.
Attachment #8667283 - Flags: review?(pbrosset) → review+
Attachment #8667294 - Flags: review?(pbrosset) → review+
Attachment #8667291 - Flags: review?(pbrosset) → review+
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.
Attachment #8667290 - Flags: review?(pbrosset)
Attachment #8667287 - Flags: review?(pbrosset) → review+
Attachment #8667285 - Flags: review?(pbrosset) → review+
Blocks: 1029459
Attached patch edit selector in authored CSS text (obsolete) (deleted) — Splinter Review
Updated per review.
Attachment #8667290 - Attachment is obsolete: true
Attachment #8667887 - Flags: review+
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.
Attachment #8667292 - Flags: review?(bgrinstead)
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?
Attachment #8667293 - Flags: review?(bgrinstead) → review+
(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.
Resurrect the missing test.
Attachment #8667293 - Attachment is obsolete: true
Comment on attachment 8668078 [details] [diff] [review] preserve edited style sheets across toolbox closes Now with the missing test - sorry about that!
Attachment #8668078 - Flags: review?(bgrinstead)
Attachment #8668078 - Flags: review?(bgrinstead) → review+
(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.
Attached patch update rule link after changes are made (obsolete) (deleted) — Splinter Review
Updated per review.
Attachment #8667292 - Attachment is obsolete: true
Attachment #8668441 - Flags: review?(bgrinstead)
No longer blocks: 979574
Attachment #8668441 - Flags: review?(bgrinstead)
Attached patch update rule link after changes are made (obsolete) (deleted) — Splinter Review
Fix a silly typo.
Attachment #8668441 - Attachment is obsolete: true
Attachment #8668473 - Flags: review?(bgrinstead)
Attached patch as-authored styles in the rule view (obsolete) (deleted) — Splinter Review
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.
Attachment #8667283 - Attachment is obsolete: true
Attachment #8668516 - Flags: review+
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();
Attachment #8667295 - Flags: review?(bgrinstead) → review+
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
Attachment #8668473 - Flags: review?(bgrinstead)
Attached patch update rule link after changes are made (obsolete) (deleted) — Splinter Review
Updated per review.
Attachment #8668473 - Attachment is obsolete: true
Attachment #8669075 - Flags: review?(bgrinstead)
Attachment #8669075 - Flags: review?(bgrinstead) → review+
Attached patch get correct default indentation for style edits (obsolete) (deleted) — Splinter Review
Updated per review.
Attachment #8667295 - Attachment is obsolete: true
(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.
Attachment #8669101 - Flags: review?(bgrinstead)
Attachment #8669101 - Flags: review?(bgrinstead) → review+
Attached patch as-authored styles in the rule view (obsolete) (deleted) — Splinter Review
Now that bug 979574 landed, this patch required a small update to the test case introduced there.
Attachment #8668516 - Attachment is obsolete: true
Attachment #8669703 - Flags: review+
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.
Attachment #8667285 - Attachment is obsolete: true
Attachment #8667287 - Attachment is obsolete: true
Attachment #8667288 - Attachment is obsolete: true
Attachment #8667289 - Attachment is obsolete: true
Attachment #8667291 - Attachment is obsolete: true
Attachment #8667294 - Attachment is obsolete: true
Attachment #8667296 - Attachment is obsolete: true
Attachment #8667887 - Attachment is obsolete: true
Attachment #8668078 - Attachment is obsolete: true
Attachment #8669075 - Attachment is obsolete: true
Attachment #8669101 - Attachment is obsolete: true
Attachment #8669703 - Attachment is obsolete: true
Attachment #8670248 - Flags: review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Awesome work Tom!
Depends on: 1213412
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
Flags: needinfo?(pbrosset)
(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.
Attached image screenshot (deleted) —
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?
Flags: needinfo?(ttromey)
(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!
Flags: needinfo?(ttromey)
Thanks for the clarification. According to Comment 235, I'm marking this issue as Verified - Fixed.
Status: RESOLVED → VERIFIED
Flags: needinfo?(pbrosset)
This is a large improvement, we should mention it with a relnote. :tromey, could you set firefox-relnote to ? and complete the comment?
Flags: needinfo?(ttromey)
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)]:
relnote-firefox: --- → ?
Flags: needinfo?(ttromey)
Depends on: 1221297
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.
Flags: needinfo?(pbrosset)
(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.
Flags: needinfo?(pbrosset)
Blocks: 1075632
Blocks: 1061074
Depends on: 1301854
Product: Firefox → DevTools
Depends on: 1478184
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: