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.
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.)
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).
<rdar://problem/5712856>
Isn't it fixed by now ?
I don't think so. I think we still just show a preview for SVG images, and no way to see the source.
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?
Created attachment 304332 [details] Patch
Created attachment 304334 [details] [Image] After Patch is applied
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.
Created attachment 304341 [details] Patch
Created attachment 304342 [details] [Image] After Patch is applied
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.
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.
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.
Created attachment 304417 [details] Patch
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
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
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.
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>
Comment on attachment 304417 [details] Patch Clearing flags on attachment: 304417 Committed r213999: <http://trac.webkit.org/changeset/213999>
All reviewed patches have been landed. Closing bug.
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.