Bug 234615 – Web Inspector: Sources: cannot copy grouping of blackboxed call frames

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

# STEPS TO REPRODUCE
1. inspect <http://bogojoker.com/shell/>
2. create a "click" global Event Breakpoint
3. blackbox the <http://bogojoker.com/shell/js/jquery.js> script
4. click on the down arrow in the page
-> should pause on <http://bogojoker.com/shell/js/easySlider.min.js:64> with two blackboxed call frames
5. click on the first call frame in the Call Stack section of the navigation sidebar
6. press ⌘A to select all call frames
7. press ⌘C to copy

## EXPECTED
```
(anonymous function) (easySlider.min.js:56)
handle (jquery.js:1358)
(anonymous function) (jquery.js:1215)
--- addEventListener ---
(anonymous function) (jquery.js:1227)
each (jquery.js:449)
add (jquery.js:1218)
(anonymous function) (jquery.js:1470)
each (jquery.js:449)
each (jquery.js:67)
bind (jquery.js:1469)
(anonymous function) (jquery.js:1588)
(anonymous function) (easySlider.min.js:55)
each (jquery.js:449)
each (jquery.js:67)
(anonymous function) (easySlider.min.js:20)
(anonymous function) (shell:28)
(anonymous function) (jquery.js:1515)
(anonymous function) (jquery.js:1528)
each (jquery.js:449)
ready (jquery.js:1527)
--- addEventListener ---
bindReady (jquery.js:1542)
ready (jquery.js:1510)
Global Code (facebox.js:342)
```

## ACTUAL
```
(anonymous function) (easySlider.min.js:56)
--- addEventListener ---
(anonymous function) (easySlider.min.js:55)
(anonymous function) (easySlider.min.js:20)
(anonymous function) (shell:28)
--- addEventListener ---
Global Code (facebox.js:342)
```

[Radar WebKit Bug Importer]

<rdar://problem/86989248>

[Devin Rousso]

Created attachment 448357 [details]
Patch

The only downside I can think of as a result of this is that up/down will now traverse through blackboxed call frame groups, meaning that it's an extra step (or more) to select the previous/next call frame using the arrow keys when there's a blackboxed call frame group in the middle.  I don't think this is that big of a deal though given that this interaction requires focus to be in the Call Stack section and can be easily avoided using the mouse (which I'd bet is more commonly used than the arrow keys).

We could make it so that selecting a blackboxed call frame group tree element will jump to the location of the topmost blackboxed call frame, but that maybe could be a bit confusing since we don't actually show that call frame in the Call Stack section.  Either way, this can be a followup.

[Patrick Angle]

Comment on attachment 448357 [details]
Patch

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

r=me

> The only downside I can think of as a result of this is that up/down will now traverse
> through blackboxed call frame groups, meaning that it's an extra step (or more) to
> select the previous/next call frame using the arrow keys when there's a blackboxed
> call frame group in the middle.

I tried this patch locally, and this doesn't appear to be a problem. I can arrow through the list of call frames as I would expect to be able to do, where each press of the down key moves me down to the next visible call frame, even if there are multiple blackboxes call frames represented by the currently selected frame.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:316
> +        if (treeElement.toggleOnClick || treeElement.isEventWithinDisclosureTriangle(event)) {

Aside: It's kinda strange to me that several subclasses of TreeElement have a toggleOnClick member set in their constructor, but TreeElement itself doesn't... I wonder if it makes more sense for there to be a public `get toggleOnClick()` in TreeElement that returns `false`, and is override by subclasses that want to return `true`.

[Devin Rousso]

Comment on attachment 448357 [details]
Patch

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

Thanks for the review!

> > The only downside I can think of as a result of this is that up/down will now traverse through blackboxed call frame groups, meaning that it's an extra step (or more) to select the previous/next call frame using the arrow keys when there's a blackboxed call frame group in the middle.
> 
> I tried this patch locally, and this doesn't appear to be a problem. I can arrow through the list of call frames as I would expect to be able to do, where each press of the down key moves me down to the next visible call frame, even if there are multiple blackboxes call frames represented by the currently selected frame.

Hmm, so when you press ↓ and the selection shifts to a blackboxed call frame group tree element, what happens?  IIRC I didn't see any change in the currently shown content view (i.e. just like as if a folder is selected in the resources tree in the navigation sidebar of the Sources Tab).

I think I may have been confusing by saying "traverse through".  What I meant was that before this patch, pressing ↓ would "skip" over the blackboxed call frame group tree element (i.e. it would never be selected).  Now with this patch, pressing ↓ will select the blackboxed call frame group tree element, but because it doesn't have a `representedObject` (and we don't have special handling for it in `WI.SourcesNavigationSidebarPanel.prototype._handleTreeSelectionDidChange`) nothing will change in the currently shown content view, meaning that the developer would have to press ↓ _again_ in order to get the functionality that they had before this change (which would be to show the source code location of the call frame before the blackboxed call frame group).

>> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:316
>> +        if (treeElement.toggleOnClick || treeElement.isEventWithinDisclosureTriangle(event)) {
> 
> Aside: It's kinda strange to me that several subclasses of TreeElement have a toggleOnClick member set in their constructor, but TreeElement itself doesn't... I wonder if it makes more sense for there to be a public `get toggleOnClick()` in TreeElement that returns `false`, and is override by subclasses that want to return `true`.

Agreed lol.  This is _old_ code.  FWIW I actually tried to do a basic refactoring of things like this on `WI.TreeOutline` and `WI.TreeElement` a few years ago, but it caused all manner of havoc so I haven't tried again since 😅  This would probably be much easier to do though given how rare it is used.

[Patrick Angle]

(In reply to Devin Rousso from comment #4)
> > > The only downside I can think of as a result of this is that up/down will now traverse through blackboxed call frame groups, meaning that it's an extra step (or more) to select the previous/next call frame using the arrow keys when there's a blackboxed call frame group in the middle.
> > 
> > I tried this patch locally, and this doesn't appear to be a problem. I can arrow through the list of call frames as I would expect to be able to do, where each press of the down key moves me down to the next visible call frame, even if there are multiple blackboxes call frames represented by the currently selected frame.
> 
> Hmm, so when you press ↓ and the selection shifts to a blackboxed call frame
> group tree element, what happens?  IIRC I didn't see any change in the
> currently shown content view (i.e. just like as if a folder is selected in
> the resources tree in the navigation sidebar of the Sources Tab).
> 
> I think I may have been confusing by saying "traverse through".  What I
> meant was that before this patch, pressing ↓ would "skip" over the
> blackboxed call frame group tree element (i.e. it would never be selected). 
> Now with this patch, pressing ↓ will select the blackboxed call frame group
> tree element, but because it doesn't have a `representedObject` (and we
> don't have special handling for it in
> `WI.SourcesNavigationSidebarPanel.prototype._handleTreeSelectionDidChange`)
> nothing will change in the currently shown content view, meaning that the
> developer would have to press ↓ _again_ in order to get the functionality
> that they had before this change (which would be to show the source code
> location of the call frame before the blackboxed call frame group).

Ah, yes it behaves as a collapsed folder now, which conceptually makes sense to me. I thought you were saying you would have to down-arrow through a bunch of invisible call frames, which is what prompted me to apply the patch locally and play around some. This makes the UI more accessible from a keyboard IMO since now it is possible to key to the blackboxed group and reveal the frames via the keyboard.

[EWS]

Committed r288030 (246056@main): <https://commits.webkit.org/246056@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448357 [details].