Bug 148141

Summary: Web Inspector: Links for rules in <style> are incorrect, do not account for <style> offset in the document
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, hi, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix none

Joseph Pecoraro
Reported 2015-08-18 14:19:47 PDT
* 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
Attachments
[PATCH] Proposed Fix (26.76 KB, patch)
2015-08-18 14:35 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
[PATCH] Proposed Fix (27.03 KB, patch)
2015-08-18 17:30 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2015-08-18 14:20:22 PDT
<rdar://problem/22331966>
Joseph Pecoraro
Comment 2 2015-08-18 14:35:05 PDT
Blocked by cleanup in bug 148143.
Joseph Pecoraro
Comment 3 2015-08-18 14:35:45 PDT
Created attachment 259299 [details] [PATCH] Proposed Fix
Matt Baker
Comment 4 2015-08-18 15:05:30 PDT
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.
Blaze Burg
Comment 5 2015-08-18 15:07:41 PDT
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?
Joseph Pecoraro
Comment 6 2015-08-18 15:13:16 PDT
(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!
Joseph Pecoraro
Comment 7 2015-08-18 15:17:23 PDT
(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.
Devin Rousso
Comment 8 2015-08-18 17:03:15 PDT
(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.
Joseph Pecoraro
Comment 9 2015-08-18 17:30:59 PDT
Created attachment 259329 [details] [PATCH] Proposed Fix
Blaze Burg
Comment 10 2015-08-18 21:17:40 PDT
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.
WebKit Commit Bot
Comment 11 2015-08-18 22:00:44 PDT
Comment on attachment 259329 [details] [PATCH] Proposed Fix Clearing flags on attachment: 259329 Committed r188631: <http://trac.webkit.org/changeset/188631>
WebKit Commit Bot
Comment 12 2015-08-18 22:00:51 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.