Bug 149742

Summary: Regression: Web Inspector: Sometimes in Elements panel two elements showed as selected at the same time
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, joepeck, lance, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Animated GIF] Bug
none
[PATCH] Proposed Fix none

Description Nikita Vasilyev 2015-10-02 00:38:51 PDT
Created attachment 262321 [details]
[Animated GIF] Bug

See the attached GIF. So far I was unable to find exact steps to reproduce the problem.
Comment 1 Radar WebKit Bug Importer 2015-10-02 00:39:05 PDT
<rdar://problem/22946966>
Comment 2 Nikita Vasilyev 2015-10-02 00:40:34 PDT
It's started happening for me a couple of months ago. Possibly a regression.
Comment 3 Joseph Pecoraro 2015-10-02 10:47:46 PDT
Were there exceptions in the inspector?
Comment 4 Nikita Vasilyev 2015-10-04 21:09:19 PDT
(In reply to comment #3)
> Were there exceptions in the inspector?

No.
Comment 5 Nikita Vasilyev 2015-10-08 00:06:38 PDT
I recently observed this exception:

Assertion Failed
markAsNeedsRefresh — StyleDetailsPanel.js:85
visibilityDidChange — CSSStyleDetailsSidebarPanel.js:148
removeSidebarPanel — Sidebar.js:98
showDetailsSidebarPanels — ContentBrowserTabContentView.js:168
dispatch — Object.js:151
dispatchEventToListeners — Object.js:158
_dispatchCurrentRepresentedObjectsDidChangeEvent — ContentBrowser.js:437
Comment 6 Timothy Hatcher 2015-10-08 07:57:06 PDT
That is an assert, not an exception. So it wouldn't cause random breakage. We should file a separate bug for that.
Comment 7 Joseph Pecoraro 2016-02-08 21:54:45 PST
<rdar://problem/24492481>
Comment 8 Joseph Pecoraro 2016-02-08 21:56:49 PST
* SUMMARY
Regression produced by r187496.

* TEST
<body>
<div>
    <p>Test</p>
    <p>Test</p>
    <p>Test</p>
    <p>Test</p>
    <button id="start">Start</button>
    <button id="stop">Stop</button>
</div>
<script>
setInterval(function() {
    var elem = document.body.appendChild(document.createElement("div"));
    elem.style.cssText = "width: 100px; height: 100px; background: blue";
    setTimeout(function() {
        document.body.removeChild(elem);
    }, 0);
}, 500);
</script>

* STEPS TO REPRODUCE
1. Inspect a <p> on the test page
2. Click the other <p> tags
  => multiple selections left over

* NOTES
- The TreeOutline loses the selectedTreeElement, but the inner selected TreeElement still sees itself as selected...
Comment 9 Joseph Pecoraro 2016-02-08 22:08:37 PST
Created attachment 270913 [details]
[PATCH] Proposed Fix

This fixes the issue, and keyboard navigation within the DOM Tree gets fixed as well (previously when this was happened the tree would lose focus).

However I'm marking as cq- because I still fail to see how this was caused by r187496, which means there is something I don't yet fully understand here, so I want to investigate this a bit more.
Comment 10 Joseph Pecoraro 2016-02-09 21:15:25 PST
> However I'm marking as cq- because I still fail to see how this was caused
> by r187496, which means there is something I don't yet fully understand
> here, so I want to investigate this a bit more.

I now understand!

The same optimization that I'm adding to moveChild(...) effectively happened before. While iterating over the child list the old code would skip over tree elements in the correct place:

>    while (child) {
>        var currentTreeElement = treeElement.children[treeChildIndex];
>        if (!currentTreeElement || currentTreeElement.representedObject !== child) {
>            ...
>        }
>        ...
>    }


Basically we now do that first check in moveChild(...). The old code could presumably have a bug that this addresses with the correct selecting.

Either way, I'm now confident that this can land.
Comment 11 WebKit Commit Bot 2016-02-09 22:02:54 PST
Comment on attachment 270913 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 270913

Committed r196362: <http://trac.webkit.org/changeset/196362>
Comment 12 WebKit Commit Bot 2016-02-09 22:02:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Timothy Hatcher 2016-02-24 11:24:29 PST
*** Bug 154645 has been marked as a duplicate of this bug. ***