Bug 192711 - Web Inspector: REGRESSION (r238602): Elements: deleting the last child of a collapsed parent selects the parent's next sibling
Summary: Web Inspector: REGRESSION (r238602): Elements: deleting the last child of a c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P3 Normal
Assignee: Matt Baker
URL: https://devinrousso.com/demo/WebKit/t...
Keywords: InRadar
Depends on:
Blocks: 197051
  Show dependency treegraph
 
Reported: 2018-12-14 12:52 PST by Devin Rousso
Modified: 2019-04-18 13:38 PDT (History)
8 users (show)

See Also:


Attachments
WIP (2.95 KB, patch)
2019-03-28 13:21 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
WIP (3.97 KB, patch)
2019-03-28 14:11 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (2.60 MB, application/zip)
2019-03-28 14:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (3.01 MB, application/zip)
2019-03-28 15:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.56 MB, application/zip)
2019-03-28 15:55 PDT, Build Bot
no flags Details
Patch (11.65 KB, patch)
2019-03-28 18:25 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (12.25 KB, patch)
2019-03-28 18:51 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (12.74 KB, patch)
2019-03-29 17:39 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (14.33 KB, patch)
2019-04-01 11:08 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (14.63 KB, patch)
2019-04-10 15:09 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-12-14 12:52:39 PST
# 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
Comment 1 Radar WebKit Bug Importer 2018-12-14 12:53:25 PST
<rdar://problem/46738990>
Comment 2 Matt Baker 2019-03-28 13:21:02 PDT
Created attachment 366197 [details]
WIP
Comment 3 Matt Baker 2019-03-28 13:24:50 PDT
Need to add special-case handling for TreeOutline, so that both sibling axes are searched before going up the parent chain.
Comment 4 Matt Baker 2019-03-28 14:11:15 PDT
Created attachment 366202 [details]
WIP
Comment 5 Build Bot 2019-03-28 14:50:45 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-03-28 14:50:47 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-03-28 15:04:30 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2019-03-28 15:04:32 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2019-03-28 15:55:22 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2019-03-28 15:55:24 PDT Comment hidden (obsolete)
Comment 11 Matt Baker 2019-03-28 18:25:10 PDT
Created attachment 366230 [details]
Patch
Comment 12 Matt Baker 2019-03-28 18:51:34 PDT
Created attachment 366232 [details]
Patch
Comment 13 Matt Baker 2019-03-28 18:52:37 PDT
(In reply to Matt Baker from comment #12)
> Created attachment 366232 [details]
> Patch

After a delete, the new selected TreeElement is revealed.
Comment 14 Devin Rousso 2019-03-29 13:46:20 PDT
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".
Comment 15 Matt Baker 2019-03-29 17:39:57 PDT
Created attachment 366333 [details]
Patch
Comment 16 Matt Baker 2019-03-29 17:40:44 PDT
Comment on attachment 366333 [details]
Patch

Clearing r?; need to make one more change.
Comment 17 Matt Baker 2019-03-29 18:53:28 PDT
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.
Comment 18 Matt Baker 2019-04-01 11:08:35 PDT
Created attachment 366408 [details]
Patch
Comment 19 Devin Rousso 2019-04-10 15:09:58 PDT
Created attachment 367168 [details]
Patch
Comment 20 WebKit Commit Bot 2019-04-10 15:44:55 PDT
Comment on attachment 367168 [details]
Patch

Clearing flags on attachment: 367168

Committed r244155: <https://trac.webkit.org/changeset/244155>
Comment 21 WebKit Commit Bot 2019-04-10 15:44:57 PDT
All reviewed patches have been landed.  Closing bug.