1024693 - Copy CSS declarations

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

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

Attachment: Copy_the_rule_including_all_its_properties_to_the_clipboard_and_Hacker_News_and_Enter_Bug__Firefox.png

From uservoice: 

"Just like Firebug, add context menu entries to copy the (enabled) Rule/Style/Property declarations."

http://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/6043223-inspector-copy-css-declarations

Comment 1

[Patrick Brosset <:pbro>]

In bug 1020291, I'm adding a 'getNodeInfo' to the rule-view and computed-view panels.
Using this function, it'll be pretty straightforward to know what element the user just right-clicked on.
So it should easy to add:
- copy selector
- copy rue
- copy property name
- copy property value
- copy declaration
items to the ctx menu.

Since we already have the rest (clipboard management, and ctx menu), I'm flagging this as a good first bug.

Comment 2

[vikneshwar]

It will be helpful if anyone provide me more info on this Bug , so i could work on . Thank you :)

Comment 3

[Patrick Brosset <:pbro>]

Thanks for your interest.

Here is what I can think of so far (take a look at the code and come back with more questions if this isn't enough):
- Most of the work is going to be done in rule-view.js
- The rule-view panel is being managed by the CssRuleView class in this file
- This class creates a contextual menu for the panel: see the _buildContextMenu function
- You should add new items in this menu
- For each of your new item, you'll have to define command callbacks that get executed when the item is clicked
- In your callbacks, you can then call this.getNodeInfo(target), passing it the node that was clicked
- This should return you relevant information about what has been clicked (whether it's a property name or value).
- Thanks to this, you may grey-out some menu items if they don't apply, or get the information and store it in the clipboard
- For clipboard access, see the _onCopy function that already uses it

Comment 4

[arvind.ravulavaru]

Hello Patrick. I am a newbie into the Firefox developer world & I am interested to start off with this bug. Request you to kindly guide me. Thanks - Arvind.

Comment 5

[Patrick Brosset <:pbro>]

(In reply to arvind.ravulavaru from comment #4)
> Hello Patrick. I am a newbie into the Firefox developer world & I am
> interested to start off with this bug. Request you to kindly guide me.
> Thanks - Arvind.
Hello Arvind. This bug was flagged as a "good *next* bug", not a "good first bug", which means there's a little code complexity involved. Good first bugs are usually trivial to fix but are a good way to set up your dev environment and familiarize yourself with bugzilla/patches/irc.
Having said that, thanks for your interest in that bug, I can help you get started. You should start here: https://wiki.mozilla.org/DevTools/GetInvolved#Hacking
And you should join our #devtools IRC channel if you have any questions related to setting up the environment and making changes to the devtools code.
Once done, comment 3 should guide you to the right files and functions that need changing.

Comment 6

[arvind.ravulavaru]

Hello Patrick. Thanks! 

I have followed your instructions and things went well till the callback is called. Inside the callback,

_onCopyRule : function (event) {
      var nodeInfo = this.getNodeInfo(event.target);
      console.log(nodeInfo);
  },

The above console log - logs null. Upon debugging further, I found out that getNodeInfo() is also called from : firefox/browser/devtools/styleinspector/style-inspector-overlays.js line no : 118, on hover. Onhover, the first time values are returned correctly and from next time, they return null.

Can you please let me know, if I am missing anything. Thanks!

Comment 7

[Patrick Brosset <:pbro>]

`this.getNodeInfo(event.target)` will return information if event.target is a reference to a node that actually represents something in the rule-view panel.
What this method does is it looks at the DOMNode passed as argument, and checks if that node is a property, a value, or a selector.
It returns null if the node isn't any of those.

I think you should start with the following:
- copy value,
- copy property,
- copy selector
since they can be pretty easily achieved. Let's do the patch in steps. First step, just add these 3 menu items to the ctx menu, always enabled (we'll sort out enabling/disabling items depending on where you click later).
Then create one callback for each (onCopyValue, onCopyProperty ...), and use getNodeInfo in each of them.
If it returns null, just return and do nothing. If, however, it returns an object like {type: selector, value: "div span"} or similar, then get the value and store it in the clipboard.

As a step 2, you could work on:
- copy declaration
This one is also pretty straightforward, what you will need to do is if the user clicked either on a property or an a value, then getNodeInfo will return an object that, anyway, contains both property and value, so it should be easy to store property:value; in the clipboard.

I think we should leave 
- copy rule
out of scope for now and focus on it later. It will require some more complex changes to be made.

Comment 8

[vikneshwar]

Attachment: copy_css_declaration

I just started working on this bug :) i haven't tested the patch , just uploading it to make sure whether i am doing it right :) 

I am not sure what accessKey to use , so gave some dummy accesskey . Can you provide feedback so that i can proceed further :)

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8494793 [details] [diff] [review]
copy_css_declaration

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

This looks like an unrelated patch.

Comment 10

[vikneshwar]

Attachment: copy_declaration

Sorry , i messed up my mercurial queue . Here is the correct one :)

Comment 11

[Patrick Brosset <:pbro>]

(In reply to vikneshwar from comment #8)
> I am not sure what accessKey to use , so gave some dummy accesskey . Can you
> provide feedback so that i can proceed further :)
Don't worry about access keys for now, we can always add some later.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8495358 [details] [diff] [review]
copy_declaration

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

That's a good start! Thanks for the patch.
I've made a few minor comments in the code.

Now, I think from that patch, we can go to the next step: disabling/enabling menu items depending on where the user clicks, and this will actually allow us to refactor the code you wrote and make it shorter. Here's the idea:

In '_contextMenuUpdate' (which is called when the ctx-menu appears, let's call 'this.getNodeInfo(event.target)', and based on what it returns, let's enable or disable the corresponding menu items.
For instance, if it returns a type selector, then you can disable "copy value", "copy name" and "copy declaration" and only keep "copy selector" enabled.
If it returns a property name, then you can keep "copy name" and "copy declaration" enabled, and disable the rest.

Also, calling getNodeInfo in this function means that we can call store the returned value on, say, 'this._clickedNodeInfo' or something like this, and then inside each of your command callbacks (e.g. _onCopyValue), you can just access it instead of having to call getNodeInfo again.
In fact, we can go one step further, and instead of storing the node info, we can store the string to be copied to the clipboard directly. So, if I sum up:

- in contextMenuUpdate, disable all your menu items,
- then call getNodeInfo,
- based on the returned value, enable only the items that should be enabled
- store on this._clickedNodeInfoString what needs to be put into the clipboard
- then instead of having one command callback per menu item (in createMenuItems), just define one only, like _onCopyPartOfRule (or something better)
- and in this callback, just put in the clipboard the string you already built in clickedNodeInfoString.

Let me know if that makes sense.

::: browser/devtools/styleinspector/rule-view.js
@@ +1188,5 @@
>      });
>  
> +    this.menuitemCopyValue = createMenuItems({
> +      label: "ruleView.contextmenu.copyValue",
> +      accesskey: "ruleView.contextmenu.copyValue.accessKey",

As said earlier, no need for access keys yet.

@@ +1189,5 @@
>  
> +    this.menuitemCopyValue = createMenuItems({
> +      label: "ruleView.contextmenu.copyValue",
> +      accesskey: "ruleView.contextmenu.copyValue.accessKey",
> +      command: this._onCopyValue

Can you rename this callback as 'this._onCopyPropertyValue'

@@ +1195,5 @@
> +
> +    this.menuitemCopyProperty = createMenuItems({
> +      label: "ruleView.contextmenu.copyProperty",
> +      accesskey: "ruleView.contextmenu.copyProperty.accessKey",
> +      command: this._onCopyProperty

And rename this one as 'this._onCopyPropertyName'

@@ +1495,5 @@
>          rule.editor.updateSourceLink();
>        }
>      }
>    },
> +  

nit: trailing whitespace

@@ +1505,5 @@
> +    } else {
> +      return;
> +    }
> +  },
> +  

nit: trailing whitespace

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +132,5 @@
>  # the computed view context menu "Select all" entry.
>  computedView.contextmenu.copy.accessKey=C
> +
> +# LOCALIZATION NOTE (computedView.contextmenu.copyValue): Text displayed in the
> +# computed view context menu for copying the property value.

s/computed view/rule view

@@ +137,5 @@
> +ruleView.contextmenu.copyValue=Copy Property Value
> +
> +# LOCALIZATION NOTE (computedView.contextmenu.copyValue.accessKey): Access key for
> +# computed view context menu for copying the property value.
> +ruleView.contextmenu.copyValue.accessKey=V

Let's get rid of these access keys for now, we'll add some later.

@@ +140,5 @@
> +# computed view context menu for copying the property value.
> +ruleView.contextmenu.copyValue.accessKey=V
> +
> +# LOCALIZATION NOTE (computedView.contextmenu.copyProperty): Text displayed in the
> +# computed view context menu for copying the property name.

s/computed view/rule view

@@ +148,5 @@
> +# computed view context menu for copying the property name.
> +ruleView.contextmenu.copyProperty.accessKey=N
> +
> +# LOCALIZATION NOTE (computedView.contextmenu.copySelector): Text displayed in the
> +# computed view context menu for copying the selector.

s/computed view/rule view

@@ +156,5 @@
> +# computed view context menu for copying the selector.
> +ruleView.contextmenu.copySelector.accessKey=E
> +
> +# LOCALIZATION NOTE (computedView.contextmenu.copyDeclaration): Text displayed in the
> +# computed view context menu for copying the declaration.

s/computed view/rule view

Comment 13

[vikneshwar]

Attachment: copy-css.patch

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8497753 [details] [diff] [review]
copy-css.patch

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

You have obviously not tested this patch at all. There are at least 4 syntax errors which basically make the rule-view loading fail.

    this.menuitemCopyPropertyValue = createMenuItems({
      label: "ruleView.contextmenu.copyValue",
      command: this._onCopyPartOfRule("value")
    }),

The comma here should be a semi-colon (same with all the createMenuItems blocks you added).

Also, you added a _buildString method which you then call with _buildString(option); this is incorrect as _buildString is defined on the prototype, and is therefore available via this._buildString instead, not globally.

Please next time make sure you actually build and test your patch locally before asking for feedback/review.

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8497753 [details] [diff] [review]
copy-css.patch

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

I think the main logic is more or less there.
The patch needs some more work, but you're getting there.

::: browser/devtools/styleinspector/rule-view.js
@@ +1188,5 @@
>      });
>  
> +    this.menuitemCopyPropertyValue = createMenuItems({
> +      label: "ruleView.contextmenu.copyValue",
> +      command: this._onCopyPartOfRule("value")

This is incorrect, you are executing the function in place here and attaching the return value to the command property of the menu item.

It should instead be command: this._onCopyPartOfRule

@@ +1263,5 @@
> +    this.menuitemCopySelector.enabled = false;
> +
> +    this._clickedNodeInfo = this.getNodeInfo(this.target);
> +
> +    if (clickedNodeInfo.type == 1) {

Can you use the constants instead of the number values: overlays.VIEW_NODE_SELECTOR_TYPE
All constants are defined here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/style-inspector-overlays.js#41

@@ +1265,5 @@
> +    this._clickedNodeInfo = this.getNodeInfo(this.target);
> +
> +    if (clickedNodeInfo.type == 1) {
> +      this.menuitemCopySelector.enabled = true;
> +    } else if (clickedNodeInfo.type == 2) {

What about type 3? VIEW_NODE_VALUE_TYPE.
I think the logic should be:

if (clickedNodeInfo.type === overlays.VIEW_NODE_SELECTOR_TYPE) {
  this.menuitemCopySelector.enabled = true;
} else if (clickedNodeInfo.type === overlays.VIEW_NODE_PROPERTY_TYPE) {
  this.menuitemCopyPropertyName.enabled = true;
  this.menuitemCopyDeclaration.enabled = true;
} else if (clickedNodeInfo.type === overlays.VIEW_NODE_VALUE_TYPE) {
  this.menuitemCopyPropertyValue.enabled = true;
  this.menuitemCopyDeclaration.enabled = true;
}

Also, all of this code (from this.menuitemCopyPropertyValue.enabled = false; up to here) should be in a separate function which you call from here.

@@ +1508,5 @@
>    },
>  
> +  _onCopyPartOfRule: function(option) {
> +    _buildString(option);
> +    let text = this._clickedNodeInfoString;

This should instead be let text = this._buildString();

@@ +1509,5 @@
>  
> +  _onCopyPartOfRule: function(option) {
> +    _buildString(option);
> +    let text = this._clickedNodeInfoString;
> +    clipboardHelper.copyString(text,this.doc);

You should only call this if there is text to be copied. We don't want to erase the user's clipboard with an empty string.

Comment 16

[vikneshwar]

Attachment: copy-css-declarations

I made changes and tested my patch . Menu items gets created , but getNodeInfo returns null all the time .

I just commented the line for now , since it caused me error as we dint give any accesskey while creating the menu :  

item.setAttribute("accesskey", _strings.GetStringFromName(aAttributes.accesskey));

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8502746 [details] [diff] [review]
copy-css-declarations

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

::: browser/devtools/styleinspector/rule-view.js
@@ +1256,5 @@
>      let accessKey = label + ".accessKey";
>      this.menuitemSources.setAttribute("accesskey",
>                                        _strings.GetStringFromName(accessKey));
>  
> +    this._enableItems(event.target);

event.target actually returns the menu element itself, not the node that was clicked on, that's why you always get 'null' when calling getNodeInfo with it.

I researched this a bit on MDN (because I don't know this part at all): https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide/ContextMenus#Determining_what_was_Context_Clicked
It looks like you need to use event.target.triggerNode instead. But to be honest, I've tried it and it also is always null ...

@@ +1261,4 @@
>      this.menuitemAddRule.disabled = this.inspector.selection.isAnonymousNode();
>    },
>  
> +  _enableItems: function(target) {

This method should have a better name, it's too generic as is, and also, it turns out _contextMenuUpdate also deals with enabling/disabling items, so it's confusing.

@@ +1261,5 @@
>      this.menuitemAddRule.disabled = this.inspector.selection.isAnonymousNode();
>    },
>  
> +  _enableItems: function(target) {
> +    this.menuitemCopyPropertyValue.enabled = false;

Enabling menu items is done the other way around:
this.menuitemCopyPropertyValue.disabled = true;

@@ +1266,5 @@
> +    this.menuitemCopyPropertyName.enabled = false;
> +    this.menuitemCopyDeclaration.enabled = false;
> +    this.menuitemCopySelector.enabled = false;
> +
> +    let _clickedNodeInfo = this.getNodeInfo(target);

'let _clickedNodeInfo' should be 'this._clickedNodeInfo', otherwise the info isn't going to be stored on the object instance.

@@ +1513,5 @@
>        }
>      }
>    },
>  
> +  _onCopyPropertyValue: function() {

You can probably simplify all the _onCopy... methods like this:

_onCopyPropertyValue: function() {
  if (!this._clickedNodeInfo) {
    return;
  }
  this._copyToClipboard(this._clickedNodeInfo.value.value);
}

This saves 2 lines, removes the need for the useless return statement, and removes one level of indentation.

@@ +1551,5 @@
> +  },
> +
> +  _copyToClipboard: function(text) {
> +    if(text) {
> +      clipboardHelper.copyString(text,this.doc);

I didn't remember setting a string in the clipboard was so easy. Since it is, we might as well just remove the _copyToClipboard function and replace all occurences of 'this._copyToClipboard(...)' with 'clipboardHelper.copyString(..., this.doc);'

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #17)
> event.target actually returns the menu element itself, not the node that was
> clicked on, that's why you always get 'null' when calling getNodeInfo with
> it.
> 
> I researched this a bit on MDN (because I don't know this part at all):
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide/
> ContextMenus#Determining_what_was_Context_Clicked
> It looks like you need to use event.target.triggerNode instead. But to be
> honest, I've tried it and it also is always null ...
So far, I haven't been able to determine why event.target.triggerNode is always null in this case. Maybe this has got to do with the fact that the rule-view is a HTML document, not XUL. Can you ask on #xul?
Another alternative is to add a "mousedown" listener on the document, and in the handler, just set this._clickedNodeInfo = this.getNodeInfo(event.target)
This should work but requires 2 event listeners instead of one, which is too bad. So this is last resort solution if there's no solution to event.target.triggerNode being null.

Comment 19

[vikneshwar]

Attachment: copy.patch

this.doc.popupNode.parentNode worked !! Using this i was able to enable menu items in _enableItems . However , referencing _clickedNodeInfo in _onCopyProperty functions returns **undefined** (so was not able to copy string to clipboard ) . 

i am not sure what name i should use instead of _enableItems , would be helpful if you could suggest a name :) thank you :)

Comment 20

[Patrick Brosset <:pbro>]

(In reply to vikneshwar from comment #19)
> Created attachment 8506365 [details] [diff] [review]
> copy.patch
> 
> this.doc.popupNode.parentNode worked !! Using this i was able to enable menu
> items in _enableItems . However , referencing _clickedNodeInfo in
> _onCopyProperty functions returns **undefined** (so was not able to copy
> string to clipboard ) . 
Hmm, that's weird, if you said that this.doc.popupNode worked and that using it you were able to enable/disable menu items in _enableItems, then I don't see how this._clickedNodeInfo could be undefined in _onCopyProperty.
I'm reviewing the patch now and will apply it locally in a minute and see what's going on.
> i am not sure what name i should use instead of _enableItems , would be
> helpful if you could suggest a name :) thank you :)
I'll propose a better name in the review in a minute.

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 8506365 [details] [diff] [review]
copy.patch

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

Nice, getting there ... Please see my comments below.

::: browser/devtools/styleinspector/rule-view.js
@@ +1186,5 @@
>        accesskey: "ruleView.contextmenu.showOrigSources.accessKey",
>        command: this._onToggleOrigSources,
>        type: "checkbox"
>      });
> +    

Please remove the trailing whitespaces here.

@@ +1189,5 @@
>      });
> +    
> +    this.menuitemCopyPropertyValue = createMenuItem(this._contextmenu, {
> +      label: "ruleView.contextmenu.copyValue",
> +      command: this._onCopyPropertyValue

Ok so here's why 'this._clickedNodeInfo' is undefined in the callback: you're passing here a reference to the callback but you're not telling it on which object scope it should be executed.
In other words, when '_onCopyPropertyValue' will run, the value of 'this' in the function won't be the current rule view object, it will be the menu item XUL node instead.

To make this work you need to *bind* the function to this, so that calling it will call it in the right scope.

See http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#1112
Around this line, we do a bunch of bindings already. You should do the same for all your callbacks.

@@ +1250,5 @@
>  
>      var showOrig = Services.prefs.getBoolPref(PREF_ORIG_SOURCES);
>      this.menuitemSources.setAttribute("checked", showOrig);
>  
> +    this._enableItems(this.doc.popupNode);

What about renaming this '_enableCssCopyItems' instead?

@@ +1258,5 @@
> +  _enableItems: function(target) {
> +    this.menuitemCopyPropertyValue.disabled = true;
> +    this.menuitemCopyPropertyName.disabled = true;
> +    this.menuitemCopyDeclaration.disabled = true;
> +    this.menuitemCopySelector.disabled = true;

I'm wondering if we should not be hiding the menu items altogether instead, to keep the menu small and easy to use.
'this.menuItemCopySelector.hidden = true;' should work.

@@ +1508,5 @@
>      }
>    },
>  
> +  _onCopyPropertyValue: function() {
> +    if(!this._clickedNodeInfo) {

nit: please add a space after 'if'

Same for all _onCopy*** functions.

@@ +1510,5 @@
>  
> +  _onCopyPropertyValue: function() {
> +    if(!this._clickedNodeInfo) {
> +      return;
> +    } else {

You can remove the 'else' part:

if (!this._clickedNodeInfo) {
  return;
}
clipboardHelper.copyString(this._clickedNodeInfo.value.value, this.doc);

This makes the function shorter and gets rid of one indentation level which is good.

Same for all _onCopy*** functions.

@@ +1511,5 @@
> +  _onCopyPropertyValue: function() {
> +    if(!this._clickedNodeInfo) {
> +      return;
> +    } else {
> +       clipboardHelper.copyString(this._clickedNodeInfo.value.value,this.doc);

nit: please add a space after ',' and before 'this.doc'.

Same for all _onCopy*** functions.

@@ +1586,5 @@
> +      this.menuitemCopySelector.removeEventListener("command", this._onCopySelector);
> +      this.menuitemCopySelector = null;
> +
> +      this.menuitemCopyDeclaration.removeEventListener("command", this._onCopyDeclaration);
> +      this.menuitemCopyDeclaration = null;

You should also do 'this._clickedNodeInfo = null;' in this function.

Comment 22

[vikneshwar]

Attachment: css-copy.patch

Tested the patch , working !!

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 8507895 [details] [diff] [review]
css-copy.patch

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

Looks good.
Do you want to move on to adding new mochitests for this?
There are already several css copy tests in browser/devtools/styleinspector/test I think. So you should be able to start from an existing test.
Your test(s) should verify that menu items exist, and that the right ones are shown depending on which piece of text your right-click.

::: browser/devtools/styleinspector/rule-view.js
@@ +1535,5 @@
> +  _onCopyDeclaration: function() {
> +    if (!this._clickedNodeInfo) {
> +      return;
> +    }
> +    clipboardHelper.copyString(this._clickedNodeInfo.value.property + ": " + this._clickedNodeInfo.value.value, this.doc);

nit: Can you append
+ ";"
to the string?

Comment 24

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

Patrick - this is a Firebug parity feature, I'd like to consider resurrecting this bug/patch.

Comment 25

[Patrick Brosset <:pbro>]

No activity since half a year, resetting the assignee.
I don't have time right now to work on this bug, but I can mentor anyone who wants to continue.
It's very close, not much remains to be added, so that should be an easy pick.

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1024693.patch WIP

Rebased

Comment 27

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1024693.patch WIP

Comment 28

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1024693.patch WIP

Comment 29

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1024693.patch [1.0]

Adds the following context menu items in the rule view:
- Copy Location
- Copy Rule
- Copy Property Declaration
- Copy Property Name
- Copy Property Value
- Copy Selector 

Changes:
- Reordered the context menu with separators
- Needed an additional class check for the selector in getNodeInfo() since some selector text are added directly to the span
- We also take into account whether or not a property is enabled or disabled and comment out properties that are disabled

Comment 30

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 31

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1024693.patch [2.0]

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

Comment 32

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1024693.patch [3.0]

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

Comment 33

[Patrick Brosset <:pbro>]

Comment on attachment 8616500 [details] [diff] [review]
1024693.patch [3.0]

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

Thanks Gabriel for picking this up.
It works great.
I have some general comments first:

- "copy declaration" should probably also copy the ';', so it should copy 'margin: 0px;' instead of 'margin: 0px', this would help for pasting it later.
- "copy selector" should copy all selectors in the list I guess, right now if you have several selector parts, separated by a comma, it only copies the part that you right-clicked on.
- commenting-out disabled rules is really cool!
- I'm unsure about the position of the "copy" item in the menu and I think it may help if we renamed it to "Copy Selected Text" instead. There are so many copy types now that I think we should be specific about this one too (please do create a new string key in the properties file for this though).
- Also perhaps remove the separator between "copy" and "select all", they go together well and I don't a reason to separate them.

And then some more comments in the code.

::: browser/devtools/styleinspector/rule-view.js
@@ +1705,5 @@
> +      return;
> +    }
> +
> +    let value = this._clickedNodeInfo.value;
> +    let declaration = value.property + ": " + value.value;

Can you extract this part (and the if below with the commenting-out logic) into another method, maybe named stringifyDeclaration.

This way here you can use:
let value = this._clickedNodeInfo.value;
clipboardHelper.copyString(this.stringifyDeclaration(value), this.doc);

And that means you can also reuse this in _onCopyRule below.

Generally speaking, I think it's better not to have much logic in event handlers but instead in dedicated functions. This helps because these functions are fare more reusable in the future, and are also testable in isolation from xpcshell tests (which run faster than mochitests).

Btw, we already have xpcshell tests in browser/devtools/styleinspector/test/unit/, so you could add a new one there that makes sure all the css stringifiers do work correctly.

@@ +1740,5 @@
> +
> +  /**
> +   * Copy the rule of the current clicked node.
> +   */
> +  _onCopyRule: Task.async(function() {

Why Task.async? I can't see any 'yield' in the function below, this is probably a left-over.
Also, same comment as earlier, please keep as little logic as possible in this event handler and instead move it to a place where it's more reusable.
In this case, I think it makes sense to have a stringifyRule method on the Rule class, so that you can do something like:

let ruleEditor = this.doc.popupNode.parentNode.offsetParent._ruleEditor;
let rule = ruleEditor.rule;
clipboardHelper.copyString(rule.stringifyRule(), this.doc);

and move all the rest inside the stringiyRule function.

@@ +1750,5 @@
> +
> +    for (let textProp of rule.textProps) {
> +      let propertyName = textProp.name;
> +      let propertyValue = textProp.editor.committed.value;
> +      let propertyDeclaration = propertyName + ": " + propertyValue + ";";

This and the if/else below should be a stringifyDeclaration method on TextProperty.prototype, so that, again, it can be reused and tested in isolation.
I just realized that, unfortunately, _onCopyPropertyDeclaration doesn't have a reference to a TextProperty, so it wouldn't be able to reuse this. Maybe getNodeInfo should pass an extra property that references the corresponding TextProperty when possible?

::: browser/devtools/styleinspector/test/browser_ruleview_copy_styles.js
@@ +22,5 @@
> +  let ruleEditor = getRuleViewRuleEditor(view, 1);
> +
> +  let data = [
> +    {
> +      node: ruleEditor.rule.textProps[0].editor.nameSpan,

Can you add a 'desc' property to each and every item in the test data array?
When investigating failures in tests like this, it gets really tedious to know which of the item has failed. Having a unique text description for each item is nice because then the for loop can 'info(desc)' it which helps when reading test logs.

@@ +112,5 @@
> +  }
> +
> +  yield disableProperty(view);
> +
> +  data = [

I'd prefer to have all test data in just 1 array and have just 1 main test for loop. So maybe to make this work, you could add an optional 'setup' function property to the data item so that this can be called:

data = [{
  setup: function*() {
    yield disableProperty(view);
  },
  node: ...
  menuItem: ...
}, {
  ...
}];

This way you can declare all test cases up top, and then only have one test loop, which would make the test easier to read.

for (let { setup, node, menuItem, expectedPattern, hidden } of data) {
  if (setup) {
    yield setup();
  }
  yield checkCopyStyle(view, node, menuItem, expectedPattern, hidden);
}

@@ +190,5 @@
> +     hidden.copyRule);
> +
> +  yield waitForClipboard(() => menuItem.click(),
> +    () => checkClipboardData(expectedPattern)).then(
> +    () => {}, () => failedClipboard(expectedPattern));

No need for an empty resolution handler function here, you can pass null instead, or even use catch.
Also, you're using yield waitForClipboard, so no need to attach a .then handler, you can use try/catch to catch rejections.

try {
  yield waitForClipboard(() => menuItem.click(), () => checkClipboardData(expectedPattern));
} catch (e) {
  failedClipboard(expectedPattern);
}

@@ +224,5 @@
> +  // accounts for windows sometimes adding a newline to our copied data.
> +  expectedPattern = expectedPattern.trimRight();
> +  actual = actual.trimRight();
> +
> +  dump("TEST-UNEXPECTED-FAIL | Clipboard text does not match expected ... " +

Why dump something that looks like a test failure while you can use is(actual, expected) instead?
If I understand the test correctly, when failedClipboard gets called, you know the test has failed, so you could also use something as simple as ok(false) to make the test harness report the error, and then you can log using info statements if that helps.

::: browser/devtools/styleinspector/test/doc_copystyles.css
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#testid {

Can you add more selector parts here, to test that all are copied?

Comment 34

[Julian Descottes]

For Bug 1164343 I am currently extracting the code related to the styleinspector context menu to a separate widget. This widget will be shared by the computedview and ruleview.

I had a quick look at the current patch, and it looks inline with the way other menu items were declared before, so it shouldn't be a big deal for me to integrate your changes. I'm adding this bug as a blocker for 1164343.

Should some of these menu items be shared by both views ?

Comment 35

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #1)
> In bug 1020291, I'm adding a 'getNodeInfo' to the rule-view and
> computed-view panels.
> Using this function, it'll be pretty straightforward to know what element
> the user just right-clicked on.
> So it should easy to add:
> - copy selector
> - copy rue
> - copy property name
> - copy property value
> - copy declaration
> items to the ctx menu.
> 
> Since we already have the rest (clipboard management, and ctx menu), I'm
> flagging this as a good first bug.

(In reply to Julian Descottes from comment #34)
> For Bug 1164343 I am currently extracting the code related to the
> styleinspector context menu to a separate widget. This widget will be shared
> by the computedview and ruleview.
> 
> I had a quick look at the current patch, and it looks inline with the way
> other menu items were declared before, so it shouldn't be a big deal for me
> to integrate your changes. I'm adding this bug as a blocker for 1164343.
> 
> Should some of these menu items be shared by both views ?

These new menu items are rule view specific currently. So, I wouldn't worry about sharing between both views yet and leave that for another bug if we do decide to in the future.

I would note the order of the new menu items and the added method to create menu separators during your refactoring for Bug 1164343.

Comment 36

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1024693.patch [4.0]

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

Comment 37

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1024693.patch [5.0]

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

Comment 38

[Gabriel Luong [:gl] (ΦωΦ)]

Just to comment on the latest changes:
- Addressed all of the feedback
- I choose to keep "Copy" as-is but moved it to only appear as the first Copy menu item when the text is selected. This is the same behaviour as Firebug. Since "Copy" is used in every text input, I think the user easily recognizes it as the default behaviour and it may appear more natural to them compare to "Copy Selected Text".
- I didn't add any xpcshell tests since it seems to me that I would need to create a rule and populate the TextProperty and Rule in order to test the stringify methods, which is already done in the current tests.

Comment 39

[Patrick Brosset <:pbro>]

Comment on attachment 8617766 [details] [diff] [review]
1024693.patch [5.0]

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

Ah, I was reviewing the 4.0 patch just as you uploaded the 5.0, and somehow, bugzilla threw away all my comments...
anyway, I just had one important one: that you removed a function from head.js that was still used in at least one test, but I see you've fixed that in this new version.
I like the last changes you made, very nice code now.

Comment 40

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1024693.patch [6.0]

Rebased

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

Comment 41

[Pulsebot]

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

Comment 42

[Carsten Book [:Tomcat]]

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

Comment 43

[Lawrence Mandel [:lmandel] (use needinfo)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Copy CSS rule declaration from inspector
[Suggested wording]: Copy element CSS rule declarations with the Copy Rule Declaration context menu item in the Inspector
[Links (documentation, blog post, etc)]:

Comment 45

[Will Bamberg [:wbamberg]]

I've added this: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Copy_rules

Let me know if this covers it. Thanks!

Comment 46

[Gabriel Luong [:gl] (ΦωΦ)]

You(In reply to Will Bamberg [:wbamberg] from comment #45)
> I've added this:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_CSS#Copy_rules
> 
> Let me know if this covers it. Thanks!

You can also "Copy Location" when you mouseover the source link for the rule.

Comment 47

[Will Bamberg [:wbamberg]]

Thanks Gabriel. I've added a note about that here: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Link_to_CSS_file.

Comment 48

[Saheda Reza [:Antora]]

I didn't found following rule-view items on firfox nightly
- copy rule
- copy property name
- copy property declaration

BuildID	        20140612030349
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:33.0) Gecko/20100101 Firefox/33.0

I found these rule-view items on Firefox Developer Edition

Build ID 	20150731004008
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:41.0) Gecko/20100101 Firefox/41.0

 [Bugday-201507031]

Comment 49

[Bogdan Maris, Desktop Test Engineering]

Marking verified fixed based on comment 48.