Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tooltips should work until the last editor gets destroyed #12641

Merged
merged 6 commits into from
Oct 14, 2022

Conversation

oleq
Copy link
Member

@oleq oleq commented Oct 12, 2022

Suggested merge commit message (convention)

Fix (ui): Tooltips should continue showing up until the last editor gets destroyed. Closes #12602.

Internal (editor-classic,editor-inline,editor-balloon,editor-decoupled): Changed the order of destruction in respective `*EditorUI` classes to ensure the base `EditorUI` class is destroyed before the view of the editor.

Additional information

There are two other PRs that need to be merged together with this one:

Also, this one is quite interesting:

  1. Turns out, if TooltipManager#balloonPanelView was used at least once to render a tooltip (injected into some editor's body view collection) it got destroyed along with that editor via editor.destroy() -> editor.ui.destroy() ->  editor.ui.view.destroy() -> editor.ui.view.body.destroy() -> [ all children of editor.ui.view.body ].forEach( child => child.destroy() )  chain.
  2. The balloon remained in DOM, though. And the whole logic of TooltipManager worked just fine, but because once it was destroyed, the BalloonPanelView instance lost its dynamic templating and would no longer react to change of the #isVisible property. That's why the tooltip no longer showed up.
  3. The solution was making sure, the TooltipManager#balloonPanelView is ejected from the body view collection of the destroyed editor so the destruction chain I mentioned does not reach it.
  4. It all would be fine if not for the fact that for historical reasons, all editor implementations with their *EditorUI call super.destroy() at the very end of their own destroy() (see when it was first introduced for ClassicEditorUI, we don't have this API any more). And that means, they destroy their #view first before EditorUI#destroy() calls TooltipManager.destroy(). So even though the fix I described in the previous point was there, it was executed too late, after the TooltipManager#balloonPanelView has already been destroyed.
  5. I saw two solutions:
    1. TooltipManager could create a high priority listener on Editor.destroy() and save its balloon panel before everything happens. The problem with this solution is that it breaks the already established flow of destruction (Editor should destroy its EditorUI and EditorUI should destroy its sub-components, including the TooltipManager, TooltipManager should remain passive) and introduces an ugly "reverse-dependency".
    2. Clean up *EditorUI so they destroy things in a natural order. I went with that one because it feels like the right thing to do: first call super() to handle the parent, then clean up the sub-class (and its view, if it has one).
  6. The drawback of the solution I chose is that it requires changes across packages, docs and repositories. And even though it looks safe, we'll never know for sure.

@oleq oleq merged commit 5809d20 into master Oct 14, 2022
@oleq oleq deleted the ck/12602-tooltip-manager-destruction branch October 14, 2022 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants