Bug 16079 – Web Inspector: SVG image content view should toggle between image and source

• Component: Web Inspector
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

No way to view source for SVGImage resources in inspector

The Inspector has not yet been educated about SVG images and the fact that they have source which is human readable.  I discovered this today when looking at if bug 16056 applied to CachedImage (for the SVGImage case).  bug 16056 does apply to CachedImage, but only if the Inspector is able to display the source for .svg files used by <img> tags or CSS backgrounds.

[Timothy Hatcher]

Where does the SVG image show up in resources? I assume under the Images category?

We could have Preview, Source and DOM views for SVG images (a three part segmented-button in the toolbar.)

[Eric Seidel (no email)]

Yes, it shows up under Images.  Preview/Source/DOM sounds great to me. :)  Obviously this is not a high priority issue, but once we work out more of the bugs in SVGImage, I do see that as the major way SVG will be used on the web (in HTML via <img> tags and CSS background-image: properties).

[Adam Roben (:aroben)]

<rdar://problem/5712856>

[Anthony Ricaud]

Isn't it fixed by now ?

[Timothy Hatcher]

I don't think so. I think we still just show a preview for SVG images, and no way to see the source.

[Alexei Masterov]

There are at least two ways that I know of that and SVG can be included into page:

1. <img="name.svg">

2. <object data="name.svg">

in the first case - the browser treats it as a static image and renders it as such, in the second case - the browser treats it as an organic part of the page, allows javascript that is inside it to interact with the page DOM, etc.

Inspector is consistent with the browser in a way that it displays the content of an SVG in both cases.  In the first case it treats it as an image, in the second case it treats it as a source file, allows javascript debugging, and includes it's DOM as a subtree into the DOM of the page.

Can you please provide a reason for why it would be beneficial to the end user to show more than what the browser sees in the first case?

[Devin Rousso]

Created attachment 304332 [details]
Patch

[Devin Rousso]

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

[Joseph Pecoraro]

Comment on attachment 304332 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:43
> +        if (this._resource.mimeTypeComponents.type === "image/svg+xml") {
> +            if (!this._showSourceButtonNavigationItem) {
> +                this._showSourceButtonNavigationItem = new WebInspector.ActivateButtonNavigationItem("svg-show-source", WebInspector.UIString("Show Source"), WebInspector.UIString("Show Image"), "Images/NavigationItemCurleyBraces.svg", 13, 13);
> +                this._showSourceButtonNavigationItem.addEventListener(WebInspector.ButtonNavigationItem.Event.Clicked, this._handleShowSourceClick, this);
> +            }
> +            return [this._showSourceButtonNavigationItem];
> +        }

I think we should experiment with a ResourceClusterView like approach and have a toggle between "Image" and "Text" or "Source" (and maybe eventually "DOM" or "Tree"). This button is okay, but I think the other approach is more generic and expands to more than 2 displays. I've stopped the review here because I think we both agree we should try out this approach.

[Devin Rousso]

Created attachment 304341 [details]
Patch

[Devin Rousso]

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

[Blaze Burg]

Comment on attachment 304332 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:43
>> +        }
> 
> I think we should experiment with a ResourceClusterView like approach and have a toggle between "Image" and "Text" or "Source" (and maybe eventually "DOM" or "Tree"). This button is okay, but I think the other approach is more generic and expands to more than 2 displays. I've stopped the review here because I think we both agree we should try out this approach.

I agree as well.

[Dean Jackson]

Comment on attachment 304341 [details]
Patch

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

This looks good to me, but I'll let a WI team member put the r+

> Source/WebInspectorUI/UserInterface/Views/SVGImageResourceClusterContentView.js:140
> +                    representedObject += contentViewToShow.textEditor.string = fileReader.result;

I'm not sure about WI coding guidelines, but I'd do this in two separate lines. Also, I'm not sure why you need representedObject here at all - you used it above when creating the TextContentView, but I don't see where you use it again.

[Joseph Pecoraro]

Comment on attachment 304341 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Main.html:665
>      <script src="Views/StorageSidebarPanel.js"></script>
> +    <script src="Views/SVGImageResourceClusterContentView.js"></script>
>      <script src="Views/SyntaxHighlightingSupport.js"></script>

Nit: Sort using `sort`. Capitalizes are less than lower case. So "SVG" < "St".

> Source/WebInspectorUI/UserInterface/Views/SVGImageResourceClusterContentView.js:43
> +        this._imagePathComponent = createPathComponent(WebInspector.UIString("Image"), "image", WebInspector.SVGImageResourceClusterContentView.Identifier.ShowImage);
> +        this._sourcePathComponent = createPathComponent(WebInspector.UIString("Source"), "source", WebInspector.SVGImageResourceClusterContentView.Identifier.ShowSource);

Do these have icons? They should have something.

> Source/WebInspectorUI/UserInterface/Views/SVGImageResourceClusterContentView.js:66
> +    shown()

Add a `// Protected` comment above these.

> Source/WebInspectorUI/UserInterface/Views/SVGImageResourceClusterContentView.js:137
> +            this._resource.requestContent()
> +            .then((result) => {

Style: I'd read this easier as:

    this._resource.requestContent().then((result) => {
        ...
    }).catch(handlePromiseException);

I understand others disagree.

>> Source/WebInspectorUI/UserInterface/Views/SVGImageResourceClusterContentView.js:140
>> +                    representedObject += contentViewToShow.textEditor.string = fileReader.result;
> 
> I'm not sure about WI coding guidelines, but I'd do this in two separate lines. Also, I'm not sure why you need representedObject here at all - you used it above when creating the TextContentView, but I don't see where you use it again.

Indeed, representedObject is unused here, so you can drop the var everywhere and just inline an empty string above.

> Source/WebInspectorUI/UserInterface/Views/SVGImageResourceClusterContentView.js:168
> +WebInspector.SVGImageResourceClusterContentView.Identifier = {
> +    ShowImage: "show-image",
> +    ShowSource: "show-source",
> +};

Why the "Show" prefix? Seems Identifier.Image and Identifier.Source would be sufficient.

[Devin Rousso]

Created attachment 304417 [details]
Patch

[Build Bot]

Comment on attachment 304417 [details]
Patch

Attachment 304417 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3323797

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/fire-success-event-exception.html

[Build Bot]

Created attachment 304432 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 304417 [details]
Patch

What is left to review of this patch? I still think there should be icons for the options, but other than that this looks good.

[Devin Rousso]

Comment on attachment 304417 [details]
Patch

(In reply to comment #18)
> Comment on attachment 304417 [details]
> Patch
> 
> What is left to review of this patch? I still think there should be icons
> for the options, but other than that this looks good.

<https://webkit.org/b/169687>

[WebKit Commit Bot]

Comment on attachment 304417 [details]
Patch

Clearing flags on attachment: 304417

Committed r213999: <http://trac.webkit.org/changeset/213999>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

Comment on attachment 304417 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SVGImageResourceClusterContentView.js:42
> +        this._imagePathComponent = createPathComponent(WebInspector.UIString("Image"), "image", WebInspector.SVGImageResourceClusterContentView.Identifier.Image);

I'm on the fence on if this should say "Image" or "Preview". Eventually we might even want "DOM Tree", and then it really might be "Preview" if changes to the DOM are allowed.