Bug 235047 – Web Inspector: Inline swatch popovers should hide when inline swatches are removed

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

[Nikita Vasilyev]

Created attachment 448794 [details]
[Video] Bug

Popovers hide when clicking outside of them but they don't hide when their corresponding inline swatch is removed from DOM.

Popovers currently stay visible when:
- selecting a different element using keyboard arrow keys (demo in the attached video)
- selected element is removed from DOM

In both of these cases the inline swatch is removed from DOM.

[Radar WebKit Bug Importer]

<rdar://problem/87687975>

[Nikita Vasilyev]

The bug affects all instances of popovers. For instance, CSS documentation popovers also don't hide when selecting a different element.

Possible resolutions:

1. Detect when the inline swatch element is removed from DOM, and hide its popover.

1.1 DOMNodeRemoved event. This event is deprecated and it's relatively slow https://developer.mozilla.org/en-US/docs/Web/API/MutationEvent.

1.2 MutationObserver, that has replaced DOM Mutation Events (such as DOMNodeRemoved). However, to track if inline swatch element is removed we'd have to potentially observe document.body with {subtree: true} option, which would be an insane overkill for our task.

1.3 setInterval that is active only when the popover is visible and checks for swatchElement.isConnected.

2. Add logic for every case where an inline swatch is removed from DOM (such as SpreadsheetStyleProperty#detached).
Cons: fastest execution.
Pros: adds code complexity.

[Nikita Vasilyev]

Created attachment 453708 [details]
Patch (setInterval)

[Nikita Vasilyev]

Created attachment 453710 [details]
Patch (manually clean up)

I think I like this one less. Having the popover itself handle detachment (like in the other patch) makes it easier to use.

[Devin Rousso]

Comment on attachment 453710 [details]
Patch (manually clean up)

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

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:141
> +        console.assert(this._popover);

NIT: This is a public method, so technically there's nothing stopping a client from calling this at any time, so I don't think this `console.assert` is correct.  I'd remove it.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:265
> +        this._popover = popover;

NIT: I'd replace the local `popover` variable with `this._popover` so that we dont have two variables for one value
```
this._popover = new WI.Popover(this);
```

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:598
> +        this._inlineSwatches.push(swatch);

I don't think it's realistically possible for more than one `WI.Popover` to be shown at a time.  Perhaps instead of having an array of `WI.InlineSwatch` we could just have `_activeInlineSwatch` that you set when handling `WI.InlineSwatch.Event.Activated` and unset when handling `WI.InlineSwatch.Event.Deactivated`.

[Nikita Vasilyev]

Devin, do you think the patch you've commented on is the way to go (and not the other)?

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #6)
> Devin, do you think the patch you've commented on is the way to go (and not the other)?

Yeah, I don't think polling is a good idea.  Perhaps there's something we could do with `MutationObserver`?  If not, having a manual cleanup is probably fine.

[Nikita Vasilyev]

Created attachment 454726 [details]
Patch

[Patrick Angle]

Comment on attachment 454726 [details]
Patch

r=me

Is it possible for us to fix this more holistically? This patch works, but only fixes the contextual documentation and inline swatch popovers, but the same issue actually exists for all the other popovers in Web Inspector. I can reproduce this by showing any popover in the frontend, and then from the Develop menu in Safari choosing "Show JavaScript Console" to jump to the console. Also reproduces if I use a Web Inspector keyboard shortcut like Shift+Cmd+F to jump to the search tab. I think long-term MutationObserver and having Popover handle this logic itself (which will require making Popover aware of the element it is supposed to be attached to) will be a much better solution. In the interim, I think what you have here is fine though unless you think we might already be close to just solving this with MutationObserver. If we aren't, and we land this patch, please just open a bug to track a more general solution that also removes these changes.

[Nikita Vasilyev]

I initially tried to fix this more holistically with MutationObserver. As I've mentioned: "to track if inline swatch element is removed we'd have to potentially observe document.body with {subtree: true} option, which would be an insane overkill for our task".

Here's an example:

<body>
  <a>
    <b>
      <c/>

Say, I want to detect when <c> is detached from the DOM. This can happened when <c> is removed, but also when <b> or <a> are removed. So, to detect when <c> is detached, I have to:
1. Observe <body>. I can't just observe <b>.
2. Check every MutationRecord if the target element is either <c> or one of its ancestors.

This is quite cumbersome and potentially slow (although, I haven't done profiling).

After realizing that, I implemented a version with based on setInterval (Comment 3). Polling isn't ideal, but unlike MutationObserver it has a very predictable performance - it doesn't depend on how many elements are being removed in Web Inspector.

[EWS]

Committed r291628 (248721@main): <https://commits.webkit.org/248721@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454726 [details].