Skip to content

Fix tooltip listeners in non-global documents - #20233

Open
marco-ms wants to merge 1 commit into
ckeditor:masterfrom
marco-ms:fix/ui-document-ownership
Open

marco-ms wants to merge 1 commit into
ckeditor:masterfrom
marco-ms:fix/ui-document-ownership

Conversation

@marco-ms

Copy link
Copy Markdown

🚀 Summary

This PR fixes tooltips for editor UI rendered in iframes and other non-global documents.

TooltipManager now listens to every document associated with a registered body collection instead of listening only to the global document. It synchronizes listeners as body collections are registered or unregistered, while continuing to render each tooltip in the matching document.

A regression test covers tooltip activation inside an iframe and verifies that document listeners are removed after unregistering the body collection. A user-facing changelog entry is included.


📌 Related issues

N/A


💡 Additional information

The behavior was manually verified with an editor UI hosted in a non-global document. The tooltip appeared in the correct document, and contextual editor UI continued to work as expected.

The automated test was added but could not be run locally because dependency installation was blocked by a package-feed resolution failure.


🧾 Checklists

Author checklist

  • Is the changelog entry intentionally omitted?
  • Is the change backward-compatible?
  • Have you considered the impact on different editor setups and core interactions?
  • Has the change been manually verified in the relevant setups?
  • Does this change affect any of the above?
  • Is performance impacted?
  • Is accessibility affected?
  • Have tests been added that fail without this change (against regression)?
  • Have the API documentation, guides, feature digest, and related feature sections been updated where needed?
  • Have metadata files (ckeditor5-metadata.json) been updated if needed?
  • Are there any changes the team should be informed about?
  • Were these changes documented (in Logbook)?

Reviewer checklist

  • PR description explains the changes and the chosen approach.
  • The changelog entry is clear, user- or integrator-facing, and describes any breaking changes.
  • All new external dependencies have been approved and mentioned in LICENSE.md.
  • All human-readable, translatable strings have been introduced using t().
  • I manually verified the change.
  • The target branch is correct.

@jacek-cke
jacek-cke requested a review from Mati365 September 11, 2026 08:17
@Mati365

Mati365 commented Sep 11, 2026

Copy link
Copy Markdown
Member

@marco-ms Hi! Thanks for the PR - I'll try to take a look at it. Quick question - did you happen to test these changes alongside other features that also listen to the document element?

@Mati365 Mati365 self-assigned this Sep 11, 2026
@marco-ms

marco-ms commented Sep 11, 2026

Copy link
Copy Markdown
Author

@marco-ms Hi! Thanks for the PR - I'll try to take a look at it. Quick question - did you happen to test these changes alongside other features that also listen to the document element?

Hi sorry for not giving you a heads up, Teams is having a lot of issues with CKEditor v47 and using the built-in toolbars, it has a lot of references to global document, both in the popup manager and in other parts of the code base, specifically: tooltipmanager.ts, editoruiview.ts and bodycollection.ts, which in a multi window app makes the tooltips and other UX elements parented to the wrong window (aka they are not going to be visible) or listening to the events of the wrong window, which also causes flickers or not clickable elements.

I noticed v48 fixed a ton of those issues, however such fixes were never backported to LTS, I think this PR addresses a left over in main; unfortunately I did not test it, because mainly we are sticking to v47 (LTS version) with local patches to address this short-coming.

Feel free to re-work this PR and add an e2e test that mimic a real multi-window test app and find a real use case, I am sure there is more than one. I did create a basic test, please check it out.

Strongly suggested is an eslint rule to prevent further use of global document/window, which sometimes is subtle like using global CSS object also creates issues in a multiwindow environment, but obviously more explicitly by using global document or global window to create children, assuming that is the right one, which may well not be.

@andrzejkala

Copy link
Copy Markdown
Contributor

Hi @marco-ms, thanks a lot for putting this together — and for the detailed write-up of what you're hitting. The context about multi-window setups and the TooltipManager / EditorUIView / BodyCollection references to the global document is genuinely useful, and the ESLint rule idea is a fair point we'll discuss internally.

One thing I wanted to raise: since you have a premium support plan with us, this is a case where it'd be much more productive to continue the conversation there. There's quite a bit we'd want to understand about your setup before settling on a direction - how the multi-window app is structured, which UI elements break in practice, and, importantly, what your situation is regarding the v47 LTS line versus v48, given you mentioned you're staying on v47 with local patches.

LTS backports are handled through the commercial channel, so that discussion belongs there too.

Could you open a ticket and reference this PR? We'll pick it up from there and make sure the right people are looped in. We'll keep this PR open in the meantime.

As a side note: the CLA check on this PR is currently failing. Before we could merge any contribution from you, we'd need the Contributor License Agreement signed. You can do that here: https://cla.ckeditor.com/check/ckeditor/ckeditor5/20233 - worth sorting out regardless of where the technical discussion lands.

Thanks again!

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.

3 participants