Bug 17224 – DOM nodes/attributes should be editable

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Adam Roben (:aroben)]

It should be possible to edit DOM nodes/attributes right in the Inspector's DOM tree.

[Adam Roben (:aroben)]

<rdar://problem/5732825>

[Adam Roben (:aroben)]

Created attachment 19623 [details]
Refactor CSS editing code so it can be shared with DOM editing

[Adam Roben (:aroben)]

Comment on attachment 19623 [details]
Refactor CSS editing code so it can be shared with DOM editing

@@ -320,7 +320,7 @@ WebInspector.changeFocus = function(event)
     }
 
     if (!nextFocusElement)
-        nextFocusElement = event.target.firstParentWithClass("focusable");
+        nextFocusElement = event.target.firstParentOrSelfWithClass("focusable");
 
     this.currentFocusElement = nextFocusElement;
 }

I forgot to mention this change in the ChangeLog. I'm actually not entirely sure why it wasn't needed before this. If the receiver of the mousedown event that triggered the call to changeFocus was itself focusable (rather than having a focusable parent), changeFocus would move focus to the next focusable element. This was causing the CSS property being edited to lose focus as soon as you clicked on it.

[Adam Roben (:aroben)]

Comment on attachment 19623 [details]
Refactor CSS editing code so it can be shared with DOM editing

Landed as r30931

[Adam Roben (:aroben)]

Created attachment 19645 [details]
Make attributes editable

This patch makes attributes editable. A future patch will make text nodes editable.

[Adam Roben (:aroben)]

Comment on attachment 19645 [details]
Make attributes editable

Landed as r30947

[Adam Roben (:aroben)]

Created attachment 19648 [details]
Make text nodes editable

Final part of the fix. Makes text nodes editable.

[Adam Roben (:aroben)]

Comment on attachment 19648 [details]
Make text nodes editable

The change in nodeTitleInfo in utilities.js makes it so we wrap text node contents in a new webkit-html-text-node span. I've added this to the ChangeLog locally.

[Timothy Hatcher]

We should make element tag names editable too.

[Matt Lilek]

Don't forget about the need to update various parts of the UI when we do this.  What's currently in the tree doesn't update the breadcrumb bar if you change an ID/class, nor does the sidebar update.

[Adam Roben (:aroben)]

(In reply to comment #9)
> We should make element tag names editable too.

I thought about doing this, but wasn't sure how useful it is. Firebug doesn't let you do it. But I guess we could do it in the name of consistency.

(In reply to comment #10)
> Don't forget about the need to update various parts of the UI when we do this. 
> What's currently in the tree doesn't update the breadcrumb bar if you change an
> ID/class, nor does the sidebar update.

Good point! I'll make up a followup patch to fix these issues.

[Matt Lilek]

(In reply to comment #11)
> (In reply to comment #9)
> > We should make element tag names editable too.
> 
> I thought about doing this, but wasn't sure how useful it is. Firebug doesn't
> let you do it. But I guess we could do it in the name of consistency.
> 

If we're not going to make those editable, we perhaps we should make double clicking the tag name add a new attribute.  That should definitely be the case for nodes without any attributes and I don't see how that behavior could hurt even for ones that do.

[Adam Roben (:aroben)]

Comment on attachment 19648 [details]
Make text nodes editable

Committed as r30962

[Adam Roben (:aroben)]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > We should make element tag names editable too.
> > 
> > I thought about doing this, but wasn't sure how useful it is. Firebug doesn't
> > let you do it. But I guess we could do it in the name of consistency.
> > 
> 
> If we're not going to make those editable, we perhaps we should make double
> clicking the tag name add a new attribute.  That should definitely be the case
> for nodes without any attributes and I don't see how that behavior could hurt
> even for ones that do.

I think having some easy way of adding an attribute is a good idea (i.e., something more than just typing a second attribute in when editing an attribute). I also think having some easy way of adding a CSS property is a good idea.

We're running into conflicting behaviors for double-click here. Before any of my patches landed, if you double-clicked anywhere on a row in the DOM tree that row would become the root of the tree. Now this only happens if you double-click on a part of the row that's not an attribute or a text node.

We need to figure out how we want to fix this. Firebug uses single-click to edit -- maybe we should as well?

[Matt Lilek]

(In reply to comment #14)
> We're running into conflicting behaviors for double-click here. Before any of
> my patches landed, if you double-clicked anywhere on a row in the DOM tree that
> row would become the root of the tree. Now this only happens if you
> double-click on a part of the row that's not an attribute or a text node.
> 
> We need to figure out how we want to fix this. Firebug uses single-click to
> edit -- maybe we should as well?
> 

Duh, I totally forgot about double clicking to drill down.  In theory, I like the idea of using single click to edit, but I think too often it would result in unwanted editing when people are just looking to focus the node and become annoying.

[Adam Roben (:aroben)]

(In reply to comment #15)
> In theory, I like
> the idea of using single click to edit, but I think too often it would result
> in unwanted editing when people are just looking to focus the node and become
> annoying.

How about this?

If the node is not focused and you click on any part of it, we focus the node.
If the node is focused and you click on an editable part of it, we start editing that part.

[Adam Roben (:aroben)]

(In reply to comment #16)
> If the node is not focused and you click on any part of it, we focus the node.
> If the node is focused and you click on an editable part of it, we start
> editing that part.

Of course, if we do this then your first click of a double-click will start editing and the second click will re-root the tree, which might be strange.

[Timothy Hatcher]

(In reply to comment #16)
> 
> How about this?
> 
> If the node is not focused and you click on any part of it, we focus the node.
> If the node is focused and you click on an editable part of it, we start
> editing that part.

I like that plan.

[Adam Roben (:aroben)]

Hm, single-click-to-edit breaks our linkified URLs. Do we still care about linkified URLs in the DOM view? If we do, how do we handle this?

[Timothy Hatcher]

I think a click on the URL should take you to the resource. You can always click on the attr name to edit. Also we should make sure when editing URLs, that we hide the underline and the cursor does not change to a hand. (I am not sure we hide those things currently.)

[Adam Roben (:aroben)]

(In reply to comment #20)
> I think a click on the URL should take you to the resource. You can always
> click on the attr name to edit. Also we should make sure when editing URLs,
> that we hide the underline and the cursor does not change to a hand. (I am not
> sure we hide those things currently.)

I worry that the URL behavior will be surprising, especially for URLs that we don't underline (i.e., URLs to external resources). In most cases, clicking an attribute value will start editing that value. If the value happens to be a URL, you'll be taken away from the DOM view altogether.

[Adam Roben (:aroben)]

Created attachment 19689 [details]
Change to single-click-to-edit

This patch makes us edit on single-click, if the element is already selected (as discussed above). We now follow URLs on Alt/Option-click.

[Adam Roben (:aroben)]

Created attachment 19690 [details]
Make URLs not be underlined while editing

[Adam Roben (:aroben)]

(In reply to comment #9)
> We should make element tag names editable too.

After thinking about this some more, I'm not so sure there's a clear way forward with this. AFAICT, there's no way to change the an element's tag name -- you have to create a new element with the correct tag name, migrate all attributes/descendants to it, and replace the original element in the tree with the new one. But there are some things you can't migrate, such as event listeners, so it'll be an imperfect copy. And given all the copying/replacement going on behind the scenes, presenting this to the user as an "edit" of the tag name seems disingenuous.

[Timothy Hatcher]

(In reply to comment #24)
> (In reply to comment #9)
> > We should make element tag names editable too.
> 
> After thinking about this some more, I'm not so sure there's a clear way
> forward with this. AFAICT, there's no way to change the an element's tag name
> -- you have to create a new element with the correct tag name, migrate all
> attributes/descendants to it, and replace the original element in the tree with
> the new one. But there are some things you can't migrate, such as event
> listeners, so it'll be an imperfect copy. And given all the copying/replacement
> going on behind the scenes, presenting this to the user as an "edit" of the tag
> name seems disingenuous.
> 

It is fine to not do tag name editing then. And since Firebug doesn't allow it, we are good. Might be nice someday when those issues can be addressed.

[Adam Roben (:aroben)]

Comment on attachment 19689 [details]
Change to single-click-to-edit

Committed as r31009

[Adam Roben (:aroben)]

Comment on attachment 19690 [details]
Make URLs not be underlined while editing

Committed as r31010

[Adam Roben (:aroben)]

Created attachment 19720 [details]
Update styles/metrics/breadcrumb after editing

I think this is the final patch needed to close this bug.

[Adam Roben (:aroben)]

Comment on attachment 19720 [details]
Update styles/metrics/breadcrumb after editing

Committed as r31024