Status

defect
VERIFIED FIXED
5 years ago
10 months ago

People

(Reporter: canuckistani, Assigned: gl, Mentored)

Tracking

(Blocks 1 bug, {dev-doc-complete})

30 Branch
Firefox 41
Dependency tree / graph

Firefox Tracking Flags

(firefox41 verified, relnote-firefox 41+)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 attachments, 14 obsolete attachments)

49.56 KB, image/png
Details
37.83 KB, patch
Details | Diff | Splinter Review
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.
Depends on: 1020291
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good next bug][lang=js][mentor=pbrosset]
Mentor: pbrosset
Whiteboard: [good next bug][lang=js][mentor=pbrosset] → [good next bug][lang=js]

Comment 2

5 years ago
It will be helpful if anyone provide me more info on this Bug , so i could work on . Thank you :)
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

5 years ago
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.
(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

5 years ago
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!
`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

5 years ago
Posted patch copy_css_declaration (obsolete) — Splinter Review
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 :)
Assignee: nobody → lviknesh
Status: NEW → ASSIGNED
Attachment #8494793 - Flags: feedback?(pbrosset)
Comment on attachment 8494793 [details] [diff] [review]
copy_css_declaration

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

This looks like an unrelated patch.
Attachment #8494793 - Flags: feedback?(pbrosset)

Comment 10

5 years ago
Posted patch copy_declaration (obsolete) — Splinter Review
Sorry , i messed up my mercurial queue . Here is the correct one :)
Attachment #8494793 - Attachment is obsolete: true
Attachment #8495358 - Flags: feedback?(pbrosset)
(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 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
Attachment #8495358 - Flags: feedback?(pbrosset) → feedback+

Comment 13

5 years ago
Posted patch copy-css.patch (obsolete) — Splinter Review
Attachment #8495358 - Attachment is obsolete: true
Attachment #8497753 - Flags: feedback?(pbrosset)
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.
Attachment #8497753 - Flags: feedback?(pbrosset) → feedback-
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

5 years ago
Posted patch copy-css-declarations (obsolete) — Splinter Review
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));
Attachment #8497753 - Attachment is obsolete: true
Attachment #8502746 - Flags: feedback?(pbrosset)
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);'
Attachment #8502746 - Flags: feedback?(pbrosset)
(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

5 years ago
Posted patch copy.patch (obsolete) — Splinter Review
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 :)
Attachment #8502746 - Attachment is obsolete: true
Attachment #8506365 - Flags: feedback?(pbrosset)
(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 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.
Attachment #8506365 - Flags: feedback?(pbrosset)

Comment 22

5 years ago
Posted patch css-copy.patch (obsolete) — Splinter Review
Tested the patch , working !!
Attachment #8506365 - Attachment is obsolete: true
Attachment #8507895 - Flags: feedback?(pbrosset)
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?
Attachment #8507895 - Flags: feedback?(pbrosset) → feedback+
Patrick - this is a Firebug parity feature, I'd like to consider resurrecting this bug/patch.
Flags: needinfo?(pbrosset)
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.
Assignee: lviknesh → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(pbrosset)
Whiteboard: [good next bug][lang=js] → [good first bug][lang=js]
(Assignee)

Updated

4 years ago
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
(Assignee)

Comment 26

4 years ago
Posted patch 1024693.patch WIP (obsolete) — Splinter Review
Rebased
Attachment #8507895 - Attachment is obsolete: true
(Assignee)

Comment 27

4 years ago
Posted patch 1024693.patch WIP (obsolete) — Splinter Review
Attachment #8614118 - Attachment is obsolete: true
(Assignee)

Comment 28

4 years ago
Posted patch 1024693.patch WIP (obsolete) — Splinter Review
Attachment #8614761 - Attachment is obsolete: true
(Assignee)

Comment 29

4 years ago
Posted patch 1024693.patch [1.0] (obsolete) — Splinter Review
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
Attachment #8615444 - Attachment is obsolete: true
Attachment #8615901 - Flags: review?(pbrosset)
(Assignee)

Comment 31

4 years ago
Posted patch 1024693.patch [2.0] (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=567488d5290e
Attachment #8615901 - Attachment is obsolete: true
Attachment #8615901 - Flags: review?(pbrosset)
Attachment #8616407 - Flags: review?(pbrosset)
(Assignee)

Comment 32

4 years ago
Posted patch 1024693.patch [3.0] (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d64afdd5f5c
Attachment #8616407 - Attachment is obsolete: true
Attachment #8616407 - Flags: review?(pbrosset)
Attachment #8616500 - Flags: review?(pbrosset)
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?
Attachment #8616500 - Flags: review?(pbrosset)

Updated

4 years ago
Blocks: 1164343

Comment 34

4 years ago
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 ?
(Assignee)

Comment 35

4 years ago
(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.
(Assignee)

Comment 36

4 years ago
Posted patch 1024693.patch [4.0] (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ce164f89e6
Attachment #8616500 - Attachment is obsolete: true
Attachment #8617677 - Flags: review?(pbrosset)
(Assignee)

Updated

4 years ago
Attachment #8617677 - Flags: review?(pbrosset)
(Assignee)

Comment 37

4 years ago
Posted patch 1024693.patch [5.0] (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c34859cc2351
Attachment #8617677 - Attachment is obsolete: true
Attachment #8617766 - Flags: review?(pbrosset)
(Assignee)

Comment 38

4 years ago
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 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.
Attachment #8617766 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/ade6d46effae
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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)]:
(Assignee)

Updated

4 years ago
Duplicate of this bug: 985358
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!
Flags: needinfo?(gabriel.luong)
(Assignee)

Comment 46

4 years ago
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.
Flags: needinfo?(gabriel.luong)
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]

Updated

4 years ago
QA Whiteboard: [bugday-20150729]
Marking verified fixed based on comment 48.
Status: RESOLVED → VERIFIED

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.