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 <hi>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, 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

Devin Rousso
Reported 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
Attachments
Patch (3.43 KB, patch)
2018-11-28 14:25 PST, Matt Baker 		no flags
DetailsFormatted DiffDiff
Patch (6.58 KB, patch)
2018-12-02 13:36 PST, Matt Baker 		no flags
DetailsFormatted DiffDiff
Patch for landing (7.25 KB, patch)
2018-12-03 13:57 PST, Matt Baker 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Matt Baker
Comment 1 2018-11-28 14:25:11 PST
Created attachment 355917 [details] Patch
Radar WebKit Bug Importer
Comment 2 2018-11-28 14:27:19 PST
<rdar://problem/46321795>
Devin Rousso
Comment 3 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
Devin Rousso
Comment 4 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
Matt Baker
Comment 5 2018-12-02 13:36:10 PST
Created attachment 356347 [details] Patch
Matt Baker
Comment 6 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.
Devin Rousso
Comment 7 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);
Matt Baker
Comment 8 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.
Matt Baker
Comment 9 2018-12-03 13:57:03 PST
Created attachment 356402 [details] Patch for landing
WebKit Commit Bot
Comment 10 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.
WebKit Commit Bot
Comment 11 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.
WebKit Commit Bot
Comment 12 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.
WebKit Commit Bot
Comment 13 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.
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2018-12-03 15:16:36 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.