Web Inspector: REGRESSION (r238602): Elements: deleting the last child of a collapsed parent selects the parent's next sibling

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

# STEPS TO REPRODUCE:
1. open WebInspector
2. go to the Elements tab
3. select the last child of the <section>
4. collapse the <section> (make sure to not change the selection when collapsing)
5. press ⌫ to delete any selected elements
 => the next sibling of the <section> becomes selected, instead of the deleted child's previous sibling

Another thing of note is that if the same steps are repeated except for (4), the closing tag of the parent is briefly selected before the selection changes to be the deleted child's previous sibling

[Radar WebKit Bug Importer]

<rdar://problem/46738990>

[Matt Baker]

Created attachment 366197 [details]
WIP

[Matt Baker]

Need to add special-case handling for TreeOutline, so that both sibling axes are searched before going up the parent chain.

[Matt Baker]

Created attachment 366202 [details]
WIP

[EWS Watchlist]

Comment on attachment 366202 [details]
WIP

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

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

Created attachment 366212 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366202 [details]
WIP

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

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

Created attachment 366213 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366202 [details]
WIP

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

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

Created attachment 366217 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Matt Baker]

Created attachment 366230 [details]
Patch

[Matt Baker]

Created attachment 366232 [details]
Patch

[Matt Baker]

(In reply to Matt Baker from comment #12)
> Created attachment 366232 [details]
> Patch

After a delete, the new selected TreeElement is revealed.

[Devin Rousso]

Comment on attachment 366232 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:19
> +        When a hidden node is selected, its selection area is drawn with a height
> +        of 0px. The selection area needs to be updated once the hidden node's
> +        parent is expanded. AFAIK, this has always been broken.

So technically, this is a "drive-by"?  If so, please add that before the comment for clarity.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:323
> +        if (this.selectedTreeElement)
> +            this.selectedTreeElement.reveal();

Should we be iterating over `this.selectedTreeElements` and calling `reveal()` on each instead?  If not, are we assuming that there will only ever be one selected item after any deletion?  If so, please add a `console.assert`.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:840
> -        const skipUnrevealed = true;
> +        const skipUnrevealed = false;

Why was this change made?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:845
> +        // to check siblings along the opposite axis, if they exist.

"opposite axis" is a weird phrase.  I'd use "other direction" (assuming that I understand your meaning).

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:846
> +        if (!treeElement.previousSibling && treeElement.nextSibling)

Should we limit the `previousSibling`/`nextSibling` to be only ones that are visible?  If we have a  hidden (or non-`selectable`) `TreeElement` before/after the given `item`, I don't think we should count that as being "valid".

[Matt Baker]

Created attachment 366333 [details]
Patch

[Matt Baker]

Comment on attachment 366333 [details]
Patch

Clearing r?; need to make one more change.

[Matt Baker]

I just noticed the change to TreeOutline.prototype.selectionControllerPreviousSelectableItem breaks up-arrow key traversal up the parent chain.

DOMTreeOutline could override selectionControllerPreviousSelectableItem. The overridden version could check whether this._treeElementsToRemove exists, and if it does, perform the additional check in the opposite sibling direction.

[Matt Baker]

Created attachment 366408 [details]
Patch

[Devin Rousso]

Created attachment 367168 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 367168 [details]
Patch

Clearing flags on attachment: 367168

Committed r244155: <https://trac.webkit.org/changeset/244155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.