Summary: | Web Inspector: DOM Debugger: descendant breakpoints should be able to be enabled/disabled/deleted from a collapsed parent | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, mattbaker, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Devin Rousso
2019-06-28 11:58:42 PDT
(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. |