Misleading background when debugging using column breakpoints

RESOLVED FIXED in Firefox 68

Status

defect
P2
normal
RESOLVED FIXED
3 months ago
Last month

People

(Reporter: Honza, Assigned: bmiriam1230)

Tracking

(Blocks 1 bug)

unspecified
Firefox 68
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(4 attachments)

Posted image image.png

When the debugger hits a column breakpoints the code at the paused line is using background color to indicate (at least it seams to) the next expression being evaluated.

See the attached screenshot:

The second row: Pause at column 13 (the square(a) function is about to be executed)

But the entire line starting with square(a) is highlighted.

Shouldn't just square(a) expression be highlighted?

The entire example is:

function square(a) {
  return a * a;
}

function sum(a, b) {
  return a + b;
}

function test(a, b) {
  return sum(square(a), square(b));
}

Honza

Shouldn't just square(a) expression be highlighted?

We don't have any way to know the end of the expression that is being evaluated. All we know is the line/column of the breakpoint itself. It seems like our options are either:

  • Highlight the whole line starting from the breakpoint position (which is what we do now)
  • Highlight the single token found at the location of the breakpoint
Priority: -- → P2

Do you have an opinion here Harald?

Flags: needinfo?(hkirschner)

It would be nice to support highlighting the expression (start & end). This is both a chrome parity issue and usability win.

In the short/medium term, I'd vote for the current behavior which makes it clear where the debug expression begins.

It would be nice to support highlighting the expression (start & end). This is both a chrome parity issue and usability win.

As far as I know, Chrome does not highlight the whole expression. I also don't think I'd support that as the behavior. The intention of the highlight is as an indication of roughly where execution is currently paused, not tell you what expression you are currently inside of. From looking at Chrome, it seems like they highlight the token starting at the current position and if that token is punctuation, also the next token, or something?

Hmm, interesting... This definitely warrants some more thought...

I moved your attachment to a google drawing so it would be easier to add more examples and comment

It can be confusing, as we currently highlight too much. Maybe just reducing this to the expression helps as quickfix.

With the column breakpoint information we have, could we use the next column breakpoint as end location for the highlight?

Flags: needinfo?(hkirschner)

The only information we have is the exact location of the breakpoint itself. We can't know when the expression starts or ends unless we parse the content ourselves, which would be slow and not a good idea.

The next column breakpoint location also isn't super useful because that could be anywhere. For instance in this case

var foo = 4; for(var i = 0; ...
          ^              ^

the arrows would be the two column locations, and highlighting between them would cover other unrelated code.

I think realistically all we can do is highlight using the simple text processing. Right now we go from the location to the end of the line, but it should be pretty straightforward to go from the location to the end of the current token.

I think realistically all we can do is highlight using the simple text processing. Right now we go from the location to the end of the line, but it should be pretty straightforward to go from the location to the end of the current token.

WFM, argument being that selecting less is better than too much.

Priority: P2 → P3
Priority: P3 → P2
Assignee

Comment 9

2 months ago

From what I can tell, the logic responsible for highlighting on paused column breakpoints is also responsible for highlighting in contexts other than column breakpoints. While experimenting with changing the highlighting column breakpoints, I realized my change extended this behavior to all highlighting.

If we highlight from location to end of current token for column breakpoints, should we also use that same rule for all highlighting in general? I personally think extending the rule to all situations will be more consistent and clearer. I’m curious to hear what other people think.

Hey Miriam, it seems worth exploring :)

I think this might line up well with how we do preview.

Assignee

Comment 11

Last month

I would like to claim this issue. Thanks!

Assignee

Comment 12

Last month

With the changes I made, any time the debugger is paused, only the token directly after the column breakpoint is highlighted. My commit also changes highlighting rules for debug lines without column breakpoints. I collected examples in the document below demonstrating cases with and without column breakpoints.

https://docs.google.com/document/d/1w9kaAzVgns5oQnONTPuPiREyhaq6NGd4mp6yvVUEU1Y/edit?usp=sharing

I changed one CSS file because without that change, the next column breakpoint after the token would also be highlighted which was misleading.

I would appreciate any feedback on these changes. Thank you!

Comment 13

Last month
Pushed by jlaster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/840bfd021063
highlight from location to end of current token when paused on breakpoint

Comment 14

Last month
bugherder
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Assignee: nobody → bmiriam1230
You need to log in before you can comment on or make changes to this bug.