Bug 139153 - Web Inspector: support live editing of rule selectors
Summary: Web Inspector: support live editing of rule selectors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-12-01 14:21 PST by Brian Burg
Modified: 2015-07-27 14:22 PDT (History)
6 users (show)

See Also:


Attachments
[Proposed] Patch (4.81 KB, patch)
2015-07-23 21:27 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Proposed] After Patch is applied (18.04 MB, video/quicktime)
2015-07-23 21:30 PDT, Devin Rousso
no flags Details
Patch (6.68 KB, patch)
2015-07-27 12:40 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-12-01 14:21:43 PST
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.
Comment 1 Radar WebKit Bug Importer 2014-12-01 14:22:02 PST
<rdar://problem/19106757>
Comment 2 Devin Rousso 2015-07-23 21:27:46 PDT
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.
Comment 3 Devin Rousso 2015-07-23 21:30:17 PDT
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.
Comment 4 Timothy Hatcher 2015-07-24 11:15:20 PDT
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.
Comment 5 Timothy Hatcher 2015-07-24 11:17:08 PDT
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.
Comment 6 Devin Rousso 2015-07-27 12:40:58 PDT
Created attachment 257582 [details]
Patch
Comment 7 Timothy Hatcher 2015-07-27 13:31:32 PDT
Comment on attachment 257582 [details]
Patch

Nice!
Comment 8 WebKit Commit Bot 2015-07-27 14:22:01 PDT
Comment on attachment 257582 [details]
Patch

Clearing flags on attachment: 257582

Committed r187450: <http://trac.webkit.org/changeset/187450>
Comment 9 WebKit Commit Bot 2015-07-27 14:22:04 PDT
All reviewed patches have been landed.  Closing bug.