Bug 199332 – Web Inspector: DOM Debugger: descendant breakpoints should be able to be enabled/disabled/deleted from a collapsed parent

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

# STEPS TO REPRODUCE:
1. inspect any page
2. set a DOM breakpoint on a child of some node
3. collapse the parent node of the child node from step 2 in the DOM tree
4. right-click on the breakpoint icon in the "gutter" of the DOM tree
 => "Enabled Breakpoints"/"Disable Breakpoints" and "Delete Breakpoints" missing

NOTE: if you set a breakpoint on the parent node (from step 3) itself, then those menu items appear, but then the "Reveal Breakpoint" item disappears.

It should also be possible to click to toggle _all_ child breakpoints on/off.

[Devin Rousso]

(In reply to Devin Rousso from comment #0)
> It should also be possible to click to toggle _all_ child breakpoints on/off.
Actually, maybe not, as the current UI is a "hollow" breakpoint, which just indicates that there are children breakpoints, not their state.

[Matt Baker]

(In reply to Devin Rousso from comment #1)
> (In reply to Devin Rousso from comment #0)
> > It should also be possible to click to toggle _all_ child breakpoints on/off.
> Actually, maybe not, as the current UI is a "hollow" breakpoint, which just
> indicates that there are children breakpoints, not their state.

You can also use "Reveal Breakpoint" from the hollow indicator's context menu to expand the DOM tree to reveal the DOM having the breakpoint.

This works decently for simple cases, but doesn't scale well to numerous descendant breakpoints.

Also, when the collapsed DOM node itself has a breakpoint, there is no indication when one of its descendants has a breakpoint.

[Matt Baker]

(In reply to Matt Baker from comment #2)
> (In reply to Devin Rousso from comment #1)
> > (In reply to Devin Rousso from comment #0)
> > > It should also be possible to click to toggle _all_ child breakpoints on/off.
> > Actually, maybe not, as the current UI is a "hollow" breakpoint, which just
> > indicates that there are children breakpoints, not their state.
> 
> You can also use "Reveal Breakpoint" from the hollow indicator's context
> menu to expand the DOM tree to reveal the DOM having the breakpoint.
> 
> This works decently for simple cases, but doesn't scale well to numerous
> descendant breakpoints.
> 
> Also, when the collapsed DOM node itself has a breakpoint, there is no
> indication when one of its descendants has a breakpoint.

There is really only so much state you can reasonably show in the gutter.

[Devin Rousso]

Created attachment 373161 [details]
Patch

[Matt Baker]

Comment on attachment 373161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373161&action=review

r-, only because I'm concerned about selecting the descendant DOM nodes when revealing breakpoints. Other than that, this is awesome!

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:408
> +localizedStrings["Enable Descendant Breakpoints"] = "Enable Descendant Breakpoints";

IMO, having "Descendant" in the context menu items makes the menu appear very busy/cluttered. It's much more clear though, so I think it is a worthwhile tradeoff.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:150
> +                resolvedBreakpoints = resolvedBreakpoints.concat(Array.from(domBreakpointNodeIdentifierMap.values()));

Nice cleanup.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:179
> +        return breakpoints ? Array.from(breakpoints) : [];

Ditto!

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:189
> +            let children = node.children.slice();

Why not `let children = Array.from(node.children)`?

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:454
> +                    console.assert(breakpoint.domNodeIdentifier === nodeIdentifier);

Nice catch! Did you encounter a situation where a DOM breakpoint was resolved more than once?

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:121
> +            this.treeOutline.selectTreeElements(subtreeBreakpointTreeElements);

I don't think we want to select nodes in the DOM tree for the revealed breakpoints. It could unexpectedly change an existing selection. What about extending the animated highlight to all the revealed tree elements?

[Devin Rousso]

Comment on attachment 373161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373161&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:189
>> +            let children = node.children.slice();
> 
> Why not `let children = Array.from(node.children)`?

No real reason.  I'll change it.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:454
>> +                    console.assert(breakpoint.domNodeIdentifier === nodeIdentifier);
> 
> Nice catch! Did you encounter a situation where a DOM breakpoint was resolved more than once?

Yeah, if a DOM breakpoint is set and then the page is reloaded, and the node is both added almost immediately after load and is shallow enough in the DOM tree to be requested by default, it would do both of these paths at the same time.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:121
>> +            this.treeOutline.selectTreeElements(subtreeBreakpointTreeElements);
> 
> I don't think we want to select nodes in the DOM tree for the revealed breakpoints. It could unexpectedly change an existing selection. What about extending the animated highlight to all the revealed tree elements?

I'm fine with not selecting them honestly.  In point of fact, the context menu item just says "Reveal" anyways, and they're obvious enough from the breakpoint arrow in the left "gutter".

[Devin Rousso]

Created attachment 373308 [details]
Patch

[Matt Baker]

Comment on attachment 373308 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 373308 [details]
Patch

Clearing flags on attachment: 373308

Committed r247053: <https://trac.webkit.org/changeset/247053>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52526953>