[markup view] Context menu to easily edit, add and delete attributes

RESOLVED FIXED in Firefox 44

Status

defect
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: pbro, Assigned: grisha, Mentored)

Tracking

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

unspecified
Firefox 44
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

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

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

5 years ago
In today's markup-view (FF30), attributes of nodes can be edited by double-clicking (or focusing and pressing ENTER) on them, and new attributes can be added by doing the same but near the end of the open tag of a node.

In order to offer another, easier, more discoverable?, way of doing these 2 operations, it would be good to have "edit attribute" and "add attribute" options in the context menu when right-clicking on a node in the markup-view.

The edit attribute label should probably read something like "edit attribute class" if the user right clicks on the class attribute part of the node.

Once the option is selected, it should be enough to just focus the right element to start the edition.
(Reporter)

Comment 1

5 years ago
This seems like a good first bug to get into the markup-view code.
I can give guidance to whoever is interested in tackling this.
Whiteboard: [good first bug][lang=js][mentor=pbrosset]
(Reporter)

Comment 2

5 years ago
One more thing we could add to this bug is the option to delete an attribute from the contextual menu.
Summary: [markup view] Context menu to easily edit and add attributes → [markup view] Context menu to easily edit, add and delete attributes
(Reporter)

Updated

5 years ago
Blocks: firebug-gaps

Comment 3

5 years ago
Hi Patric

I am interested in fixing this issue. I have already contributed some patches to the Network and Console code and I am familiar with the devtools structure and the basic CVS and Bugzilla work flow.

I have not looked into the HTML panel code yet, but by reading the issue's description the task looks relatively easy to fix.

Thomas
Flags: needinfo?(pbrosset)
(Reporter)

Comment 4

5 years ago
Great! Yes the task should be relatively easy. It's also possible to split it in several bugs if we find that it's more work than expected (add, delete, edit could be done separately).

To start with, the contextual menu for the markup-view is here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector.xul#36
(note that it's also used from the breadcrumbs, so you'll need to make sure it works from there too).

The menu items all define an `oncommand` attribute that is used to call methods on the inspector object. Having said that, I believe we are currently removing all occurrences of inline scripts in HTML and XUL, so you should probably not use this attribute.

Rather, from the markup-view itself, you should add event listeners to the menu.
Somewhere around here http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#100 you could perhaps add a call to a new init menu method, which would do the following:
- add a listener to the add attribute menu item
- add a listener to the edit attribute menu item
- add a listener to the delete attribute menu item
- add a listener to the menu itself, so that when it shows (something like this should work: this._menu.addEventListener("popupshowing", this._doSomething, true);) you can detect which attribute element was clicked on and therefore update the label next to the edit and delete items (or grey them out if the click didn't occur on an attribute).

This is where attributes are created: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1844 (and are made editable with the editableField function).
This is where the new attribute placeholder is created: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1767

When right-clicking on a node to open the context menu, you'll need to find out what you clicked on. You should probably create a method in MarkupContainer for that, which receives the click target and decides what has been clicked on, which is easy because it knows the markup used for a node, so can check tag names and/or classes.

That's what I can say from the top of my head. I'll let you dig into the code now. Don't hesitate to ping me on IRC or ask me more questions here if you need.

Thanks for taking that bug!
Assignee: nobody → thomas
Flags: needinfo?(pbrosset)

Comment 5

5 years ago
Hi again,
Thank you for the introduction. I have been digging around and I think I am starting to kind of grasping some things ;)

I have the target node from the popupshown event, but since the created attribute has already been configured with a trigger:dblclick  in the _createAttribute function I am not sure how to start the editor from the eg. editattribute menu item's command.
The only thing I can think of at the moment is to dispatch the dblclick event, but it sounds hackish.
Did you have another way in mind?

Thomas

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1872
(Reporter)

Comment 6

5 years ago
(In reply to Thomas Andersen from comment #5)
> Hi again,
> Thank you for the introduction. I have been digging around and I think I am
> starting to kind of grasping some things ;)
> 
> I have the target node from the popupshown event, but since the created
> attribute has already been configured with a trigger:dblclick  in the
> _createAttribute function I am not sure how to start the editor from the eg.
> editattribute menu item's command.
> The only thing I can think of at the moment is to dispatch the dblclick
> event, but it sounds hackish.
> Did you have another way in mind?
Yeah dispatching the event isn't the best way of doing this, but I'm afraid the right way will require some changes to inplace-editor.js which is the module we use to manage the editors.
The markup-view calls `editableField` which basically (via `editableItem`) adds an event listener to an element and, when the event is fired, creates a `InplaceEditor` instance on top of the element.
That means that the InplaceEditor isn't created before the event is fired.

My suggestion is to add a new method on the prototype of `editableItem`, something like `startEdition`.
What this method would do is simply what `editableItem` does when the trigger event is fired (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/inplace-editor.js#105). So this code would have to be moved to `startEdition`.

Makes sense?

There's a mochitest specifically for the inplace-editor, so once you've done this, it would be good to add a new test case for this.

Once done, it means that, from the markup-view, you'll be able to turn any inplace-editor field into edition programmatically, by just calling the function.

Comment 7

5 years ago
> Makes sense?
Yes.


> My suggestion is to add a new method on the prototype of `editableItem`, something like `startEdition`.
What this method would do is simply what `editableItem` does when the trigger event is fired (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/inplace-editor.js#105). So this code would have to be moved to `startEdition`.

Just so I get it. Should we refactor todays 'editableItem' to a constructor function or just export a new 'startEdition' function in inline-editor.js

Thomas
Flags: needinfo?(pbrosset)
(Reporter)

Comment 8

5 years ago
What I'd do is add:
`editableItem.prototype.startEdition = function() {...}`

Which would roughly do what the function editableItem do today inside:
`element.addEventListener(trigger, function(evt) {...}, false);`

The markup-view today calls `editableField` to turn a span into something editable, and `editableField` does `return editableItem(...)` so it gets an `editableItem` instance already today. Therefore it would be able to call `startEdition` on it.

Of course this also means that whatever is inside `element.addEventListener(trigger, function(evt) {...}, false);` in `editableItem` would change to just calling `this.startEdition()`.

That's my advice just quickly looking at the code of course. I'm opened to anything else that works. But it seems like it should do the trick.
Flags: needinfo?(pbrosset)

Comment 9

5 years ago
Posted patch 994555-menu-items.patch (obsolete) — Splinter Review
Hi

Sorry for the delay ;)

I had to scrap my old code make a clean start. I guess we can split this task into two rounds.
Where the first round is to get the menu configuration up and running.

As you can see I have so far figured out how to get the container and a way to solve if the context clicked node is an attribute. Let me know if there are better ways to do this.

I guess we could add the attribute name to the menu item labels too. 

Eg. Edit Attribute "rel"

Waiting for some feedback on this. 
In the mean time I will look further into the inline editor changes discussed in comment #8

Thomas
Attachment #8420571 - Flags: review?(pbrosset)
(Reporter)

Comment 10

5 years ago
Comment on attachment 8420571 [details] [diff] [review]
994555-menu-items.patch

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

No problem for the delay :) Take your time, this is going to be a pretty good feature, let's take time to make it right.
First of all, thanks, this first part works nicely. I think we're on the right track.
See my comments inline below.

As for the other parts:
- adding the attribute name after "edit attribute" and "delete attribute" would be great indeed, good idea,
- we need to make this small change to inplace-editor.js so that we can focus an editor programmatically,
- we'll need to add automated tests to test this new feature.

::: browser/devtools/markupview/markup-view.js
@@ +1212,5 @@
>      }.bind(this), 1000);
> +  },
> +
> +  _initMenu: function() {
> +    let doc = this.doc.defaultView.parent.document;

I think this._inspector.panelDoc should work, and would be easier to understand.

@@ +1218,5 @@
> +    let addAttributeMenuItem = menuPopup.querySelector("#node-menu-addattribute");
> +    let editAttributeMenuItem = menuPopup.querySelector("#node-menu-editattribute");
> +    let deleteAttributeMenuItem = menuPopup.querySelector("#node-menu-deleteattribute");
> +
> +    menuPopup.addEventListener("popupshowing", (event) => {

You need to remove this event listener once the markup-view is destroyed.
This means that you need to define the callback on the prototype instead of just inline here, and that you need to call removeEventListener in 'MarkupView.prototype.destroy'.

So I think only these lines should be in '_initMenu':
_initMenu: function() {
  this._onMenuShowing = this._onMenuShowing.bind(this);
  let popup = this._inspector.panelDoc.querySelector("#...");
  popup.addEventListener("popupshowing, this._onMenuShowing);
}

And then, on the prototype, you'll need to add:
_onMenuShowing: function(event) {
  /* all the rest ... */
}

This will allow you to do this in destroy:
popup.removeEventListener("popupshowing, this._onMenuShowing);

@@ +1310,5 @@
>    toString: function() {
>      return "[MarkupContainer for " + this.node + "]";
>    },
>  
> +  getClickedAttributeName: function(node) {

Checking the classList here is fine. I have a few suggestions though for this new method:

- it should reside in 'ElementEditor.prototype', that's where the actual DOM elements for the tag and attributes are created, so that's where the responsibility for checking if a given node is an attribute should be, to ease future refactoring.

This means that in 'MarkupView.prototype._initMenu', you'll need to check if 'this._selectedContainer.editor instanceof ElementEditor', and if yes, call the method.

- I think we can make it a little bit more useful, by calling it something more generic like 'getInfoAtNode(node)' (or something better, I can't find anything good right now), and by making it return information about the node that was clicked.

The return value could be something like:
'{type: "attribute", name: "id", value: "the-id", el: node}'
'{type: "tag", name: "div", el: node},
'{type: "new-attribute", el: node}'
(we'll see later what really needs to be returned).

You can keep using the classList to retrieve this information, and also make use of 'this.tag' and 'this.attrs' in 'ElementEditor' if needed.
Attachment #8420571 - Flags: review?(pbrosset) → feedback+

Comment 11

5 years ago
Posted patch 994555-menu-items-rev2.patch (obsolete) — Splinter Review
Hi and thank you for the feedback

This patch addresses the issues discussed in comment #10.

As you see I store the original label text in a data attribute on menu item in order to use it when an attribute is not clicked.
Could this string be fetched from somewhere else?


> I think we can make it a little bit more useful, by calling it something more generic like 'getInfoAtNode(node)' (or something better, I can't find anything good right now), and by making it return information about the node that was clicked.

Yes. I made it as simple as possible for now and we can carve this out as we go.

Suggestion:
As you see right now the edit and delete menu items are updated if the clicked node is an attribute.
We could expand on this so the menuitems also are updated when an attribute value is clicked.
What do you think?

> - we need to make this small change to inplace-editor.js so that we can focus an editor programmatically,

I will start looking at this next.

Thomas
Attachment #8420571 - Attachment is obsolete: true
Attachment #8421898 - Flags: review?(pbrosset)
(Reporter)

Comment 12

5 years ago
Are you sure you attached the right patch? The only difference between the last 2 patches is one empty line.
(Reporter)

Comment 13

5 years ago
Comment on attachment 8421898 [details] [diff] [review]
994555-menu-items-rev2.patch

Removing the R? flag for now as I think the patch isn't the one you intended.
Attachment #8421898 - Flags: review?(pbrosset)

Comment 14

5 years ago
Posted patch 994555-menu-items-rev2.patch (obsolete) — Splinter Review
> Are you sure you attached the right patch? The only difference between the last 2 patches is one empty line.

Ai, you are right. I guess I forgot to export it to my Mozilla patches directory.
This should be the patch I intended to submit.

Thomas
Attachment #8421898 - Attachment is obsolete: true
Attachment #8422523 - Flags: review?(pbrosset)
(Reporter)

Comment 15

5 years ago
Comment on attachment 8422523 [details] [diff] [review]
994555-menu-items-rev2.patch

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

Ok, looking good.

I'm setting F+ instead of R+ because I'd like to see one of the function re-arranged a tiny bit and also because this is just the first part.
Are you planning on providing the rest as separate patches, or all in one patch?

Anyway, good progress on this.

::: browser/devtools/markupview/markup-view.js
@@ +1123,5 @@
>  
>      this.tooltip.destroy();
>      this.tooltip = null;
>  
> +    this.menuPopup.removeEventListener("popupshowing", this._onMenuShowing, false);

nit: you didn't pass the third param when adding the event listener, don't pass it here either to make it consistent

@@ +1251,5 @@
> +    let deleteAttrMenuItem = this.menuPopup.querySelector("#node-menu-deleteattribute");
> +
> +    let nodeInfo = null;
> +    if (triggerNode && this._selectedContainer.editor instanceof ElementEditor) {
> +      nodeInfo = this._selectedContainer.editor.getInfoAtNode(triggerNode);

Can I suggest that you move these 2 lines as conditions at the beginning of the function, and use them to do an early return? This would simplify the rest of the function. Example:

let triggerNode = event.target.triggerNode;
let isElement = this._selectedContainer.editor instanceof ElementEditor;
let nodeInfo = this._selectedContainer.editor.getInfoAtNode(triggerNode);

if (!triggerNode || !isElement || !nodeInfo) {
  // do an early return here by setting the attribute as disabled
  // ...
  return;
}

// This means the rest of the function can be more simple

let addAttrMenuItem = this.menuPopup.querySelector("#node-menu-addattribute");
let editAttrMenuItem = this.menuPopup.querySelector("#node-menu-editattribute");
let deleteAttrMenuItem = this.menuPopup.querySelector("#node-menu-deleteattribute");

// ...

@@ +1869,5 @@
> +   * Returns information about a node.
> +   * Used by the context menu.
> +   *
> +   * @param aNode
> +   *        The node in the mark-up view.

This comment should be more specific as the function doesn't deal with *all* nodes in the markup-view, but rather nodes part of the ElementEditor.

@@ +1875,5 @@
> +   * @return object
> +   *         An object literal with the following information.
> +   *         Example: {type: "attribute", name: "rel", value: "Index", el: node}
> +   */
> +  getInfoAtNode: function(aNode) {

I think this function should check if aNode is part of this ElementEditor's container node.
Attachment #8422523 - Flags: review?(pbrosset) → feedback+

Comment 16

5 years ago
Posted patch 994555-menu-items-rev3.patch (obsolete) — Splinter Review
Hi again Patric

This patch should address the issues discussed in comment #15

> Are you planning on providing the rest as separate patches, or all in one patch?
I am still learning to work with patches and my initial idea was to have separate patches, but if it works better for you I can do the whole task in one patch. Incrementing as we go. I guess separate patches is more tedious when testing the patch/es?.

I will look at the functionality for the menu item commands next.

Thomas
Attachment #8422523 - Attachment is obsolete: true
Attachment #8424333 - Flags: review?(pbrosset)
(Reporter)

Comment 17

5 years ago
(In reply to Thomas Andersen from comment #16)
> I am still learning to work with patches and my initial idea was to have
> separate patches, but if it works better for you I can do the whole task in
> one patch. Incrementing as we go. I guess separate patches is more tedious
> when testing the patch/es?.
Several patches is fine. Easier to review too.
(Reporter)

Comment 18

5 years ago
Comment on attachment 8424333 [details] [diff] [review]
994555-menu-items-rev3.patch

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

Looks good. R+ with a couple of minor changes for this part.
About the several patches, as I said in my previous comment, it's totally fine, and it eases review. The only thing we need to worry about is whether we want to land this as several patches because, if yes, each patch needs to work on its own.
It's not the case here because there are 'console.log' statements in the code.
What I would suggest is keep this patch as is, then work on the other ones, and at the end, when everything reviewed, we can always fold all the patches together pretty easily.
Thanks for your work.

::: browser/devtools/markupview/markup-view.js
@@ +1248,5 @@
> +    let isElement = this._selectedContainer.editor instanceof ElementEditor;
> +    let nodeInfo = this._selectedContainer.editor.getInfoAtNode(triggerNode);
> +
> +    if (!(triggerNode || isElement || nodeInfo)) {
> +      return;

Don't you need to also disable the 3 menu entries here?
Imagine the use case where I right-click on a node, so the code below this line is executed and menu items are enabled as appropriate, and then I right-click somewhere in the markup-view, below the last node (in the white area). In this case, the items that had been enabled before will remain enabled.

Maybe you should have:
addAttrMenuItem.setAttribute("disabled", true);
editAttrMenuItem.setAttribute("disabled", true);
deleteAttrMenuItem.setAttribute("disabled", true);
at the top of this function, so that when it executes, you're sure that all items are diabled by default.

@@ +1255,5 @@
> +    let addAttrMenuItem = this.menuPopup.querySelector("#node-menu-addattribute");
> +    let editAttrMenuItem = this.menuPopup.querySelector("#node-menu-editattribute");
> +    let deleteAttrMenuItem = this.menuPopup.querySelector("#node-menu-deleteattribute");
> +
> +    let triggerNodeIsAttr = nodeInfo && nodeInfo.type == "attribute";

nit: I think we usually prefer naming boolean variables with the 'is' part first: isTriggerNodeAttr.
Attachment #8424333 - Flags: review?(pbrosset) → review+
Comment hidden (obsolete)
Comment hidden (obsolete)

Comment 21

5 years ago
Hi again and sorry for the delay.
I am currently moving to a new apartment to another place in the country so my time in front of the keyboard is limited at the moment.

> Don't you need to also disable the 3 menu entries here?

I expected feedback on this one ;)

Maybe the naming of things is a bit bad or there is a need for some commenting in the code.

The isElement variable is always true (as far as I can see) so the code after the IF block always gets executed. I am not sure if the isElement variable is needed at all.
I guess it is true because there is always a _selectedContainer in the view. Eg. when I open the Inspector (from browser menu or the hotkey) an element on the page is always selected in Inspector (eg. <body>)
Of-course I may be wrong and it is just luck that the current code is working and we should reset the menu items (disable and update labels) each time the popup is shown.

Need some feedback on this.

Thomas

PS. Hopefully when my moving project is settled I will get some more focus on this issue.
Flags: needinfo?(pbrosset)
(Reporter)

Comment 22

5 years ago
(In reply to Thomas Andersen from comment #21)
> Hi again and sorry for the delay.
> I am currently moving to a new apartment to another place in the country so
> my time in front of the keyboard is limited at the moment.
No need to apologize, no problem at all

> > Don't you need to also disable the 3 menu entries here?
> 
> I expected feedback on this one ;)
> 
> Maybe the naming of things is a bit bad or there is a need for some
> commenting in the code.
> 
> The isElement variable is always true (as far as I can see) so the code
> after the IF block always gets executed. I am not sure if the isElement
> variable is needed at all.
> I guess it is true because there is always a _selectedContainer in the view.
> Eg. when I open the Inspector (from browser menu or the hotkey) an element
> on the page is always selected in Inspector (eg. <body>)
> Of-course I may be wrong and it is just luck that the current code is
> working and we should reset the menu items (disable and update labels) each
> time the popup is shown.
Hmm, not sure that isElement is always going to be true. If you right-click on a comment node, or text node, or DOCTYPE node, or even on no node at all (below the closing </html> tag in the markup-view).
And nodeInfo can also be null in some cases too.
Flags: needinfo?(pbrosset)
Mentor: pbrosset
Whiteboard: [good first bug][lang=js][mentor=pbrosset] → [good first bug][lang=js]
Hey Thomas,
Are you still working on this?
Flags: needinfo?(thomas)

Comment 24

5 years ago
Hi Akshendra

No, not right now. Feel free to continue. If not I have to wait a bit before continuing.

Regards, 
Thomas
Flags: needinfo?(thomas)
(Reporter)

Updated

4 years ago
Assignee: thomas → nobody

Updated

4 years ago
tracking-win: --- → ?

Updated

4 years ago
tracking-win: ? → ---
(Assignee)

Comment 25

4 years ago
Posted patch Patch for review (obsolete) — Splinter Review
I have faced a problem with attribute removal – it removes attributes only in editor and not on the backend side. It is done this way:
> removeAttribute: function() {
>   let container = this.markup.getContainer(this.selection.nodeFront);
>   let attrName = container.editor.getInfoAtNode(this.nodemenuTriggerNode).name;
>   let attr = container.editor.attrElements.get(attrName);
>   attr.remove();
> }

Also I have created this.nodemenuTriggerNode in InspectorPanel to use it in oncommand functions later. Didn't find other way how I can pass event node to those functions.

And markupContainer in _setupNodeMenu is now declared on top and shared between entire function.
Attachment #8670496 - Flags: feedback?(pbrosset)
(Assignee)

Comment 26

4 years ago
Posted patch 994555-menu-items-rev5.patch (obsolete) — Splinter Review
Attribute removal works now, as well as undo / redo functionality.
Attachment #8670496 - Attachment is obsolete: true
Attachment #8670496 - Flags: feedback?(pbrosset)
Attachment #8670969 - Flags: feedback?(pbrosset)
(Reporter)

Comment 27

4 years ago
Comment on attachment 8670969 [details] [diff] [review]
994555-menu-items-rev5.patch

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

This is looking quite good. I've made some comments below. Mostly about re-organizing some of the code and placing the new menu items in a sub-menu.

::: devtools/client/inspector/inspector-panel.js
@@ +75,5 @@
>    this._target = toolbox._target;
>    this.panelDoc = iframeWindow.document;
>    this.panelWin = iframeWindow;
>    this.panelWin.inspector = this;
> +  this.nodemenuTriggerNode = null;

Please use camelcase for nodeMenu, and menu no need to repeat the word node, so:
this.nodeMenuTrigger = null;
would be enough.

@@ +650,5 @@
>    /**
>     * Disable the delete item if needed. Update the pseudo classes.
>     */
> +  _setupNodeMenu: function(event) {
> +    let markupContainer = this.markup.importNode(this.selection.nodeFront, false);

I know this code was here before, but it looks weird to me. When _setupNodeMenu is called, we know the markupcontainer has been imported already.
So instead, you could do:

let markupContainer = this.markup.getContainer(this.selection.nodeFront);

to just get it.

@@ +778,5 @@
>      } else {
>        copyImageData.setAttribute("disabled", "true");
>      }
> +
> +    // Enable / disable "Add Attribute", "Edit Attribute"

Can you move this big block of code (from here to the end of the function) to a new separate function please? _setupAttributeMenu for example.
I want to keep this function not too long.

@@ +783,5 @@
> +    // and "Remove Attribute" items
> +    let addAttribute = this.panelDoc.getElementById("node-menu-add-attribute");
> +    let editAttribute = this.panelDoc.getElementById("node-menu-edit-attribute");
> +    let removeAttribute = this.panelDoc.getElementById("node-menu-remove-attribute");
> +    let triggerNode = this.nodemenuTriggerNode;

No need to define a local variable here since you're using it only once. Just use this.nodeMenuTrigger.

@@ +784,5 @@
> +    let addAttribute = this.panelDoc.getElementById("node-menu-add-attribute");
> +    let editAttribute = this.panelDoc.getElementById("node-menu-edit-attribute");
> +    let removeAttribute = this.panelDoc.getElementById("node-menu-remove-attribute");
> +    let triggerNode = this.nodemenuTriggerNode;
> +    let nodeInfo = (typeof markupContainer.editor.getInfoAtNode === "function") &&

Testing for markupContainer.editor.getInfoAtNode should work, right? Also, we usually either use if statements or ternary expressions in this case in devtools code, not condition && expression. So:

let nodeInfo = markupContainer.editor.getInfoAtNode
               ? markupContainer.editor.getInfoAtNode(this.nodeMenuTrigger)
               : null;

@@ +795,5 @@
> +      addAttribute.setAttribute("disabled", "true");
> +    }
> +
> +    // Enable "Edit Attribute" and "Remove Attribute" only on attribute click
> +    if (isEditableElement && nodeInfo.type === "attribute") {

Ah, I see now why getInfoAtNode returned an empty object instead of null. Still I think returning null states the meaning better, so please change this to also test if nodeInfo isn't null.

@@ +798,5 @@
> +    // Enable "Edit Attribute" and "Remove Attribute" only on attribute click
> +    if (isEditableElement && nodeInfo.type === "attribute") {
> +      editAttribute.removeAttribute("disabled");
> +      editAttribute.setAttribute("label",
> +        editAttribute.getAttribute("data-label") + " \"" + nodeInfo.name + "\"");

Ok, so you need data-label here in order to keep the original value of the label, I see.
I think there's a better way to do this though.

Add the string to \browser\locales\en-US\chrome\browser\devtools\inspector.properties instead of inspector.dtd and use a template string:

inspector.menu.editAttribute.label=Edit attribute %S
inspector.menu.deleteAttribute.label=Edit attribute %S

This way, you can do something like:

editAttribute.setAttribute("label", strings.formatStringFromName("inspector.menu.editAttribute.label", [nodeInfo.name], 1));

@@ +1235,5 @@
>  
> +  /**
> +   * Add attribute to node.
> +   */
> +  addAttribute: function() {

Please rename to onAddAttribute, and explain in the comment that this is a callback for the menu, not to be called directly.

@@ +1243,5 @@
> +
> +  /**
> +   * Remove attribute from node.
> +   */
> +  removeAttribute: function() {

Please rename to onRemoveAttribute, and explain in the comment that this is a callback for the menu, not to be called directly.

@@ +1266,5 @@
> +
> +  /**
> +   * Edit attribute for node.
> +   */
> +  editAttribute: function() {

Please rename to onEditAttribute, and explain in the comment that this is a callback for the menu, not to be called directly.

@@ +1268,5 @@
> +   * Edit attribute for node.
> +   */
> +  editAttribute: function() {
> +    let container = this.markup.getContainer(this.selection.nodeFront);
> +    let attrName = container.editor.getInfoAtNode(this.nodemenuTriggerNode).name;

I think _setupNodeMenu should store nodeInfo on something like this.nodeMenuTriggerInfo so that you can reuse it here instead of calling again getInfoAtNode.

And I think this code should be in markup-view.js instead, not here. So maybe something like this would be better:

/**
 * This is a callback for the node context menu, called when the user
 * clicks the edit attribute item.
 */
onEditAttribute: function() {
  if (!this.nodeMenuTriggerInfo) {
    return;
  }
  let container = this.markup.getContainer(this.selection.nodeFront);
  container.editAttribute(this.nodeMenuTriggerInfo.name);
}

And the rest should be done in the markup container class.

Please do something similar with addAttribute and removeAttribute.

::: devtools/client/inspector/inspector.xul
@@ +106,5 @@
>        <menuitem id="node-menu-delete"
>          label="&inspectorHTMLDelete.label;"
>          accesskey="&inspectorHTMLDelete.accesskey;"
>          oncommand="inspector.deleteNode()"/>
> +      <menuitem id="node-menu-add-attribute"

We have recently filed bug 1211613 to clean up this menu. It's getting really long and the more menu items we add, the harder it becomes to find something.
We need to introduce some second-level menus in there somewhere.

I'd like us to work on bug 1211613 first before this one, but at the same time, I don't want to slow you down, so I'm going to suggest that you add a new "attributes" sub-menu here that contains these 3 items.

@@ +112,5 @@
> +        accesskey="&inspectorAddAttribute.accesskey;"
> +        oncommand="inspector.addAttribute()"/>
> +      <menuitem id="node-menu-edit-attribute"
> +        label="&inspectorEditAttribute.label;"
> +        data-label="&inspectorEditAttribute.label;"

Why do you need data-label here?

@@ +117,5 @@
> +        accesskey="&inspectorEditAttribute.accesskey;"
> +        oncommand="inspector.editAttribute()"/>
> +      <menuitem id="node-menu-remove-attribute"
> +        label="&inspectorRemoveAttribute.label;"
> +        data-label="&inspectorRemoveAttribute.label;"

and here?

::: devtools/client/markupview/markup-view.js
@@ +2549,5 @@
> +   * @return object
> +   *         An object literal with the following information:
> +   *         { type: "attribute", name: "rel", value: "Index", el: node }
> +   */
> +  getInfoAtNode: function(aNode) {

Please rename aNode to node. We're trying to get rid of the aArg syntax style.

@@ +2554,5 @@
> +    let type = null;
> +    let name = null;
> +    let value = null;
> +
> +    // When aNode is null

No need for this comment, the code is self explanatory.

@@ +2555,5 @@
> +    let name = null;
> +    let value = null;
> +
> +    // When aNode is null
> +    if (!aNode) return {};

Please move this up so it's first in the function. No need to do anything else before that if aNode is null.
Also, please use {} around the if body.
And I think returning null instead of an empty object would be more meaningful.

getInfoAtNode: function(node) {
  if (!node) {
    return null;
  }

  ...
}

@@ +2570,5 @@
> +      type: type,
> +      name: name,
> +      value: value,
> +      el: aNode
> +    };

This can be simplified a bit using ES2015:

return {type, name, value, el: node};
Attachment #8670969 - Flags: feedback?(pbrosset) → feedback+
(Assignee)

Comment 28

4 years ago
Thanks for review, Patrick!

In new patch this.nodeMenuTrigger became unnecessary because we have this.nodeMenuTriggerInfo.
Attachment #8670969 - Attachment is obsolete: true
Attachment #8672162 - Flags: review?(pbrosset)
(Reporter)

Comment 29

4 years ago
Comment on attachment 8672162 [details] [diff] [review]
994555-context-menu-attributes.patch

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

Thanks! I see all of my comments have been addressed. This looks great.
I just have a few additional comments before I R+ this. Also, we need to create automated tests so that we know when this feature regresses, if it does in the future.
Have you ever written devtools mochitests before? Do you need help? Let me know, I can also write a skeleton file for a new test if that helps.

::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
@@ +107,5 @@
> +
> +# LOCALIZATION NOTE (inspector.menu.editAttribute.label): This is the label of a
> +# sub-menu "Attributes" in the inspector contextual-menu that appears
> +# when the user right-clicks on the node in the inspector, and that allows
> +# to add new attribute to this node.

to edit an attribute on this node.

::: devtools/client/inspector/inspector-panel.js
@@ +653,5 @@
> +  _setupNodeMenu: function(event) {
> +    let markupContainer = this.markup.getContainer(this.selection.nodeFront);
> +    this.nodeMenuTriggerInfo = markupContainer.editor.getInfoAtNode
> +      ? markupContainer.editor.getInfoAtNode(event.target.triggerNode)
> +      : null;

Sorry for commenting again on the same piece of code as in my last review, but it would be simpler to just implement getInfoAtNode in all instances of Editors: TextEditor, GenericEditor and ElementEditor.
So, all of them should have a getInfoAtNode method, but the 2 first ones could just return null.

::: devtools/client/inspector/inspector.xul
@@ +106,5 @@
>        <menuitem id="node-menu-delete"
>          label="&inspectorHTMLDelete.label;"
>          accesskey="&inspectorHTMLDelete.accesskey;"
>          oncommand="inspector.deleteNode()"/>
> +      <menu id="node-menu-attributes-submenu"

I don't think you need to define an id for this menu element.

::: devtools/client/markupview/markup-view.js
@@ +2334,5 @@
> +        doMods.apply();
> +      }, () => {
> +        undoMods.apply();
> +      });
> +    } catch(x) {

The code inside try looks good (I wish it were simpler, and that do/undo were managed on the backend, but that's another discussion), but I forget why we even need a try/catch here. Do you mind removing it and checking what can actually fail here?

@@ +2576,5 @@
>  
>    /**
> +   * Returns information about node in the editor.
> +   *
> +   * @param aNode

@param {DOMNode} node

@@ +2579,5 @@
> +   *
> +   * @param aNode
> +   *        The node to get information from.
> +   *
> +   * @return object

@return {Object}
Attachment #8672162 - Flags: review?(pbrosset) → feedback+
(Assignee)

Comment 30

4 years ago
> The code inside try looks good (I wish it were simpler, and that do/undo were managed on the
> backend, but that's another discussion), but I forget why we even need a try/catch here.
> Do you mind removing it and checking what can actually fail here?

I have tried to remove try / catch block and everything continue to work fine. In markup-view.js there are 2 invocations of undo.do with try / catch and 4 without. Using debugger I found throw in nested code near spec.request.write, but I didn't find reasons why it could behave differently between code with and without try / catch. So I have removed every try / catch block in this patch.

About tests. There are test cases in browser_inspector_menu-01-sensitivity.js for checking enabled / disabled state for attribute items. And another test cases are in browser_inspector_menu-05-attribute-items.js with menu clicks and keyboard inputs.
Doing it first time, likely it looks a bit awkward.

Also I have renamed "Attributes" submenu item to "Attribute".
Attachment #8672162 - Attachment is obsolete: true
Attachment #8674527 - Flags: review?(pbrosset)
(Reporter)

Updated

4 years ago
Attachment #8424333 - Attachment is obsolete: true
(Reporter)

Comment 31

4 years ago
Comment on attachment 8674527 [details] [diff] [review]
994555-context-menu-attribute-items.patch

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

I see you've addressed my last comments, thanks.
Did you 'hg move' the 05-other into 06-other or did you delete and re-create it? If you didn't move it, this means the file history will be broken, which we don't want. Maybe you should just keep 05-other as it is and name your new file 06-attribute-items.js

Other than this, this works very well.
I just made some comments about the test.

::: devtools/client/inspector/test/browser_inspector_menu-01-sensitivity.js
@@ +38,5 @@
>    "node-menu-pseudo-active",
>    "node-menu-pseudo-focus",
>    "node-menu-scrollnodeintoview",
>    "node-menu-screenshotnode"
> +].concat(PASTE_MENU_ITEMS, ACTIVE_ON_DOCTYPE_ITEMS, ATTRIBUTE_MENU_ITEMS);

Does this array need to be in a particular order? If not, since you're not reusing ATTRIBUTE_MENU_ITEMS anywhere else, I suggest removing this const and just listing the 3 new item IDs inside the ALL_MENU_ITEMS array, as strings.

@@ +199,5 @@
> +  {
> +    desc: "<element> with context menu triggered on attribute, empty clipboard",
> +    selector: "#attributes",
> +    disabled: PASTE_MENU_ITEMS.concat(["node-menu-copyimagedatauri"]),
> +    attributeTrigger: "data-edit"

Really like this new attributeTrigger property. However, it's kind of hard for someone who doesn't know the test to know what this does. Could you add a comment at the top of TEST_CASES that says something along these lines:

// Test cases, each item of this array may define the following properties:
// desc: a string that will be logged
// selector: the selector of the node to be selected
// disabled: ...
// attributeTrigger: ...

::: devtools/client/inspector/test/browser_inspector_menu-05-attribute-items.js
@@ +21,5 @@
> +    ok(addAttribute, "the popup menu has a 'Add Attribute' item");
> +
> +    info("Triggering 'Add Attribute' and waiting for mutation to occur");
> +    dispatchCommandEvent(addAttribute);
> +    EventUtils.synthesizeKey('class="u-hidden"', {});

You know what, I've never seen synthesizeKey used with a string before, I've only ever used it with key codes like VK_RETURN, etc... Cool that this works.

@@ +23,5 @@
> +    info("Triggering 'Add Attribute' and waiting for mutation to occur");
> +    dispatchCommandEvent(addAttribute);
> +    EventUtils.synthesizeKey('class="u-hidden"', {});
> +    EventUtils.synthesizeKey('VK_RETURN', {});
> +    yield inspector.once("markupmutation");

I think starting to listen for the event before doing the action that causes is clearer:

let onMutation = inspector.once("markupmutation");
EventUtils.synthesizeKey('VK_RETURN', {});
yield onMutation;

@@ +25,5 @@
> +    EventUtils.synthesizeKey('class="u-hidden"', {});
> +    EventUtils.synthesizeKey('VK_RETURN', {});
> +    yield inspector.once("markupmutation");
> +
> +    let classList = content.document.getElementById('attributes').classList;

So here you're accessing the content document directly, through 'content'. We used to do this, and it still somehow works, but we trying to get rid of it for a couple of reasons:
- e10s (electrolysis): this is the multi-process version of firefox that, when used in tests, means that the test and the test page run in separate process, and therefore the test does not have direct access to the test page. 'content' still works though, thanks to CPOWs (cross-process object wrappers), but we try and not use them at all.
- luciddream: this is a new test infrastructure we're trying to put in place that allows to run tests against remote devices. This means that the test does not have access to the test page either, even through CPOWs.

Instead, you should use the testActor, which is an object that exposes async methods to check things in the test page. It's made specifically for this.
Take a look at the testActor's code: devtools\client\shared\test\test-actor.js
I think you could use this:

let hasClass = yield testActor.hasNode("#attributes.u-hidden");
ok(hasClass, "attributes was successfully added");

@@ +44,5 @@
> +    };
> +    dispatchCommandEvent(editAttribute);
> +    EventUtils.synthesizeKey("data-edit='edited'", {});
> +    EventUtils.synthesizeKey('VK_RETURN', {});
> +    yield inspector.once("markupmutation");

Same comment about listening before pressing RETURN.

@@ +47,5 @@
> +    EventUtils.synthesizeKey('VK_RETURN', {});
> +    yield inspector.once("markupmutation");
> +
> +    let attributeValue =
> +      content.document.getElementById('attributes').getAttribute('data-edit');

Same comment about using the testActor here.
By the way, if the methods in testActor aren't enough to do what you need here, feel free to add more.
Note also that testActor exposes a handy 'eval' function that you may want to use in certain cases to execute random code.

@@ +64,5 @@
> +      type: "attribute",
> +      name: "data-remove"
> +    };
> +    dispatchCommandEvent(removeAttribute);
> +    yield inspector.once("markupmutation");

And same comment here.

@@ +66,5 @@
> +    };
> +    dispatchCommandEvent(removeAttribute);
> +    yield inspector.once("markupmutation");
> +
> +    let element = content.document.getElementById('attributes');

And here.

::: devtools/client/inspector/test/browser_inspector_menu-06-other.js
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +

I'm assuming this is just what 05-other used to be.
Attachment #8674527 - Flags: review?(pbrosset) → feedback+

Comment 32

4 years ago
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #31)
> ::: devtools/client/inspector/test/browser_inspector_menu-06-other.js
> @@ +1,5 @@
> > +/* vim: set ts=2 et sw=2 tw=80: */
> > +/* Any copyright is dedicated to the Public Domain.
> > +http://creativecommons.org/publicdomain/zero/1.0/ */
> > +"use strict";
> > +
> 
> I'm assuming this is just what 05-other used to be.

If it is, can you do an `hg rename` on the old file instead of deleting then readding it ? It'll be less confusing that way.
(Assignee)

Comment 33

4 years ago
I have done renaming for browser_inspector_menu-05-other.js correctly now.

Thanks for explaining what e10s and luciddream is. Also hasNode method on testActor is super useful, I can make all attribute checks just with it.
Attachment #8674527 - Attachment is obsolete: true
Attachment #8675923 - Flags: review?(pbrosset)
(Reporter)

Comment 34

4 years ago
Comment on attachment 8675923 [details] [diff] [review]
994555-context-menu-attribute-items.patch

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

Alright, this looks really good. I'll mark the patch as R+ with some comments below. This means you should address these comments and re-upload a new patch, but don't need to ask me for review again. When you upload the new patch, just mark it as R+ yourself.

One last thing: can you change the commit message to contain the name of the reviewer:

Bug 994555 - [markup view] Context menu to easily edit, add and delete attributes; r=pbro

::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
@@ +107,5 @@
> +# LOCALIZATION NOTE (inspector.menu.editAttribute.label): This is the label of a
> +# sub-menu "Attribute" in the inspector contextual-menu that appears
> +# when the user right-clicks on the node in the inspector, and that allows
> +# to edit an attribute on this node.
> +

nit: please remove empty line here.

::: devtools/client/inspector/test/browser_inspector_menu-05-attribute-items.js
@@ +15,5 @@
> +  yield testRemoveAttribute();
> +
> +  function* testAddAttribute() {
> +    info("Testing 'Add Attribute' menu item");
> +    yield selectNode("#attributes", inspector);

No need to select the same node again in each of the 3 functions.
Please move this to the top, just after openInspectorForURL, and remove it from the 3 test functions.

@@ +17,5 @@
> +  function* testAddAttribute() {
> +    info("Testing 'Add Attribute' menu item");
> +    yield selectNode("#attributes", inspector);
> +    let addAttribute = inspector.panelDoc.getElementById("node-menu-add-attribute");
> +    ok(addAttribute, "the popup menu has a 'Add Attribute' item");

The 2 last lines are repeated in all 3 functions. Please create another function to put that code in common:

function checkMenuItem(id, name) {
  let attribute = inspector.panelDoc.getElementById(id);
  ok(attribute, "The popup menu has a '" + name + "' item");
}

@@ +34,5 @@
> +  function* testEditAttribute() {
> +    info("Testing 'Edit Attribute' menu item");
> +    yield selectNode("#attributes", inspector);
> +    let editAttribute = inspector.panelDoc.getElementById("node-menu-edit-attribute");
> +    let attributeSubmenu = inspector.panelDoc.getElementById("node-menu-attribute-submenu");

This isn't used anywhere. Please remove this line.
Attachment #8675923 - Flags: review?(pbrosset) → review+
(Reporter)

Updated

4 years ago
Assignee: nobody → grisha
(Reporter)

Comment 35

4 years ago
Here's a TRY build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1191e19182bb
This is to check that all tests pass on all platforms. If the result is green, I'll mark the patch for check-in.
Grisha: thanks for your work here, really appreciated! Looks like you're done for now (unless TRY comes back with test failures, in which case we'll have to investigate why).
If you want to contribute more to devtools, http://firefox-dev.tools is a good way to find more bugs to work on. Or come and join #devtools on the mozilla IRC server and chat with us.
(Reporter)

Comment 36

4 years ago
Ok try looks good, we can check this in. Unfortunately bug 1208864 has been marked for checkin before and is about to go in. That bug touches the same menu and so there will for sure be merge conflicts to solve if we mark this patch for checkin.

Grisha: Could you either wait for bug 1208864 to be checked in, or apply its patch locally, and then rebase yours on top of it, and then re-upload it here?
Let me know if you would like me to do this instead.
Depends on: 1208864
(Reporter)

Updated

4 years ago
Blocks: 1203303
(Reporter)

Updated

4 years ago
Blocks: de-44-polish
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][polish-backlog]
(Reporter)

Comment 37

4 years ago
Rebased and addressed last review comments.
Attachment #8675923 - Attachment is obsolete: true
Attachment #8676250 - Flags: review+
(Reporter)

Comment 38

4 years ago
My previous rebase had some merge problems (a lot of empty lines got removed). This should be better.
Attachment #8676250 - Attachment is obsolete: true
Attachment #8676283 - Flags: review+
(Assignee)

Comment 40

4 years ago
While I was away everything was done. You are so fast, Patrick! :)

As I saw in a last posted link, this task has 44.0a1 milestone. Does it mean it will be integrated in Nightly build and how soon it will be?
(Reporter)

Comment 41

4 years ago
(In reply to Grisha Pushkov from comment #40)
> While I was away everything was done. You are so fast, Patrick! :)
> 
> As I saw in a last posted link, this task has 44.0a1 milestone. Does it mean
> it will be integrated in Nightly build and how soon it will be?
This should land in the next Nightly build (tomorrow). After this, the next merge to Developer Edition is on the Nov. 2nd. Developer Edition is one of the versions of Firefox that has a whole lot of devtools users.
https://hg.mozilla.org/mozilla-central/rev/d615cb506271
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
I've updated the bit on the popup menu to include these items: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Element_popup_menu

I've also changed the format of that from a screenshot to a bulleted list of links into the table. Hopefully this is a bit easier to navigate, although not as pretty. We could omit the list of links, and just have the table? But I think it might make it hard to find things, now we have 31 items in the menu.
Flags: needinfo?(pbrosset)
(Reporter)

Comment 44

4 years ago
Yeah, we have far too many items ... Bug 1211613 should help a little (we'd have the same number of items, but they'd be easier to find).
Anyway, as for the doc, I think it's great, but I do agree that we could emit the list. It looks like duplicated information.
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #44)
> Yeah, we have far too many items ... Bug 1211613 should help a little (we'd
> have the same number of items, but they'd be easier to find).
> Anyway, as for the doc, I think it's great, but I do agree that we could
> emit the list. It looks like duplicated information.

I asked the same question in bug 1208864, and ntim said he liked the list[1]. I knew this would happen if I asked more than 1 person :-P. Since I think it is useful, I will keep it in for now, but can remove it if you or enough other people feel strongly.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1208864#c28

Updated

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