Conversation
|
@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 |
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. |
|
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 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 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! |
🚀 Summary
This PR fixes tooltips for editor UI rendered in iframes and other non-global documents.
TooltipManagernow 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
Reviewer checklist
t().