Bug 199332 - Web Inspector: DOM Debugger: descendant breakpoints should be able to be enabled/disabled/deleted from a collapsed parent
Summary: Web Inspector: DOM Debugger: descendant breakpoints should be able to be enab...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-28 11:58 PDT by Devin Rousso
Modified: 2019-07-02 09:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (27.28 KB, patch)
2019-06-28 16:26 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (27.80 KB, patch)
2019-07-02 00:09 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-06-28 11:58:42 PDT
# 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.
Comment 1 Devin Rousso 2019-06-28 11:59:26 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.
Comment 2 Matt Baker 2019-06-28 14:57:07 PDT
(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.
Comment 3 Matt Baker 2019-06-28 14:57:42 PDT
(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.
Comment 4 Devin Rousso 2019-06-28 16:26:23 PDT
Created attachment 373161 [details]
Patch
Comment 5 Matt Baker 2019-07-01 20:32:56 PDT
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 6 Devin Rousso 2019-07-02 00:07:11 PDT
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".
Comment 7 Devin Rousso 2019-07-02 00:09:18 PDT
Created attachment 373308 [details]
Patch
Comment 8 Matt Baker 2019-07-02 08:45:26 PDT
Comment on attachment 373308 [details]
Patch

r=me
Comment 9 WebKit Commit Bot 2019-07-02 09:16:01 PDT
Comment on attachment 373308 [details]
Patch

Clearing flags on attachment: 373308

Committed r247053: <https://trac.webkit.org/changeset/247053>
Comment 10 WebKit Commit Bot 2019-07-02 09:16:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-07-02 09:17:38 PDT
<rdar://problem/52526953>