Bug 138810 – Web Inspector: New CSS Rules should go into a new Stylesheet Resource that can be viewed/edited/saved

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
New CSS Rules should go into a new Stylesheet Resource that can be viewed/edited/saved. Currently to copy out the contents of test CSS added this way it is difficult. Having all the additions in one place and easy to copy/paste would be useful for developers. It may also be useful to just create a new Stylesheet resource entirely instead of non-obviously creating it by adding a new CSS Rule to a random element.

* NOTES
- Chrome - when adding new CSS rules creates a stylesheet resource you can jump too
- Firefox - you can create new stylesheet resources

[Radar WebKit Bug Importer]

<rdar://problem/19008875>

[Thomas van Diepen]

In my opinion, it would be a better solution to be able to assign a rule to an existing css resource, since it would eventually end up there.

[Devin Rousso]

Created attachment 310411 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 310411 [details]
Patch

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

• What happens when you select an Inspector Style Sheet and reload? Is there an error in the console?
• What is the Inspector StyleSheet's url normally?
• How does this feel when there are sub-frames? and each frame gets its own Inspector Style Sheet?

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:491
> +    _updateContent(styleSheet)

Note to self: I haven't reviewed this part of the patch.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleSheetTreeElement.js:40
> +        this.mainTitle = WebInspector.UIString("Inspector Style Sheet");

Nit: This should be the `title` up above.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:49
> +        WebInspector.cssStyleManager.addEventListener(WebInspector.CSSStyleManager.Event.StyleSheetAdded, this._styleSheetAdded, this);

This needs to be removed somewhere otherwise we leak every FrameTreeElement that is created.

Shouldn't there be code in onpopulate that looks up an inspector stylesheet if one exists? onpopulate happens the first time this tree element is attached. Most of the time an Inspector Style Sheet probably does not exist, but you'd have to sufficiently prove it can't exist or just match the existing pattern of having onpopulate generate all initial children.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:268
> +    _styleSheetAdded(event)
> +    {
> +        if (!event || !event.data || !event.data.styleSheet)
> +            return;

We shouldn't need to be this paranoid in our own code. This entire check should be eliminated.

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:401
> +        // Only show "inspector" stylesheets in the Resources Sidebar.

Lets drop the quotes around inspector. Maybe drop this comment entirely.

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:203
> +        if (this.resource instanceof WebInspector.CSSStyleSheet)
>              this._textEditor.readOnly = false;
> +        else {
> +            // Check the MIME-type for CSS since Resource.Type.Stylesheet also includes XSL, which we can't edit yet.
> +            if (this.resource.type === WebInspector.Resource.Type.Stylesheet && this.resource.syntheticMIMEType === "text/css")
> +                this._textEditor.readOnly = false;
> +
> +            // Allow editing any local file since edits can be saved and reloaded right from the Inspector.
> +            if (this.resource.urlComponents.scheme === "file")
> +                this._textEditor.readOnly = false;
> +        }

This code is ugly when all it does is set the same value for 3 difference conditions.

It is also unclear to me why this needs to wait until _contentWillPopulate and isn't handled earlier (like the constructor) because the `resource` should be known.

How about something like a helper:

    _shouldBeEditable()
    {
        if (this.resource instanceof CSSStyleSheet)
            return true;

        // Blah blah.
        if (...)
           return true;
        ...
        return false;
    }

[Devin Rousso]

Created attachment 310458 [details]
Patch

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #4)
> • What happens when you select an Inspector Style Sheet and reload? Is there
> an error in the console?

This was an issue previously, but I have resolved it in a new patch (I'll upload it shortly).

> • What is the Inspector StyleSheet's url normally?

It matches the URL of the frame.

> • How does this feel when there are sub-frames? and each frame gets its own
> Inspector Style Sheet?

Correct, each frame will have it's own Inspector Style Sheet (assuming the user has created a new rule for a node inside that frame).

[Joseph Pecoraro]

Comment on attachment 310458 [details]
Patch

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

After applying I found some issues.

1. Styles sidebar shows a URL for the rule. It should show "Web Inspector".

r=me for this because it is a regression that makes a lot of sense.

2. Styles Sidebar doesn't immediately show changes to StyleSheet after switching tabs

Steps to Reproduce:
1. Inspect this page
2. Show Elements Tab
3. Select <body>
4. Show Styles Rules sidebar
5. Click the (+) to add a new rule
6. Switch to the Resources tab
7. Modify the Inspector Style Sheet rule for "body {}"
8. Switch back to the Elements tab
  => Rule still has cursor and its empty. Clicking out suddenly loads the content.

I think we will want to improve this back and forth. It might happen regardless of the Inspector Style Sheet, but its something we should look into fixing. I don't think we always want to blur the sidebar when switching tabs (maybe thats what we end up on) but certain if the content changes out from under it we should consider something here.

3. Uncaught exception!

[Error] Unhandled Promise Rejection: TypeError: representedObject.isInspectorStyleSheet is not a function. (In 'representedObject.isInspectorStyleSheet()', 'representedObject.isInspectorStyleSheet' is undefined)
	fetchedStyleSheetContent (CSSStyleManager.js:527)
	fetchedStyleSheetContent
	promiseReactionJob

---

Other thoughts. I think we'll want a (+) button at the bottom of resources to create an inspector stylesheet if one doesn't exist for the current page. Because this is really super useful to just have these scratchpad stylesheets without having to hunt for how to create it (creating a new rule in the styles sidebar is obtuse).

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:170
> -    preferredInspectorStyleSheetForFrame(frame, callback)
> +    preferredInspectorStyleSheetForFrame(frame, callback, dontCreateIfMissing)

Style: Don't use dont and Never use never. Use `doNot` and notInTheForseeableFuture.

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:404
> -    _lookupStyleSheetForResource(resource, callback)
> +    _lookupStyleSheetForSourceCode(sourceCode, callback)
>      {
> -        this._lookupStyleSheet(resource.parentFrame, resource.url, callback);
> +        this._lookupStyleSheet(sourceCode.parentFrame, sourceCode.url, callback);
>      }

WebInspector.SourceCode doesn't have a `parentFrame` accessor, but Resource does. This is starting to get hairy.

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:467
>          // Ignore if it isn't a CSS stylesheet.
> -        if (resource.type !== WebInspector.Resource.Type.Stylesheet || resource.syntheticMIMEType !== "text/css")
> +        if (!(sourceCode instanceof WebInspector.CSSStyleSheet) && (sourceCode.type !== WebInspector.Resource.Type.Stylesheet || sourceCode.syntheticMIMEType !== "text/css"))
>              return;

I think this will get called if Scripts get edited. So you may want a helper which will make everything read easier and more correct:

    _isStyleSheet(sourceCode)
    {
        if (sourceCode instanceof WebInspector.CSSStyleSheet)
            return true;
        if (sourceCode instanceof WebInspector.Resource)
            return sourceCode.type === WebInspector.Resource.Type.Stylesheet && sourceCode.syntheticMIMEType === "text/css";
        return true;
    }

    ...

    if (!this._isStyleSheet(sourceCode))
        return;

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:528
> +            if (representedObject.isInspectorStyleSheet())
> +                representedObject.dispatchEventToListeners(WebInspector.SourceCode.Event.ContentDidChange);

Is there any reason for this conditional? Can't we do this always?

Also, its possible that representedObject here is no longer a CSSStyleSheet given it might change to a Resource above on line 508.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleSheetTreeElement.js:44
> +        this._styleSheet = styleSheet;
> +    }
> +
> +    // Public
> +
> +    get styleSheet() { return this._styleSheet; }

You can drop the member and just use this.representedObject here.

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:278
> +    _shouldBeReadOnly()

Having seen this now, the opposite name makes more sense. _shouldBeEditable.

[Joseph Pecoraro]

FWIW I don't think we need to solve (2) above before this lands (I think its an issue that affects existing code). But we should think about how to solve it and make our experience better.

[Devin Rousso]

Created attachment 310704 [details]
Patch

Created two new bugs based on previous comments:
- <https://webkit.org/b/172387> Web Inspector: Add method for creating Inspector Style Sheet resources
- <https://webkit.org/b/172388> Web Inspector: Changes are not applied in CSS sidebar when switching to Resources tab without blurring editor

[Joseph Pecoraro]

Comment on attachment 310704 [details]
Patch

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

r=me

I think there is still some hairiness around CSSStyleSheet being used like a Resource but not being a Resource. I wish that would be cleaned up but I think the general approach in this patch is sound.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:257
> +                    options.nameStyle = WebInspector.SourceCodeLocation.NameStyle.None;

This is exactly why I don't like using `const` for things that aren't const (namely: `options` here).

[Devin Rousso]

Created attachment 310947 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 310947 [details]
Patch

Clearing flags on attachment: 310947

Committed r217258: <http://trac.webkit.org/changeset/217258>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.