Bug 139153 – Web Inspector: support live editing of rule selectors

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

[Brian Burg]

Use case: user wants to tweak what nodes a style rule applies to. For example, refactoring to use class selectors instead of child/sibling/id.
Current behavior: if the user changes a selector such that the selected node is no longer matched, the rule disappears from the sidebar and the user can't edit the selector any more.

Perhaps we can temporarily pin the rule being edited while it has focus, and show whether or not it will still match.
When focus goes elsewhere (or an X is clicked), the rule can be hidden if it doesn't apply.

[Radar WebKit Bug Importer]

<rdar://problem/19106757>

[Devin Rousso]

Created attachment 257431 [details]
[Proposed] Patch

I definitely agree that there should be a way to "preview" which nodes will match the selector.  I don't think, however, that having a "sticky" section is the right way to do that as that may cause some confusion as to what rules match each node.  I feel that this functionality is better suited to the current layout.  I will upload a video right after this.

[Devin Rousso]

Created attachment 257433 [details]
[Proposed] After Patch is applied

Essentially, any time the user edits a selector in the sidebar, all nodes matching that edited selector will be highlighted.  In this way, the user can determine if the selector is matching the nodes that they are attempting to match by checking to see if they are highlighted.

[Timothy Hatcher]

Comment on attachment 257431 [details]
[Proposed] Patch

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

I like this! We should do this. Historically we did pin the rule you were editing, and it would disappear if you clicked away (if it no longer applied). That way if you mistyped a selector, you could still edit it to make it apply again.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:54
> +    this._selectorElement.addEventListener("keyup", this._highlightNodesWithSelector.bind(this));

This is risky. If _highlightNodesWithSelector ever starts taking an argument, it will break because this event listener will send an event object as the first argument and it could cause undefined behavior.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:131
> +    this._highlightConfig = {
> +        borderColor: {r: 255, g: 229, b: 153, a: 0.66},
> +        contentColor: {r: 111, g: 168, b: 220, a: 0.66},
> +        marginColor: {r: 246, g: 178, b: 107, a: 0.66},
> +        paddingColor: {r: 147, g: 196, b: 125, a: 0.66},
> +        showInfo: true
> +    };

We should find a central place for this to be shared with other places we have the config. CSSStyleManager should really encapsulate DOMAgent.highlightSelector and DOMAgent.highlightNode, along with the colors used.

[Timothy Hatcher]

Comment on attachment 257431 [details]
[Proposed] Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:131
>> +    };
> 
> We should find a central place for this to be shared with other places we have the config. CSSStyleManager should really encapsulate DOMAgent.highlightSelector and DOMAgent.highlightNode, along with the colors used.

Actually DOMTreeManager is likely the right manager. It has some of this encapsulation already, it should have more. Agent calls should stay in the managers as much as possible.

[Devin Rousso]

Created attachment 257582 [details]
Patch

[Timothy Hatcher]

Comment on attachment 257582 [details]
Patch

Nice!

[WebKit Commit Bot]

Comment on attachment 257582 [details]
Patch

Clearing flags on attachment: 257582

Committed r187450: <http://trac.webkit.org/changeset/187450>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.