Bug 172487 – Web Inspector: Allow user to choose stylesheet when creating new rules

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

[Devin Rousso]

Allow user to choose stylesheet when creating new rules.

[Devin Rousso]

Created attachment 310975 [details]
Patch

[Devin Rousso]

Created attachment 311238 [details]
Patch

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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.

[Devin Rousso]

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`.

[Devin Rousso]

(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.

[Devin Rousso]

Created attachment 311359 [details]
Patch

[Timothy Hatcher]

Screenshot?

[Devin Rousso]

Created attachment 311889 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 312252 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 312252 [details]
Patch

Clearing flags on attachment: 312252

Committed r217911: <http://trac.webkit.org/changeset/217911>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

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.