Bug 200425 – Web Inspector: Sources: provide a way to create an arbitrary Inspector Style Sheet

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

[Devin Rousso]

Right now, the only way to create an Inspector Style Sheet is by creating a new rule in the Styles sidebar of the Elements Tab.  This is unnecessarily restrictive, especially for those who don't use the Elements tab.

[Devin Rousso]

Created attachment 375498 [details]
Patch

[Devin Rousso]

Created attachment 375570 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 375498 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/FilterBar.css:46
> +.filter-bar > input[type="search"] + .navigation-bar > .item:first-child {

What did this look like before/after?

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:-182
> -        const doNotCreateIfMissing = true;
> -        WI.cssManager.preferredInspectorStyleSheetForFrame(this._frame, this.addRepresentedObjectToNewChildQueue.bind(this), doNotCreateIfMissing);

This removes the only `doNotCreateIfMissing` use. It should now always be false. I think we can eliminate that parameter now.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:237
> +            let createResourceButtonNavigationItem = new WI.ButtonNavigationItem("create-resource", WI.UIString("Create Resource"), "Images/Plus15.svg", 15, 15);

Should Web Inspector consider handling ⌘N in the Sources Tab? I don't think so right now because this isn't completely generic. Were it to be generic I'd think about this again.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:241
> +            this.filterBar.element.appendChild(createResourceNavigationBar.element);

Where does this appear in the FilterBar? It sounds like on the right of the "filter bar buttons" / search bar?
Normally UI for this is in the bottom left (Xcode File Navigator sidebar, TextEditor's "New Document", TextMate sidebar, BBEdit sidebar). Are there examples of it in the bottom of a sidebar in a different location?

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:594
> +        if (treeElement.hasChildren) {
> +            for (let child of treeElement.children) {
> +                if (child instanceof WI.IssueTreeElement)
> +                    return true;
> +            }
> +        }

I'd expect this to be more recursive. Instead of searching just 1 level below, should it recurse down all levels?

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1673
> +    _populateCreateResourceContextMenu(contextMenu)
> +    {
> +        if (InspectorBackend.domains.CSS) {
> +            contextMenu.appendCheckboxItem(WI.UIString("Inspector Style Sheet"), () => {
> +                WI.cssManager.preferredInspectorStyleSheetForFrame(WI.networkManager.mainFrame, (styleSheet) => {
> +                    WI.showRepresentedObject(styleSheet);
> +                });
> +            }, WI.cssManager.inspectorStyleSheetsForFrame(WI.networkManager.mainFrame).length);
> +        }
> +    }

This feels misleading:

    The UI seems to portray:
    "Create Resource > Inspector Style Sheet"

    But the behavior is doing:
    "Open/Create Resource > Inspector Style Sheet for Main Frame"

It isn't that much more misleading than the breakpoints "+" button. I think I'm fine with this but I'd like a clearer story.

[Devin Rousso]

Comment on attachment 375498 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/FilterBar.css:46
>> +.filter-bar > input[type="search"] + .navigation-bar > .item:first-child {
> 
> What did this look like before/after?

There was no "before", as this is the first time something was added _after_ the `<input type="search">`.  I'll upload a screenshot.

>> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:-182
>> -        WI.cssManager.preferredInspectorStyleSheetForFrame(this._frame, this.addRepresentedObjectToNewChildQueue.bind(this), doNotCreateIfMissing);
> 
> This removes the only `doNotCreateIfMissing` use. It should now always be false. I think we can eliminate that parameter now.

Awesome =D

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:237
>> +            let createResourceButtonNavigationItem = new WI.ButtonNavigationItem("create-resource", WI.UIString("Create Resource"), "Images/Plus15.svg", 15, 15);
> 
> Should Web Inspector consider handling ⌘N in the Sources Tab? I don't think so right now because this isn't completely generic. Were it to be generic I'd think about this again.

Yeah, I don't think we should add it, especially since we'd likely have other types of resources (e.g. Inspector Bootstrap Script) and don't have a "picker" for it.

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:241
>> +            this.filterBar.element.appendChild(createResourceNavigationBar.element);
> 
> Where does this appear in the FilterBar? It sounds like on the right of the "filter bar buttons" / search bar?
> Normally UI for this is in the bottom left (Xcode File Navigator sidebar, TextEditor's "New Document", TextMate sidebar, BBEdit sidebar). Are there examples of it in the bottom of a sidebar in a different location?

Yes, this appears after the `<input type="search">`.  I personally don't like putting the + on the left side, as it's so far away (way in the corner), and is less likely to be noticed.  Furthermore, that's where the filter action buttons are, and I didn't want the two to be confused.

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:594
>> +        }
> 
> I'd expect this to be more recursive. Instead of searching just 1 level below, should it recurse down all levels?

This is already done by `WI.NavigationSidebarPanel`.  If anything, we could probably even remove the `if (treeElement.hasChildren)`, as that's going to be checked too, but I'd rather keep this change smaller.

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1673
>> +    }
> 
> This feels misleading:
> 
>     The UI seems to portray:
>     "Create Resource > Inspector Style Sheet"
> 
>     But the behavior is doing:
>     "Open/Create Resource > Inspector Style Sheet for Main Frame"
> 
> It isn't that much more misleading than the breakpoints "+" button. I think I'm fine with this but I'd like a clearer story.

I was on the fence about having a checkbox.  I think it'd be clearer to make it be just an action item.

[Devin Rousso]

Created attachment 375608 [details]
Patch

[Devin Rousso]

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

[Joseph Pecoraro]

> >> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:241
> >> +            this.filterBar.element.appendChild(createResourceNavigationBar.element);
> > 
> > Where does this appear in the FilterBar? It sounds like on the right of the "filter bar buttons" / search bar?
> > Normally UI for this is in the bottom left (Xcode File Navigator sidebar, TextEditor's "New Document", TextMate sidebar, BBEdit sidebar). Are there examples of it in the bottom of a sidebar in a different location?
> 
> Yes, this appears after the `<input type="search">`.  I personally don't
> like putting the + on the left side, as it's so far away (way in the
> corner), and is less likely to be noticed.  Furthermore, that's where the
> filter action buttons are, and I didn't want the two to be confused.

I'd rather we move the filter actions to the right side and put the plus in the bottom left. But we can do that later.

[Joseph Pecoraro]

Comment on attachment 375608 [details]
Patch

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

r=me

I still think the (+) button should be in the left corner. Have you played with that? Can we file a follow-up to experiment with that and see if it feels better? That matches the rest of the system, and the consistently of knowing "the bottom left corner will always have this button" is a conceptual win.

> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:244
> +    if (options.allowDirectoryAsName && url !== "about:blank" && (!displayName || urlComponents.path.endsWith(displayName + "/")))

This makes me think we probably have others ("about:srcdoc" comes to mind).

Also `javascript:`, `mailto:`, `about:`, `blob:` and other common prefixes.

Most of what I've been doing recently is checking if the url starts with "http:" to be more proactive then reactive. Does that make sense here?

We should really cover `WI.displayNameForURL` with tests.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1679
> +            let addInspectorStyleSheetCheckbox = (parent, frame) => {

`parent` sounds like a node or tree element. Perhaps `menu` would be clearer.

> LayoutTests/inspector/unit-tests/url-utilities-expected.txt:252
> +FAIL: origin should be: 'http://example.com'
> +    Expected: "http://example.com"
> +    Actual: null

Whats up with these failures?

[Devin Rousso]

Comment on attachment 375608 [details]
Patch

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

(In reply to Joseph Pecoraro from comment #8)
> I still think the (+) button should be in the left corner. Have you played with that? Can we file a follow-up to experiment with that and see if it feels better? That matches the rest of the system, and the consistently of knowing "the bottom left corner will always have this button" is a conceptual win.
I'm not against this idea at all.  I just haven't had a chance to try it out.  I'll give it a shot and see how it feels.

>> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:244
>> +    if (options.allowDirectoryAsName && url !== "about:blank" && (!displayName || urlComponents.path.endsWith(displayName + "/")))
> 
> This makes me think we probably have others ("about:srcdoc" comes to mind).
> 
> Also `javascript:`, `mailto:`, `about:`, `blob:` and other common prefixes.
> 
> Most of what I've been doing recently is checking if the url starts with "http:" to be more proactive then reactive. Does that make sense here?
> 
> We should really cover `WI.displayNameForURL` with tests.

Hmmm, good point.  I'll rework this as a whitelist and add some tests.

>> LayoutTests/inspector/unit-tests/url-utilities-expected.txt:252
>> +    Actual: null
> 
> Whats up with these failures?

It appears that `parseURL` is missing some functionality, such as being able to parse URLs with user passwords (in addition to a username) or "autofill" a scheme.  Given that this patch doesn't change the way `parseURL` works (`origin` is just `scheme + host + port`), I'm inclined to fix it separately.

[Devin Rousso]

Created attachment 376436 [details]
Patch

[Devin Rousso]

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

[WebKit Commit Bot]

Comment on attachment 376436 [details]
Patch

Clearing flags on attachment: 376436

Committed r248753: <https://trac.webkit.org/changeset/248753>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54371657>

[Joseph Pecoraro]

Comment on attachment 376436 [details]
Patch

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

> LayoutTests/inspector/unit-tests/url-utilities.html:461
> +                {url: "http://example.com/path/to/page.html?a=1&b=2#test", expected: "page.html"},

I'd expect to see a test of a URL with encoded components "http://example.com/For%20Example"

> LayoutTests/inspector/unit-tests/url-utilities.html:468
> +                {url: "file://foo/bar", expected: "bar"},

I think the file: url should really have 3 slashes

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/248753/webkit

broke inspector/css/add-rule.html on macOS

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcss%2Fadd-rule.html

I was able to reproduce this by just running the test on 248753 which failed and 248752 which passed.

[Devin Rousso]

Comment on attachment 376436 [details]
Patch

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

>> LayoutTests/inspector/unit-tests/url-utilities.html:461
>> +                {url: "http://example.com/path/to/page.html?a=1&b=2#test", expected: "page.html"},
> 
> I'd expect to see a test of a URL with encoded components "http://example.com/For%20Example"

Oooh good call!  I was thinking about `parseURL`, which doesn't do any decoding, but `WI.displayNameForURL` does! =D

I'll add some followup tests.

>> LayoutTests/inspector/unit-tests/url-utilities.html:468
>> +                {url: "file://foo/bar", expected: "bar"},
> 
> I think the file: url should really have 3 slashes

You are right!  My mistake.

[Devin Rousso]

(In reply to Truitt Savell from comment #16)
> It looks like the changes in https://trac.webkit.org/changeset/248753/webkit
> 
> broke inspector/css/add-rule.html on macOS
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcss%2Fadd-rule.html
> 
> I was able to reproduce this by just running the test on 248753 which failed and 248752 which passed.

Fixed in r248774 <https://trac.webkit.org/r248774>.

[Devin Rousso]

Comment on attachment 376436 [details]
Patch

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

>>> LayoutTests/inspector/unit-tests/url-utilities.html:461
>>> +                {url: "http://example.com/path/to/page.html?a=1&b=2#test", expected: "page.html"},
>> 
>> I'd expect to see a test of a URL with encoded components "http://example.com/For%20Example"
> 
> Oooh good call!  I was thinking about `parseURL`, which doesn't do any decoding, but `WI.displayNameForURL` does! =D
> 
> I'll add some followup tests.

I added some followup tests in r248774 <https://trac.webkit.org/r248774>.

I noticed there were some URLs that didn't display as I'd expect (e.g. "http://example.com?key=alpha/beta" is "http://example.com?key=alpha/beta", not "example.com").  I'll take a look at addressing them in <https://webkit.org/b/165155>.