895272 - don't show the list of breakpoints for black boxed sources

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

follow up to bug 877686 comment 8:

> Also, once you start adding breakpoints, things start to look a bit funky IMHO
> with a L-shaped blue selection. Fixable in a followup, but I'd like that to get
> figured out.

Comment 1

[Victor Porof [:vporof][:vp]]

From an IRC discussion with Nick:

The current implementation and UI isn't quite obvious about what exactly blackboxing does.

1. No breakpoints in blackboxed sources will be hit, even though the frontend displays all such breakpoints as "enabled".
2. This is in contradiction with the current checkbox placement (the "eye"), which is in the middle of a source+breakpoints block in the sources widget.
3. If the user specifically added a breakpoint in a blackboxed source, then it's probably reasonable to expect it to be hit if not otherwise hinted in the UI.
4. Having a million eyes staring down at you from the sources list is at least confusing, if not frightening :)

So I have a few suggestions that would make this stuff, I hope, more ergonomic:

* No special handling of breakpoints in the case of blackboxed sources should happen on the serverside. Even if a source is blackboxed, user-added breakpoints need to hit. Only stepping behavior is changed.
* When you blackbox a source, the frontend also disables all the breakpoints for that sepcific source, effectively emulating the current behavior.
* The eye is moved near the source label.

Additionally, there are two routes we can go for making the eye icons less distracting:

either
* Make the eyes smaller in size
or
* Add an off-by-default eyes visibility toggle somewhere in the UI (discussed options are toolbars, context menus, gear menus).

I would vote the former suggestion about making the eyes smaller. Other approaches could camuflate this functionality and reduce visibility of a great feature.

Comment 2

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #1)
> From an IRC discussion with Nick:
> 
> The current implementation and UI isn't quite obvious about what exactly
> blackboxing does.
> 
> 1. No breakpoints in blackboxed sources will be hit, even though the
> frontend displays all such breakpoints as "enabled".

How about dimming them a bit ? Like opacity: 0.5

> 2. This is in contradiction with the current checkbox placement (the "eye"),
> which is in the middle of a source+breakpoints block in the sources widget.

Why not place the eye only to the left of the source name, such that the eye and the breakpoint checkboxes are more or less vertically in one line, and do not form a T

> 3. If the user specifically added a breakpoint in a blackboxed source, then
> it's probably reasonable to expect it to be hit if not otherwise hinted in
> the UI.
> 4. Having a million eyes staring down at you from the sources list is at
> least confusing, if not frightening :)
> 
> So I have a few suggestions that would make this stuff, I hope, more
> ergonomic:
> 
> * No special handling of breakpoints in the case of blackboxed sources
> should happen on the serverside. Even if a source is blackboxed, user-added
> breakpoints need to hit. Only stepping behavior is changed.

Once the breakpoint is hit , what happens if the user clicks step-in/over ?
If the breakpoint got hit, the user might actually want to step over a few lines to see what the source is doing (or whatever for the reason he added the breakpoint). Now in these cases, do we blackbox the next lines and jump to the next unblackboxed source, or should we unblackbox the source ? If we unblackbox, then that means that the source with breakpoints cannot be blackboxed.

> * When you blackbox a source, the frontend also disables all the breakpoints
> for that sepcific source, effectively emulating the current behavior.

Is this option a choice b/w this one and the above one ? because both behaviors cannot exist together.

> * The eye is moved near the source label.
Yup, like I suggested above.
> 
> Additionally, there are two routes we can go for making the eye icons less
> distracting:
> 
> either
> * Make the eyes smaller in size
> or
> * Add an off-by-default eyes visibility toggle somewhere in the UI
> (discussed options are toolbars, context menus, gear menus).
> 
> I would vote the former suggestion about making the eyes smaller. Other
> approaches could camuflate this functionality and reduce visibility of a
> great feature.

Can't we do any other icon that an eye ? an Eye icon usually symbolizes the visibility of things, like the eye icon in Photoshop's layers, or in Style Editor where is means that this style will not have any effect, or in other words, invisible.

We can need some UX help ..

Stephen, what say ?

Comment 3

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #2)
> 
> How about dimming them a bit ? Like opacity: 0.5
> 

No, they should just be unchecked (basically disabling all breakpoints for that specific source).

> Why not place the eye only to the left of the source name, such that the eye
> and the breakpoint checkboxes are more or less vertically in one line
> 

I literally suggested this a few lines below :)
 
> Once the breakpoint is hit , what happens if the user clicks step-in/over ?

I wouldn't like the idea of special casing such things. I think we should follow the general rules and step out of the source. If the behavior is undesired, then the source can be simply unblackboxed.

> 
> Is this option a choice b/w this one and the above one ? because both
> behaviors cannot exist together.
> 

Yes they can. Breakpoints are disabled. Users can re-enable them.

Comment 4

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #3)
> (In reply to Girish Sharma [:Optimizer] from comment #2)
> > 
> > How about dimming them a bit ? Like opacity: 0.5
> > 
> 
> No, they should just be unchecked (basically disabling all breakpoints for
> that specific source).
> 
> > Why not place the eye only to the left of the source name, such that the eye
> > and the breakpoint checkboxes are more or less vertically in one line
> > 
> 
> I literally suggested this a few lines below :)

Yeah, did not read it before I typed :)

> > Once the breakpoint is hit , what happens if the user clicks step-in/over ?
> 
> I wouldn't like the idea of special casing such things. I think we should
> follow the general rules and step out of the source. If the behavior is
> undesired, then the source can be simply unblackboxed.

I think skipping the source might render the purpose of stopping at that breakpoint useless since in most of the cases the use case is not to just stop at the breakpoint, do some stuff without moving, and then resume. Let's have some more opinion on this and if we agree on unblackboxing the source if it has checkboxes, then the below case would not be possible
 
> > Is this option a choice b/w this one and the above one ? because both
> > behaviors cannot exist together.
> > 
> 
> Yes they can. Breakpoints are disabled. Users can re-enable them.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

After talking with victor some more, we are going to just hide the list of breakpoints for black boxed sources.

Comment 6

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> After talking with victor some more, we are going to just hide the list of
> breakpoints for black boxed sources.

Don't forget about also not showing the source editor and friends.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Victor Porof [:vp] from comment #6)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> > After talking with victor some more, we are going to just hide the list of
> > breakpoints for black boxed sources.
> 
> Don't forget about also not showing the source editor and friends.

bug 895543

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-895272-dont-show-black-boxed-breakpoints.patch

Comment 9

[Victor Porof [:vporof][:vp]]

Comment on attachment 782180 [details] [diff] [review]
bug-895272-dont-show-black-boxed-breakpoints.patch

Review of attachment 782180 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/linux/devtools/debugger.css
@@ +49,5 @@
>  .side-menu-widget-item-checkbox[checked] > .checkbox-check {
>    background-position: 0 0;
>  }
>  
> +.side-menu-widget-item-checkbox:not([checked]) ~ .side-menu-widget-item-contents > .dbg-breakpoint {

This could prove to be a bit fragile in the long run, but for now it's gorgeous.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/fx-team/rev/ad20fa115118

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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