Bug 193089 – Web Inspector: Elements tab should toggle visibility for all selected nodes

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Summary:
Elements tab should toggle visibility for all selected nodes.

DOMTreeElement provides toggleElementVisibility(), which does just that. If multiple DOMTreeElements are selected, and the selection contains both visible and hidden nodes, we can either:

1) Toggle Visibility (current behavior)
2) Hide Elements (if at least one node is visible), otherwise Show Elements

The latter is the approach taken by Xcode when multiple breakpoints are selected, and the selection contains both enabled and disabled breakpoints. I like this approach, and it can be easily applied to breakpoints when/if we allow multiple selection in that TreeOutline.

[Radar WebKit Bug Importer]

<rdar://problem/47009256>

[Matt Baker]

When only one node is selected, the context menu should continue to show the "Toggle Visibility" item.

[Matt Baker]

Created attachment 358241 [details]
Patch

[Devin Rousso]

Comment on attachment 358241 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:58
> +        this._hidden = false;

Rather than trying to keep this on the view object, we can be smarter about it by basing our `isElementHidden` off of the `WI.DOMNode`’s list of classes. That way, if the WebInspector hidden class is removed by the user via the DOM tree, we don’t have the wrong value. This would also mean that you could move the “toggle hidden” code to `WI.DOMNode`.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:321
> +            this.classList.toggle(hideElementStyleSheetIdOrClassName, forceHidden);

NIT: you could drop the `contains` call as `toggle` returns true/false just like `contains`

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:332
> +                result.release();

We should always be releasing, even in the case that it’s not a Boolean.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:745
> +            contextMenu.appendItem(label, () => { this.treeOutline.toggleSelectedElementsVisibility(); });

Style: I prefer putting these on separate lines

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:747
> +            contextMenu.appendItem(WI.UIString("Toggle Visibility"), this.toggleElementVisibility.bind(this));

Style: arrow functions are nicer to read than a bind IMO

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:205
> +        const forceHidden = !allHidden;

NIT: this shouldn’t be `const`

[EWS Watchlist]

Comment on attachment 358241 [details]
Patch

Attachment 358241 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10613083

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html

[EWS Watchlist]

Created attachment 358246 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Devin Rousso]

Comment on attachment 358241 [details]
Patch

r- for now, as I'm pretty sure we can make a version of this that isn't able to get out of sync with the actual DOM tree.  If you can prove otherwise, I'll r+ (after the other changes).

[Matt Baker]

I'm preparing an updated version of this patch, and thought I'd add some clarifying notes:

----

In the Elements tab there are two ways to hide/unhide a DOM element: 

    1) Right-clicking an element and choosing "Toggle Element"
    2) Hitting 'H' in the DOM tree to toggle the selected element.

Both do the same thing. Multiple selection complicates things, since both shown and hidden elements can be selected simultaneously. Pressing 'H' in the DOM tree could either:

    1) Toggle each node, showing hidden nodes and hiding visible nodes
    2) Show all nodes
    3) Hide all nodes

Option 1 feels like a path to madness, making it a toss up betweem 2 and 3. Xcode shows "Disable Breakpoints" when right-clicking multiple selected breakpoints where at least one is enabled. Let's go with option 3, as it's philosophically similar to the behavior in Xcode, and eventually we'll want to support multiple selection in Breakpoints too.

What was once a simple "Toggle Visibility" command (whether via the context menu or keyboard shortcut) is now contextual: if a mix of shown and hidden elements are selected, the command becomes "hide all", otherwise it remains a toggle. To complicate matters further, the context menu item shown when right-clicking needs to reflect the action that will be carried out:

    1) If a single element is selected, or the right-clicked (target) element isn't selected, show "Toggle Visibility".
    2) If the target belongs to a multiple selection containing both shown/hidden elements, display "Hide Elements"
    3) If the target belongs to a multiple selection of all hidden elements, display "Show Elements"
    4) If the target belongs to a multiple selection of all shown elements, display "Hide Elements"

Cases 1, 3, and 4 above all result in a familiar "toggle elements" operation. Only 2 is new behavior in that it forces elements to be hidden.

[Matt Baker]

Created attachment 360402 [details]
Patch

[Matt Baker]

Created attachment 360404 [details]
Patch

[Devin Rousso]

Comment on attachment 360404 [details]
Patch

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

r=me, much nicer than the last version :)

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:295
> +    toggleElementVisibility(forceHidden)

If only we could share more of this code with `DOMNode.prototype.toggleClass` :(

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:727
> +        let selectedTreeElements = this.treeOutline ? this.treeOutline.selectedTreeElements : [];

I'd rather you inline this in the `if` than construct a temporary array.

    if (this.selected && this.treeOutline && this.treeOutline.selectedTreeElements.length > 1)

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:201
> +        if (!selectedTreeElements.length)

NIT: I find these types of checks unnecessary, as we'd effectively return inside the loop anyways (iterating over an array of no length has the same effect as returning early, albeit the latter is "maybe" quicker).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:554
> +        let allHidden = this.selectedTreeElements.every((treeElement) => treeElement.isNodeHidden);

This should either match the name of the parameter (e.g. `forceHidden`), or be inlined.  Since the function is defined on this class, it's much easier for a developer to be able to see this quickly than if it were somewhere else.

[Matt Baker]

Comment on attachment 360404 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:554
>> +        let allHidden = this.selectedTreeElements.every((treeElement) => treeElement.isNodeHidden);
> 
> This should either match the name of the parameter (e.g. `forceHidden`), or be inlined.  Since the function is defined on this class, it's much easier for a developer to be able to see this quickly than if it were somewhere else.

Will change to:

let forceHidden = !this.selectedTreeElements.every((treeElement) => treeElement.isNodeHidden);

I'll update the similar code in DOMTreeElement.prototype._populateTagContextMenu.

[Matt Baker]

Created attachment 360426 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 360426 [details]
Patch for landing

Clearing flags on attachment: 360426

Committed r240639: <https://trac.webkit.org/changeset/240639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.