Bug 192119 – Web Inspector: Elements: $0 is shown for all selected elements

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

[Devin Rousso]

Created attachment 355934 [details]
[Image] Screenshot of Issue

I see a few options:
1. Only show $0 for the most recently selected node
2. Make $0 into an array when more than one node is selected
3. Change all but the most recently selected node to be $$0 (or something to that effect) and create a $$0 available to the console that returns the array of all selected nodes

[Radar WebKit Bug Importer]

<rdar://problem/46327554>

[Matt Baker]

Created attachment 356270 [details]
Patch

[Matt Baker]

(In reply to Devin Rousso from comment #0)
> Created attachment 355934 [details]
> [Image] Screenshot of Issue
> 
> I see a few options:
> 1. Only show $0 for the most recently selected node
> 2. Make $0 into an array when more than one node is selected
> 3. Change all but the most recently selected node to be $$0 (or something to
> that effect) and create a $$0 available to the console that returns the
> array of all selected nodes

I went with 1, as it is the simplest.

[Joseph Pecoraro]

Comment on attachment 356270 [details]
Patch

r=me

[Joseph Pecoraro]

> > I see a few options:
> > 1. Only show $0 for the most recently selected node
> > 2. Make $0 into an array when more than one node is selected
> > 3. Change all but the most recently selected node to be $$0 (or something to
> > that effect) and create a $$0 available to the console that returns the
> > array of all selected nodes
> 
> I went with 1, as it is the simplest.

r=m assuming that `$0` really does resolve to the last selected element.

[Matt Baker]

Created attachment 356274 [details]
[Image] There can be only one

[Matt Baker]

(In reply to Matt Baker from comment #6)
> Created attachment 356274 [details]
> [Image] There can be only one

LOL, oops.

[Matt Baker]

Created attachment 356280 [details]
Patch

[Matt Baker]

Thanks for prodding Joe. While the inspected node often resolved to the last selected TreeElement, this wasn't always the case.

The updated patch corrects this, and makes some progress toward simplifying some of the more convoluted aspects of TreeOutline. Eventually I'd like to eliminate all of the TreeElement `on*` overloads, and have all TreeElement selection go through TreeOutline.

[Devin Rousso]

Comment on attachment 356280 [details]
Patch

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

I'd like to see my comments be addressed before I r+.  Looking good!

> Source/WebInspectorUI/ChangeLog:57
> [phone]  Matt Baker  <[e-mail]>

Double ChangeLog :(

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:462
> +            WI.domManager.highlightDOMNode(domNode.id);

This will error if `domNode` is null (meaning there is no selected element).  Only call this if `domNode` is valid.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:464
> +        this._domTreeOutline.updateSelection();

This name is pretty confusing considering all of the new `SelectionController` logic :/

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-630
> -        this.treeOutline.suppressRevealAndSelect = true;

Is this necessary?  It wasn't copied over.  If not, you should remove the getter/setter in `WI.DOMTreeOutline` as well.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-635
> -        this.treeOutline.suppressRevealAndSelect = false;

Ditto (<630)

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501
> +    select(omitFocus, selectedByUser, suppressNotification)

While you're at it, you should turn this into an optional object.

    select({omitFocus, selectedByUser, suppressNotification} = {})

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:527
> +    revealAndSelect(omitFocus, selectedByUser, suppressNotification)

Ditto (>501)

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:533
> +    deselect(suppressNotification)

Ditto (>501)

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:819
> +                this._previousSelectedTreeElement.listItemElement.classList.remove("last-selected");

Perhaps now would be a good time to add something similar to `WI.GeneralTreeElement.prototype.addClassName`.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:821
> +                selectedTreeElement.listItemElement.classList.add("last-selected");

Ditto (>819)

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:823
> +            this._previousSelectedTreeElement = selectedTreeElement;

Style: our usual "pattern" is to assign, and then do something to the new value if present:

    if (this._previousSelectedTreeElement)
        this._previousSelectedTreeElement.listItemElement.classList.remove("last-selected");

    this._previousSelectedTreeElement = selectedTreeElement;

    if (this._previousSelectedTreeElement)
        this._previousSelectedTreeElement.listItemElement.classList.add("last-selected");

[Matt Baker]

Comment on attachment 356280 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:464
>> +        this._domTreeOutline.updateSelection();
> 
> This name is pretty confusing considering all of the new `SelectionController` logic :/

I'll change it to `updateSelectionArea()`, which matches the DOMTreeElement method.

>> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501
>> +    select(omitFocus, selectedByUser, suppressNotification)
> 
> While you're at it, you should turn this into an optional object.
> 
>     select({omitFocus, selectedByUser, suppressNotification} = {})

I'm not a huge fan of this style, and I'd rather not add any unnecessary churn to this patch.

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:819
>> +                this._previousSelectedTreeElement.listItemElement.classList.remove("last-selected");
> 
> Perhaps now would be a good time to add something similar to `WI.GeneralTreeElement.prototype.addClassName`.

I suppose the addClassName/removeClassName methods in GeneralTreeElement could be pushed into TreElement, but I'd rather not change that here.

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:823
>> +            this._previousSelectedTreeElement = selectedTreeElement;
> 
> Style: our usual "pattern" is to assign, and then do something to the new value if present:
> 
>     if (this._previousSelectedTreeElement)
>         this._previousSelectedTreeElement.listItemElement.classList.remove("last-selected");
> 
>     this._previousSelectedTreeElement = selectedTreeElement;
> 
>     if (this._previousSelectedTreeElement)
>         this._previousSelectedTreeElement.listItemElement.classList.add("last-selected");

Yeah this reads better.

[Matt Baker]

Created attachment 356401 [details]
Patch

[Devin Rousso]

Comment on attachment 356401 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:465
> +        if (domNode && selectedByUser)
> +            WI.domManager.highlightDOMNode(domNode.id);

If `domNode` is falsy, we should hide the highlight by calling `WI.domManager.hideDOMNodeHighlight();`.

Side note: I just noticed, but I think this is broken: <https://webkit.org/b/192354>

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:468
> +        this._domTreeOutline.suppressRevealAndSelect = false;
> +        this._domTreeOutline.updateSelectionArea();

Not sure that it matters, but this is reversed from what was there before.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-628
> -    onselect(treeElement, selectedByUser)

My only concern with something like this is the order of operations, in that `onselect` was guaranteed to happen before event dispatch, but considering that this doesn't change the selection, I think it's fine.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:81
> +.tree-outline.dom li.last-selected > span::after {

The rule after this (`.tree-outline.dom:focus li.selected > span::after`) should probably also be changed to `.last-selected`.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501
> -    select(omitFocus, selectedByUser, suppressOnSelect, suppressOnDeselect)
> +    select(omitFocus, selectedByUser, suppressNotification)

I really would prefer that you change this to an optional object in this patch.  You're already changing almost all of the callers due to the argument removal, so it's not crazy to change them to this pattern at the same time.  Using a list of flags is really not very readable or future-proof, as evident in much of this patch.  I had to keep going back/forth multiple times to remember what each argument meant in all of the callers of `select`, `revealAndSelect`, and `deselect` (even worse for callers that just pass `true` or `false`).  Using an optional object makes it clear at the call-site what each argument means, and makes it so that the callers only have to provide values for things they want "on".

[Matt Baker]

Comment on attachment 356401 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:468
>> +        this._domTreeOutline.updateSelectionArea();
> 
> Not sure that it matters, but this is reversed from what was there before.

it shouldn't matter, but I'll change it to match the original.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-628
>> -    onselect(treeElement, selectedByUser)
> 
> My only concern with something like this is the order of operations, in that `onselect` was guaranteed to happen before event dispatch, but considering that this doesn't change the selection, I think it's fine.

True, but in practice there is only only one event handler for the DOMTreeContentView's SelectionDidChange event.

[Matt Baker]

Comment on attachment 356401 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:465
>> +            WI.domManager.highlightDOMNode(domNode.id);
> 
> If `domNode` is falsy, we should hide the highlight by calling `WI.domManager.hideDOMNodeHighlight();`.
> 
> Side note: I just noticed, but I think this is broken: <https://webkit.org/b/192354>

This matches the original. The highlight is hidden on mouseout, dragstart, and when the tree is hidden.

[Matt Baker]

Comment on attachment 356401 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501
>> +    select(omitFocus, selectedByUser, suppressNotification)
> 
> I really would prefer that you change this to an optional object in this patch.  You're already changing almost all of the callers due to the argument removal, so it's not crazy to change them to this pattern at the same time.  Using a list of flags is really not very readable or future-proof, as evident in much of this patch.  I had to keep going back/forth multiple times to remember what each argument meant in all of the callers of `select`, `revealAndSelect`, and `deselect` (even worse for callers that just pass `true` or `false`).  Using an optional object makes it clear at the call-site what each argument means, and makes it so that the callers only have to provide values for things they want "on".

Refactoring this would mean touching DebuggerSidebarPanel, OpenResourceDialog, ThreadTreeElemen, ContentBrowserTabContentView, FolderizedTreeElement, and at least four other files. I'd rather not do that as part of this patch.

[Matt Baker]

(In reply to Matt Baker from comment #16)
> Comment on attachment 356401 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356401&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:501
> >> +    select(omitFocus, selectedByUser, suppressNotification)
> > 
> > I really would prefer that you change this to an optional object in this patch.  You're already changing almost all of the callers due to the argument removal, so it's not crazy to change them to this pattern at the same time.  Using a list of flags is really not very readable or future-proof, as evident in much of this patch.  I had to keep going back/forth multiple times to remember what each argument meant in all of the callers of `select`, `revealAndSelect`, and `deselect` (even worse for callers that just pass `true` or `false`).  Using an optional object makes it clear at the call-site what each argument means, and makes it so that the callers only have to provide values for things they want "on".
> 
> Refactoring this would mean touching DebuggerSidebarPanel,
> OpenResourceDialog, ThreadTreeElemen, ContentBrowserTabContentView,
> FolderizedTreeElement, and at least four other files. I'd rather not do that
> as part of this patch.

I guess we're modifying most of these anyway. Still, I'd prefer not to refactor old TreeOutline code at this time.

[Matt Baker]

Created attachment 356476 [details]
Patch

[EWS Watchlist]

Comment on attachment 356476 [details]
Patch

Attachment 356476 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10263159

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-video-only-dataavailable.html

[EWS Watchlist]

Created attachment 356495 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Matt Baker]

Created attachment 356501 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 356501 [details]
Patch for landing

Clearing flags on attachment: 356501

Committed r238859: <https://trac.webkit.org/changeset/238859>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.