Bug 16529 – Web Inspector: support drag and drop of CSS classes and ids onto DOM nodes

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Danny Bloemendaal]

Something missing in firebug (ok, we take that as an example ;-)) is that you cannot create new classes and connect it to a node. Together with issue #16528 this could be a very good addition.

[Adam Roben (:aroben)]

<rdar://problem/5712891>

[Joseph Pecoraro]

Adding/Editing Selectors was added in bug 27124.  This allows you to create a "new class".

As for "connecting it to a node" this is more complex. Currently you can do this by editing or creating the "class" attribute on the element in the element tree hierarchy.

This could be a nice case for HTML5 Drag and Drop (DnD).  Drag a Selector and drop it onto a node in the hierarchy.  The problem I see here is that it wouldn't be immediately obvious.  Take the following scenarios:

- Drag CSS Selector named ".highlight" onto a node in the tree and add the class name "highlight" to that node.  This makes sense because the selector itself is a "class" selector.

- Drag CSS Selector named "#footer" onto a node in the tree to apply all of the style rules inside the selector to that node's style attribute.  This is not immediately obvious, and should probably be avoided.  Editing the selector (already possible) might remedy this problem.

If we add DnD between CSS Selectors and the Elements Tree then I propose that only ".classname" selectors be given that privilege.  As always, discoverability is  major problem with these kinds of situations.  Otherwise I think DnD for the Styles Panel would be better suited to reordering the styles (changing the specificity).

Thoughts?

[Timothy Hatcher]

(In reply to comment #2)
> If we add DnD between CSS Selectors and the Elements Tree then I propose that
> only ".classname" selectors be given that privilege.  As always,
> discoverability is  major problem with these kinds of situations.  Otherwise I
> think DnD for the Styles Panel would be better suited to reordering the styles
> (changing the specificity).

I love the idea and agree with this. Tricky when it is a complex nested selector.

[Devin Rousso]

Created attachment 287994 [details]
Patch

[Blaze Burg]

Comment on attachment 287994 [details]
Patch

Code change looks pretty good. Devin is going to try adding live-preview for the drag target, so clearing r?.

[Devin Rousso]

(In reply to comment #5)
> Comment on attachment 287994 [details]
> Patch
> 
> Code change looks pretty good. Devin is going to try adding live-preview for
> the drag target, so clearing r?.

So according to the spec, the data inside a drag event is only visible from said event during the "dragstart" and "drop" events.  As such, I could make it so that there is some sort of static variable on CSSStyleDetailsSidebarPanel (or some other class), but it seems like a layering violation.  I know that adding a simple visual indicator (like a box around the DOM node tree element) is doable, but it seems like having live-preview would require more effort than I anticipated.  If we are fine with the (assumed) layering-violation, or there is a simpler workaround, then I would prefer to do that (unless live-preview causes other performance/visual problems).

[Joseph Pecoraro]

Comment on attachment 287994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287994&action=review

I'll let Brian do a full review. I just had a comment when skimming.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:434
> +            function toggleClass(className, flag)
> +            {
> +                this.classList.toggle(className, flag);
> +            }

Style: I've been naming functions that run on the inspected page with an obvious prefix and indication of what the this object is. Because we support older backends (like iOS 7), the code that runs inside this function may need to be run in an environment lacking some modern features (iOS 7 supports ES5 with very little ES6). That is not an issue here, but its a good indication to the person reading the code that they need to be careful.

Lets name this function:

    function inspectedPage_node_toggleClass(className, flag) {
        this.classList.toggle(className, flag);
    }

I realize now after reading more of the patch that this is just moved code. I think it would be worth the rename, either now or later I can follow-up and rename them all.

[Blaze Burg]

Comment on attachment 287994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287994&action=review

r=me, but please rename the injected script function.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:431
> +            function toggleClass(className, flag)

Side note: I really wish we didn't use `this` in injected functions, because using an arrow function (perhaps in a refactoring) will break the code. I personally prefer python's approach of passing self as first argument.

[Devin Rousso]

Created attachment 289021 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 289021 [details]
Patch

Clearing flags on attachment: 289021

Committed r206008: <http://trac.webkit.org/changeset/206008>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.