# 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.
(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.
(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.
(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.
Created attachment 373161 [details] Patch
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?
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".
Created attachment 373308 [details] Patch
Comment on attachment 373308 [details] Patch r=me
Comment on attachment 373308 [details] Patch Clearing flags on attachment: 373308 Committed r247053: <https://trac.webkit.org/changeset/247053>
All reviewed patches have been landed. Closing bug.
<rdar://problem/52526953>