STR: - Load https://chromeperf.appspot.com/group_report?rev=371208 - Open developer tools; in my case they open on the console view. - Observe the 4 "Error in parsing value for 'display'." errors. If you don't observe them, make sure "Warnings" is checked under "CSS" for the console. - Click the third error Expected: the style-editor opens up, with the right style-sheet selected, and scrolled to the right line Actual: the style-editor opens up, but that's it. Note that if you click on the first error instead (the one in base.css) then it works as expected. So this seems to be something related to inline <style> stylesheets only.

Quick investigation: When the link is clicked in the console, that ends up calling this helper function: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/view-source.js#26 This loads/opens the styleeditor panel and calls panel.selectStyleSheet(sourceURL, sourceLine) This, in turn, after a couple more function calls ends up iterating over all stylesheets loaded in the styleeditor and checking if they're the right one using _isEditorToSelect: https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/StyleEditorUI.jsm#656 This piece of code is incorrect for <style> stylesheets. Here is the test that gets executed: isHref && editor.styleSheet.href == toSelect.stylesheet where isHref is true, toSelect.stylesheet is the URL of the current page, and editor.styleSheet.href is null. For <style> stylesheets, there's no href, however, we can use editor.styleSheet.nodeHref instead.

I'd like to take a look (my first real contribution). My patch loads the correct stylesheet, but at the wrong line: - In the second link from your repro the CSS error is reported as line 69. - `toSelect.col` is correspondingly set to 68 at https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/StyleEditorUI.jsm#642 However that is the line number in the HTML document. The style editor needs the offset from the start of the style block. The line number is passed from https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/view-source.js#30. I don't know how - or where - to get the inline style start line to calculate the offset. The column number is not passed in at this point so `selectStyleSheet` always goes to column 0, but I guess that is less of a concern. The patch also causes this test to fail: https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/test/browser_styleeditor_selectstylesheet.js#21 I am not 100% sure but I think the test is in error. It calls ` ui.selectStyleSheet(editor.styleSheet.href, LINE_NO);` with `editor.styleSheet.href` is `null` because `editors[1]` refers to an inline stylesheet. The test passed before this patch because `editor.styleSheet.href === toSelect.stylesheet === null` at https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/StyleEditorUI.jsm#664. Changing the test in any of these ways makes it pass with my patch: - Test `editors[0]` instead of `editors[1]` - because the external stylesheet has a non-null `href` - Pass `editor.styleSheet.nodeHref`; instead of `editor.styleSheet.href` - Pass `editor.styleSheet` instead of `editor.styleSheet.href` I guess one of these is the right action but I don't know which makes most sense. I have left the test failing in my first attempt patch. Could you give me some feedback and guidance on the failing test and line offset? Thanks!

We can expose the start line number on either inIDOMUtils or CSSStyleSheet directly. But it'll still be confusing to have a line number listed in the console but have a totally different one pop up in the style editor. Unless we can feed the start line into the editor and have it number lines starting at something other than 1?

(In reply to Joe Whitfield-Seed from comment #3) > I'd like to take a look (my first real contribution). > > My patch loads the correct stylesheet, but at the wrong line: > > - In the second link from your repro the CSS error is reported as line 69. > - `toSelect.col` is correspondingly set to 68 at > https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/ > StyleEditorUI.jsm#642 > > However that is the line number in the HTML document. The style editor needs > the offset from the start of the style block. > > The line number is passed from > https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/view- > source.js#30. I don't know how - or where - to get the inline style start > line to calculate the offset. You're right, there's an offset here. In fact I discovered an other problem while playing with your patch: if you click on 4th error in the console, then the style-editor selects the wrong stylesheet. Indeed, in this HTML page there are 2 <style> tags, and they therefore have the same href. The mechanism we use at the moment will just choose the first one it finds. I guess we have a few options: - Do what the debugger does with inline scripts: display the whole HTML source in the style-editor. This way we'd have only one entry in the style-editor sidebar for the HTML document. All inline <style> would be in it, and there would be no offset problems. The problems with this are: this will cause a bigger rewrite of the logic of the style-editor. Right now it basically iterates over all stylesheets in the document. Also, we'd have to somehow track where edits are made in the text editor. They should only be made inside a <style> tag. It would be a worse UX too, more scrolling involved, etc.. - Or, keep showing individual inline <style> stylesheets in the sidebar as separate entries, but offset the line number when opening them. In view-source.js/viewSourceInStyleEditor the normal, HTML-indexed line number should be kept. In styleeditor-panel.js/selectStyleSheet, same, we should keep on using the HTML-indexed line number. In StyleEditorUI.jsm/_isEditorToSelect however, we'll need to get more data about the editor.styleSheet object and not only compare nodeHref to the expected href. We'd want to know what's the offset of the inline stylesheet in the parent HTML document? And how many lines long it is. This way we'd be able to test if, say, line 240 really is line 5 in the context of a given stylesheet. And we'd also be able to find out which stylesheet we really want to select when there are many in the HTML document. Unfortunately, we don't yet have this information on that object. The editor.styleSheet is a StyleSheetFront object. This object is defined in devtools/server/actors/stylesheets.js. This is an object we use to communicate to the devtools debugger server. It represents, on the client-side, an instance of the StyleSheetActor that's on the server-side. All information in StyleSheetFront ultimately comes from the corresponding StyleSheetActor. The StyleSheetActor lives in the same process as the page currently being inspected. So it has access to the raw stylesheet object. So from there, we should be able to know the offset and length of the stylesheet and set its form (see the form method in StyleSheetActor, whatever this returns is then serialized and sent to the client, via StyleSheetFront, and will therefore be ultimately available in isEditorToSelect where we need it). bz suggested in comment 4 that we made the start line number on inIDOMUtils or CSSStyleSheet. That would be pretty useful. > The column number is not passed in at this point so `selectStyleSheet` > always goes to column 0, but I guess that is less of a concern. I agree that that's a bit less of a concern and totally ok to fix this later (even in a separate bug btw). But I do think we need to fix it anyway. This could become a problem with minified stylesheets. The style-editor right now auto-prettyprint stylesheets anyway, but I've heard of various problems with this, and it'd just be better if we could move the cursor to the exact location. > The patch also causes this test to fail: > https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/ > test/browser_styleeditor_selectstylesheet.js#21 > > I am not 100% sure but I think the test is in error. It calls ` > ui.selectStyleSheet(editor.styleSheet.href, LINE_NO);` with > `editor.styleSheet.href` is `null` because `editors[1]` refers to an inline > stylesheet. Good catch, I think you're right. > The test passed before this patch because `editor.styleSheet.href === > toSelect.stylesheet === null` at > https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/ > StyleEditorUI.jsm#664. > > Changing the test in any of these ways makes it pass with my patch: > > - Test `editors[0]` instead of `editors[1]` - because the external > stylesheet has a non-null `href` > - Pass `editor.styleSheet.nodeHref`; instead of `editor.styleSheet.href` > - Pass `editor.styleSheet` instead of `editor.styleSheet.href` > > I guess one of these is the right action but I don't know which makes most > sense. I have left the test failing in my first attempt patch. Not sure I understand passing `editor.styleSheet`, we need selectStyleSheet to only care about urls, not styleSheet objects, because it doesn't deal with those, only StyleEditorUI does. So to fix this particular case in the test, I would pass `editor.styleSheet.nodeHref` instead. But really, we need a test that covers both use cases: inline style tags and external stylesheets. So you should probably update this test so it tries both.

(In reply to Boris Zbarsky [:bz] from comment #4) > We can expose the start line number on either inIDOMUtils or CSSStyleSheet > directly. That'd be great indeed. Could you briefly explain how to do this? Maybe Joe could try this out? Or let us know if he'd prefer someone else to do this. > But it'll still be confusing to have a line number listed in the > console but have a totally different one pop up in the style editor. Unless > we can feed the start line into the editor and have it number lines starting > at something other than 1? I think I agree with this, we should offset line numbers shown in the codemirror text editor gutter. But I think there's also a risk of confusing users if lines aren't starting at 1. With one solution, we're confusing people who click on an error in the console. With the other solution, we're confusing people who just open stylesheets in the style-editor. Not sure what's best to be honest. I suggest starting with keeping line numbers from 1 and fixing the offset when clicking from the console (as explained in comment 5).

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #6) > (In reply to Boris Zbarsky [:bz] from comment #4) > > We can expose the start line number on either inIDOMUtils or CSSStyleSheet > > directly. > That'd be great indeed. Could you briefly explain how to do this? Maybe Joe > could try this out? Or let us know if he'd prefer someone else to do this. inIDOMUtils already has getRuleLine and getRelativeRuleLine. So one way would be to get a rule, compute both of those, and then the difference is the starting line of the style sheet. However, it's easy enough to add a new method. You can see how by reading the implementation of getRelativeRuleLine.

Indeed, see getRelativeRuleLine for how to get the line number of a sheet. I would prefer the non-hacky approach of exposing the sheet line number explicitly, because a sheet may not have any rules in it, in general (but may still have CSS parse errors; in fact it may not have rules _because_ it has CSS parse errors).

Awesome, thanks Tom and Boris for this. Joe, would you like to do this? Feel free to needinfo either Tom or Boris here if you need help with this.

Oh, and as for the rest of comment 6... If I click a link to an error on "line 10" in the console and the actual line that gets highlighted is line 6, I would just assume that the line-highlighting code screwed up because that sort of thing tends to be buggy (unlike basic things like counting lines), look at line 10, and be very confused when its text has nothing like what the error message was talking about. It gets even worse if I go back to the console to look at the next error, look at its line number, and then try to find it in the sheet _without_ clicking the link in the console itself. I really think we want to present the right line numbers in the CSS viewer...

(In reply to Boris Zbarsky [:bz] from comment #10) > I really think we want to present the right line numbers in the CSS viewer... The style editor prettification code also negatively affects this. See bug 724505.

Yes, sure. As an actual consumer of this stuff, I _really_ wish I could just go back to console links for CSS errors just opening in view source. :(

Yes, I'd like to take this on. Thanks for the guidance. I will start with opening links at the right line in the right editor. Line numbers will be unchanged. > We'd want to know what's the offset of the inline stylesheet in the parent HTML document? And how many lines long it is. This way we'd be able to test if, say, line 240 really is line 5 in the context of a given stylesheet. And we'd also be able to find out which stylesheet we really want to select when there are many in the HTML document. What about several inline sheets on the same line of HTML? I guess we would need the start column as well. Is there a util for testing if a line/col position is within a range? > Unless we can feed the start line into the editor and have it number lines starting at something other than 1? That sounds good - I would expect the editor to highlight a line with the number I just clicked on. However if we have two stylesheets: 100 <style> ... 110 </style> 111 <style> ... 120 </style> What if the user adds 5 lines to the first sheet in the style editor? I guess we could: - Do lines 111-115 now exist in both editors? Or are subsequent editor line numbers bumped? neither sounds good. Given this + prettification issues I think it better for now to leave the style editor starting at line 1 and have a visual cue that the line number is offset from the document. Maybe the stylesheet tab could include the offset - something like: <inline style sheet #n> 7 rules /document/path line 205 And/or a tooltip over relative line numbers giving the document line number. A third option is to change the links to have stylesheet-relative line numbers. I think this is the worst option because you can't glance at an error and see what line to open in an external editor.

(In reply to Boris Zbarsky [:bz] from comment #12) > Yes, sure. As an actual consumer of this stuff, I _really_ wish I could > just go back to console links for CSS errors just opening in view source. :( Yeah, sounds like the way the styleeditor works today doesn't match your use case. It'd be easy enough to have a pref/devtools setting that reverts back to opening view source instead of the style-editor to be honest. Probably as a separate bug though, we still want the viewSourceInStyleEditor machinery to work correctly.

(In reply to Joe Whitfield-Seed from comment #13) > However if we have two stylesheets: > > 100 <style> > ... > 110 </style> > 111 <style> > ... > 120 </style> > > What if the user adds 5 lines to the first sheet in the style editor? I > guess we could: > - Do lines 111-115 now exist in both editors? Or are subsequent editor line > numbers bumped? neither sounds good. Good point. When changes are made to a stylesheet, StyleSheetActor instances should figure out their new offsets. I wonder if that's going to be possible easily though. The offset we're getting from the platform by using getRuleLine doesn't seem to be updated correctly in all situations. In your example, if rule was document.styleSheets[1].cssRules[0], and if you added 5 lines to the first sheet, then: getRuleLine(rule) would still return 112 (the first line of code in the second stylesheet). Most probably because the styleeditor doesn't really change the text in the <style> element in the DOM. It just asks gecko to re-apply the new text using DOMUtils.parseStyleSheet(sheet, text)

I guess I'm not really sure what other use cases there are for console links to CSS errors other than mine. That is, other than finding the lines that correspond to the errors and seeing the text on them...

Also see the ideas in bug 880831 for associating a serial number with the style sheet.

Resetting the assignee since there hasn't been any activity for 3 years.