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

RESOLVED FIXED in Firefox 67

Status

enhancement
P2
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: pbro, Assigned: b17071, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

unspecified
Firefox 67
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 months ago

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.

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:


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!

Mentor: nchevobbe
Keywords: good-first-bug
Priority: -- → P2
Assignee

Comment 2

3 months ago

Hi Nicolas,

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

thanks
Aaditya

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

Assignee: nobody → b17071
Status: NEW → ASSIGNED
Assignee

Comment 4

3 months ago

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

Assignee

Comment 5

3 months ago

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

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?

Attachment #9047678 - Attachment description: pass CSS warning column through viewSouceInStyleEditor Fixed → Bug 1530612 - Pass CSS warning column through viewSouceInStyleEditor; r=nchevobbe.
Assignee

Comment 7

3 months ago

(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

Flags: needinfo?(nchevobbe)

Comment 8

3 months ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d860aa8a661b
Pass CSS warning column through viewSouceInStyleEditor; r=nchevobbe.

clearing needinfo

Flags: needinfo?(nchevobbe)

Comment 10

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Reporter

Updated

3 months ago
Blocks: 1528426
You need to log in before you can comment on or make changes to this bug.