Bug 166000 – Web Inspector: Debugger sidebar panel should not have multiple tree selections

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

[Matt Baker]

Created attachment 297414 [details]
[Image] two selected elements

Summary:
Debugger sidebar panel should not have multiple tree selections.

Steps to Reproduce:
1. Goto daringfireball.net
2. Open Debugger tab
3. Select any special breakpoint (e.g. All Exceptions)
4. Reload inspected page
  => Breakpoint and main resource tree elements are both selected (see screenshot)

Notes:
NavigationSidebarPanel tries to sync selection between content tree outlines, but relies on selection events which can be suppressed. Tree element selections made at startup (cookie restore, etc) don't trigger events.

[Matt Baker]

Created attachment 297415 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/29721988>

[Blaze Burg]

Comment on attachment 297415 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:10
> +        (logical) tree outline, with one selected element permitted at a time.

We may want some subtrees or elements to be multiple selected in the future. For example, call frames.

[Matt Baker]

Comment on attachment 297415 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:10
>> +        (logical) tree outline, with one selected element permitted at a time.
> 
> We may want some subtrees or elements to be multiple selected in the future. For example, call frames.

When the day comes, it should be simple to add logic for TreeOutline.prototype.allowsMultipleSelection (or its ilk).

[Blaze Burg]

Comment on attachment 297415 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:140
> +        this._visibleContentTreeOutlines.add(contentTreeOutline);

Should this be handled by the group as well, now that this class does not directly manage .hidden?

> Source/WebInspectorUI/UserInterface/Views/TreeOutlineGroup.js:97
> +    _wrapTreeOutline(treeOutline)

This wrapping business makes me nervous and we don't really do stuff like this as far as I know.. Why can't we just make TreeOutline/TreeElement emit selection events, or keep an up-pointer to its containing group and send delegate messages?

[Matt Baker]

Comment on attachment 297415 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutlineGroup.js:97
>> +    _wrapTreeOutline(treeOutline)
> 
> This wrapping business makes me nervous and we don't really do stuff like this as far as I know.. Why can't we just make TreeOutline/TreeElement emit selection events, or keep an up-pointer to its containing group and send delegate messages?

Initially I'd started with a delegate approach because of similar reservations about wrapping, but thought this was less intrusive. Will rework this.

[Matt Baker]

Comment on attachment 297415 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:140
>> +        this._visibleContentTreeOutlines.add(contentTreeOutline);
> 
> Should this be handled by the group as well, now that this class does not directly manage .hidden?

Yep, _visibleContentTreeOutlines is no longer needed.

[Matt Baker]

Created attachment 297669 [details]
Patch

[Timothy Hatcher]

Comment on attachment 297669 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:14
> +        This adds a new class, TreeOutlineGroup, which restricts tree element selection
> +        inside a group of tree outlines by receiving messages directly from TreeElement.

Nice design.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:516
> +        let treeOutlineGroup = WebInspector.TreeOutlineGroup.groupForTreeOutline(treeOutline);
> +        if (!treeOutlineGroup)
> +            return;
> +
> +        treeOutlineGroup.didSelectTreeElement(this);

This might just be better with a getter / setter on TreeOutline.

> Source/WebInspectorUI/UserInterface/Views/TreeOutlineGroup.js:66
> +        delete treeOutline[WebInspector.TreeOutlineGroup.GroupForTreeOutlineSymbol];

Should be = null;

[Blaze Burg]

What's up with this patch? Can we rebase and land it?

[Matt Baker]

Created attachment 301227 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 301227 [details]
Patch for landing

Clearing flags on attachment: 301227

Committed r212171: <http://trac.webkit.org/changeset/212171>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.