Bug 36481 – Web Inspector: Edit Tag Names

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

[Joseph Pecoraro]

There is "Add Attribute" and "Edit Attribute", how about "Add Child" and "Edit Tag"?

[Joseph Pecoraro]

Created attachment 51398 [details]
[PATCH] Adds "Add Child" and "Edit Tag" to the Context Menu

I don't think this is a final patch yet. But I wanted to open this up for discussion. Video to come soon.

[Joseph Pecoraro]

Video of feature:
http://screencast.com/t/YzhhMTcxYTkt

[Pavel Feldman]

Comment on attachment 51398 [details]
[PATCH] Adds "Add Child" and "Edit Tag" to the Context Menu

Looks cool! Menu gets populated!!!

General notes go first:
- Given a node with N children, Add Child only solves 1/N part of the problem. I.e. Besides "Add Child" we need "Insert Above" and "Insert Below".
- I did not track it in the code, but according to the video, adding text nodes capability is missing
- Taking #2 to the greater level, I think Add / Insert Child should be implemented as Edit as HTML. I.e. accept free flow text as an input. That way user would be able to paste portions of HTML as a source for new nodes. I know we lose nice structuring of added nodes, but we gain huge flexibility in return.



> +using namespace HTMLNames;
> +

I think our relationship with HTMLNames is not strong enough to start using using.

> +        frontend->didRemoveNode(callId, -1);

Bug. -> didAddChild ?

> +
> +        newElem->copyNonAttributeProperties(oldElem);

Does this take care of JS properties on wrappers?

> +    if (code) {
> +        frontend->didChangeTagName(callId, -3);

Do you handle these error codes? If not, why differentiating?

> +            // Short delay for the UI to catch up
> +            setTimeout(function() {

Oh no.

I'll review front-end code in greater detail later.

[Timothy Hatcher]

I think it is fine to just have "add" and no "insert". Later I think we will have drag and drop reordering that will allow you to fix the order as a developer. Or if it matters, use edit HTML to control placement better.

Or have 2 other menu items, Add Previous Sibling and Add Next Sibling and you are done. Add Child would be there too.

I like Edit Tag! I think it is easier than Edit HTML. I would like to see the close tag update as you type the new tag name. It was weird to me to see it stay as the old tag until commit, espicually in a structured edit mode (not free form),

I think I might agree with Pavel that adding a new element should just just be free-form and not structured. But I'm on the fence. I usually prefer the structured mode…

[Joseph Pecoraro]

Thanks for the comments. I'll further then discussion.

> - Given a node with N children, Add Child only solves 1/N part of the problem.
> I.e. Besides "Add Child" we need "Insert Above" and "Insert Below".

> Or have 2 other menu items, Add Previous Sibling and Add Next Sibling and you
> are done. Add Child would be there too.

I think Drag & Drop reordering would handle this nicely. I did originally plan on having "Add Next Sibling" but I didn't get it in before midnight so I stopped with this patch =).

> - I did not track it in the code, but according to the video, adding text nodes
> capability is missing

You're correct. Great point! Anything left? Processing Instructions / Comments would be over overkill.

> - Taking #2 to the greater level, I think Add / Insert Child should be
> implemented as Edit as HTML. I.e. accept free flow text as an input. That way
> user would be able to paste portions of HTML as a source for new nodes. I know
> we lose nice structuring of added nodes, but we gain huge flexibility in
> return.

> I like Edit Tag! I think it is easier than Edit HTML. I would like to see the
> close tag update as you type the new tag name. It was weird to me to see it
> stay as the old tag until commit, espicually in a structured edit mode (not
> free form),
> 
> I think I might agree with Pavel that adding a new element should just just be
> free-form and not structured. But I'm on the fence. I usually prefer the
> structured mode…

I am a big fan of the structured mode. Mostly because it typically handles errors more gracefully. At the very least, if I want to create an element right now the first thing that comes to mind is _not_ "Edit HTML". Instead I'd rather $0.appendChild(document.createElement(...).


It sounds like I should add the following context menu items. Any way to limit this without something too clever? With Drag and Drop we could just have "Add Sibling" and have the user drag it after making it.

  - Edit Tag
  - Add Child Element
  - Add Child Text Node
  - Add Previous Sibling
  - Add Next Sibling





> > +using namespace HTMLNames;
> > +
> 
> I think our relationship with HTMLNames is not strong enough to start using
> using.

Point taken. Is there a convention here?


> > +        frontend->didRemoveNode(callId, -1);
> 
> Bug. -> didAddChild ?

Good catch.


> > +        newElem->copyNonAttributeProperties(oldElem);
> 
> Does this take care of JS properties on wrappers?

I actually don't even know what it does. Shame on me. 

What do you mean by property wrappers? Just generic properties added to the node via JS? Can anyone think of things I should copy other than Attributes and Children?


> > +    if (code) {
> > +        frontend->didChangeTagName(callId, -3);
> 
> Do you handle these error codes? If not, why differentiating?

This was actually debugging code for me, to know when it error'd out. I can change them all to -1.

> > +            // Short delay for the UI to catch up
> > +            setTimeout(function() {
> 
> Oh no.
> 
> I'll review front-end code in greater detail later.

Again, I wanted to get some discussion out the door (which it looks like I did). I have to investigate a number of UI issues with this patch. You can hold off a detailed look, I may ask you some questions on IRC the next time I look at this.

[Pavel Feldman]

> I am a big fan of the structured mode. Mostly because it typically handles
> errors more gracefully.

I actually think this is very wrong. I came from the IDE business where it is considered a classical mistake. Although this approach is very appealing to the implementor from the strict coding point of view, developers do not use structured editing. Do you use anything like that in your coding life? Things like IDE's ability to add classes / attributes via the tree view to the left? I bet you don't.

The right way of handling this is free flow editing with syntax highlighting and error underline. It is much more flexible and much, much more usable and faster. Fixing attribute value while typing element's body is a nightmare using structured editing. And I think that even with no highlighting and error detection free text editing would be more usable. You just open an editor field for the user to type things in.

> At the very least, if I want to create an element right
> now the first thing that comes to mind is _not_ "Edit HTML". Instead I'd rather
> $0.appendChild(document.createElement(...).

The real reason here is that you don't want to mess with context menu and html parsing (scrolling to the end). You would much rather type a lot in the console. And you don't use structured console editing - it is free flow. And you are nor afraid to make mistakes :P.

> 
>   - Edit Tag

This really should be triggered as double click on the tag name as well (as we do for attributes). Context menu action is only useful for discoverability.

>   - Add Child Element
>   - Add Child Text Node

I don't want to make such a decision early. I am not yet sure I'll do text or I want to wrap it with <span>. Or I changed my mind as I was doing it. As pointed out above, I'd like simple text area where I can put my html and surround it with <span> half-baked.

>   - Add Previous Sibling
>   - Add Next Sibling

These are formally correct, but Insert Above and Insert Below look much more friendly.
> 
> I actually don't even know what it does. Shame on me. 
> 
> What do you mean by property wrappers? Just generic properties added to the
> node via JS? Can anyone think of things I should copy other than Attributes and
> Children?

Custom properties added to the node via JS was what I was talking about. Local action such as tweaking tag name makes you responsible for preserving literally everything, including js properties and element identity (references to it in JS). I am not sure you can achieve this (and that is probably the reason behind the lack of such an action in FB). Edit As HTML removes this responsibility from you.
I know that people would use this for design purposes (probably to toggle between div and span - I've never needed such a thing for anything else), but those loosing references to their objects would be surprised. How do we manage this?

[Joseph Pecoraro]

Created attachment 51810 [details]
[PATCH] Converge on `long` for Inspector* id's

This has been bothering me for a long time. Sometimes they are "long", other times they are "int". I've made everything into a long. The only "int" that remains is the contextMenu, which I didn't track down why it was an int.

Searching for remaining ints produced nothing obvious:

> shell> cd inspector/
> shell> ack 'int .*?id ='
> shell> ack 'int .*?Id ='
> InspectorDatabaseResource.cpp
> 43:int InspectorDatabaseResource::s_nextUnusedId = 1;
> InspectorDOMStorageResource.cpp
> 50:int InspectorDOMStorageResource::s_nextUnusedId = 1;

[Joseph Pecoraro]

Created attachment 51811 [details]
[PATCH] "Edit Tag" and "Add Child Element"

This really just cleans up the code and adds a few ideas but has some serious UI issues that I have to work out. Some notes in no particular order:

- Blacklisted elements for "Edit Tag". I do not allow editing the tag name of html, head, title, or body in either the double click or context menu. I think doing so would likely be accidental.

- Update the closing tag. Unfortunately I do this onkeyup, which is noticeably slow. On key down does not yet have the content in a convenient form... I will likely need to switch this to onkeydown and do a String.fromCharCode( event.charCode ) and make it lowercase or not depending on if the shift key is down or not. I felt that was considerably ugly. The closing tag can be in any number of states (non-expanded, expanded, non-existent). I gracefully handle updating it all cases.

- There are serious UI jitters between committing the change, the front end getting the update, and the tree being repopulated. I have to work on making this a lot smoother. I know the setTimeout is unacceptable. I may instead add a function to a queue of actions to be performed when the updates arrive directly to the front-end instead of the hacky wait. I also need to prevent an expanded node collapsing the tree before the new update comes in and expands again.

Any ideas on the UI problems would be much appreciated.

[Pavel Feldman]

Comment on attachment 51810 [details]
[PATCH] Converge on `long` for Inspector* id's

r+ with comments.

>> This has been bothering me for a long time. Sometimes they are "long", other times they are "int".

True, it is just that it was not that easy before Yury introduced "long" setters in the ScriptObject bindings.
Please file a separate bug and land it there.

[Pavel Feldman]

(In reply to comment #8)
> Created an attachment (id=51811) [details]
> [PATCH] "Edit Tag" and "Add Child Element"
> 
> This really just cleans up the code and adds a few ideas but has some serious
> UI issues that I have to work out. Some notes in no particular order:
> 
> - Blacklisted elements for "Edit Tag". I do not allow editing the tag name of
> html, head, title, or body in either the double click or context menu. I think
> doing so would likely be accidental.
> 

I would agree on html, head and body. Not so sure about title.

> - Update the closing tag. Unfortunately I do this onkeyup, which is noticeably
> slow. On key down does not yet have the content in a convenient form... I will
> likely need to switch this to onkeydown and do a String.fromCharCode(
> event.charCode ) and make it lowercase or not depending on if the shift key is
> down or not. I felt that was considerably ugly. The closing tag can be in any
> number of states (non-expanded, expanded, non-existent). I gracefully handle
> updating it all cases.
> 

Did you try textInput event? It should come when you need it and have an appropriate data in place. Lowercase is no good for XML/XHTML where tag names are case sensitive. Wrt closing tag states, can we refactor the code so that you could handle it all at once? (Note that Alexander is currently working on making close tag selectable, so one of you would need to merge).

> - There are serious UI jitters between committing the change, the front end
> getting the update, and the tree being repopulated. I have to work on making
> this a lot smoother. I know the setTimeout is unacceptable. I may instead add a
> function to a queue of actions to be performed when the updates arrive directly
> to the front-end instead of the hacky wait. I also need to prevent an expanded
> node collapsing the tree before the new update comes in and expands again.
> 
> Any ideas on the UI problems would be much appreciated.

It is not that setTimeout is unacceptable, it will simply not work in Chromium since front-end is truly asynchronous. The reason you have delay is that we do coalescing dom update upon events in the front-end (see updateModifiedNodesSoon vs updateModifiedNodes). You should call updateModifiedNodes from within didChangeTagName to have instant update (look at how Edit as HTML is implemented).

[Pavel Feldman]

Comment on attachment 51811 [details]
[PATCH] "Edit Tag" and "Add Child Element"

> +void InspectorBackend::addChildElement(long callId, long nodeId)
> +{

This code should be in InspectorDOMAgent that is responsible for mutating DOM. See other dom-related methods in backend. InspectorBackend also should not be HTMLNames aware. Sorry if I missed it previously.
InspectorBackend is just a facade to InspectorController from the front-end side. It only handles trivial InspectorController duties in place. Heavy-weight stuff goes either into InspectorController or InspectorDOMAgent or to where appropriate.

> +void InspectorBackend::changeTagName(long callId, long nodeId, const AtomicString& tagName)

Ditto. Please look at other DOM-related methods.

> +        if (WebInspector.ElementsTreeElement.EditTagBlacklist.indexOf(this.representedObject.localName) === -1) {

Nit: I got used to using maps for these even if there are not too many items. Appending .keySet() to array definition makes a set.

> +        contextMenu.appendItem(WebInspector.UIString("Add Child Element"), this._addChildElement.bind(this));

Please do not ignore my concerns about adding text nodes or mixed content. It really bothers me and I think I have a strong point.

> +            // FIXME: Short delay for the UI to catch up.
> +            setTimeout(function() {

See suggestions on the bug comments.

[Joseph Pecoraro]

Created attachment 51850 [details]
[PATCH] Part 1 - Add "Edit Tag" on Double Click

Break things out a bit.  This is just Edit Tag Name. Changes are:
- follow the editHTML style of updates. Much smoother!
- no context menu item, only double click.

I am still not 100% certain if InspectorDOMAgent::changeTagName is 100% correct. I will be investigating that the next time I look at this patch.

[Joseph Pecoraro]

Created attachment 51852 [details]
[PATCH] Part 2 - Fix Tabbing and Attribute Editing

While enabling a backwards tab from attributes to the tag name I found a bunch of (new to me) issues with editing attributes. Here I fix a number of them and clean up the code a bit.
- Added: tab between tag name and attributes
- Fixed: tab to new attribute, tab back, tab forward; nolonger has extra whitespace
- Fixed: create new attribute, tab forward; no longer has mis-styled attribute

I can break this one down further. But I should post a video in a moment.

[Joseph Pecoraro]

Video as promised:
http://screencast.com/t/MzVhY2Fm

I forgot to mention I still do keyup when mirroring the closing tag. So that is another open issue right now =/.

[Timothy Hatcher]

WebCore adds a <br> if you delete everything in a contenteditable area. So that is what you see. We can maybe style any <br> in these area to no cause a break.

Looks good!

[Joseph Pecoraro]

(In reply to comment #15)
> WebCore adds a <br> if you delete everything in a contenteditable area. So that
> is what you see. We can maybe style any <br> in these area to no cause a break.

Awesome! That saved me a lot of time!

This made it work perfectly:

  .editing br { display: none; }

Are there any places you can think of where we would want a <br> while editing? Otherwise I'll add this to the patch (in inspector.css).

[Pavel Feldman]

Comment on attachment 51850 [details]
[PATCH] Part 1 - Add "Edit Tag" on Double Click

> +void InspectorDOMAgent::changeTagName(long callId, long nodeId, const AtomicString& tagName, bool expanded)
> +{
> +    Node* oldNode = nodeForId(nodeId);
> +    if (!oldNode) {
> +        // Use -1 to denote an error condition.
> +        m_frontend->didChangeTagName(callId, -1);
> +        return;

Assert non-element case early as well?

> +    }
> +
> +    // FIXME: Exception codes are ignored until the end.
> +    ExceptionCode code;
> +    RefPtr<Node> newNode = oldNode->document()->createElement(tagName, code);
> +
> +    // Copy over the original node's attributes.
> +    if (oldNode->isElementNode() && newNode->isElementNode()) {
> +        Element* oldElem = static_cast<Element*>(oldNode);
> +        Element* newElem = static_cast<Element*>(newNode.get());
> +
> +        newElem->copyNonAttributeProperties(oldElem);
> +        if (oldElem->attributes())
> +            newElem->attributes()->setAttributes(*(oldElem->attributes(true)));
> +    }
> +
> +    // Copy over the original node's children.
> +    Node* child;
> +    while ((child = oldNode->firstChild()))
> +        newNode->appendChild(child, code);

If you ignore previous call code, you should at least clear it before the subsequent one I guess.

>  
> +    _tagNameEditingCommitted: function(element, newText, oldText, tagName, moveDirection)
> +    {
> +        delete this._editing;
> +
> +        function cancel()
> +        {
> +            var closingTagElement = this._distinctClosingTagElement();
> +            if (closingTagElement)
> +                closingTagElement.textContent = "</" + tagName + ">";
> +
> +            this._editingCancelled(element, tagName);
> +            moveToNextAttributeIfNeeded.call(this);
> +        }
> +
> +        function moveToNextAttributeIfNeeded()

Nit: can we generalize traversal logic more and extract it from here?

> +        try {
> +            document.createElement(newText);
> +        } catch(e) {
> +            cancel.call(this);
> +            return;
> +        }

I am not sure this is necessary. You should rely on error codes on backend side.

[Pavel Feldman]

Comment on attachment 51852 [details]
[PATCH] Part 2 - Fix Tabbing and Attribute Editing

> +            var previous = element.previousSibling;
> +            if (!previous || previous.nodeType !== Node.TEXT_NODE)
> +                element.parentNode.insertBefore(document.createTextNode(" "), element);
> +            element.outerHTML = "<span class=\"webkit-html-attribute\">" +
> +                                  "<span class=\"webkit-html-attribute-name\">" + attr.name.escapeHTML() + "</span>=&#8203;\"" +
> +                                  "<span class=\"webkit-html-attribute-value\">" + attr.value.escapeHTML() + "</span>&#8203;\"" +
> +                                "</span>";

I really don't like these snippets spread all over the code in various methods. We really should do it in single place!

[Joseph Pecoraro]

Committed r56683
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorDOMAgent.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	M	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/treeoutline.js
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorDOMAgent.cpp
r56683 = bb9632f89ccbbeb6ff2a4c8913567bbe2337180f (trunk)
http://trac.webkit.org/changeset/56683

Committed r56684
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	M	WebCore/inspector/front-end/inspector.css
http://trac.webkit.org/changeset/56684


(In reply to comment #17)
> (From update of attachment 51850 [details])
> > +void InspectorDOMAgent::changeTagName(long callId, long nodeId, const AtomicString& tagName, bool expanded)
> Assert non-element case early as well?
> 

Done. I thought about this before and I agree. I also removed usage of "newNode" and just used "newElem" throughout, since Element is the result from document()->createElement(...).

> If you ignore previous call code, you should at least clear it before the
> subsequent one I guess.

ContainerNode::appendChild clears out the exception code each time. And, I don't check it for these operations, since even if I'm half way through, I want to keep going.

> > +        try {
> > +            document.createElement(newText);
> > +        } catch(e) {
> > +            cancel.call(this);
> > +            return;
> > +        }
> 
> I am not sure this is necessary. You should rely on error codes on backend
> side.

I removed this in favor of checking the exception code from the backend's createElement call.

[Joseph Pecoraro]

(In reply to comment #18)
> I really don't like these snippets spread all over the code in various methods.
> We really should do it in single place!

See bug 36719.

[Joseph Pecoraro]

Created attachment 51855 [details]
[PATCH] Fix Blacklist

The Blacklist was broken. It does not handle all cases that the tag name could be edited (namely, tabbing back from an attribute). I moved the black list check directly into _startEditingTagName so the blacklist is always checked. I also moved the code to a more convenient place, alongside similar code I didn't notice before!

[Joseph Pecoraro]

Created attachment 51856 [details]
[PATCH] Fix Blacklist

I was too greedy with git! This is the correct patch.

[Joseph Pecoraro]

Follow up fix committed:

Committed r56687
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
r56687 = 1d333db025c4379abe021858b6a6942b35fa84ac (trunk)
http://trac.webkit.org/changeset/56687