Bug 192353 - Web Inspector: REGRESSION(r238602): Elements: collapsing a DOM node with the left arrow doesn't work
Summary: Web Inspector: REGRESSION(r238602): Elements: collapsing a DOM node with the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL: https://devinrousso.com/demo/WebKit/t...
Keywords: InRadar
Depends on: 192059
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-03 22:00 PST by Devin Rousso
Modified: 2018-12-06 12:38 PST (History)
8 users (show)

See Also:


Attachments
[Video] Screenrecording of issue (12.03 MB, video/quicktime)
2018-12-03 22:02 PST, Devin Rousso
no flags Details
Patch (1.65 KB, patch)
2018-12-04 15:02 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews203 for win-future (12.77 MB, application/zip)
2018-12-04 17:16 PST, Build Bot
no flags Details
Patch for landing (1.81 KB, patch)
2018-12-05 16:28 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.62 MB, application/zip)
2018-12-05 17:25 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-12-03 22:00:23 PST
The issue is intermittent, but it happens often, and when it does it completely breaks navigation.  My guess is that some state is getting stuck somewhere.

In the attached video, any "pause" where no navigation/clicking occurs is me mashing the left arrow button 😅
Comment 1 Devin Rousso 2018-12-03 22:02:23 PST
Created attachment 356466 [details]
[Video] Screenrecording of issue
Comment 2 Radar WebKit Bug Importer 2018-12-04 09:19:53 PST
<rdar://problem/46455019>
Comment 3 Radar WebKit Bug Importer 2018-12-04 09:19:54 PST
<rdar://problem/46455020>
Comment 4 Matt Baker 2018-12-04 15:02:30 PST
Created attachment 356543 [details]
Patch
Comment 5 Nikita Vasilyev 2018-12-04 16:10:18 PST
(In reply to Devin Rousso from comment #1)
> Created attachment 356466 [details]
> [Video] Screenrecording of issue

By the way, Keycastr can visualize key presses. Otherwise, I don't know when you pressed left arrow key and it didn't work.

https://github.com/keycastr/keycastr
Comment 6 Nikita Vasilyev 2018-12-04 16:15:10 PST
I was able to reproduce the bug. With the patch applied, I could no longer reproduce it!

Trying to understand what it actually does now.
Comment 7 Nikita Vasilyev 2018-12-04 16:27:18 PST
Comment on attachment 356543 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:12
> +        Don't early return when the element is not the selected tree element.
> +        This condition no longer holds now that TreeOutline supports multiple selection.

This makes sense. Looks good to me.
Comment 8 Build Bot 2018-12-04 17:15:53 PST
Comment on attachment 356543 [details]
Patch

Attachment 356543 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10270768

New failing tests:
http/tests/misc/resource-timing-resolution.html
Comment 9 Build Bot 2018-12-04 17:16:04 PST
Created attachment 356560 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 10 Devin Rousso 2018-12-05 15:28:30 PST
Comment on attachment 356543 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:535
> +        if (!this.treeOutline || !this.selected)

We should add an assert here, just so that we can observe other non-multiple-selection callers and make sure we aren't generating any new issues:

    console.assert(this.treeOutline.allowsMultipleSelection || this.treeOutline.selectedTreeElement === this);

or even:

    console.assert(this.treeOutline.selectedTreeElements.includes(this));
Comment 11 Matt Baker 2018-12-05 16:28:20 PST
Created attachment 356676 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2018-12-05 16:55:21 PST
The commit-queue encountered the following flaky tests while processing attachment 356676 [details]:

inspector/console/js-isLikelyStackTrace.html bug 192440 (authors: bburg@apple.com, drousso@apple.com, and joepeck@webkit.org)
The commit-queue is continuing to process your patch.
Comment 13 Build Bot 2018-12-05 17:25:53 PST
Comment on attachment 356676 [details]
Patch for landing

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

New failing tests:
http/tests/misc/resource-timing-resolution.html
Comment 14 Build Bot 2018-12-05 17:25:54 PST
Created attachment 356683 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 15 WebKit Commit Bot 2018-12-06 12:38:00 PST
Comment on attachment 356676 [details]
Patch for landing

Clearing flags on attachment: 356676

Committed r238938: <https://trac.webkit.org/changeset/238938>
Comment 16 WebKit Commit Bot 2018-12-06 12:38:02 PST
All reviewed patches have been landed.  Closing bug.