Closed Bug 1555333 Opened 6 years ago Closed 5 years ago

Editing breakpoints should re-enable breakpoints globally

Categories

(DevTools :: Debugger, enhancement, P3)

enhancement

Tracking

(firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: jdescottes, Assigned: ruchikag826)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

After clicking on the "Disable breakpoints" icon in the debugger, the icon becomes blue and the Breakpoint panel changes its background to grey and fades slightly (opacity 0.6)

However you can still enable/disable breakpoints, so if you clicked on this button by mistake, it can be difficult to understand that something is disabled.

Maybe we could:

  • decrease even more the opacity
  • add a title displayed when hovering the panel
  • update the panel header from "Breakpoints" to "Breakpoints (disabled)"

I assume we don't want to fully disable the checkboxes in this panel because it can be useful to disable some breakpoints after clicking on "disable breakpoints".

Comparing with Chrome's implementation, I noticed an interesting difference: in Chrome, as soon as you create a new breakpoint in a source file, it will enable breakpoints again. I think that can be a good way to bail out for users who activated this feature by mistake.

We discussed this 3 months ago and agreed we should follow chrome's design, which can be summarized as when breakpoints are disabled:

  1. clicking a breakpoint in the gutter turns off the "disabled breakpoints" state
  2. clicking a breakpoint in the breakpoint panel turns off the "disabled breakpoints" state

basically anything that edits a breakpoint should re-enable breakpoints :)

Keywords: good-first-bug
Priority: -- → P3
Summary: The "disable breakpoints" feature should be more visible when enabled → Editing breakpoints should re-enable breakpoints globally

(In reply to Jason Laster [:jlast] from comment #1)

We discussed this 3 months ago and agreed we should follow chrome's design, which can be summarized as when breakpoints are disabled:

  1. clicking a breakpoint in the gutter turns off the "disabled breakpoints" state
  2. clicking a breakpoint in the breakpoint panel turns off the "disabled breakpoints" state

basically anything that edits a breakpoint should re-enable breakpoints :)

I just looked at these two points on chrome. The first one ('clicking a breakpoint in the gutter turns off the "disabled breakpoints" state') worked exactly as you described. However the second point did not work the work for me. Clicking a breakpoint in the breakpoint panel did not turn off the disabled breakpoints state. I'll demonstrate this in a gif but please let me know if I did not do the steps to reproduce correctly or am misunderstanding something. If I am correct, do we still want to implement both points? I think they still make sense even if chrome doesn't do both.

@jlast Here is the gif I mentioned in above comment. (Trying second point out in chrome): https://giphy.com/embed/SIJ9jRKj2jul40kf1f

Oh yeah, great point. I consider that a bug in Chrome :)

Hello :jdescottes,
I found this bug tagged as a 'good-first-bug'. With your help and guidance, I would like to work on this if it's available.

Hi Karan, I'd be happy to help mentor.

feel free to stop by here: https://devtools-html-slack.herokuapp.com

Hi Jason,
Thank you for helping, I'll reach out to you on slack.

Assignee: nobody → sapoliakaran

I like Chrome’s use:

  1. When adding, enable breakpoints again (skipPausing = false)
  2. When disabling/removing, do nothing to skipPausing

I like that the user can turn off all pausing, remove or disable all the breakpoints they don’t want, then turn things back on. We have to remember that the “skip pausing” button is a deliberate action — i.e. the user meant to do it. We should be very careful about changing that preference for people.

@davidwalsh Do you agree that clicking a breakpoint in the breakpoint side panel should have the same effect on skipPausing as described in your previous comment?

Yep, that makes sense janel.

+1 for enabling pausing again when users add breakpoints. I would even extend this to checking/adding additional options in XHR/Event/DOM/etc Breakpoints panels

Ok, so what needs to be done is:

  1. When a user clicks on a gutter to add a break point, skipPausing is set to false (if it was set to true).
  2. When a user enables a breakpoint in the secondary panel, skipPausing is set to false (if it was set to true).
Blocks: 1565711
Blocks: 1565713
Assignee: sapoliakaran → nobody

Karan, i'm un-assigning you as it has been a couple of months. happy to help mentor on other bugs

Assignee: nobody → ruchikag826

Maybe just add something like

if (skipPausing) { toggleSkipPausing(); }

to the gutter-click and breakpoint-checkbox handler functions?

No longer blocks: 1565711
No longer blocks: 1565713

hello everyone!
Sorry for the delay, I was not well. I have started working on this bug as of now. AFAIK, I need to do this:

1. When a user clicks on a gutter to add a breakpoint, skipPausing is set to false (if it was set to true).
2. When a user enables a breakpoint in the secondary panel, skipPausing is set to false (if it was set to true).

And the code will look something like:
if (skipPausing) { toggleSkipPausing(); }

I understand all of this, and would like like to know which file/folder will I have to edit exactly? or are they a multitude of them?

Thank you all!

Hello Ruchika! I dabbled with this patch and would love your review!

https://phabricator.services.mozilla.com/D42990

David

@davidwalsh thank you for your help! I saw this and now I understand what exactly is happening here :)

Pushed by dwalsh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c50fea61ff70 Toggle skipPausing when adding or enabling a breakpoint via reducers r=jlast
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Regressions: 1590594
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: