Bug 30428 – Web Inspector: Handle the Enter Key in the Elements Tree Hierarchy

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

[Joseph Pecoraro]

When pushing Enter in the Elements Tree Outline:

1. if TextNode => Edit Text Node
2. if Has Attributes => Edit First Attribute
3. otherwise => Create New Attribute

[Joseph Pecoraro]

Created attachment 41267 [details]
[PATCH] Enter Key in Tree Hierarchy

This handles the above and fixes 2 minor bugs:
- moving backwards when there are no attributes caused an error
- adding the new attribute "button" should only happen on nodes that can have attributes (was causing a lot of errors)

[Brian Weinstein]

3 is only if the selected item can have attributes, correct?

[Joseph Pecoraro]

Brian, you're correct. Although I don't explicitly say it in the code the following check ensures any of the operations are valid (its a text node or its an element node which can have attributes):

> if (this.representedObject.nodeType !== Node.ELEMENT_NODE && this.representedObject.nodeType !== Node.TEXT_NODE)

This could be simplified (in multiple places) instead to be:

> if (!this._canAddAttributes && this.representedObject.nodeType !== Node.TEXT_NODE)

After this check the other operations look safe. But would you rather I split this up?

if (this.representedObject.nodeType === Node.TEXT_NODE) {
  // handle text node
} else if (this._canAddAttributes) {
  // handle attribute cases
}

That sounds better!

[Joseph Pecoraro]

Created attachment 41269 [details]
[PATCH] Enter Key in Tree Hierarchy (improved)

Improved based on the last comment.

[Joseph Pecoraro]

Created attachment 41270 [details]
[PATCH] Enter Key in Tree Hierarchy (improved)

Updated the wrong one!

[Timothy Hatcher]

Comment on attachment 41270 [details]
[PATCH] Enter Key in Tree Hierarchy (improved)


> +    _startEditingGeneral: function()

Maybe this could just be _startEditing, not sure what "General" means.


> +                return this._startEditingTextNode(textNode);
> +
> +            return;

Remove the empty line here.

[Joseph Pecoraro]

(In reply to comment #6)
> > +    _startEditingGeneral: function()
> 
> Maybe this could just be _startEditing, not sure what "General" means.

Done.

> > +                return this._startEditingTextNode(textNode);
> > +
> > +            return;
> 
> Remove the empty line here.

Done.



Landed in http://trac.webkit.org/changeset/49709
r49709 = c44830e80b27325d93c76e839c0f36eef937fcdd

[Pavel Feldman]

These 1 -> 2 -> 3 don't seem to be intuitive to me. I am sure we don't want user to play sequence like this in his head prior to hitting the enter. Like what do we do if the line is <span class="foo">Text</span>? Shouldn't we support inline editing of the whole line instead? That would allow editing text, attributes, adding new ones and would be 100% predictable...

[Joseph Pecoraro]

(In reply to comment #8)
> These 1 -> 2 -> 3 don't seem to be intuitive to me. I am sure we don't want
> user to play sequence like this in his head prior to hitting the enter. Like
> what do we do if the line is <span class="foo">Text</span>?

I think its pretty intuitive in the UI.  If you can edit the selected node then pushing Enter starts editing that something!  Text Nodes you can only edit their contents. Elements you can only edit their attributes. The "add new attribute" is just a convenience, but the thought process is that if its an Element Node you are always editing attributes.

> Shouldn't we support inline editing of the whole line instead? That
> would allow editing text, attributes, adding new ones and would
> be 100% predictable...

I think there is an open bug for supporting inline editing (if not then one should probably be made... is bug 16477 could be an umbrella bug for this concept).  Once implemented "Enter" could be changed to toggle this more powerful editing mode.

However, I tend to like these individual edits more then free flow editing. With individual edits you're "sandboxed" from making major mistakes and removing things you didn't want to remove.  Plus its a nice keyboard shortcut that Firebug doesn't have.

---

I did make one mistake in this patch. Editing attributes starts presets the selection to the attribute name. It should set it to be the attribute value, as that would almost always be the use case.  Should I commit a trivial fix or wait for some more discussion on this in general?