1530612 - Pass CSS warning column location through viewSourceInStyleEditor so clicking on CSS warnings in console jumps to the right location in style editor

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

Steps to reproduce this issue:

• open the console on this page
• make sure the CSS category is ON in the filter toolbar
• find the CSS warning related to the tab-size property in prism.css
• click on the stylesheet link to the right of the message

Expected:
The style editor should open and the cursor should be at the right location, as in, the right line and column.

Actual:
The tool does open, and the right line is selected, but the cursor isn't moved to the right column.

The style editor panel supports this with selectStyleSheet. And the CSS error message in the console does have a column value too. So, if we passed this value through the various pieces of the puzzle (viewSourceInStyleEditor in toolbox is the main one I guess), that'd make it work.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Let's have a look at the issue.
This is where we defined what we call the location of a message, i.e. the url where the issue is: devtools/client/webconsole/components/Message.js#268-276.

In there, we can see an onClick: onFrameClick, meaning the clicking on this element will call the onFrameClick function, which is defined just before:

let onFrameClick;
if (serviceContainer && frame) {
  if (source === MESSAGE_SOURCE.CSS) {
    onFrameClick = serviceContainer.onViewSourceInStyleEditor
      || serviceContainer.onViewSource;
  } else if (/^Scratchpad\/\d+$/.test(frame.source)) {
    onFrameClick = serviceContainer.onViewSourceInScratchpad
      || serviceContainer.onViewSource;
  } else {
    // Point everything else to debugger, if source not available,
    // it will fall back to view-source.
    onFrameClick = serviceContainer.onViewSourceInDebugger
      || serviceContainer.onViewSource;
  }
}

We can see that for CSS messages (source === MESSAGE_SOURCE.CSS), we call onViewSourceInStyleEditor (if it exists, or onViewSource if not, but here we should be good).

This is where this function is defined: devtools/client/webconsole/webconsole-wrapper.js#280-287 :

What we do basically is

this.toolbox.viewSourceInStyleEditor(frame.url, frame.line);

The toolbox function is defined here: devtools/client/framework/toolbox.js#3192-3198 :

viewSourceInStyleEditor: function(sourceURL, sourceLine) {
    return viewSource.viewSourceInStyleEditor(this, sourceURL, sourceLine);
  }

We call viewSource.viewSourceInStyleEditor, which is defined in devtools/client/shared/view-source.js#10-35
We start to get to the bottom of it with:

await panel.selectStyleSheet(sourceURL, sourceLine);
await toolbox.selectTool("styleeditor");

selectStyleSheet is defined here devtools/client/styleeditor/panel.js#96-114, and surprise, it does accept a column argument:

  /**
   * Select a stylesheet.
   *
   * @param {string} href
   *        Url of stylesheet to find and select in editor
   * @param {number} line
   *        Line number to jump to after selecting. One-indexed
   * @param {number} col
   *        Column number to jump to after selecting. One-indexed
   * @return {Promise}
   *         Promise that will resolve when the editor is selected and ready
   *         to be used.
   */
  selectStyleSheet: function(href, line, col) {

And luckily for us, we do have the column in the frame object in webconsole-wrapper.js.
So there, we should do:

this.toolbox.viewSourceInStyleEditor(frame.url, frame.line, frame.column);

and modify whole the functions that we call along the way to add a column parameter:

• In devtools/client/framework/toolbox.js#3192-3198
• and in devtools/client/shared/view-source.js#10-35

---

Now we all the function modified, it should work. We should then make sure it is, with an automated test.
We already have a test to check that clicking on a CSS message opens the style editor at the right place: devtools/client/webconsole/test/mochitest/browser_webconsole_location_styleeditor_link.js.
The test can be run with ./mach test devtools/client/webconsole/test/mochitest/browser_webconsole_location_styleeditor_link.js.

What the test is missing is checking that the cursor is at the right column position.

We already have something for the line:

  info("style editor window focused");
  const href = frameLinkNode.getAttribute("data-url");
  const line = frameLinkNode.getAttribute("data-line");
  ok(line, "found source line");

We should retrive the column as well (there's a data-column attribute on the frameLinkNode object).
And then, we should rename performLineCheck to checkCursorPosition, and pass it the column we retrieved (we might also need to do a - 1 on it, as the cursor position we get might be 0-based).

Then, in the renamed checkCursorPosition, we need to assert that the column is the expected one.
We can copy what is done for the line:

is(editor.sourceEditor.getCursor().line, line, "correct line is selected");

And then we should be good!

Comment 2

[Aaditya Arora]

Hi Nicolas,

I would like to work on this bug, can you please assign it to me?

thanks
Aaditya

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Hello Aaditya, thanks for helping us :)
I assigned you the bug, feel free to ask any question!

Comment 4

[Aaditya Arora]

Attachment: Bug 1530612 - Pass CSS warning column through viewSouceInStyleEditor; r=nchevobbe.

Pass CSS warning column location through viewSourceInStyleEditor
so clicking on CSS warnings in console jumps to the right location in style editor

Comment 5

[Aaditya Arora]

Hi Nicolas,

I think you should not provide that much of information about the bugs because after having that much of information, the fun part of brainstorming and solving the bug decreases. and afterwards it looks like that I am just copy pasting stuff.

thanks
Aaditya

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Hello Aaditya, thanks for taking this and submitting a patch so quickly.
I'll review and test it shortly.

Yeah, I'm still trying to adjust the amount of information I put in bugs. Since it was set as a good first bug, I think that's still okay to guide people a bit through the code. I understand it might be too easy for more experienced people though.
If you plan to contribute a bit more, maybe the next bug you take shouldn't be a good-first-bug, so it will be a bit more challenging?

Comment 7

[Aaditya Arora]

(In reply to Nicolas Chevobbe from comment #6)

If you plan to contribute a bit more, maybe the next bug you take shouldn't be a good-first-bug, so it will be a bit more challenging?

Yaa, I am interested. Can you please assign to me any bug which is quite more challenging. I looked for it but not able to find one.

thanks
Aaditya

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d860aa8a661b
Pass CSS warning column through viewSouceInStyleEditor; r=nchevobbe.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

clearing needinfo

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d860aa8a661b