Bug 208364 – REGRESSION(r257380): Web Inspector: deleting node causes TreeOutline to lose focus

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

# STEPS TO REPRODUCE:
1. inspect any page
2. go to the Elements Tab
3. select any node
4. delete ⌫ that node
=> selection moves to the previous node, but in an unfocused state, so you can't use the arrow keys for further navigation

# STEPS TO REPRODUCE:
1. inspect any page
2. go to the Sources Tab
3. add a few JavaScript breakpoints to any file
4. select one of the JavaScript breakpoints
5. delete ⌫ that JavaScript breakpoint
=> selection moves to the previous JavaScript breakpoint, but in an unfocused state, so you can't reselect the breakpoint in order to re-enable keyboard navigation

Regressed in r257380.

[Radar WebKit Bug Importer]

<rdar://problem/59871772>

[Nikita Vasilyev]

Created attachment 392041 [details]
Patch

[Nikita Vasilyev]

Created attachment 392042 [details]
[Video] With patch applied

[Devin Rousso]

Comment on attachment 392041 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:533
> +        else if (treeOutline.element.contains(document.activeElement)) {

It seems really odd that `omitFocus === true` can still change the focus.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:-535
> -        // Focusing on another node may detach "this" from tree.
> -        let treeOutline = this.treeOutline;
> -        if (!treeOutline)
> -            return;

I don't think this should be removed/moved.  I think the original intention of this comment and check was that calling the `.focus()` above may be handled by an event listener, which could remove `this` from `this.treeOutline`, so we need to check that we're still attached to a tree before selecting.

[Nikita Vasilyev]

Comment on attachment 392041 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:533
>> +        else if (treeOutline.element.contains(document.activeElement)) {
> 
> It seems really odd that `omitFocus === true` can still change the focus.

It does seem a bit odd. I don't know a better alternative. I can rename it to something like `omitForceFocus`.

I tried removing omitFocus parameter entirely. That was quite a rabbit holes — after a day of working on it I only introduced several other regressions.

>> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:-535
>> -            return;
> 
> I don't think this should be removed/moved.  I think the original intention of this comment and check was that calling the `.focus()` above may be handled by an event listener, which could remove `this` from `this.treeOutline`, so we need to check that we're still attached to a tree before selecting.

It doesn't seem to make a difference. I can keep this as-is.

[Nikita Vasilyev]

Created attachment 393520 [details]
Patch

I removed the drive-by change.

[Devin Rousso]

Comment on attachment 393520 [details]
Patch

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

r=me, nice fix.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:528
> +        else if (this.treeOutline?.element.contains(document.activeElement)) {

It's unnecessary to `?.element` because we know that `this.treeOutline` is truthy because of the early return at the beginning of this function.

Also, I'd be fine if you moved the `let treeOutline = this.treeOutline;` to the beginning of this function so that we don't have to continue referencing it via `this.treeOutline`.

[Nikita Vasilyev]

Created attachment 393711 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 393711 [details]
Patch

Clearing flags on attachment: 393711

Committed r258536: <https://trac.webkit.org/changeset/258536>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.