575234 - Create attribute-value editor for HTML inspector

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

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

Create a popup editor panel for editing DOM nodes in the HTML panel, DOM panel and styles in the Style panel.

https://wiki.mozilla.org/Firefox/Projects/Inspector#0.6

Comment 1

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

Attachment: popup editor

initial editor patch. Still needs a unittest.

Comment 2

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

Attachment: popup editor

minor fix in onEditorChange.

Comment 3

[Kevin Dangoor]

Comment on attachment 462831 [details] [diff] [review]
popup editor

Changing reviewer to dolske, as gavin is out for a few days.

Comment 4

[Kevin Dangoor]

Requesting blocking because live editing of the page is a key desired feature of the Inspector.

Comment 5

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

Attachment: Inspector popup editor 2010-08-13

Updated patch rebased today on latest tree panel.

There's a bug in the editing code. When a node is changed and applied (currently by clicking away from the editor to commit the change), the node is not updated in the tree panel. Clicking on that node again in the tree panel causes a different node to appear.

needs a little work.

Comment 6

[Patrick Walton (:pcwalton)]

Comment on attachment 465822 [details] [diff] [review]
Inspector popup editor 2010-08-13

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -276,16 +276,17 @@
>                        accesskey="&inspectDOMButton.accesskey;"
>                        class="toolbarbutton-text"
>                        oncommand="InspectorUI.toggleDOMPanel();"/>
>       </toolbar>
>       <browser id="inspector-tree-browser"
>                flex="1"
>                src="chrome://browser/content/inspector.html"
>                onclick="InspectorUI.onTreeClick(event);"
>+               ondblclick="InspectorUI.onTreeDblClick(event);"
>                disablehistory="true" />
>       <hbox align="end">
>         <spacer flex="1" />
>         <resizer dir="bottomend" />
>       </hbox>
>     </panel>
> 
>     <panel id="inspector-style-panel"

Might be nice to also be able to edit a DOM element by pressing Enter.

>@@ -1729,16 +1861,44 @@ var InspectorUI = {
>       if (leftSpace < 0 || rightSpace < 0) {
>         let centerX = offset.x - (scrollBox.clientWidth / 2);
>         scrollBox.scrollLeft = centerX;
>       }
>     }
>   },
> 
>   /**
>+   * Return a textual representation of a DOM node.
>+   * e.g., <tag attr1="val"..attrn>innerHTML</tag>
>+   * @param aNode
>+   *        The node to convert
>+   * @returns string
>+   */
>+  nodeTextValue: function IUI_nodeTextValue(aNode)
>+  {
>+    let nodeTag = aNode.tagName.toLowerCase();
>+    let attributes = [];
>+
>+    if (aNode.hasAttribute("id"))
>+      attributes.push('id="' + aNode.id +'"');
>+    for (let i = 0; i < aNode.attributes.length; ++i) {
>+      let item = aNode.attributes[i];
>+      if (item.name.toLowerCase() == "id")
>+        continue;
>+      attributes.push(item.name + '="' + item.value + '"');
>+    }
>+
>+    let attributeText = attributes.join(" ");
>+
>+    return "<" + nodeTag + " " + attributeText + ">" +
>+      aNode.innerHTML + "</" + nodeTag + ">";
>+  },
>+
>+  /**
>+   * Debug logging facility.

What if the item value contains double quotes?

It might be more helpful, not to mention easier for you, to strip off the inner HTML and only present the element itself for editing. That way, the user can edit outer body nodes without being overwhelmed by text. (WebKit does this.)

Also, before this gets pushed, we'll need to have a "save" button. We'll also probably want the textarea to be set in a monospace font.

Comment 7

[Patrick Walton (:pcwalton)]

Another thought: How about using contentEditable to allow editing of the nodes directly in the HTML panel? I did a quick spike on this and it seems to work fine. The only caveat is that the editable <span>s need to be turned into <div>s with the "display: inline-block" CSS rule attached.

This could yield a better user experience with even less code.

Comment 8

[Kevin Dangoor]

We've had more discussion about this out of this bug. The issues with using contentEditable here are that:

1. the user experience is only good if you're just trying to replace a single attribute value or text node value. Once you want to start adding tags and attributes it gets considerably more painful to work with.

2. the code also gets more complex once you start accounting for all of those other operations.

(I'm changing this bug's summary to reflect that it's specifically for the HTML editor as the style editor is a separate bug.)

Comment 9

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

Attachment: HTML Editor, first draft

rewrote the editor to use a div in the html panel. Also removed some extraneous styles from inspector.css that were unused.

Comment 10

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

Attachment: HTML Editor, first draft, now with focus!

Added a focus() call to set focus when a user activates the editor by double-clicking.

Comment 11

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

Attachment: HTML Editor, first draft, now with focus and node toggling!!

somehow lost the method to toggle nodes from this patch. Readded it. It probably belongs in the Tree Panel patches proper.

Comment 12

[Patrick Walton (:pcwalton)]

Comment on attachment 466784 [details] [diff] [review]
HTML Editor, first draft, now with focus and node toggling!!

>diff --git a/browser/base/content/inspector.html b/browser/base/content/inspector.html
>--- a/browser/base/content/inspector.html
>+++ b/browser/base/content/inspector.html
>@@ -3,11 +3,19 @@
> 
> <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
> <head>
>   <title>Inspector</title>
>   <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>
>   <link rel="stylesheet" href="chrome://browser/skin/inspector.css" type="text/css"/>
> </head>
> <body role="application">
>-<div class="panelNode" role="progressbar">  ....loading....</div>
>+<div id="inspector-editor-html" class="editorRight">
>+  <div id="editor-text" class="editorText" contenteditable="true">
>+    this is an editor div.
>+  </div>

Does the "this is an editor div" text need to be present?

>+  <div id="editor-buttons" class="editorButtons" align="right">
>+    <button id="html-editor-save">Save</button>
>+    <button id="html-editor-discard">Discard</button>
>+  </div>
>+</div>
> </body>
> </html>
>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
>--- a/browser/base/content/inspector.js
>+++ b/browser/base/content/inspector.js
>@@ -739,19 +739,88 @@ InsideOutBox.prototype =
>       if (!parentChildBox)
>         return null;
> 
>       return this.findChildObjectBox(parentChildBox, aObject);
>     }
>   },
> 
>   /**
>+   * Append an object box to a parent.
>+   * @param aParentNodeBox
>+   *        The box to attach the object to.
>+   * @param aRepObject
>+   *        A object with an associated objectbox.
>+   * @returns an object box or null
>+   */
>+  appendChildBox: function IOBox_appendChildBox(aParentNodeBox, aRepObject)
>+  {
>+    let childBox = this.getChildObjectBox(aParentNodeBox);
>+    let objectBox = this.findChildObjectBox(childBox, aRepObject);
>+
>+    // aRepObject already a child of aParentNodeBox, return
>+    if (objectBox)
>+      return objectBox;
>+
>+    objectBox = this.view.createObjectBox(aRepObject);
>+    if (objectBox) {
>+      let childBox = this.getChildObjectBox(aParentNodeBox); // todo - redundant?
>+      childBox.appendChild(objectBox);
>+    }
>+
>+    return objectBox;
>+  },
>+
>+  /**
>+   * Insert a child before a given sibling in the tree.
>+   * @param aParentNodeBox
>+   *        The parent node to insert the object under.
>+   * @param aRepObject
>+   *        The object to create an object box for that will be inserted.
>+   * @param aNextSibling
>+   *        The sibling to insert the repObject before.
>+   * @returns objectbox or null
>+   */
>+  insertChildBoxBefore:
>+  function IOBox_insertChildBoxBefore(aParentNodeBox, aRepObject, aNextSibling)
>+  {
>+    let childBox = this.getChildObjectBox(aParentNodeBox);
>+    let objectBox = this.findChildObjectBox(childBox, aRepObject);
>+
>+    // aRepObject is already in the tree
>+    if (objectBox)
>+      return objectBox;
>+
>+    objectBox = this.view.createObjectBox(aRepObject);
>+    if (objectBox) {
>+      let siblingBox = this.findChildObjectBox(childBox, aNextSibling);
>+      childBox.insertBefore(objectBox, siblingBox);
>+    }
>+    return objectBox;
>+  },
>+
>+  /**
>+   * Remove the given object from the tree described by the parentNodeBox.
>+   * @param aParentNodeBox
>+   *        A subtree from which to remove aRepObject
>+   * @param aRepObject
>+   *        The object to remove from the tree
>+   */
>+  removeChildBox: function IOBox_removeChildBox(aParentNodeBox, aRepObject)
>+  {
>+    let childBox = this.getChildObjectBox(aParentNodeBox);
>+    let objectBox = this.findChildObjectBox(childBox, aRepObject);
>+    if (objectBox)
>+      childBox.removeChild(objectBox);
>+  },
>+
>+  /**
>    * We want all children of the parent of repObject.
>    */
>-  populateChildBox: function(repObject, nodeChildBox)
>+  populateChildBox: function IOBox_populateChildBox(repObject, nodeChildBox)
>   {
>     if (!repObject)
>       return null;
> 
>     let parentObjectBox = this.view.style.getAncestorByClass(nodeChildBox, "nodeBox");
> 
>     if (parentObjectBox.populated)
>       return this.findChildObjectBox(nodeChildBox, repObject);
>@@ -1320,24 +1389,26 @@ var InspectorUI = {
> 
>   /**
>    * Select an object in the tree view.
>    * @param aNode
>    *        node to inspect
>    * @param forceUpdate
>    *        force an update?
>    */
>-  select: function IUI_select(aNode, forceUpdate)
>+  select: function IUI_select(aNode, forceUpdate, modified)

The documentation should probably be updated here.

>   {
>     if (!aNode)
>       aNode = this.defaultSelection;
> 
>     if (forceUpdate || aNode != this.selection) {
>       this.selection = aNode;
>       let box = this.ioBox.createObjectBox(this.selection);
>+      if (modified)
>+        this.style.setClass(box, "modified");
>       if (!this.inspecting) {
>         this.highlighter.highlightNode(this.selection);
>         this.updateStylePanel(this.selection);
>         this.updateDOMPanel(this.selection);
>       } else {
>         box.scrollIntoView(true); // todo scrollIntoCenterView would be nicer
>       }
>       this.updateSelection(this.selection);
>@@ -1477,16 +1548,57 @@ var InspectorUI = {
>   {
>     if (this.inspecting || !this.isDOMPanelOpen) {
>       return;
>     }
> 
>     this.domTreeView.data = aNode;
>   },
> 
>+  /**
>+   * Replace the old node with the new node in the IOBox.
>+   * @param aOldNode
>+   *        the node to be replaced.
>+   * @param aNewNode
>+   *        the replacement node.
>+   */
>+  replaceBoxNode: function IUI_replaceIOBoxNode(parent, aOldNode, aNewNode)

Documentation doesn't describe the "parent" parameter (also, rename "parent" to "aParent").

>+  onEditorChange: function IUI_onEditorChange(event)

aEvent

>+  {
>+    this._log("onEditorChange: " + this.editorText.textContent);

Should this debug message be removed for production?

>+      let nodePosition = -1;
>+      for (let i = 0; i < parent.childNodes.length; ++i) {
>+        if (parent.childNodes[i] == this.editingNode)
>+          nodePosition = i;

Maybe a break; here?

>+      }

>+      parent.replaceChild(fragment, this.editingNode);
>+      if (nodePosition < 0) {
>+        return;
>+      }

The brace style with single-line if bodies is slightly inconsistent in this patch: sometimes they're braced, sometimes they aren't.

>+  onEditorDiscard: function IUI_onEditorDiscard(event)

aEvent

>+    this._log("onEditorDiscard: " + this.editorText.textContent);

Another log message... might want to remove it if this is production code.

>+    if (aNode.hasAttribute("id"))
>+      attributes.push('id="' + aNode.id +'"');

Might need to escape embedded quotes in attribute values.

>+    let attributeText = attributes.join(" ");
>+
>+    return "<" + nodeTag + " " + attributeText + ">" +
>+      aNode.innerHTML + "</" + nodeTag + ">";

Nodes with no attributes (like "<p>") will end up with their opening tags as "<p >". Perhaps instead of join(" "), prepending a space to
each attribute and using join("") would be better?

>+  font-family: Menlo, monospace;

Do we use Menlo in the rest of the Firefox UI?

f+ with changes

Comment 13

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

Attachment: HTML Editor 2010-08-26

rebased after tree panel review.

Comment 14

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

given that I'm still working on a testcase for this and trying to get the html tree landed, this may have to land in beta6. Moving out.

Comment 15

[Dietrich Ayala (:dietrich)]

was already blocking, don't need to re-request.

Comment 16

[Axel Hecht [:Pike]]

The save and discard buttons are real, I suppose? If so, browser/base/content/inspector.html should probably move to be xhtml so that we can use a a DTD to localize them.

Comment 17

[Kevin Dangoor]

Inspector feature postponed. Removing blocking flags from Inspector bugs.

Comment 18

[Kevin Dangoor]

Removing items from kd4b6.

Comment 19

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

Attachment: Inspector popup editor 2010-09-16

Working patch. Now with tests!

Comment 20

[Kyle Simpson]

Attachment: attribute-value editor (only) for HTML panel

WIP... no tests yet

Comment 21

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

Comment on attachment 548082 [details] [diff] [review]
attribute-value editor (only) for HTML panel

 <body role="application">
+  <div id="attribute-editor">
+    <input id="attribute-editor-input" />
+    <button id="attribute-editor-save">Save</button>
+    <button id="attribute-editor-discard">Discard</button>

Could we maybe get rid of the buttons and have a keyboard handler to check for enter and escape? I wasn't too fond of the buttons in the first place, but wanted an easier implementation to start with.

    */
   stopInspecting: function IUI_stopInspecting()
   {
+    if (this.editingContext)
+      this.closeEditor();
+

Not sure this is needed. We should prevent editing while inspecting.

   onTreeClick: function IUI_onTreeClick(aEvent)
   {
+    if (this.editingContext) {
+      this.closeEditor();
+
+      aEvent.preventDefault();
+      return false;
+    }

Not sure about this behavior either. Should we stop the event if a user clicks in the tree? They might want to expand or collapse a node. I could see this causing problems if the user collapsed the parent node of the node being edited, so maybe this is the easiest way to do this.

+  editAttributeValue: function IUI_editAttributeValue(attrObj, repObj, attrName, attrVal)

+  onTreeDblClick: function IUI_onTreeDblClick(aEvent)
+  {
...
+
+    let target = aEvent.target;

+    let repObj;
+    let attrName;
+    let attrVal;

Don't really need to declare these here (except for target). You can do it at assignment.

+  editAttributeValue: function IUI_editAttributeValue(attrObj, repObj, attrName, attrVal)

We typically use an 'a' prefix on parameter names to denote "argument". Should rename them to be consistent here.

skipping past the HACK section for now.

+    // calculate position for the editor according to the attribute node (horizontally centered, directly below it)

How about placing the editor directly over the attrNode?

Another possibility: Have you considered setting the contentEditable attribute to true for the attribute value node? That would make all of this awkward positioning code go away. We'd just need to handle the changing value.

A lot of this patch becomes simpler with that in mind, so it might be worth investigating.

+  handleEditorKeydown: function IUI_handleEditorKeydown(aEvent)

aha! you already have the startings of a keyboard handler.

+  {
+    if (aEvent.keyCode == 27) {

which key is this? Should use a constant (KeyEvent.VK_ESCAPE).

+      this.closeEditor();
+    }
+  },

+  /**
+   * Handle keypress events in the editor.
+   * @param aEvent
+   *        The keyboard event.
+   */
+  handleEditorKeypress: function IUI_handleEditorKeypress(aEvent)
+ 

Not sure you need 2 handlers.

+  {
+    if (aEvent.which == 13) {

same as above. Use one of the KeyEvent constants please.

in winstripe...inspector.css

\ No newline at end of file

Make sure you have a newline at the end so we don't get these complaints from hg diff.

Also, you'll need equivalents in pinstripe and gnomestripe to match what you end up using.

If you use contentEditable, you could do something like:

.nodeBox.nodeHidden .nodeLabel > .nodeLabelBox > .nodeAttr > .nodeValue[contentEditable="true"]
{
  /* some pretty css borders and backgrounds here */
}

This will need tests too, obviously.

Let me know how you want to proceed. I prefer the contentEditable option without the buttons but am open to ideas.

Comment 22

[Kyle Simpson]

(In reply to comment #21)

Thanks for the review comments!

> Could we maybe get rid of the buttons and have a keyboard handler to check
> for enter and escape? I wasn't too fond of the buttons in the first place,
> but wanted an easier implementation to start with.

I would prefer the button-less approach, going only with keyboard <enter> and <escape>, instead of the buttons. I put the buttons in there since that's what the previous patch I was working from had, and I was trying to be as straightforward as possible. I agree dropping the buttons is the right approach. Very easy to do, since (as you saw) I already have the keyboard handling anyway.


>    stopInspecting: function IUI_stopInspecting()
>    {
> +    if (this.editingContext)
> +      this.closeEditor();
> +
> 
> Not sure this is needed. We should prevent editing while inspecting.

AFAIK, `stopInspecting` is called when the inspector stops (or is closed). If the inspector is closed/stopped while the editor is open, I think it's cleanest to call `closeEditor` so that it unbinds the event handlers, etc.

Perhaps I'm misunderstanding "inspecting" as a state, but I believe that's any point while the highlighter is still visible, correct? Because you can "highlight" a node, which finds it in the HTML tree, and then double-click the attr-value on that node in the tree right away, and go right into editing. 

Not sure how that would be useful to prevent them from doing that? Would it make sense to a user that they had to close the highlighter first before they could access the tree to edit it? Also, it appears (at least currently) that when you click the "X" to close the highlighter, it also closes the HTML tree panel. So, unless I'm misunderstanding, I think the "highlighter" and the tree and its editor all have to stay visible/active at the same time. Am I missing something there?


>    onTreeClick: function IUI_onTreeClick(aEvent)
>    {
> +    if (this.editingContext) {
> +      this.closeEditor();
> +
> +      aEvent.preventDefault();
> +      return false;
> +    }
> 
> Not sure about this behavior either. Should we stop the event if a user
> clicks in the tree? They might want to expand or collapse a node. I could
> see this causing problems if the user collapsed the parent node of the node
> being edited, so maybe this is the easiest way to do this.

Right, there's a variety of ways it could be confusing to the user. I found the cleanest approach was to make sure the editor closed if they click outside of the editor, so that's what this hook is for.

Also, if we indeed go with the button-less approach, I think the traditional UX precedent from popup experiences like this (like for instance light-boxes) is that a click outside the popup automatically dismisses/closes the popup. I was just following that type of UX precedent here, as it seems natural to me.


> +  editAttributeValue: function IUI_editAttributeValue(attrObj, repObj,
> attrName, attrVal)
> 
> +  onTreeDblClick: function IUI_onTreeDblClick(aEvent)
> +  {
> ...
> +
> +    let target = aEvent.target;
> 
> +    let repObj;
> +    let attrName;
> +    let attrVal;
> 
> Don't really need to declare these here (except for target). You can do it
> at assignment.

`var` gets hoisted to the top of the block, so it's safe to intersperse (although perhaps bad practice). `let` however will bind only inside a block, including an if-block like it is here, right? It seemed more problematic future-wise to have let declarations scoped inside a block, in case in the future you need them outside the block. In this usage, there's no future landmines in terms of variable scoping if this function is changed in some way to use the variables elsewhere. Seems difficult to predict if if-statement scoped variables in this case are future-safe or not. I went with the conservative approach of making them function-scoped.


> How about placing the editor directly over the attrNode?

I prefer the editor to be visible at the same time as the original value, because I find when editing values like this, it's helpful to see the original value in case I'm mis-typing or forget what I'm doing or how I'm changing a variable. A single value like "foobar" might not be quite so evident, but in changing a long value like a `style` attribute, where there's lots of data in the value, and lots of potential to mess up what you're typing, I always find it better to be able to see the original value.


> Another possibility: Have you considered setting the contentEditable
> attribute to true for the attribute value node? That would make all of this
> awkward positioning code go away. We'd just need to handle the changing
> value.

The in-place editing experience is, I think (as I explained just above) inferior to the popup editing. I know there are plenty of people who may disagree with that assertion, but I don't know that it's universally held to be better, given that you can't see the original value, nor can you (as easily/semantically) get it back once you've started typing, as you can with a "discard" from inside a popup.

However, I do know that some people really prefer this experience. FWIW, in my other rewrite, the plan was to let the user be able to easily choose/toggle this behavior, so that both preferences are easily accessible. I don't however think we should do both for *this* patch.

I vote for the popup, because I think it has fewer downsides, and lets us come back and revisit this with feedback from people on if they just can't stand the popup experience.


> +  /**
> +   * Handle keypress events in the editor.
> +   * @param aEvent
> +   *        The keyboard event.
> +   */
> +  handleEditorKeypress: function IUI_handleEditorKeypress(aEvent)
> + 
> 
> Not sure you need 2 handlers.

I've had some long-standing belief that some keys can only be trapped properly in keydown handlers, while others can only be trapped properly in keypress handlers, owing to the fact that in a keydown, the keyCode hasn't yet been mapped to an actual ascii character code, but it is mapped in a keypress.

I had some reused code from many past projects which split <escape> and <enter> as shown, which is why I relied on that past experience here. I'll double check to see if it will work correctly if both are in the same type of handler, and which type of handler that is. IIRC, there was an issue where you needed to trap <escape> early in keydown before it gets translated to ascii in keypress, but I fail to have a solid explanation, just anecdotal experience.


> Also, you'll need equivalents in pinstripe and gnomestripe to match what you
> end up using.

Interesting. I (thought I) was following the example from the actual HTML panel itself, where, if I'm not mistaken, the styles for the little arrows ("twisty"'s) in the tree are only in the winstripe.css, and not in the other two.


> Let me know how you want to proceed. I prefer the contentEditable option
> without the buttons but am open to ideas.

I'm not a fan of the content-editable experience as I explained above, as I think the popup is the simpler approach here without some possibly pesky questions to deal with (like, how to revert the value programmatically if the value is being edited when the inspector is closed, etc). But that's just my 2 cents.

Comment 23

[Kyle Simpson]

Attachment: updates from review (still needs tests)

addressed most of the review comments (besides my responses in Comment #22)... yes, still needs tests.

Comment 24

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

(In reply to comment #22)
> (In reply to comment #21)

> >    stopInspecting: function IUI_stopInspecting()
> >    {
> > +    if (this.editingContext)
> > +      this.closeEditor();
> > +
> > 
> > Not sure this is needed. We should prevent editing while inspecting.
> 
> AFAIK, `stopInspecting` is called when the inspector stops (or is closed).
> If the inspector is closed/stopped while the editor is open, I think it's
> cleanest to call `closeEditor` so that it unbinds the event handlers, etc.
> 
> Perhaps I'm misunderstanding "inspecting" as a state, but I believe that's
> any point while the highlighter is still visible, correct? Because you can
> "highlight" a node, which finds it in the HTML tree, and then double-click
> the attr-value on that node in the tree right away, and go right into
> editing. 

no, InspectorUI.inspecting is set when the highlighter is in "active highlighting" mode. When you're able to hover over a node and the highlighter updates to the new position.

stopInspecting() is called to deactivate that mode. I don't think it should be possible to get into an editing state while in active highlighting mode, which would remove the need to close the editor from stopInspecting.

Maybe add a check for inspecting in onTreeDblClick and if true, bail out early.

> Not sure how that would be useful to prevent them from doing that? Would it
> make sense to a user that they had to close the highlighter first before
> they could access the tree to edit it? Also, it appears (at least currently)
> that when you click the "X" to close the highlighter, it also closes the
> HTML tree panel. So, unless I'm misunderstanding, I think the "highlighter"
> and the tree and its editor all have to stay visible/active at the same
> time. Am I missing something there?
> 
> 
> >    onTreeClick: function IUI_onTreeClick(aEvent)
> >    {
> > +    if (this.editingContext) {
> > +      this.closeEditor();
> > +
> > +      aEvent.preventDefault();
> > +      return false;
> > +    }
> > 
> > Not sure about this behavior either. Should we stop the event if a user
> > clicks in the tree? They might want to expand or collapse a node. I could
> > see this causing problems if the user collapsed the parent node of the node
> > being edited, so maybe this is the easiest way to do this.
> 
> Right, there's a variety of ways it could be confusing to the user. I found
> the cleanest approach was to make sure the editor closed if they click
> outside of the editor, so that's what this hook is for.

OK.

> Also, if we indeed go with the button-less approach, I think the traditional
> UX precedent from popup experiences like this (like for instance
> light-boxes) is that a click outside the popup automatically
> dismisses/closes the popup. I was just following that type of UX precedent
> here, as it seems natural to me.

Yes, agreed.

> > +  editAttributeValue: function IUI_editAttributeValue(attrObj, repObj,
> > attrName, attrVal)
> > 
> > +  onTreeDblClick: function IUI_onTreeDblClick(aEvent)
> > +  {
> > ...
> > +
> > +    let target = aEvent.target;
> > 
> > +    let repObj;
> > +    let attrName;
> > +    let attrVal;
> > 
> > Don't really need to declare these here (except for target). You can do it
> > at assignment.
> 
> `var` gets hoisted to the top of the block, so it's safe to intersperse
> (although perhaps bad practice). `let` however will bind only inside a
> block, including an if-block like it is here, right? It seemed more
> problematic future-wise to have let declarations scoped inside a block, in
> case in the future you need them outside the block. In this usage, there's
> no future landmines in terms of variable scoping if this function is changed
> in some way to use the variables elsewhere. Seems difficult to predict if
> if-statement scoped variables in this case are future-safe or not. I went
> with the conservative approach of making them function-scoped.

That's not a convention we use. We can move the variables if we need to later. Common practice in mozilla-land is to define the variables when they're assigned.

> > How about placing the editor directly over the attrNode?
> 
> I prefer the editor to be visible at the same time as the original value,
> because I find when editing values like this, it's helpful to see the
> original value in case I'm mis-typing or forget what I'm doing or how I'm
> changing a variable. A single value like "foobar" might not be quite so
> evident, but in changing a long value like a `style` attribute, where
> there's lots of data in the value, and lots of potential to mess up what
> you're typing, I always find it better to be able to see the original value.

Why not put the original value in the editor itself?

> > Another possibility: Have you considered setting the contentEditable
> > attribute to true for the attribute value node? That would make all of this
> > awkward positioning code go away. We'd just need to handle the changing
> > value.
> 
> The in-place editing experience is, I think (as I explained just above)
> inferior to the popup editing. I know there are plenty of people who may
> disagree with that assertion, but I don't know that it's universally held to
> be better, given that you can't see the original value, nor can you (as
> easily/semantically) get it back once you've started typing, as you can with
> a "discard" from inside a popup.
> 
> However, I do know that some people really prefer this experience. FWIW, in
> my other rewrite, the plan was to let the user be able to easily
> choose/toggle this behavior, so that both preferences are easily accessible.
> I don't however think we should do both for *this* patch.
> 
> I vote for the popup, because I think it has fewer downsides, and lets us
> come back and revisit this with feedback from people on if they just can't
> stand the popup experience.

*sigh*

> > +  /**
> > +   * Handle keypress events in the editor.
> > +   * @param aEvent
> > +   *        The keyboard event.
> > +   */
> > +  handleEditorKeypress: function IUI_handleEditorKeypress(aEvent)
> > + 
> > 
> > Not sure you need 2 handlers.
> 
> I've had some long-standing belief that some keys can only be trapped
> properly in keydown handlers, while others can only be trapped properly in
> keypress handlers, owing to the fact that in a keydown, the keyCode hasn't
> yet been mapped to an actual ascii character code, but it is mapped in a
> keypress.

evidence trumps belief.

> I had some reused code from many past projects which split <escape> and
> <enter> as shown, which is why I relied on that past experience here. I'll
> double check to see if it will work correctly if both are in the same type
> of handler, and which type of handler that is. IIRC, there was an issue
> where you needed to trap <escape> early in keydown before it gets translated
> to ascii in keypress, but I fail to have a solid explanation, just anecdotal
> experience.

Shouldn't be necessary here.

> > Also, you'll need equivalents in pinstripe and gnomestripe to match what you
> > end up using.
> 
> Interesting. I (thought I) was following the example from the actual HTML
> panel itself, where, if I'm not mistaken, the styles for the little arrows
> ("twisty"'s) in the tree are only in the winstripe.css, and not in the other
> two.

the other two have their own "twisty" arrows I think they've purloined from elsewhere in the code base. I'd have to verify that, but at a bare minimum, if you're making style changes in one of the files, you'll have to update it in all three locations.

> > Let me know how you want to proceed. I prefer the contentEditable option
> > without the buttons but am open to ideas.
> 
> I'm not a fan of the content-editable experience as I explained above, as I
> think the popup is the simpler approach here without some possibly pesky
> questions to deal with (like, how to revert the value programmatically if
> the value is being edited when the inspector is closed, etc). But that's
> just my 2 cents.

Understood. We'll give it a try. Thanks!

Comment 25

[Kyle Simpson]

Attachment: further updates based on review

still needs tests :)

Comment 26

[Kyle Simpson]

Attachment: trying to add tests

i'm trying to add tests... but I'm getting an error when targeting a mouse event ("click") to an element that's inside an <iframe> that's inside the XUL popup panel. not sure how to get around this error:

http://pastebin.mozilla.org/1284448

Comment 27

[Kyle Simpson]

Attachment: finally with working tests!

Finally got the tests working properly. This should be ready for review now.

Comment 28

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

Comment on attachment 549303 [details] [diff] [review]
finally with working tests!

quibbles:

+   * Handle starting the editing of an attribute value.

This line sounds a little cumbersome. How about:

* Handle attribute value editing.

?

+   * @param aAttrObj
+   *        The DOM object representing the attribute value in the HTML Tree
+   * @param aRepObj
+   *        The original DOM (target) DOM object being inspected/edited

Seems like one too many DOMs. Maybe:

*   The original DOM object being inspected/edited.

+   * @param aAttrName
+   *        The name of the attribute being edited
+   * @param aAttrVal
+   *        The current value of the attribute being edited
+   */
+  editAttributeValue: function IUI_editAttributeValue(aAttrObj, aRepObj, aAttrName, aAttrVal)

This line's a bit long. You could and should break it up like so:

  editAttributeValue:
  function IUI_editAttributeValue(aAttrObj, aRepObj, aAttrName, aAttrVal)

(keep lines under 80 characters wide so they format properly in MXR)

less quibbly, more seriously:

+    // figure out actual viewable viewport dimensions (sans scrollbars)
+    /* START HACK */

Do we really need to do this? I'm not convinced of it. It's a lot of code, says "HACK" right on the tin and mucks about in the iframe's DOM.

In testing this, it seems this algorithm isn't really doing the right thing. For some nodes, it seems to halve the width of the editor. For others, it's a smaller fraction. If we can display the entire contents of the text, we should.

I think we should default to the full width of the node's value.

see: http://cl.ly/8sdc

I'd much prefer a non-hacky solution that gets it right.

(some time later)

editAttributeValue: function IUI_editAttributeValue(aAttrObj, aRepObj, aAttrName, aAttrVal)
  {
    let editor = this.treeBrowserDocument.getElementById("attribute-editor");
    let editorInput = this.treeBrowserDocument.getElementById("attribute-editor-input");
    let attrDims = aAttrObj.getBoundingClientRect();

    // saves the editing context for use when the editor is saved/closed
    this.editingContext = {
      attrObj: aAttrObj,
      repObj: aRepObj,
      attrName: aAttrName
    };

    editorInput.value = aAttrVal;

    editor.style.visibility = "visible";
    let editorDims = editor.getBoundingClientRect();
    editorInput.select();

    // calculate position for the editor directly below the attribute node
    let editorLeft = attrDims.left + this.treeIFrame.contentWindow.scrollX;
    let editorTop = attrDims.bottom + this.treeIFrame.contentWindow.scrollY;

    // position the editor
    editor.style.left = editorLeft + "px";
    editor.style.top = editorTop + "px";
    editor.style.width = editor.style.maxWidth = attrDims.width + "px";
    editorInput.style.width =
    editorInput.style.maxWidth = attrDims.width - 4 + "px"; // 2 for borders, 2 for padding

    // listen for editor specific events
    this.bindEditorEvent(editor, "click", function(aEvent) {
      aEvent.stopPropagation();
    });
    this.bindEditorEvent(editor, "dblclick", function(aEvent) {
      aEvent.stopPropagation();
    });
    this.bindEditorEvent(editor, "keypress", this.handleEditorKeypress.bind(this));

    // event notification    
    Services.obs.notifyObservers(null, INSPECTOR_NOTIFICATIONS.EDITOR_OPENED, null);
  },

This method works pretty well, avoids the unnecessary hack and 

The rest of this looks good. Was a little concerned about leak-potential, but the call in closeInspectorUI to closeEditor should take care of it.
I might delete this.editingEvents just to be safe.

in your CSS files:

+#attribute-editor-input {
+  display: block;

don't need that.

+  margin: 0px;
+  border: none;
+  padding: 3px;

I changed the padding to 2px.

+  width: 75px;

remove the width.

r- because of the "hack" section. This is close!

Comment 29

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

"This method works pretty well, avoids the unnecessary hack and "

Kinda trailed off there. Meant to say, "and gets the sizing of the editor node accurate."

Didn't look too closely at the test code, but a first skim looked fine. Might need to update when you change the editor positioning code as above.

again, this is looking pretty close. Thanks!

Comment 30

[Kyle Simpson]

Attachment: updated with review feedback

now with less hacks!

still has one (minor) issue unresolved, which is that the `color` rule in the CSS is not getting applied.

Comment 31

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

Comment on attachment 550554 [details] [diff] [review]
updated with review feedback

in editAttributeValue...

+  editAttributeValue: 
+  function IUI_editAttributeValue(aAttrObj, aRepObj, aAttrName, aAttrVal)
+  {

...

+    editor.style.visibility = "visible";

could probably use attribute hidden or add a class rather than twiddling the style object directly.

+    // outer editor is sized based on the <input> box inside it

After playing with this patch applied, I'm still not sold on having the editor popup underneath the node you're editing. A lot of this viewport boundary checking goes away if we just overlay the attribute value. I disagree with the argument that having the original value visible while you're editing is valuable enough to warrant all this extra tricky math.

The position of the editor gets really funky at the bottom of the viewport. It looks like a bug.

Can we just overlay this?

Unittests look great. All pass!

Comment 32

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

one other request, could we add some styling to the editor box to make it more closely resemble the underlying text?

Comment 33

[Kyle Simpson]

Attachment: more review updates

updated with more review feedback

Comment 34

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

Comment on attachment 551056 [details] [diff] [review]
more review updates

giving this r+. Asking for gavin review.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 551056 [details] [diff] [review]
more review updates

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>+  function IUI_editAttributeValue(aAttrObj, aRepObj, aAttrName, aAttrVal)

>+    editorInput.style.width = Math.min(attrDims.width, viewportWidth - 5) + "px";
>+    editorInput.style.height = Math.min(attrDims.height, viewportHeight - 5) + "px";

>+    let editorTop = attrDims.top + this.treeIFrame.contentWindow.scrollY + 
>+                    attrDims.height + 2;

Where do these magic "2" and "5" numbers come from? Seems like a comment and/or descriptive constant is in order.

>diff --git a/browser/base/content/test/inspector/browser_inspector_editor.js b/browser/base/content/test/inspector/browser_inspector_editor.js

>+/* ***** BEGIN LICENSE BLOCK *****

You can use the shorter PD license block for tests: https://www.mozilla.org/MPL/boilerplate-1.1/pd-c

>+function setupEditorTests()
>+{
>+  div = doc.createElement("div");
>+  div.setAttribute("id", "foobar");
>+  div.setAttribute("class", "barbaz");
>+  doc.body.appendChild(div);
>+  
>+  div = doc.getElementById("foobar");
>+  ok(div, "we have the div");

This isn't a particularly useful test :)

>+function doEditorTestSteps()

>+  let editorVisible = editor.className.match(/editing/);

nit: these tests can all use classList.contains(), which is more reliable in general.

>diff --git a/browser/themes/gnomestripe/browser/inspector.css b/browser/themes/gnomestripe/browser/inspector.css

>+.editingAttributeValue {

>+  color: black !important; /* this rule is getting ignored, not sure why */

You should probably figure out why that is.

r=me with those addressed.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Oh, one other nit: it would be nice if you could attach patches with at least 8 lines of context in the future (info on making that configuration change here: https://developer.mozilla.org/en/Installing_Mercurial#Configuration ).

Comment 37

[Kyle Simpson]

Attachment: [in-fx-team] updated with final review feedback

addressed final review comments

Comment 38

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

Comment on attachment 553237 [details] [diff] [review]
[in-fx-team] updated with final review feedback

http://hg.mozilla.org/integration/fx-team/rev/a2cc2d7fa547 (rebased)

Comment 39

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

Comment on attachment 553237 [details] [diff] [review]
[in-fx-team] updated with final review feedback

http://hg.mozilla.org/mozilla-central/rev/a2cc2d7fa547