Closed Bug 1779243 Opened 3 months ago Closed 2 months ago

Remove all button should not remove non-visible breakpoints

Categories

(DevTools :: Debugger, task)

task

Tracking

(firefox104 fixed)

RESOLVED FIXED
104 Branch
Tracking Status
firefox104 --- fixed

People

(Reporter: jdescottes, Assigned: bomsy)

References

Details

Attachments

(1 file)

Was quickly discussed in the devtools team meeting, and while I know we want a way to destroy zombie breakpoints reliably for users, the current approach needs more UX work in my opinion.

Two things worth discussing.

1 - the breakpoints list normally shows only the breakpoints relevant for the current website. You would expect that a button in this UI has a similar scope. With the current implementation, if I click on "remove all breakpoints" while browsing mozilla.org, it will remove all the breakpoints created in my profile.

2- this action can not be recovered, so I really wonder if having a button with no confirm step is not dangerous. Users might really be unhappy if they start clicking on the UI and permanently lose their state. You could argue that this is similar to clear console, but console messages are not something explicitly created by the user, whereas breakpoints are. In a long debugging session, you might have a carefully crafted set of breakpoints. And losing it because of a misclick could be very upsetting.

I would suggest preffing off the feature, or make it Browser Toolbox only until those items are resolved.

Thanks for attaching a patch Bomsy! We are soon at the end of the release, is this ready for review?

Flags: needinfo?(hmanilla)

Yeah the patch is ready. I'm just trying to craft a valid test which clears a non-visual breakpoint, is abit complicated.

Flags: needinfo?(hmanilla)
Assignee: nobody → hmanilla
Attachment #9285572 - Attachment description: Bug 1779243 - [devtools] Remove pendingBreakpoint from the store based on source url → Bug 1779243 - [devtools] Remove pendingBreakpoint from the store based on source url r=jdescottes
Status: NEW → ASSIGNED

Hi Victoria!

Quick question about the feature added in FF104 to the debugger via Bug 1742774

The breakpoints panel now has a "recycle bin" icon in its header. Clicking it will delete all breakpoints created by the user. Current implementation is clearing really all breakpoints everywhere, this bug is about scoping that only to the breakpoints relevant to the current tab. This is already better, but I'm still a bit concerned about having a one-click destructive action without any confirmation. There is no way to undo/recover, so I'm worried about users who might mis-click and lose their breakpoints.

What do you think about the UX here? Should we add a confirm step? Should we only have the option as a context menu item? Or is it fine as is?
(for context, Chrome only provides this feature via a context menu item)

Thanks!

Flags: needinfo?(victoria)

Hi Julian, good to hear from you :D

What's the use case for deleting all breakpoints (instead of deactivating)? Like you're done debugging one issue and want to start debugging something else? Is this a use case that devs would commonly need to do multiple times a day?

Assuming it's not a very common need, it does make sense to show a confirmation if there's currently more than 1 break point. Kind of following the pattern of "quit firefox warning only if more than one tab." We could always have a "Don't warn again" checkbox in the confirmation alert if that's not too complicated.

Something that could also help: show the trash icon only when there's one or more break points so it's a little more clear what it does, and to simplify the default UI.

Flags: needinfo?(victoria)
Attachment #9285572 - Attachment description: Bug 1779243 - [devtools] Remove pendingBreakpoint from the store based on source url r=jdescottes → Bug 1779243 - [devtools] Revert clearing all pending breakpointsin the async store on removeall r=jdescottes
Attachment #9285572 - Attachment description: Bug 1779243 - [devtools] Revert clearing all pending breakpointsin the async store on removeall r=jdescottes → Bug 1779243 - [devtools] Revert clearing all pending breakpoints in the async store on removeall r=jdescottes
Pushed by hmanilla@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7eb42fa3d81e
[devtools] Revert clearing all pending breakpoints in the async store on removeall r=jdescottes
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 104 Branch

(In reply to Victoria Wang [:victoria] from comment #5)

Hi Julian, good to hear from you :D

Likewise :) And thanks for the quick feedback!

What's the use case for deleting all breakpoints (instead of deactivating)? Like you're done debugging one issue and want to start debugging something else? Is this a use case that devs would commonly need to do multiple times a day?

That shouldn't be very common. I think the main motivation to surface the feature is that we've been trying to fix an overall debugger issue nicknamed "zombie breakpoints" where breakpoints disappear from the UI but the debugger will still break on them. The "delete all" button is a way for users to recover from that state.

Assuming it's not a very common need, it does make sense to show a confirmation if there's currently more than 1 break point. Kind of following the pattern of "quit firefox warning only if more than one tab." We could always have a "Don't warn again" checkbox in the confirmation alert if that's not too complicated.

I agree, confirm + checkbox sounds good here

Something that could also help: show the trash icon only when there's one or more break points so it's a little more clear what it does, and to simplify the default UI.

That's a great point, either removing it or disabling it. Because today it's displayed and actionable when there are no breakpoints available so you can click on it but there is no user feedback. But if we want to keep this as the entry point to remove those "zombie breakpoints" then I guess we have a conflict. Since we can't trust our UI logic to know if there are zombie breakpoints, I guess we would need to show the action all the time... So maybe the "delete really all breakpoints even invisible ones" should be a separate action, a bit more hidden?

Thanks again for taking the time to look at this Victoria :)

Ah, sounds like if there's a zombie breakpoints issue, showing the delete button all the time makes sense, rather than having two different types of "delete all."

You need to log in before you can comment on or make changes to this bug.