703643 - be able to copy from the rules view

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Kevin Dangoor]

From what I have seen, a common workflow is to get the styles looking right in a style inspector and then select and copy that rule out into an editor.

Currently, that is not possible with our rules view. What users expect from other tools (Firebug, Chrome) is that they can select and copy the rule.

It would be a nice bonus if they could copy a rule or individual property/value from the context menu.

Comment 1

[Kevin Dangoor]

bug triage. filter on PEGASUS

Comment 2

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

I have implemented a simple copy (ctrl + c) and context menu copy. Once I have added "copy property" and "copy rule" I will attach the patch.

Comment 3

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

Attachment: Copy from rule & computed views

It is expected that in the rule view dragging the mouse to select text is going to be difficult because focusing a node opens a property editor ... nevertheless, it is working fairly well.

So, the patch adds the following copy methods:
1. Click and drag to select text, ctrl+c or context menu to copy (rule & computed views).
2. Context menu copy rule (rule view).
3. Context menu copy property (rule view).

Comment 4

[Paul Rouget [:paul]]

It works very well. I am reviewing the code and in the meantime, here are some notes about the general behavior:

× Don't add an extra "\n" between the lines when several lines are selected in the computed view
× In "Copy Rule", a "\n" should be inserted before the last bracket.
× Is it possible to not include the "file:line" reference when we select text in the Rule View?

Comment 5

[Paul Rouget [:paul]]

Comment on attachment 602928 [details] [diff] [review]
Copy from rule & computed views

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

This is a first pass. This patch looks a little more complicated that I expected, but it's totally justified. I am happy with the global approach.
Here are some comments about the code and the general feature (see comment 4 as well).

-- Behavior:

The Computed View menu should have more items: "copy value", "copy property name", "copy declaration line".
The cursor in the Computed View should be a text cursor, and clicking a line should not expand it. Only clicking on the twisty should expand the declaration.

-- Tests:

And can we get some tests? The feature I'd like to see tested:
- copy of the different parts of the declarations, make sure the reformating works well.
- make sure we can always use the mouse the select the rule view as plain text,

-- Code:

::: browser/devtools/highlighter/inspector.jsm
@@ +944,5 @@
> +
> +      this.cssRuleViewBoundCopyRule = this.ruleViewCopyRule.bind(this);
> +      this.cssRuleViewBoundCopyProperty = this.ruleViewCopyProperty.bind(this);
> +
> +      this.ruleViewAddContextMenu();

Can you explain in a comment why you do here?

@@ +1058,5 @@
> +  ruleViewAddContextMenu: function IUI_ruleViewAddContextMenu()
> +  {
> +    let iframe = this.getToolIframe(this.ruleViewObject);
> +    let popupSet = this.chromeDoc.getElementById("mainPopupSet");
> +    let menu = this.chromeDoc.createElementNS(XUL_NS, "menupopup");

createElement() doesn't work? It's a XUL document.

@@ +1186,5 @@
> +
> +    let text = node.textContent;
> +
> +    // Format the rule
> +    if (osString == "WINNT") {

So we use the DOS format on Windows. Is that recommended? What do other editor do?

@@ +1221,5 @@
> +    text = text.replace(/;.*/g, ";");
> +
> +    clipboardHelper.copyString(text);
> +  },
> +

I am not sure to understand what's happening here. Can you add a couple of comments?

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +490,5 @@
> +    let popupSet = this.doc.getElementById("mainPopupSet");
> +    let label = CssHtmlTree.l10n("style.contextmenu.copy");
> +    let accessKey = CssHtmlTree.l10n("style.contextmenu.copy.accesskey");
> +
> +    let menu = this.doc.createElementNS(XUL_NS, "menupopup");

Do you need to use createElementNS? Why not just createElement?

@@ +528,5 @@
> +    let win = this.styleDocument.defaultView;
> +    let text = win.getSelection().toString();
> +
> +    // Tidy up block headings
> +    text = text.replace(/(.+)\r?\n\s+/g, "\n$1: ");

Can we get a comment about what you're doing here?

::: browser/locales/en-US/chrome/browser/devtools/styleinspector.properties
@@ +51,5 @@
>  rule.warning.title=Invalid property value
> +
> +# LOCALIZATION NOTE (style.contextmenu.copy): The computed view's context menu
> +# copy entry.
> +style.contextmenu.copy=Copy

s/"Copy"/"Copy selection" (when it's disabled because there's no selection, it's a little confusing).

::: browser/themes/gnomestripe/devtools/csshtmltree.css
@@ +163,5 @@
>    -moz-box-orient: vertical;
>    -moz-box-flex: 1;
>    overflow-y: auto;
>    border-collapse: collapse;
> +  -moz-user-select: text;

In content (same for pin|winstipe)

Comment 6

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

Attachment: Patch

(In reply to Paul Rouget [:paul] from comment #4)
> It works very well. I am reviewing the code and in the meantime, here are
> some notes about the general behavior:
> 
> × Don't add an extra "\n" between the lines when several lines are selected
> in the computed view

Removed

> × In "Copy Rule", a "\n" should be inserted before the last bracket.

One already is ... this is what I get back from "copy rule":
#container .text {
    font-family: fantasy;
}

Are you saying that you want an extra blank line before the closing brace? If you are then I disagree. If you get something different than what I get (worried look), then please let me know what you are getting.

> × Is it possible to not include the "file:line" reference when we select
> text in the Rule View?

Of course ... done.

(In reply to Paul Rouget [:paul] from comment #5)
> Comment on attachment 602928 [details] [diff] [review]
> Copy from rule & computed views
> 
> Review of attachment 602928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a first pass. This patch looks a little more complicated that I
> expected, but it's totally justified. I am happy with the global approach.
> Here are some comments about the code and the general feature (see comment 4
> as well).
> 
> -- Behavior:
> 
> The Computed View menu should have more items: "copy value", "copy property
> name", "copy declaration line".

I can add "Copy property", "Copy property name" & "Copy property value" but I am unsure what you mean by "copy declaration line". I think that the only ones that make sense for the computed view are these ones.

I have also added these to the rule view ... now we have:
Rule view:
- Copy selection
- Copy rule
- Copy property
- Copy property name
- Copy property value

Computed view:
- Copy selection
- Copy property
- Copy property name
- Copy property value

> The cursor in the Computed View should be a text cursor, and clicking a line
> should not expand it. Only clicking on the twisty should expand the
> declaration.
> 

/me has always wanted it that way ... done.

> -- Tests:
> 
> And can we get some tests? The feature I'd like to see tested:
> - copy of the different parts of the declarations, make sure the reformating
> works well.
> - make sure we can always use the mouse to select the rule view as plain
> text,
> 

I have added a bunch of tests.

> -- Code:
> 
> ::: browser/devtools/highlighter/inspector.jsm
> @@ +944,5 @@
> > +
> > +      this.cssRuleViewBoundCopyRule = this.ruleViewCopyRule.bind(this);
> > +      this.cssRuleViewBoundCopyProperty = this.ruleViewCopyProperty.bind(this);
> > +
> > +      this.ruleViewAddContextMenu();
> 
> Can you explain in a comment what you do here?
> 

Done

> @@ +1058,5 @@
> > +  ruleViewAddContextMenu: function IUI_ruleViewAddContextMenu()
> > +  {
> > +    let iframe = this.getToolIframe(this.ruleViewObject);
> > +    let popupSet = this.chromeDoc.getElementById("mainPopupSet");
> > +    let menu = this.chromeDoc.createElementNS(XUL_NS, "menupopup");
> 
> createElement() doesn't work? It's a XUL document.
> 

Fixed.

> @@ +1186,5 @@
> > +
> > +    let text = node.textContent;
> > +
> > +    // Format the rule
> > +    if (osString == "WINNT") {
> 
> So we use the DOS format on Windows. Is that recommended? What do other
> editor do?
> 

Most use UA sniffing (even in the fx codebase), Orion checks current line endings and falls back to UA sniffing. All of them use \r\n for windows boxes and \n otherwise. In my opinion getting nsIXULRuntime.OS to identify the OS makes a heckuvalot more sense.

> @@ +1221,5 @@
> > +    text = text.replace(/;.*/g, ";");
> > +
> > +    clipboardHelper.copyString(text);
> > +  },
> > +
> 
> I am not sure to understand what's happening here. Can you add a couple of
> comments?
> 

I have rewritten ruleViewCopyProperty to make it a little more robust so this code is no longer needed.

> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +490,5 @@
> > +    let popupSet = this.doc.getElementById("mainPopupSet");
> > +    let label = CssHtmlTree.l10n("style.contextmenu.copy");
> > +    let accessKey = CssHtmlTree.l10n("style.contextmenu.copy.accesskey");
> > +
> > +    let menu = this.doc.createElementNS(XUL_NS, "menupopup");
> 
> Do you need to use createElementNS? Why not just createElement?
> 

Fixed.

> @@ +528,5 @@
> > +    let win = this.styleDocument.defaultView;
> > +    let text = win.getSelection().toString();
> > +
> > +    // Tidy up block headings
> > +    text = text.replace(/(.+)\r?\n\s+/g, "\n$1: ");
> 
> Can we get a comment about what you're doing here?
> 

Done

> ::: browser/locales/en-US/chrome/browser/devtools/styleinspector.properties
> @@ +51,5 @@
> >  rule.warning.title=Invalid property value
> > +
> > +# LOCALIZATION NOTE (style.contextmenu.copy): The computed view's context menu
> > +# copy entry.
> > +style.contextmenu.copy=Copy
> 
> s/"Copy"/"Copy selection" (when it's disabled because there's no selection,
> it's a little confusing).
> 

Done

> ::: browser/themes/gnomestripe/devtools/csshtmltree.css
> @@ +163,5 @@
> >    -moz-box-orient: vertical;
> >    -moz-box-flex: 1;
> >    overflow-y: auto;
> >    border-collapse: collapse;
> > +  -moz-user-select: text;
> 
> In content (same for pin|winstipe)

The computed view had no in-content stylesheet until bug 705276 is fixed (Split style inspector CSS between content & document CSS) ... I have upped it's priority to P2.

Comment 7

[Paul Rouget [:paul]]

(In reply to Michael Ratcliffe from comment #6)
> Created attachment 603707 [details] [diff] [review]
> Patch
> > × In "Copy Rule", a "\n" should be inserted before the last bracket.
> 
> One already is ... this is what I get back from "copy rule":
> #container .text {
>     font-family: fantasy;
> }
> 
> Are you saying that you want an extra blank line before the closing brace?
> If you are then I disagree. If you get something different than what I get
> (worried look), then please let me know what you are getting.

I got something like that in the past:

#container .text {
    font-family: fantasy;}


But I can't reproduce :/

> > × Is it possible to not include the "file:line" reference when we select
> > text in the Rule View?
> 
> Of course ... done.

I still see some reference: http://i.imgur.com/q6M6d.png
I think you should just make the source not selectable in CSS (using -moz-user-select) (I am not 100% sure about this).

Other comment:

ctrl-a + ctrl-c copies should copy a formatted version of the CSS (right now, it's one single line with the file reference.

> > The Computed View menu should have more items: "copy value", "copy property
> > name", "copy declaration line".
> 
> I can add "Copy property", "Copy property name" & "Copy property value" but
> I am unsure what you mean by "copy declaration line". I think that the only
> ones that make sense for the computed view are these ones.

declaration == property + property value.
http://www.w3.org/TR/1998/REC-CSS2-19980512/syndata.html#declaration
As this is not a commonly known term, I think adding "line" at the end makes it a little more understandable.

> I have also added these to the rule view ... now we have:
> Rule view:
> - Copy selection
> - Copy rule
> - Copy property

Copy declaration

> - Copy property name

Copy property

> - Copy property value
> 
> Computed view:
> - Copy selection
> - Copy property
> - Copy property name

Same things here

> > The cursor in the Computed View should be a text cursor, and clicking a line
> > should not expand it. Only clicking on the twisty should expand the
> > declaration.
> > 
> 
> /me has always wanted it that way ... done.

Can you also make sure the right element is focused then?
When I click on a property, I want the twisty of their common parent
to be focused.

> > ::: browser/themes/gnomestripe/devtools/csshtmltree.css
> > @@ +163,5 @@
> > >    -moz-box-orient: vertical;
> > >    -moz-box-flex: 1;
> > >    overflow-y: auto;
> > >    border-collapse: collapse;
> > > +  -moz-user-select: text;
> > 
> > In content (same for pin|winstipe)
> 
> The computed view had no in-content stylesheet until bug 705276 is fixed
> (Split style inspector CSS between content & document CSS) ... I have upped
> it's priority to P2.

ok.

Comment 8

[Paul Rouget [:paul]]

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

See comment 5

Comment 9

[Paul Rouget [:paul]]

(see also bug 733880)

Comment 10

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

Attachment: Patch

(In reply to Paul Rouget [:paul] from comment #7)
> (In reply to Michael Ratcliffe from comment #6)
> > Created attachment 603707 [details] [diff] [review]
> > Patch
> > > × In "Copy Rule", a "\n" should be inserted before the last bracket.
> > 
> > One already is ... this is what I get back from "copy rule":
> > #container .text {
> >     font-family: fantasy;
> > }
> > 
> > Are you saying that you want an extra blank line before the closing brace?
> > If you are then I disagree. If you get something different than what I get
> > (worried look), then please let me know what you are getting.
> 
> I got something like that in the past:
> 
> #container .text {
>     font-family: fantasy;}
> 
> 
> But I can't reproduce :/
> 

Interesting that it copied without whitespace before the } ... I have changed the regex to cover this if it happens again.

> > > × Is it possible to not include the "file:line" reference when we select
> > > text in the Rule View?
> > 
> > Of course ... done.
> 
> I still see some reference: http://i.imgur.com/q6M6d.png

Fixed

> I think you should just make the source not selectable in CSS (using
> -moz-user-select) (I am not 100% sure about this).
> 

Unfortunately, doing it that way means that you can't drag the mouse across the text if you start from one of these headers. In fact, setting moz-user-select: none made selection really hard to use. We just need to use regexes for this kind of thing.

> Other comment:
> 
> ctrl-a + ctrl-c copies should copy a formatted version of the CSS (right
> now, it's one single line with the file reference.
> 

Fixed

> > > The Computed View menu should have more items: "copy value", "copy property
> > > name", "copy declaration line".
> > 
> > I can add "Copy property", "Copy property name" & "Copy property value" but
> > I am unsure what you mean by "copy declaration line". I think that the only
> > ones that make sense for the computed view are these ones.
> 
> declaration == property + property value.
> http://www.w3.org/TR/1998/REC-CSS2-19980512/syndata.html#declaration
> As this is not a commonly known term, I think adding "line" at the end makes
> it a little more understandable.
> 

It is an official term so I don't think we should shy away from using it. Copy declaration is fine.

> > I have also added these to the rule view ... now we have:
> > Rule view:
> > - Copy selection
> > - Copy rule
> > - Copy property
> 
> Copy declaration
> 
> > - Copy property name
> 
> Copy property
> 
> > - Copy property value
> > 
> > Computed view:
> > - Copy selection
> > - Copy property
> > - Copy property name
> 
> Same things here
> 

So now we have:
Rule view:
- Copy selection
- Copy rule
- Copy declaration
- Copy property
- Copy property value

Computed view:
- Copy selection
- Copy declaration
- Copy property
- Copy property value

> > > The cursor in the Computed View should be a text cursor, and clicking a line
> > > should not expand it. Only clicking on the twisty should expand the
> > > declaration.
> > > 
> > 
> > /me has always wanted it that way ... done.
> 
> Can you also make sure the right element is focused then?
> When I click on a property, I want the twisty of their common parent
> to be focused.
> 

Done

Comment 11

[Paul Rouget [:paul]]

Don' block this bug with bug 705276. Consider bug 705276 as a follow-up bug (I don't want to risk to miss this bug for the merge).

(I'll review your code a bit later today)

Comment 12

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

Attachment: Removed dependency on bug 705276

Agreed

Comment 13

[Paul Rouget [:paul]]

Comment on attachment 604066 [details] [diff] [review]
Removed dependency on bug 705276

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

Looks good. r- because of some nits and the behavior issues.
I haven't read the tests yet.

-- Behavior:

Mousedown then drag from a property value will start the edit-mode. We should prevent that.
Selecting a range, then click on a value start the edit-mode. We should just un-select the range.

-- Code:

::: browser/devtools/highlighter/inspector.jsm
@@ +947,5 @@
> +        this.ruleViewCopyDeclaration.bind(this);
> +      this.cssRuleViewBoundCopyProperty = this.ruleViewCopyProperty.bind(this);
> +      this.cssRuleViewBoundCopyPropertyValue =
> +        this.ruleViewCopyPropertyValue.bind(this);
> +

First, I think it's a little cleaner to do something like:
this.xxx = this.xxx.bind();
instead of:
this.boundxxx = this.xxx.bind()

(if you don't need a non-bound version of this.xxx of course)

Second, what not using on single handleEvent listener (we do that for IUI and the breadcrumbs)?

@@ +1067,5 @@
> +    let menu = this.chromeDoc.createElement("menupopup");
> +    menu.addEventListener("popupshowing", this.cssRuleViewBoundMenuUpdate);
> +    menu.id = "rule-view-context-menu";
> +
> +    // Copy

// Copy selection

@@ +1143,5 @@
> +  {
> +    let iframe = this.getToolIframe(this.ruleViewObject);
> +    let win = iframe.contentWindow;
> +
> +    // Copy Selected.

// Copy selection

@@ +1144,5 @@
> +    let iframe = this.getToolIframe(this.ruleViewObject);
> +    let win = iframe.contentWindow;
> +
> +    // Copy Selected.
> +    let disable = !win.getSelection().toString();

win.getSelection().isCollapsed

https://developer.mozilla.org/en/DOM/Selection/isCollapsed

@@ +1180,5 @@
> +  ruleViewCopy: function IUI_ruleViewCopy(aEvent)
> +  {
> +    let iframe = this.getToolIframe(this.ruleViewObject);
> +    let win = iframe.contentWindow;
> +    let text = win.getSelection().toString();

win.getSelection().isCollapsed

@@ +1183,5 @@
> +    let win = iframe.contentWindow;
> +    let text = win.getSelection().toString();
> +
> +    // Remove any double blank lines.
> +    text = text.replace(/(\r?\n)\r?\n/g, "$1");

Can this happen?

@@ +1199,5 @@
> +      .GetStringFromName("rule.inheritedSource");
> +    inheritedFrom = inheritedFrom.replace(/\s%S\s\(%S\)/g, "");
> +    rx = new RegExp("(\r?\n)" + inheritedFrom + ".*", "g");
> +    text = text.replace(rx, "$1");
> +

These 3 latest regexp seem a little fragile. Do we test that in the tests?

@@ +1254,5 @@
> +    clipboardHelper.copyString(text);
> +  },
> +
> +  /**
> +   * Copy a property from the rule view.

declaration

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +167,5 @@
> +  this.siBoundCopy = this.computedViewCopy.bind(this);
> +  this.siBoundCopyDeclaration = this.computedViewCopyDeclaration.bind(this);
> +  this.siBoundCopyProperty = this.computedViewCopyProperty.bind(this);
> +  this.siBoundCopyPropertyValue = this.computedViewCopyPropertyValue.bind(this);
> +

Overwrite the existing methods. Don't create new ones.

@@ +546,5 @@
> +   */
> +  computedViewMenuUpdate: function si_computedViewMenuUpdate()
> +  {
> +    let win = this.styleDocument.defaultView;
> +    let disable = !win.getSelection().toString();

isCollapsed

@@ +580,5 @@
> +
> +    // Tidy up block headings by moving CSS property names and their values onto
> +    // the same line and inserting a colon between them.
> +    text = text.replace(/(.+)\r?\n\s+/g, "$1: ");
> +

Do we test that?

@@ +616,5 @@
> +      if (helplink) {
> +        // Let's use the icon's textContent to produce a meaningful property
> +        // string.
> +        helplink.textContent = ": ";
> +      }

That's a little hacky :)

Can you use the text content of the 2 surrounding nodes and concat them instead?

@@ +906,5 @@
>      this.nameNode.setAttribute("class", "property-name");
>      this.nameNode.textContent = this.name;
> +    this.nameNode.addEventListener("click", function(aEvent) {
> +      this.matchedExpander.focus();
> +    }.bind(this), false);

Do we remove this listener somewhere?

Comment 14

[Dave Camp (:dcamp)]

(In reply to Paul Rouget [:paul] from comment #13)

> Mousedown then drag from a property value will start the edit-mode. We
> should prevent that.

That might be difficult with the current code.  I think we should worry about that as a followup, there's still value in this patch if we can't fix that today.

> Selecting a range, then click on a value start the edit-mode. We should just
> un-select the range.

Agreed.

> First, I think it's a little cleaner to do something like:
> this.xxx = this.xxx.bind();
> instead of:
> this.boundxxx = this.xxx.bind()

We've been asked by browser reviewers to use this pattern during superreviews, but CssRuleView does it the way paul is suggesting.  Let's go for file consistency for now (in CssRuleView use foo = foo.bind(), I don't know the file style for CssHtmlTree).

> (if you don't need a non-bound version of this.xxx of course)
> 
> Second, what not using on single handleEvent listener (we do that for IUI
> and the breadcrumbs)?

CssRuleView at least, tends toward handler-per-event.  Mike can use his judgement on that one.

Comment 15

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

Attachment: Addressed reviewer's comments

(In reply to Paul Rouget [:paul] from comment #13)
> Comment on attachment 604066 [details] [diff] [review]
> Removed dependency on bug 705276
> 
> Review of attachment 604066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. r- because of some nits and the behavior issues.
> I haven't read the tests yet.
> 
> -- Behavior:
> 
> Mousedown then drag from a property value will start the edit-mode. We
> should prevent that.

The original dev used onFocus to trigger the property editors, which seemed like a good idea at the time. This meant that clicking them with a mouse or tabbing to them would trigger the editors. It also meant that he could easily auto focus the next element to get the editor behavior that he wanted. Changing this is a bigger job than we need right now so I have logged bug 734365 for this.

> Selecting a range, then click on a value starts the edit-mode. We should just
> un-select the range.
> 

Agreed, but we can't change this until bug 734365 is fixed.

> -- Code:
> 
> ::: browser/devtools/highlighter/inspector.jsm
> @@ +947,5 @@
> > +        this.ruleViewCopyDeclaration.bind(this);
> > +      this.cssRuleViewBoundCopyProperty = this.ruleViewCopyProperty.bind(this);
> > +      this.cssRuleViewBoundCopyPropertyValue =
> > +        this.ruleViewCopyPropertyValue.bind(this);
> > +
> 
> First, I think it's a little cleaner to do something like:
> this.xxx = this.xxx.bind();
> instead of:
> this.boundxxx = this.xxx.bind()
> 
> (if you don't need a non-bound version of this.xxx of course)
> 

We will leave this (as discussed on IRC).

> Second, what not using on single handleEvent listener (we do that for IUI
> and the breadcrumbs)?
> 

Because most of the events I am adding are command listeners. If event.type == "command" for 5 out of 6 of the events then things would get ugly. Beside, Dave said that I can use my judgement on this one.

> @@ +1067,5 @@
> > +    let menu = this.chromeDoc.createElement("menupopup");
> > +    menu.addEventListener("popupshowing", this.cssRuleViewBoundMenuUpdate);
> > +    menu.id = "rule-view-context-menu";
> > +
> > +    // Copy
> 
> // Copy selection
> 

Changed

> @@ +1143,5 @@
> > +  {
> > +    let iframe = this.getToolIframe(this.ruleViewObject);
> > +    let win = iframe.contentWindow;
> > +
> > +    // Copy Selected.
> 
> // Copy selection
> 

Changed

> @@ +1144,5 @@
> > +    let iframe = this.getToolIframe(this.ruleViewObject);
> > +    let win = iframe.contentWindow;
> > +
> > +    // Copy Selected.
> > +    let disable = !win.getSelection().toString();
> 
> win.getSelection().isCollapsed
> 
> https://developer.mozilla.org/en/DOM/Selection/isCollapsed
> 

Changed

> @@ +1180,5 @@
> > +  ruleViewCopy: function IUI_ruleViewCopy(aEvent)
> > +  {
> > +    let iframe = this.getToolIframe(this.ruleViewObject);
> > +    let win = iframe.contentWindow;
> > +    let text = win.getSelection().toString();
> 
> win.getSelection().isCollapsed
> 

Changed

> @@ +1183,5 @@
> > +    let win = iframe.contentWindow;
> > +    let text = win.getSelection().toString();
> > +
> > +    // Remove any double blank lines.
> > +    text = text.replace(/(\r?\n)\r?\n/g, "$1");
> 
> Can this happen?
> 

It does for almost every line. I have corrected the comment to "// Remove any double newlines" as it wasn't quite correct.

> @@ +1199,5 @@
> > +      .GetStringFromName("rule.inheritedSource");
> > +    inheritedFrom = inheritedFrom.replace(/\s%S\s\(%S\)/g, "");
> > +    rx = new RegExp("(\r?\n)" + inheritedFrom + ".*", "g");
> > +    text = text.replace(rx, "$1");
> > +
> 
> These 3 latest regexp seem a little fragile. Do we test that in the tests?
> 

Yes

> @@ +1254,5 @@
> > +    clipboardHelper.copyString(text);
> > +  },
> > +
> > +  /**
> > +   * Copy a property from the rule view.
> 
> declaration
> 

Changed

> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +167,5 @@
> > +  this.siBoundCopy = this.computedViewCopy.bind(this);
> > +  this.siBoundCopyDeclaration = this.computedViewCopyDeclaration.bind(this);
> > +  this.siBoundCopyProperty = this.computedViewCopyProperty.bind(this);
> > +  this.siBoundCopyPropertyValue = this.computedViewCopyPropertyValue.bind(this);
> > +
> 
> Overwrite the existing methods. Don't create new ones.
> 

As discussed on irc, we will leave them as they are.

> @@ +546,5 @@
> > +   */
> > +  computedViewMenuUpdate: function si_computedViewMenuUpdate()
> > +  {
> > +    let win = this.styleDocument.defaultView;
> > +    let disable = !win.getSelection().toString();
> 
> isCollapsed
> 

Changed

> @@ +580,5 @@
> > +
> > +    // Tidy up block headings by moving CSS property names and their values onto
> > +    // the same line and inserting a colon between them.
> > +    text = text.replace(/(.+)\r?\n\s+/g, "$1: ");
> > +
> 
> Do we test that?
> 

Yes

> @@ +616,5 @@
> > +      if (helplink) {
> > +        // Let's use the icon's textContent to produce a meaningful property
> > +        // string.
> > +        helplink.textContent = ": ";
> > +      }
> 
> That's a little hacky :)
> 
> Can you use the text content of the 2 surrounding nodes and concat them
> instead?
> 

Yes, of course.

> @@ +906,5 @@
> >      this.nameNode.setAttribute("class", "property-name");
> >      this.nameNode.textContent = this.name;
> > +    this.nameNode.addEventListener("click", function(aEvent) {
> > +      this.matchedExpander.focus();
> > +    }.bind(this), false);
> 
> Do we remove this listener somewhere?

Not directly. The computed view's event listeners don't leak at all because the document is destroyed by the destroy() method where we remove all listeners that are attached to XUL elements. In fact, most of the computed views listeners are added by DOM Templater so we can't remove them. There are no leaks though, I have already checked that.

(In reply to Dave Camp (:dcamp) from comment #14)
> (In reply to Paul Rouget [:paul] from comment #13)
> 
> > Mousedown then drag from a property value will start the edit-mode. We
> > should prevent that.
> 
> That might be difficult with the current code.  I think we should worry
> about that as a followup, there's still value in this patch if we can't fix
> that today.
> 
> > Selecting a range, then click on a value start the edit-mode. We should just
> > un-select the range.
> 
> Agreed.
> 

Done

> > First, I think it's a little cleaner to do something like:
> > this.xxx = this.xxx.bind();
> > instead of:
> > this.boundxxx = this.xxx.bind()
> 
> We've been asked by browser reviewers to use this pattern during
> superreviews, but CssRuleView does it the way paul is suggesting. Let's go
> for file consistency for now (in CssRuleView use foo = foo.bind(), I don't
> know the file style for CssHtmlTree).
> 
> > (if you don't need a non-bound version of this.xxx of course)
> >

I don't add any in CssRuleView but have stuck with the style used in inspector.jsm
 
> > Second, what not using on single handleEvent listener (we do that for IUI
> > and the breadcrumbs)?
> 
> CssRuleView at least, tends toward handler-per-event.  Mike can use his
> judgement on that one.

Most of the events I am adding are command listeners, if event.type == "command" for 5 out of 6 of the events then things would get ugly. Judgement used ;o)

Comment 16

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

I need to fix browser/browser/devtools/styleinspector/test/browser_ruleview_focus.js because of oranges before we land this.

Comment 17

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

Attachment: Fixed oranges

Fixed issue with rule view focus test and windows clipboard differences.

Now all green on try.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/503a8074e7fe

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/503a8074e7fe

Comment 20

[Francesco Lodolo [:flod]]

l10n doubt: why do you need two sets of identical strings (e.g. style.contextmenu.copyselection and rule.contextmenu.copyselection)?

I'm trying to think of a reason but I don't find any (this strings don't refer to names, so there's no problem with gender, plurals, etc.).

Comment 21

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

(In reply to Francesco Lodolo [:flod] from comment #20)
> l10n doubt: why do you need two sets of identical strings (e.g.
> style.contextmenu.copyselection and rule.contextmenu.copyselection)?
> 
> I'm trying to think of a reason but I don't find any (this strings don't
> refer to names, so there's no problem with gender, plurals, etc.).

The 2 strings are for 2 completely different tools. I suppose it would solve a lot of confusion if we split the strings into 2 different files but the one file should be okay for now.

Comment 22

[Eric Shepherd [:sheppy]]

This feature is now mentioned:

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

And listed on Firefox 13 for developers.

Comment 23

[Potch [:potch]]

In Nightly (Fx 15) I attempt the following:

Inspect an element
Click on a css property value in the Rules View
Press Command+C

Result: nothing happens. When I right click, the option "copy selection" is disabled. Why is this?

Comment 24

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

You need to drag over text to select it. It would be possible to get copy working in property editors within the rule view.

If you create a new bug and CC me I can do that.

Comment 25

[Luke]

This is broken in Firefox 22+ currently - see https://bugzilla.mozilla.org/show_bug.cgi?id=879603