Bug 27673

Summary: Inspector: Tab Through Attributes When Editing
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ssoonh, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Attachments:
Description Flags
Tab Through Attributes
none
Tab Through Attributes
timothy: review-
Tab Through CSS Properties (Needs First Patch)
none
Tab Through CSS Properties (Needs First Patch)
none
Tab Through CSS Properties (Needs First Patch)
timothy: review-
Tab Through Attributes/Properties (BOTH)
none
Tab Through Attributes/Properties (BOTH) timothy: review+

Description Joseph Pecoraro 2009-07-24 23:27:46 PDT
Nice feature that is in Firebug that most developers will expect.

Here is the work in progress, patch will be applied in a minute:
http://screencast.com/t/wAsMntnR1
Comment 1 Joseph Pecoraro 2009-07-24 23:49:02 PDT
Created attachment 33486 [details]
Tab Through Attributes

Also fixes (and improves the fix) for:
https://bugs.webkit.org/show_bug.cgi?id=27526
Comment 2 Joseph Pecoraro 2009-07-24 23:51:40 PDT
*** Bug 27526 has been marked as a duplicate of this bug. ***
Comment 3 Joseph Pecoraro 2009-07-25 00:05:39 PDT
Created attachment 33487 [details]
Tab Through Attributes

I had forgotten the try/catch block from the earlier bug =).  All better now.
Comment 4 Joseph Pecoraro 2009-07-26 00:25:45 PDT
After the first patch is applied this next patch (attached in a secon) will allow for tabbing though CSS Properties!  This took absolutely forever to get perfect but I finally got it working exactly how I wanted it:
http://screencast.com/t/MMWHpaaK
Comment 5 Joseph Pecoraro 2009-07-26 00:26:26 PDT
Created attachment 33503 [details]
Tab Through CSS Properties (Needs First Patch)

See Previous Comment for a video.
Comment 6 Joseph Pecoraro 2009-07-26 00:38:30 PDT
Created attachment 33504 [details]
Tab Through CSS Properties (Needs First Patch)

Removed Excess Whitespace.
Comment 7 Joseph Pecoraro 2009-07-26 06:55:35 PDT
Rik` (Anthony Ricaud) from IRC  just mentioned:

Need a way to go to the next CSS declaration:
https://bugs.webkit.org/show_bug.cgi?id=21629

Adding new CSS style properties to HTML nodes using Web Inspector:
https://bugs.webkit.org/show_bug.cgi?id=18224

I will mark 21629 as a duplicate of this one, but 18224 still exists and can probably be solved quite similarly (or like its done with element attributes).
Comment 8 Joseph Pecoraro 2009-07-26 06:55:58 PDT

*** This bug has been marked as a duplicate of bug 21629 ***
Comment 9 Joseph Pecoraro 2009-07-26 06:56:51 PDT
Wrong way around.
Comment 10 Joseph Pecoraro 2009-07-26 07:05:51 PDT
Created attachment 33510 [details]
Tab Through CSS Properties (Needs First Patch)

I found a way to eliminate firing the "dblclick" event in one case and just directly start editing (thanks to bug 18224's patch from way back when).  This case is for the new property and there is no text to select. I might even eventually do the same for the condition (tabbing to a non-new attribute), but for right now its fine, because it gets to specifically pick something to have selected when starting to edit.
Comment 11 Timothy Hatcher 2009-07-26 07:36:50 PDT
Comment on attachment 33487 [details]
Tab Through Attributes


> +                attributeElements[i].nextSibling.nextSibling.dispatchEvent(event); // "=" textNode, then the "webkit-html-attribute-value" node

All looks fine except this. I fear this is too fragile. It would be better to loop through the next siblings until you found "webkit-html-attribute-value".
Comment 12 Timothy Hatcher 2009-07-26 07:46:10 PDT
Comment on attachment 33510 [details]
Tab Through CSS Properties (Needs First Patch)

> +                if (elem.className === "name") {

This would be safer as "elem.hasStyleClass("name")" so if we add more to the className later it still works.

> +            // Find the attribute we were supposed to start editing in this section
> +            var list = section.propertiesTreeOutline._childrenListNode.childNodes;
> +            for (var i = 0, len = list.length; i < len; ++i) {
> +                var listItem = list[i];
> +                var spans = listItem.getElementsByTagName("span");
> +                if (spans[0].textContent === nameOfPropretyToEdit) {
> +                    var event = document.createEvent("MouseEvents");
> +                    event.initMouseEvent("dblclick", true, true);
> +                    spans[1].dispatchEvent(event); // the value
> +                    return;
> +                }
> +            }
> +        }

Over all these patches look good, but I think they are too low level and fragile at this point. They are working with the DOM too much when they should just be working with the high-level TreeElements and TreeOutlines. This would require additions to StylePropertyTreeElement like "editName" or "editValue" that would start editing on one of the parts without the need for a fake event and getElementsByTagName or using private properties like _childrenListNode.
Comment 13 Timothy Hatcher 2009-07-26 07:48:04 PDT
Comment on attachment 33487 [details]
Tab Through Attributes


> +        var attributeElements = this.listItemElement.getElementsByClassName("webkit-html-attribute-name");
> +        for (var i = 0, len = attributeElements.length; i < len; ++i) {
> +            if (attributeElements[i].textContent === attributeName) {
> +                var event = document.createEvent("MouseEvents");
> +                event.initMouseEvent("dblclick", true, true);
> +                attributeElements[i].nextSibling.nextSibling.dispatchEvent(event); // "=" textNode, then the "webkit-html-attribute-value" node
> +                break;
> +            }
> +        }
> +    },

This should be done without a fake event by asking the ElementsTreeElement to start editing on a specific attribute's value or name.
Comment 14 Joseph Pecoraro 2009-07-26 10:15:09 PDT
Created attachment 33511 [details]
Tab Through Attributes/Properties (BOTH)

Both patches together.
Comment 15 Joseph Pecoraro 2009-07-26 10:27:24 PDT
Created attachment 33512 [details]
Tab Through Attributes/Properties (BOTH)

Sorry about the spam, I had forgotten a Changelog.
Comment 16 Joseph Pecoraro 2009-07-26 10:34:22 PDT
(In reply to comment #11)
> All looks fine except this. I fear this is too fragile. It would be better to
> loop through the next siblings until you found "webkit-html-attribute-value".

Done. Ultimately Element Attributes should become more like CSS Properties in the StylesSidebar.  Right now the attributes are generated HTML and I am forced to use DOM Traversal functions.  In the StylesSidebar they are actual TreeElements and I can work on a higher level.

> This would be safer as "elem.hasStyleClass("name")" so if we add more to the
> className later it still works.

No Longer Needed (see next).

> Over all these patches look good, but I think they are too low level and
> fragile at this point. They are working with the DOM too much when they should
> just be working with the high-level TreeElements and TreeOutlines.

Done. Once I understood the structure of the higher-level nodes it became much easier (TreeOutline, Pane, TreeSection, TreeElement).(In reply to comment #13)

> This should be done without a fake event by asking the ElementsTreeElement to
> start editing on a specific attribute's value or name.

Done. The only remaining fake-event is with Element Attributes of which I cannot find a workaround for until its refactored.

---

There are a lot of edge cases in ElementAttributes due to the fact that it is just loosely floating HTML.  And there are even some edge cases in CSS Properties (such as editing an attribute and then pushing tab silently wipes things!).  I believe I handle everything correctly and I've tested quite a bit.
Comment 17 Timothy Hatcher 2009-07-27 14:58:41 PDT
Landed in r46429.