Summary: | Web Inspector: Handle the Enter Key in the Elements Tree Hierarchy | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Joseph Pecoraro <joepeck> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aroben, bweinstein, joepeck, pfeldman, rik, timothy | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2009-10-15 21:49:29 PDT
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)
3 is only if the selected item can have attributes, correct? 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! Created attachment 41269 [details]
[PATCH] Enter Key in Tree Hierarchy (improved)
Improved based on the last comment.
Created attachment 41270 [details]
[PATCH] Enter Key in Tree Hierarchy (improved)
Updated the wrong one!
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. (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 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... (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? |