585563 - The inspector style panel should link to the CSS editor

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Joe Walker [:jwalker]]

It can be handy to allow web developers to tweak the values in a page they are developing. The solution should:
- Make it easy for developers to export changes to their primary development environment
- Remember that fixing a problem by understanding it, is better then fixing it by “playing randomly until it works”, so we should prioritize the former
- Take account of robcees work in this area

Comment 1

[Kevin Dangoor]

bug 584371 is for extract changes that are made to the styles.

Comment 2

[Joe Walker [:jwalker]]

Adding robcee and pwalton who have expressed an interest in this bug

Comment 3

[Rob Campbell [:rc] (:robcee)]

patrick's been working on this and had content-editable working. He's also building a stand-alone style editor. We'll have some options to choose from shortly.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Is this not a duplicate of 583041?

Comment 5

[Cedric Vivier [:cedricv]]

I think this bug is about providing a direct "link" that opens the editor right at the given rule location?

ie. "With the problem spotted, you can quickly pop up an editor, change some CSS and see the fix right away." http://hg.mozilla.org/users/kdangoor_mozilla.com/dtplanning/raw-file/tip/fx4/Inspector.html

Comment 6

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #4)
> Is this not a duplicate of 583041?

This bug is pretty stale, but I think the intention was to add some editing capability directly to the Style Panel in the previous inspector. Whether that linked to the CSS editor from bug 583041 or not was up to the implementer, and may have been the easiest route to a solution.

Comment 7

[Kevin Dangoor]

We had originally imagined the CSS Inspector allowing you to edit the styles in-place. I think linking directly to the rule in question in the CSS Editor is a better approach because it will allow the user to make changes in full stylesheet context and also allow for saving of changes if they opened via file://. I just changed the bug summary to reflect that.

Comment 8

[Cedric Vivier [:cedricv]]

Ideally this should use the same API than the one we'll use for the GCLI integration.

Comment 10

[Dave Camp (:dcamp)]

Just a note here that we'll need to be able to edit styles that aren't yet represented in the stylesheet at all.  Might need a separate bug for that.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The duplicate bug was marked minotaur so this should inherit the mark.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I have already mostly implemented this, so I may as well take the bug.

Bug triage, filter on PEGASUS.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch WIP

This patch begins the integration but the initialization and manipulation of the style editor appears to be overly complicated. We really need a simple method to do this ... maybe StyleEditor.openStylesheet(URL, line)

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch WIP

There are too many things not working here. The method used to switch to a new stylesheet is failing, needing to listen for a bunch of events and the fact that the style editor can prettify code are all things to take into account.

I can come back to this after fixing other bugs.

Comment 15

[Cedric Vivier [:cedricv]]

Comment on attachment 588394 [details] [diff] [review]
Patch WIP

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

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +583,5 @@
> +          let editor;
> +          let index;
> +          for (index in aChrome.editors) {
> +            let currEditor = aChrome.editors[index];
> +            if (currEditor._styleSheet.href == URI) {

There could be duplicates and this won't work with inline stylesheets.
Ideally the Inspector would pass the DOMStyleSheet object so that it can be matched directly against editor.styleSheet property.

@@ +589,5 @@
> +              break;
> +            }
> +          }
> +
> +          editor.addActionListener({

You need to check that the underlying SourceEditor is not already attached before listening to onAttach (which might not fire then... in a fashion similar to the document.readyState check above on a 'load' event) :

if (typeof line != "undefined") {
  if (!editor.sourceEditor) { // SourceEditor is not ready yet
    editor.addActionListener({
      onAttach: function (aEditor) {
        aEditor.sourceEditor.setCaretPosition(line - 1, 0);
      }
    });
  } else {
    editor.sourceEditor.setCaretPosition(line - 1, 0);
  }
}

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Just need to get a test working.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch

Comment 18

[Cedric Vivier [:cedricv]]

Comment on attachment 591417 [details] [diff] [review]
Patch

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

::: browser/base/content/browser.js
@@ +9049,5 @@
> +  /**
> +   * Opens the style editor
> +   *
> +   * @param {CSSStyleSheet} [aLockToSheet] stylesheet object that the style
> +   * editor would be locked to.

It sounds confusing to me that we open the Style Editor loading only one open style sheet in the UI (if I read the lockToSheet comments right).
This is even worse if you already have the Style Editor opened, as the user can then lose changes he have done in other style sheets.

IMHO should open the Style Editor as usual and just auto-select the wanted style sheet.

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Completely revised patch

(In reply to Cedric Vivier [:cedricv] from comment #18)
> Comment on attachment 591417 [details] [diff] [review]
> Patch
> 
> Review of attachment 591417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +9049,5 @@
> > +  /**
> > +   * Opens the style editor
> > +   *
> > +   * @param {CSSStyleSheet} [aLockToSheet] stylesheet object that the style
> > +   * editor would be locked to.
> 
> It sounds confusing to me that we open the Style Editor loading only one
> open style sheet in the UI (if I read the lockToSheet comments right).
> This is even worse if you already have the Style Editor opened, as the user
> can then lose changes he have done in other style sheets.
> 
> IMHO should open the Style Editor as usual and just auto-select the wanted
> style sheet.

I think that there was some miscommunication when we first discussed it. Anyhow, the revised patch loads all stylesheets and selects the line and sheet as requested.

Usage is simple:
StyleEditor.openChrome(CSSSheet, line - 1);

Line is index based for consistency with sourceEditor.setCaretPosition.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebase

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Line number is now line number instead of index

By popular demand line number is now line number instead of index

Comment 22

[Dave Camp (:dcamp)]

Comment on attachment 593918 [details] [diff] [review]
Line number is now line number instead of index

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

Would like to see some small changes in the rule view integration, but looks good other than that.  Pinging cedric for feedback to double-check the style editor parts.

::: browser/base/content/browser.js
@@ -9075,0 +9084,6 @@
> > +    let args = {
> > +      contentWindow: contentWindow,
> > +      defaultSheet: aDefaultSheet,
> > +      startLine: aLine
NaN more ...

Why is that last line necessary?

::: browser/devtools/styleeditor/StyleEditor.jsm
@@ +90,5 @@
>   *        The content document where changes will be applied to.
>   * @param DOMStyleSheet aStyleSheet
>   *        Optional. The DOMStyleSheet to edit.
>   *        If not set, a new empty style sheet will be appended to the document.
> + * @param {Number} [aLine] Zero based line on which to place caret

Looks like this comment needs to be updated.

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +899,5 @@
> +    } else {
> +      let href = this.rule.elementStyle.element.ownerDocument.location.href;
> +      this.win.openUILinkIn("view-source:" + href, "window");
> +    }
> +  },

I'd rather not have integration with a completely different part of our code so deep in the middle of the rule view's implementation.  The rule view shouldn't need to know about the chrome window in which it is embedded.

For other places that the rule view interacts with the inspector we emit an event from the CssRuleView that the inspector forwards appropriately.  Can we do that here?

Comment 23

[Dave Camp (:dcamp)]

(In reply to Dave Camp (:dcamp) from comment #22)
> ::: browser/base/content/browser.js
> @@ -9075,0 +9084,6 @@
> > > +    let args = {
> > > +      contentWindow: contentWindow,
> > > +      defaultSheet: aDefaultSheet,
> > > +      startLine: aLine
> NaN more ...
> 
> Why is that last line necessary?

Grumble, splinter.  I mean the args.wrappedJSObject = args line.

> I'd rather not have integration with a completely different part of our code
> so deep in the middle of the rule view's implementation.  The rule view
> shouldn't need to know about the chrome window in which it is embedded.
> 
> For other places that the rule view interacts with the inspector we emit an
> event from the CssRuleView that the inspector forwards appropriately.  Can
> we do that here?

To be clear, this isn't out of an abstract desire to keep the rule view "clean" or anything - having the rule view dig around in the implementation of the browser/inspector/whatever to find the style editor launching code just makes stuff harder to refactor down the road.  I've already run in to annoyances refactoring the inspector because the computed view pokes around its internals, and don't want that to spread to the rule view.

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Used custom event from rule view

cedricv: dcamp asked if you could double-check the style editor parts.

(In reply to Dave Camp (:dcamp) from comment #22)
> Comment on attachment 593918 [details] [diff] [review]
> Line number is now line number instead of index
> 
> Review of attachment 593918 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Would like to see some small changes in the rule view integration, but looks
> good other than that.  Pinging cedric for feedback to double-check the style
> editor parts.
> 
> ::: browser/devtools/styleeditor/StyleEditor.jsm
> @@ +90,5 @@
> >   *        The content document where changes will be applied to.
> >   * @param DOMStyleSheet aStyleSheet
> >   *        Optional. The DOMStyleSheet to edit.
> >   *        If not set, a new empty style sheet will be appended to the document.
> > + * @param {Number} [aLine] Zero based line on which to place caret
> 
> Looks like this comment needs to be updated.
> 

Done

> ::: browser/devtools/styleinspector/CssRuleView.jsm
> @@ +899,5 @@
> > +    } else {
> > +      let href = this.rule.elementStyle.element.ownerDocument.location.href;
> > +      this.win.openUILinkIn("view-source:" + href, "window");
> > +    }
> > +  },
> 
> I'd rather not have integration with a completely different part of our code
> so deep in the middle of the rule view's implementation.  The rule view
> shouldn't need to know about the chrome window in which it is embedded.
> 
> For other places that the rule view interacts with the inspector we emit an
> event from the CssRuleView that the inspector forwards appropriately.  Can
> we do that here?

> To be clear, this isn't out of an abstract desire to keep the rule view
> "clean" or anything - having the rule view dig around in the implementation
> of the browser/inspector/whatever to find the style editor launching code
> just makes stuff harder to refactor down the road.  I've already run in to
> annoyances refactoring the inspector because the computed view pokes around
> its internals, and don't want that to spread to the rule view.

I have used a custom event to manage this but this brings up a new question. Of course it would be better if the style inspector simply used events for things like this but, as other tools follow, we will turn out with more and more stuff in inspector.jsm that really belongs inside the tools themselves.

Should we not find a clean way of managing these event listeners from inspector.jsm instead of having to move a bunch of the tools methods into inspector.jsm? I mean, people should be able to create extensions and just plug their tools into the inspector. The obvious way is to use registertools to add extra open and close events to tools but maybe we should discuss it in a scrum.

(In reply to Dave Camp (:dcamp) from comment #23)
> (In reply to Dave Camp (:dcamp) from comment #22)
> > ::: browser/base/content/browser.js
> > @@ -9075,0 +9084,6 @@
> > > > +    let args = {
> > > > +      contentWindow: contentWindow,
> > > > +      defaultSheet: aDefaultSheet,
> > > > +      startLine: aLine
> > NaN more ...
> > 
> > Why is that last line necessary?
> 
> Grumble, splinter.  I mean the args.wrappedJSObject = args line.
> 

Because otherwise it is undefined when we try to access it from the XUL window. xpcom only allows us to pass primitives, and objects with XPCOM interfaces to XUL windows. In order to pass anything else (in this case a Javascript object) we need to wrap the object to be passed using myObj.wrappedJSObject = myObj so that xpconnect can wrap it as a generic nsISupports.

Comment 25

[Dave Camp (:dcamp)]

(In reply to Michael Ratcliffe from comment #24)

> I have used a custom event to manage this but this brings up a new question.
> Of course it would be better if the style inspector simply used events for
> things like this but, as other tools follow, we will turn out with more and
> more stuff in inspector.jsm that really belongs inside the tools themselves.
>
> Should we not find a clean way of managing these event listeners from
> inspector.jsm instead of having to move a bunch of the tools methods into
> inspector.jsm? I mean, people should be able to create extensions and just
> plug their tools into the inspector. The obvious way is to use registertools
> to add extra open and close events to tools but maybe we should discuss it
> in a scrum.

Yeah, we need to define a clean set of integration APIs from the inspector, and let registered tools access those easily.  But in the meantime, each tool passing around and digging through the browser window to find APIs is causing headaches.

Comment 26

[Cedric Vivier [:cedricv]]

Comment on attachment 594184 [details] [diff] [review]
Used custom event from rule view

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

This will be useful :)

f+ with few nits and clean ups needed.

::: browser/base/content/browser.js
@@ +9053,5 @@
>  
>  var StyleEditor = {
>    prefEnabledName: "devtools.styleeditor.enabled",
> +  /**
> +   * Opens the style editor

Opens the Style Editor UI.
If the UI is already open, it will be focused.

@@ +9055,5 @@
>    prefEnabledName: "devtools.styleeditor.enabled",
> +  /**
> +   * Opens the style editor
> +   *
> +   * @param {CSSStyleSheet} [aDefaultSheet] default Stylesheet.

I'm not sure to understand the meaning of "default" here.
Wouldn't "aInitialSheet" or "aSelectedSheet" be more self-explaining?

I would lean towards the latter, since calling openChrome() when Style Editor is already open puts it to foreground.

@@ +9056,5 @@
> +  /**
> +   * Opens the style editor
> +   *
> +   * @param {CSSStyleSheet} [aDefaultSheet] default Stylesheet.
> +   * @param {Number} [aLine] Line on which to place caret

Line on which to move caret to (one-indexed)


Also, I think we should add an optional aCol.
It will provide better UX in many cases (indented style sheets).

On minified style sheets this is critical (since the whole sheet is all on one line) - of course for the specific minified case to work, we'll have to make StyleEditor build some kind of sourcemap in a follow-up bug.

@@ +9073,5 @@
>      while (enumerator.hasMoreElements()) {
>        var win = enumerator.getNext();
>        if (win.styleEditorChrome.contentWindowID == contentWindowID) {
> +        if (aDefaultSheet) {
> +          win.styleEditorChrome.selectSheet(aDefaultSheet, aLine || 1);

aLine alone, if the optional aLine is not specified selectSheet should just select the style sheet without moving the caret.

::: browser/devtools/styleeditor/StyleEditor.jsm
@@ +90,5 @@
>   *        The content document where changes will be applied to.
>   * @param DOMStyleSheet aStyleSheet
>   *        Optional. The DOMStyleSheet to edit.
>   *        If not set, a new empty style sheet will be appended to the document.
> + * @param {Number} [aLine] Line on which to place caret

Add (one-indexed).

@param aCol (zero-indexed)

@@ +101,5 @@
>  
>    this._document = aDocument; // @see contentDocument
>    this._inputElement = null;  // @see inputElement
>    this._sourceEditor = null;  // @see sourceEditor
> +  this._defaultLine = aLine || 1;

this._initialLine - no "|| 1"

@@ +247,5 @@
>  
>        sourceEditor.setTopIndex(this._state.topIndex);
>        sourceEditor.setSelection(this._state.selection.start,
>                                  this._state.selection.end);
> +      sourceEditor.setCaretPosition(this._defaultLine - 1, 0);

if (this._initialLine) {
   setCaretPosition + col
}

::: browser/devtools/styleeditor/StyleEditorChrome.jsm
@@ +70,2 @@
>   */
> +function StyleEditorChrome(aRoot, aContentWindow, aDefaultSheet, aStartLine)

I don't think we need to complexify the constructor with aDefaultSheet and aStartLine here since we need to expose selectStyleSheet function anyways (to select when StyleEditorChrome is already instantiated).

let chrome = new StyleEditorChrome(..);
chrome.selectStyleSheet(sheet, line, col);

is simple enough for a not so common use case.

@@ +74,5 @@
>    this._root = aRoot;
>    this._document = this._root.ownerDocument;
>    this._window = this._document.defaultView;
> +  this._defaultSheet = aDefaultSheet;
> +  this._startLine = aStartLine || 0;

Remove both.

@@ +340,4 @@
>      for (let i = 0; i < document.styleSheets.length; ++i) {
>        let styleSheet = document.styleSheets[i];
>  
> +      let editor = new StyleEditor(document, styleSheet, this._startLine);

This is unexpected.
This will open all editors (for every style sheets) at the line provided in the StyleEditorChrome constructor for aDefaultSheet.

We want the caret moved to this line, only on the selected style sheet.

@@ +356,5 @@
>      }, this);
>    },
>  
>    /**
> +   * selects a stylesheet and optionally moves the cursor to a selected line

Selected - uppercase.

@@ +358,5 @@
>  
>    /**
> +   * selects a stylesheet and optionally moves the cursor to a selected line
> +   *
> +   * @param aSheet stylesheet that should be selected by default

I think we can remove the "by default" again here.
This function can be called anytime by any tool.

DOMCSSStyleSheet aSheet Style sheet to select.

@@ +359,5 @@
>    /**
> +   * selects a stylesheet and optionally moves the cursor to a selected line
> +   *
> +   * @param aSheet stylesheet that should be selected by default
> +   * @param [aLine] The line to move to

Number  Optional. Line to move the caret to (one-indexed).

@@ +361,5 @@
> +   *
> +   * @param aSheet stylesheet that should be selected by default
> +   * @param [aLine] The line to move to
> +   */
> +  selectSheet: function SEC_selectSheet(aSheet, aLine)

Would be nice to add a selectedSheet getter.

@@ +366,5 @@
> +  {
> +    let document = this.contentDocument;
> +    let needsRefresh = false;
> +    let defaultSheetIndex;
> +

selectedSheetIndex

@@ +373,5 @@
> +        let styleSheet = document.styleSheets[i];
> +        let editor = this._editors[i];
> +
> +        if (!styleSheet == editor._styleSheet) {
> +          needsRefresh = true;

I don't understand this. Why do you need to "refresh" via populateChrome?

@@ +393,5 @@
> +        editor.addActionListener({
> +          onAttach:function (aEditor)
> +          {
> +            aEditor.removeActionListener(this);
> +            aEditor.sourceEditor.setCaretPosition(aLine - 1, 0);

if (aLine) {
  setCaretPosition
}

@@ +401,5 @@
> +      } else {
> +        if (this._view.activeSummary != summary) {
> +          this._view.activeSummary = summary;
> +        }
> +        editor.sourceEditor.setCaretPosition(aLine - 1, 0);

Same.

@@ +510,5 @@
>            }
>          }, false);
>  
> +        // autofocus the default, first or new stylesheet
> +        if (this._defaultSheet) {

Remove this change.
Reuse selectSheet and the new selectedSheet getter:

if (!this.selectedSheet || editor.hasFlag(StyleEditorFlags.NEW)) {
  this.selectSheet(editor.styleSheet);
}

::: browser/devtools/styleeditor/styleeditor.xul
@@ +133,5 @@
>  Components.utils.import("resource:///modules/devtools/StyleEditorChrome.jsm");
>  let chromeRoot = document.getElementById("style-editor-chrome");
> +let args = window.arguments[0].wrappedJSObject;
> +let contentWindow = args.contentWindow;
> +let defaultSheet = args.defaultSheet;

remove defaultSheet and startLine vars

@@ +135,5 @@
> +let args = window.arguments[0].wrappedJSObject;
> +let contentWindow = args.contentWindow;
> +let defaultSheet = args.defaultSheet;
> +let startLine = args.startLine;
> +let chrome = new StyleEditorChrome(chromeRoot, contentWindow, defaultSheet, startLine);

add
if (args.selectedSheet) {
   chrome.selectSheet(args.selectedSheet, args.line, args.col);
}

::: browser/devtools/styleeditor/test/browser_styleeditor_passedinsheet.js
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const TESTCASE_URI = TEST_BASE + "singlesheet.html";

Hmm.. let's be bold and test against a page with multiple sheets :p
We can reuse simple.html so that we do not need to add the new singlesheet.html file.

@@ +20,5 @@
> +
> +function run()
> +{
> +  sheet = content.document.styleSheets[0];
> +  launchStyleEditorChrome(function attachListeners(aChrome) {

aChrome.selectSheet..

::: browser/devtools/styleeditor/test/head.js
@@ +18,5 @@
>      gBrowser.removeCurrentTab();
>    }
>  }
>  
> +function launchStyleEditorChrome(aCallback, aSheet, aLine)

no need to add aSheet/aLine here anymore.

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +984,5 @@
> +   */
> +  openStyleEditor: function(aEvent)
> +  {
> +    if (this.selectorInfo.selector._cssRule._cssSheet) {
> +      let DOMSheet = this.selectorInfo.selector._cssRule._cssSheet.domSheet;

DOMSheet - code style? "sheet"?

Comment 27

[Cedric Vivier [:cedricv]]

(In reply to Cedric Vivier [:cedricv] from comment #26)
> Review of attachment 594184 [details] [diff] [review]:

One last thing, I think it would be nicer to call the new APIs selectStyleSheet and selectedStyleSheet (long-form rather than "Sheet") for consistency with existing DOM and StyleEditor APIs.

Comment 28

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Another rewrite

Cedric, you may want to take a another pass over this. I have address most, if not all of your comments with this rewrite and fixed a couple of bugs in the listener mechanism.

(In reply to Cedric Vivier [:cedricv] from comment #26)
> Comment on attachment 594184 [details] [diff] [review]
> Used custom event from rule view
> 
> Review of attachment 594184 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This will be useful :)
> 
> f+ with few nits and clean ups needed.
> 
> ::: browser/base/content/browser.js
> @@ +9053,5 @@
> >  
> >  var StyleEditor = {
> >    prefEnabledName: "devtools.styleeditor.enabled",
> > +  /**
> > +   * Opens the style editor
> 
> Opens the Style Editor UI.
> If the UI is already open, it will be focused.
> 

Done

> @@ +9055,5 @@
> >    prefEnabledName: "devtools.styleeditor.enabled",
> > +  /**
> > +   * Opens the style editor
> > +   *
> > +   * @param {CSSStyleSheet} [aDefaultSheet] default Stylesheet.
> 
> I'm not sure to understand the meaning of "default" here.
> Wouldn't "aInitialSheet" or "aSelectedSheet" be more self-explaining?
> 
> I would lean towards the latter, since calling openChrome() when Style
> Editor is already open puts it to foreground.
> 

Done

> @@ +9056,5 @@
> > +  /**
> > +   * Opens the style editor
> > +   *
> > +   * @param {CSSStyleSheet} [aDefaultSheet] default Stylesheet.
> > +   * @param {Number} [aLine] Line on which to place caret
> 
> Line on which to move caret to (one-indexed)
> 
> 

Done

> Also, I think we should add an optional aCol.
> It will provide better UX in many cases (indented style sheets).
> 

Agreed, done

> On minified style sheets this is critical (since the whole sheet is all on
> one line) - of course for the specific minified case to work, we'll have to
> make StyleEditor build some kind of sourcemap in a follow-up bug.
> 

Bug 724505 logged

> @@ +9073,5 @@
> >      while (enumerator.hasMoreElements()) {
> >        var win = enumerator.getNext();
> >        if (win.styleEditorChrome.contentWindowID == contentWindowID) {
> > +        if (aDefaultSheet) {
> > +          win.styleEditorChrome.selectSheet(aDefaultSheet, aLine || 1);
> 
> aLine alone, if the optional aLine is not specified selectSheet should just
> select the style sheet without moving the caret.
> 

Done

> ::: browser/devtools/styleeditor/StyleEditor.jsm
> @@ +90,5 @@
> >   *        The content document where changes will be applied to.
> >   * @param DOMStyleSheet aStyleSheet
> >   *        Optional. The DOMStyleSheet to edit.
> >   *        If not set, a new empty style sheet will be appended to the document.
> > + * @param {Number} [aLine] Line on which to place caret
> 
> Add (one-indexed).
> 
> @param aCol (zero-indexed)
> 

Done

> @@ +101,5 @@
> >  
> >    this._document = aDocument; // @see contentDocument
> >    this._inputElement = null;  // @see inputElement
> >    this._sourceEditor = null;  // @see sourceEditor
> > +  this._defaultLine = aLine || 1;
> 
> this._initialLine - no "|| 1"
> 

Done

> @@ +247,5 @@
> >  
> >        sourceEditor.setTopIndex(this._state.topIndex);
> >        sourceEditor.setSelection(this._state.selection.start,
> >                                  this._state.selection.end);
> > +      sourceEditor.setCaretPosition(this._defaultLine - 1, 0);
> 
> if (this._initialLine) {
>    setCaretPosition + col
> }
> 

Not needed due to other changes

> ::: browser/devtools/styleeditor/StyleEditorChrome.jsm
> @@ +70,2 @@
> >   */
> > +function StyleEditorChrome(aRoot, aContentWindow, aDefaultSheet, aStartLine)
> 
> I don't think we need to complexify the constructor with aDefaultSheet and
> aStartLine here since we need to expose selectStyleSheet function anyways
> (to select when StyleEditorChrome is already instantiated).
> 
> let chrome = new StyleEditorChrome(..);
> chrome.selectStyleSheet(sheet, line, col);
> 
> is simple enough for a not so common use case.
> 

If you say so, done.

> @@ +74,5 @@
> >    this._root = aRoot;
> >    this._document = this._root.ownerDocument;
> >    this._window = this._document.defaultView;
> > +  this._defaultSheet = aDefaultSheet;
> > +  this._startLine = aStartLine || 0;
> 
> Remove both.
> 

Done

> @@ +340,4 @@
> >      for (let i = 0; i < document.styleSheets.length; ++i) {
> >        let styleSheet = document.styleSheets[i];
> >  
> > +      let editor = new StyleEditor(document, styleSheet, this._startLine);
> 
> This is unexpected.
> This will open all editors (for every style sheets) at the line provided in
> the StyleEditorChrome constructor for aDefaultSheet.
> 
> We want the caret moved to this line, only on the selected style sheet.
> 

This was removed when addressing other comments

> @@ +356,5 @@
> >      }, this);
> >    },
> >  
> >    /**
> > +   * selects a stylesheet and optionally moves the cursor to a selected line
> 
> Selected - uppercase.
> 

Why?

> @@ +358,5 @@
> >  
> >    /**
> > +   * selects a stylesheet and optionally moves the cursor to a selected line
> > +   *
> > +   * @param aSheet stylesheet that should be selected by default
> 
> I think we can remove the "by default" again here.
> This function can be called anytime by any tool.
> 
> DOMCSSStyleSheet aSheet Style sheet to select.
> 

Done

> @@ +359,5 @@
> >    /**
> > +   * selects a stylesheet and optionally moves the cursor to a selected line
> > +   *
> > +   * @param aSheet stylesheet that should be selected by default
> > +   * @param [aLine] The line to move to
> 
> Number  Optional. Line to move the caret to (one-indexed).
> 

And column ... done

> @@ +361,5 @@
> > +   *
> > +   * @param aSheet stylesheet that should be selected by default
> > +   * @param [aLine] The line to move to
> > +   */
> > +  selectSheet: function SEC_selectSheet(aSheet, aLine)
> 
> Would be nice to add a selectedSheet getter.
> 

Done

> @@ +366,5 @@
> > +  {
> > +    let document = this.contentDocument;
> > +    let needsRefresh = false;
> > +    let defaultSheetIndex;
> > +
> 
> selectedSheetIndex
> 

Done

> @@ +373,5 @@
> > +        let styleSheet = document.styleSheets[i];
> > +        let editor = this._editors[i];
> > +
> > +        if (!styleSheet == editor._styleSheet) {
> > +          needsRefresh = true;
> 
> I don't understand this. Why do you need to "refresh" via populateChrome?
> 

Removed

> @@ +393,5 @@
> > +        editor.addActionListener({
> > +          onAttach:function (aEditor)
> > +          {
> > +            aEditor.removeActionListener(this);
> > +            aEditor.sourceEditor.setCaretPosition(aLine - 1, 0);
> 
> if (aLine) {
>   setCaretPosition
> }
> 

Done

> @@ +401,5 @@
> > +      } else {
> > +        if (this._view.activeSummary != summary) {
> > +          this._view.activeSummary = summary;
> > +        }
> > +        editor.sourceEditor.setCaretPosition(aLine - 1, 0);
> 
> Same.
> 

Done

> @@ +510,5 @@
> >            }
> >          }, false);
> >  
> > +        // autofocus the default, first or new stylesheet
> > +        if (this._defaultSheet) {
> 
> Remove this change.
> Reuse selectSheet and the new selectedSheet getter:
> 
> if (!this.selectedSheet || editor.hasFlag(StyleEditorFlags.NEW)) {
>   this.selectSheet(editor.styleSheet);
> }
> 

Done

> ::: browser/devtools/styleeditor/styleeditor.xul
> @@ +133,5 @@
> >  Components.utils.import("resource:///modules/devtools/StyleEditorChrome.jsm");
> >  let chromeRoot = document.getElementById("style-editor-chrome");
> > +let args = window.arguments[0].wrappedJSObject;
> > +let contentWindow = args.contentWindow;
> > +let defaultSheet = args.defaultSheet;
> 
> remove defaultSheet and startLine vars
> 

Done

> @@ +135,5 @@
> > +let args = window.arguments[0].wrappedJSObject;
> > +let contentWindow = args.contentWindow;
> > +let defaultSheet = args.defaultSheet;
> > +let startLine = args.startLine;
> > +let chrome = new StyleEditorChrome(chromeRoot, contentWindow, defaultSheet, startLine);
> 
> add
> if (args.selectedSheet) {
>    chrome.selectSheet(args.selectedSheet, args.line, args.col);
> }
> 

Done

> ::: browser/devtools/styleeditor/test/browser_styleeditor_passedinsheet.js
> @@ +1,5 @@
> > +/* vim: set ts=2 et sw=2 tw=80: */
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +const TESTCASE_URI = TEST_BASE + "singlesheet.html";
> 
> Hmm.. let's be bold and test against a page with multiple sheets :p
> We can reuse simple.html so that we do not need to add the new
> singlesheet.html file.
> 

Done

> @@ +20,5 @@
> > +
> > +function run()
> > +{
> > +  sheet = content.document.styleSheets[0];
> > +  launchStyleEditorChrome(function attachListeners(aChrome) {
> 
> aChrome.selectSheet..
> 

If we did that then the first editor would be loaded (default action) and then selectSheet would move to the required one. A nightmare when we need to listen for event listeners.

> ::: browser/devtools/styleeditor/test/head.js
> @@ +18,5 @@
> >      gBrowser.removeCurrentTab();
> >    }
> >  }
> >  
> > +function launchStyleEditorChrome(aCallback, aSheet, aLine)
> 
> no need to add aSheet/aLine here anymore.
> 

There still is, passing these to openChrome still makes sense, see previous comment.

> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +984,5 @@
> > +   */
> > +  openStyleEditor: function(aEvent)
> > +  {
> > +    if (this.selectorInfo.selector._cssRule._cssSheet) {
> > +      let DOMSheet = this.selectorInfo.selector._cssRule._cssSheet.domSheet;
> 
> DOMSheet - code style? "sheet"?

Changed

(In reply to Cedric Vivier [:cedricv] from comment #27)
> (In reply to Cedric Vivier [:cedricv] from comment #26)
> > Review of attachment 594184 [details] [diff] [review]:
> 
> One last thing, I think it would be nicer to call the new APIs
> selectStyleSheet and selectedStyleSheet (long-form rather than "Sheet") for
> consistency with existing DOM and StyleEditor APIs.

Changed

Comment 29

[Cedric Vivier [:cedricv]]

Comment on attachment 595717 [details] [diff] [review]
Another rewrite

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

Cool. Just few very minor nits :)

::: browser/devtools/styleeditor/StyleEditor.jsm
@@ +928,5 @@
>        let actionHandler = listener["on" + aName];
>        if (actionHandler) {
>          actionHandler.apply(listener, aArgs);
> +        if (this._actionListeners.length < startLen) {
> +          // The listener has removed itself. Let's adjust the index accordingly.

Hehe, I also had to fix this recently in a WIP patch.
I think cloning the listeners array is a safer/shorter approach as it still works if the listeners is remove more than itself or adding new ones.

// copy the list of listeners to allow adding/removing listeners in handlers
let listeners = this._actionListeners.concat();
// trigger all listeners that have this action handler
for (let i = 0; i < listeners.length; ++i) {
  let listener = listeners[i];
  let actionHandler = listener["on" + aName];
  if (actionHandler) {
    actionHandler.apply(listener, aArgs);
  }
}

::: browser/devtools/styleeditor/StyleEditorChrome.jsm
@@ +277,5 @@
>        let handler = listener["on" + aName];
>        if (handler) {
>          handler.apply(listener, aArgs);
> +        if (this._listeners.length < startLen) {
> +          // The listener has removed itself. Let's adjust the index accordingly.

Same.

@@ +365,5 @@
> +   * @param {Number} [aCol] Column to which the caret should be moved (one-indexed).
> +   */
> +  selectStyleSheet: function SEC_selectSheet(aSheet, aLine, aCol)
> +  {
> +      let select = function DEC_select(aEditor) {

Indentation? (if Splinter is not doing nonsense, it should be 2 here not 4)
"DEC_" ?

@@ +388,5 @@
> +            });
> +          }
> +          this._view.activeSummary = summary;
> +        } else {
> +          if (this._view.activeSummary != summary) {

You can remove this check, SplitView handles this already.

@@ +411,5 @@
> +            select(aEditor);
> +          }
> +        }
> +      });
> +    } else if (aSheet) {

Unnecessary conditional, aSheet is required for this function.
We could just return in previous block and unindent this one as the default/common path anyways.

::: browser/devtools/styleeditor/test/browser_styleeditor_passedinsheet.js
@@ +49,5 @@
> +  let sourceEditor = aEditor.sourceEditor;
> +  let caretPosition = sourceEditor.getCaretPosition();
> +  is(caretPosition.line, LINE - 1, "caret row is correct"); // index based
> +  is(caretPosition.col, COL - 1, "caret column is correct");
> +  is(aEditor._styleSheet, sheet, "loaded stylesheet matches document stylesheet");

You should rather use aEditor.styleSheet, which is the public API.

::: browser/themes/gnomestripe/devtools/csshtmltree.css
@@ +131,5 @@
>  .property-content-hidden {
>    display: none;
>  }
>  
>  .rule-link {

I think we should make it look more like a link?
(maybe blue and/or underline)

Comment 30

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [in-fx-team] Addressed Cedric's comments

Dave, can you review this. Cedric has already took us through a few iterations so it should be fine now.

(In reply to Cedric Vivier [:cedricv] from comment #29)
> Comment on attachment 595717 [details] [diff] [review]
> Another rewrite
> 
> Review of attachment 595717 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool. Just few very minor nits :)
> 
> ::: browser/devtools/styleeditor/StyleEditor.jsm
> @@ +928,5 @@
> >        let actionHandler = listener["on" + aName];
> >        if (actionHandler) {
> >          actionHandler.apply(listener, aArgs);
> > +        if (this._actionListeners.length < startLen) {
> > +          // The listener has removed itself. Let's adjust the index accordingly.
> 
> Hehe, I also had to fix this recently in a WIP patch.
> I think cloning the listeners array is a safer/shorter approach as it still
> works if the listeners is remove more than itself or adding new ones.
> 
> // copy the list of listeners to allow adding/removing listeners in handlers
> let listeners = this._actionListeners.concat();
> // trigger all listeners that have this action handler
> for (let i = 0; i < listeners.length; ++i) {
>   let listener = listeners[i];
>   let actionHandler = listener["on" + aName];
>   if (actionHandler) {
>     actionHandler.apply(listener, aArgs);
>   }
> }
> 
> ::: browser/devtools/styleeditor/StyleEditorChrome.jsm
> @@ +277,5 @@
> >        let handler = listener["on" + aName];
> >        if (handler) {
> >          handler.apply(listener, aArgs);
> > +        if (this._listeners.length < startLen) {
> > +          // The listener has removed itself. Let's adjust the index accordingly.
> 
> Same.
> 

Yup, simple but effective, done.

> @@ +365,5 @@
> > +   * @param {Number} [aCol] Column to which the caret should be moved (one-indexed).
> > +   */
> > +  selectStyleSheet: function SEC_selectSheet(aSheet, aLine, aCol)
> > +  {
> > +      let select = function DEC_select(aEditor) {
> 
> Indentation? (if Splinter is not doing nonsense, it should be 2 here not 4)
> "DEC_" ?
> 

Not sure how that happened, fixed.

> @@ +388,5 @@
> > +            });
> > +          }
> > +          this._view.activeSummary = summary;
> > +        } else {
> > +          if (this._view.activeSummary != summary) {
> 
> You can remove this check, SplitView handles this already.
> 

So it does, fixed.

> @@ +411,5 @@
> > +            select(aEditor);
> > +          }
> > +        }
> > +      });
> > +    } else if (aSheet) {
> 
> Unnecessary conditional, aSheet is required for this function.
> We could just return in previous block and unindent this one as the
> default/common path anyways.
> 

The method's comment was not up to date ... here is the updated comment:
   * @param {CSSStyleSheet} [aSheet]
   *        Stylesheet that should be selected. If a stylesheet is not passed
   *        and the editor is not initialized we focus the first stylesheet. If
   *        a stylesheet is not passed and the editor is initialized we ignore
   *        the call.

> ::: browser/devtools/styleeditor/test/browser_styleeditor_passedinsheet.js
> @@ +49,5 @@
> > +  let sourceEditor = aEditor.sourceEditor;
> > +  let caretPosition = sourceEditor.getCaretPosition();
> > +  is(caretPosition.line, LINE - 1, "caret row is correct"); // index based
> > +  is(caretPosition.col, COL - 1, "caret column is correct");
> > +  is(aEditor._styleSheet, sheet, "loaded stylesheet matches document stylesheet");
> 
> You should rather use aEditor.styleSheet, which is the public API.
> 

Of course, done.

> ::: browser/themes/gnomestripe/devtools/csshtmltree.css
> @@ +131,5 @@
> >  .property-content-hidden {
> >    display: none;
> >  }
> >  
> >  .rule-link {
> 
> I think we should make it look more like a link?
> (maybe blue and/or underline)

I assume you mean in the rule view. After changing this to blue and underlining on hover it looks better, partly because it is a link and partly because it makes the rule view's properties far easier to read.

Comment 31

[Mihai Sucan [:msucan]]

Comment on attachment 596026 [details] [diff] [review]
[in-fx-team] Addressed Cedric's comments

Landed:
https://hg.mozilla.org/integration/fx-team/rev/e5ecebbd9631

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e5ecebbd9631

Comment 33

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/Tools/Page_Inspector/Style_panel

And mentioned on Firefox 13 for developers.