Bug 172487 - Web Inspector: Allow user to choose stylesheet when creating new rules
Summary: Web Inspector: Allow user to choose stylesheet when creating new rules
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-22 20:20 PDT by Devin Rousso
Modified: 2017-06-19 12:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.67 KB, patch)
2017-05-22 20:21 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (19.40 KB, patch)
2017-05-25 10:18 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (19.81 KB, patch)
2017-05-26 10:08 PDT, Devin Rousso
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
[Image] After Patch is applied (629.63 KB, image/png)
2017-06-02 17:37 PDT, Devin Rousso
no flags Details
Patch (19.40 KB, patch)
2017-06-07 16:38 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 Devin Rousso 2017-05-22 20:20:49 PDT
Allow user to choose stylesheet when creating new rules.
Comment 1 Devin Rousso 2017-05-22 20:21:34 PDT
Created attachment 310975 [details]
Patch
Comment 2 Devin Rousso 2017-05-25 10:18:14 PDT
Created attachment 311238 [details]
Patch
Comment 3 Joseph Pecoraro 2017-05-25 17:09:50 PDT
Comment on attachment 311238 [details]
Patch

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

> Source/WebCore/inspector/InspectorStyleSheet.cpp:944
> +    // Using setText() as this operation changes the stylesheet rule set.

Style: Add a newline before this. It deserves its own paragraph.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:286
> +        const justSelector = true;
> +        let selector = this._nodeStyles.node.appropriateSelectorFor(justSelector);

The _addNewRule above this uses:

    let selector = this.currentStyle().selectorText;

And this uses:

    const justSelector = true;
    let selector = this._nodeStyles.node.appropriateSelectorFor(justSelector);

What are the differences? Should these converge all on one common path?

> LayoutTests/inspector/css/add-rule.html:10
> +    let suite = InspectorTest.createAsyncSuite("CSSAgent.addRule");

Style: Name of this should be "CSS.addRule" not CSSAgent.

We should also use this opportunity to test error cases. For instance a bad stylesheet id:

    CSSAgent.addRule("bad-id", "foo");

Should respond with a reasonable error and not crash.

Using sentences for names, like you have here, seems okay. But I'd prefer something I can search for easily. For example:

    CSS.addRule.InNonExistingInspectorStyleSheet
    CSS.addRule.InExistingInspectorStyleSheet
    CSS.addRule.ExternalStyleSheet
    CSS.addRule.BadStyleSheetId

For inspection (name, conventions) check out these:
LayoutTests/inspector/dom/highlightNode.html
LayoutTests/inspector/page/setEmulatedMedia.html
LayoutTests/inspector/worker/worker-create-and-terminate.html
Comment 4 Joseph Pecoraro 2017-05-25 17:11:14 PDT
I haven't yet reviewed the entire change.

How does this perform if you combine-and-minify the frontend, inspect the inspector, and add rules to Main.css? The reparse might be expensive, I'd like to get a feel for how expensive that is. If its noticeable by the user that would be a concern.
Comment 5 Devin Rousso 2017-05-25 17:22:23 PDT
Comment on attachment 311238 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:286
>> +        let selector = this._nodeStyles.node.appropriateSelectorFor(justSelector);
> 
> The _addNewRule above this uses:
> 
>     let selector = this.currentStyle().selectorText;
> 
> And this uses:
> 
>     const justSelector = true;
>     let selector = this._nodeStyles.node.appropriateSelectorFor(justSelector);
> 
> What are the differences? Should these converge all on one common path?

I actually forgot that the Visual Styles sidebar does this.  When you create a new rule in the Rules sidebar, it sets the selector as the `appropriateSelectorFor` of the node.  In the Visual sidebar, however, it will use the selector of whatever style is currently selected (if the inline rule is selected, it falls back to the `appropriateSelectorFor` of the node).  IIRC, this was done to avoid having to add a "Duplicate Selector" context menu item.

Seeing as this cannot be done in the Rules sidebar (the concept of selecting a style doesn't apply there), I think it's OK to have the Visual and Rules sidebars differ slightly in functionality.  I will make `_addNewRuleContextMenu` behave like `addNewRuleClick`.
Comment 6 Devin Rousso 2017-05-26 09:50:55 PDT
(In reply to Joseph Pecoraro from comment #4)
> I haven't yet reviewed the entire change.
> 
> How does this perform if you combine-and-minify the frontend, inspect the
> inspector, and add rules to Main.css? The reparse might be expensive, I'd
> like to get a feel for how expensive that is. If its noticeable by the user
> that would be a concern.

I tried testing this on a few different websites with large minified CSS files and didn't see any lag in WebInspector or style changes on the page.
Comment 7 Devin Rousso 2017-05-26 10:08:47 PDT
Created attachment 311359 [details]
Patch
Comment 8 Timothy Hatcher 2017-06-02 10:13:43 PDT
Screenshot?
Comment 9 Devin Rousso 2017-06-02 17:37:33 PDT
Created attachment 311889 [details]
[Image] After Patch is applied
Comment 10 Joseph Pecoraro 2017-06-07 14:21:09 PDT
Comment on attachment 311359 [details]
Patch

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

> LayoutTests/inspector/css/add-rule-expected.txt:8
> +PASS: The selector is "body"
> +PASS: The range is [0:0,0:4]
> +PASS: The origin is "inspector"

These could all be converted to `should be` sentences. Something like:

    PASS: Rule selector should be "body".
    PASS: Rule range should be [0:0,0:4].
    PASS: Rule origin should be "inspector".

> LayoutTests/inspector/css/add-rule.html:142
> +            CSSAgent.addRule("-1", "div", ruleAdded);

Same thing about an obviously bad identifier. "DOES_NOT_EXIST" or "BAD_IDENTIFIER" etc.
Comment 11 Devin Rousso 2017-06-07 16:38:02 PDT
Created attachment 312252 [details]
Patch
Comment 12 WebKit Commit Bot 2017-06-07 16:53:14 PDT
Comment on attachment 312252 [details]
Patch

Clearing flags on attachment: 312252

Committed r217911: <http://trac.webkit.org/changeset/217911>
Comment 13 WebKit Commit Bot 2017-06-07 16:53:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Matt Lewis 2017-06-19 12:47:48 PDT
The test added in this patch, inspector/css/add-rule.html , seems to be a flaky timeout. It looks to have been since it was added.
Linked bugs in See Also.