* SUMMARY Links for rules in <style> are incorrect, do not account for <style> offset in the document * TEST <html> <head> <style> body { color: red; } </style> </head> <body>Test</body> </html> * STEPS TO REPRODUCE 1. Inspector <body> on test 2. Show Styles > Rules sidebar => location link for "body" rule/selector is test.html:2, should be test.html:4 * NOTES - Works in Chrome, kinda works in Firefox (with issues) - Related to, but does not fix Media Query location links
<rdar://problem/22331966>
Blocked by cleanup in bug 148143.
Created attachment 259299 [details] [PATCH] Proposed Fix
Comment on attachment 259299 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259299&action=review > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:873 > + console.log("BEFORE", mediaSourceCodeLocation.originalLocationString()); Debug logging? > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:875 > + console.log("AFTER", mediaSourceCodeLocation.originalLocationString()); See above.
Comment on attachment 259299 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259299&action=review Have not gotten past the test yet, might look later today. > LayoutTests/inspector/css/getAllStyleSheets.html:23 > + InspectorTest.expectThat(styleSheets.length === 3, "There are three stylesheets."); NIT: I prefer phrasing these as "should be" rather than "is/are". What do you think about this convention? > LayoutTests/inspector/css/getAllStyleSheets.html:27 > + InspectorTest.log(`URL: ${styleSheet.isInlineStyleTag() ? "<style> on " : ""}${sanitizeURL(styleSheet.url)}`); NIT: I think we should err towards not performing (ternary) logic inside template ${placeholders}. It quickly becomes hard to follow IMO. > LayoutTests/inspector/css/getAllStyleSheets.html:28 > + InspectorTest.log(`OFFSET: (${styleSheet.startLineNumber}, ${styleSheet.startColumnNumber})`); Nice. > LayoutTests/inspector/css/getAllStyleSheets.html:29 > + InspectorTest.expectThat(styleSheet.parentFrame, "Stylesheet has a frame."); (see above): "should have a" > Source/JavaScriptCore/inspector/protocol/CSS.json:100 > + { "name": "isInline", "type": "boolean", "description": "Whether this stylesheet is a <style> tag created by the parser. This is not set for document.written <style> tags." }, Why don't tags from document.write get the flag?
(In reply to comment #4) > Comment on attachment 259299 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259299&action=review > > > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:873 > > + console.log("BEFORE", mediaSourceCodeLocation.originalLocationString()); > > Debug logging? > > > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:875 > > + console.log("AFTER", mediaSourceCodeLocation.originalLocationString()); > > See above. Yeah, oops!
(In reply to comment #5) > Comment on attachment 259299 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259299&action=review > > Have not gotten past the test yet, might look later today. > > > LayoutTests/inspector/css/getAllStyleSheets.html:23 > > + InspectorTest.expectThat(styleSheets.length === 3, "There are three stylesheets."); > > NIT: I prefer phrasing these as "should be" rather than "is/are". What do > you think about this convention? > > > LayoutTests/inspector/css/getAllStyleSheets.html:27 > > + InspectorTest.log(`URL: ${styleSheet.isInlineStyleTag() ? "<style> on " : ""}${sanitizeURL(styleSheet.url)}`); > > NIT: I think we should err towards not performing (ternary) logic inside > template ${placeholders}. It quickly becomes hard to follow IMO. > > > LayoutTests/inspector/css/getAllStyleSheets.html:28 > > + InspectorTest.log(`OFFSET: (${styleSheet.startLineNumber}, ${styleSheet.startColumnNumber})`); > > Nice. > > > LayoutTests/inspector/css/getAllStyleSheets.html:29 > > + InspectorTest.expectThat(styleSheet.parentFrame, "Stylesheet has a frame."); > > (see above): "should have a" > > > Source/JavaScriptCore/inspector/protocol/CSS.json:100 > > + { "name": "isInline", "type": "boolean", "description": "Whether this stylesheet is a <style> tag created by the parser. This is not set for document.written <style> tags." }, > > Why don't tags from document.write get the flag? I think in case of a document.write / dynamically generated <style> we should link not to any location, but to the <style> node itself. As in the case of dynamic generation, source code locations don't link to any reasonable location in any Resource, as they were dynamic. CSSStyleSheet has a reference to the ownerNode that we could create a DOMNode for and push to the frontend for dynamically generated <style>s. I was just not ambitious enough to go that far.
(In reply to comment #3) > Created attachment 259299 [details] > [PATCH] Proposed Fix NIT: You have used var instead of let in a lot of places. If we are really moving over to let, we should be consistent in its addition.
Created attachment 259329 [details] [PATCH] Proposed Fix
Comment on attachment 259329 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259329&action=review > Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:61 > + return [...this._styleSheetIdentifierMap.values()]; Cool. > Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:107 > + this._fetchInfoForAllStyleSheets(function() {}); You can make it even more of a placeholder: () => {} > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:816 > + var styleSheet = id ? WebInspector.cssStyleManager.styleSheetForIdentifier(id.styleSheetId) : null; I'll 'let' it go.
Comment on attachment 259329 [details] [PATCH] Proposed Fix Clearing flags on attachment: 259329 Committed r188631: <http://trac.webkit.org/changeset/188631>
All reviewed patches have been landed. Closing bug.