Bug 192091

Summary: Web Inspector: REGRESSION(r238599): Multiple Selection: restoring selection when opening WebInspector puts the TreeElement into a permanent selected state
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, inspector-bugzilla-changes, mattbaker, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: https://devinrousso.com/demo/WebKit/test.html
See Also: https://bugs.webkit.org/show_bug.cgi?id=192112
Bug Depends on: 191483    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Devin Rousso 2018-11-28 11:45:54 PST
* STEPS TO REPRODUCE:
1. open WebInspector
2. enable the Audit tab
3. select any audit test/group
4. close WebInspector
5. reopen WebInspector
6. select a different audit test/group
 => both of the `WI.TreeElement` from (3) and (6) are selected
Comment 1 Matt Baker 2018-11-28 14:25:11 PST
Created attachment 355917 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2018-11-28 14:27:19 PST
<rdar://problem/46321795>
Comment 3 Devin Rousso 2018-11-28 15:12:43 PST
Comment on attachment 355917 [details]
Patch

r-, this doesn't fix the issue.

Another reproduction case:
1. open WebInspector
2. enable the Audit tab
3. select "Demo Audit" > "Result Data" > "data-domNodes"
4. close WebInspector
5. reopen WebInspector
6. select "Demo Audit" > "Result Levels" > "level-unsupported"
 => both "data-domNodes" and "level-unsupported" are selected
Comment 4 Devin Rousso 2018-11-28 15:26:03 PST
This also is the case for the Elements tab:
1. open WebInspector
2. go to the Elements tab
3. select the <body>
4. close WebInspector
5. reopen WebInspector
6. expand the <body> by clicking on the disclosure arrow
6. select any <div> that is a child of <body>
 => both the <div> and <body> are selected
Comment 5 Matt Baker 2018-12-02 13:36:10 PST
Created attachment 356347 [details]
Patch
Comment 6 Matt Baker 2018-12-02 13:41:17 PST
This addresses additional defects in SelectionController and TreeOutline, one of which fixes an issue where traversing the tree using the up/down arrow keys would fail to select the previous/next element when the element was on a different level of the tree.
Comment 7 Devin Rousso 2018-12-02 20:07:02 PST
Comment on attachment 356347 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:400
>          this._cachedNumberOfDescendents++;

Aside: we should assert that `_cachedNumberOfDescendants` is equal to the sum of the length of every value in `_knownTreeElements`, or even just set it to be that value (in the case that `treeElement` has already been remembered in the past :

    console.assert(this._cachedNumberOfDescendents === Object.values(this._knownTreeElements).reduce((accumulator, current) => accumulator + current.length, 0));

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:416
>          this._cachedNumberOfDescendents--;

Ditto (400)

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:994
>      _indexOfTreeElement(treeElement)

Aside: the uses of this in `selectionControllerNextSelectableIndex` and `selectionControllerPreviousSelectableIndex` repeat a lot of work, as they both iterate over the tree using `traverseNextTreeElement`.  Perhaps consider combining/reworking them (followup)?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1010
> +        return NaN;

We should add a "not-reached" assertion:

    console.assert(false, "Unable to get index for tree element", treeElement, this);
Comment 8 Matt Baker 2018-12-03 13:55:33 PST
Comment on attachment 356347 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:400
>>          this._cachedNumberOfDescendents++;
> 
> Aside: we should assert that `_cachedNumberOfDescendants` is equal to the sum of the length of every value in `_knownTreeElements`, or even just set it to be that value (in the case that `treeElement` has already been remembered in the past :
> 
>     console.assert(this._cachedNumberOfDescendents === Object.values(this._knownTreeElements).reduce((accumulator, current) => accumulator + current.length, 0));

I'd like to avoid doing work every time an element is remembered/forgotten. I'll fix the bug where `_cachedNumberOfDescendents` is incremented even if the element had been remembered previously.
Comment 9 Matt Baker 2018-12-03 13:57:03 PST
Created attachment 356402 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2018-12-03 14:46:51 PST
The commit-queue encountered the following flaky tests while processing attachment 356402 [details]:

inspector/model/stack-trace.html bug 192332 (authors: drousso@apple.com and joepeck@webkit.org)
The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2018-12-03 14:46:59 PST
The commit-queue encountered the following flaky tests while processing attachment 356402 [details]:

inspector/unit-tests/linked-list.html bug 192333 (authors: joepeck@webkit.org and nvasilyev@apple.com)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2018-12-03 15:13:45 PST
The commit-queue encountered the following flaky tests while processing attachment 356402 [details]:

workers/bomb.html bug 192050 (author: fpizlo@apple.com)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2018-12-03 15:14:00 PST
The commit-queue encountered the following flaky tests while processing attachment 356402 [details]:

inspector/unit-tests/event-listener.html bug 192338 (authors: bburg@apple.com and drousso@apple.com)
imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_webkit01.html bug 192339 (authors: cdumez@apple.com and youennf@gmail.com)
The commit-queue is continuing to process your patch.
Comment 14 WebKit Commit Bot 2018-12-03 15:16:34 PST
Comment on attachment 356402 [details]
Patch for landing

Clearing flags on attachment: 356402

Committed r238825: <https://trac.webkit.org/changeset/238825>
Comment 15 WebKit Commit Bot 2018-12-03 15:16:36 PST
All reviewed patches have been landed.  Closing bug.