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

Mention balloon panel sticks out of the limiter element while scrolling. #14662

Open
pszczesniak opened this issue Jul 26, 2023 · 6 comments
Open
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:mention squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@pszczesniak
Copy link
Contributor

📝 Provide detailed reproduction steps (if any)

  1. Open: http://localhost:8125/ckeditor5-mention/tests/manual/mention.html
  2. Set height for editing area: https://ckeditor.com/docs/ckeditor5/latest/support/faq.html#how-to-set-the-height-of-ckeditor-5
  3. Insert more content to see the scroll somewhere in the middle type @ and scroll while balloon panel is visible.

✔️ Expected result

Balloon panel should be hidden when @ (target) is outside of the visible area (limiter)

Screen.Recording.2023-07-26.at.09.16.10.mov

❌ Actual result

Screen.Recording.2023-07-25.at.15.33.43.mov

❓ Possible solution

To place the BalloonPanelView in best optimal position plugin is using function https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-utils/src/dom/position.ts#L88 that is trying to return the best fit of the element into the limiter and viewport. To get the intersection (limiter) of all target scrollable ancestors we're using function https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-utils/src/dom/getscrollableancestors.ts which takes only one parameter - HTMLElement, the problem is that MentionUi sets the target as a Rect no as a HTMLElement - that's why the only limiter is window and the optimal position is calculated wrongly.

Potential solution is to pass the DOM element domConverter.viewRangeToDom( viewRange ) from this line 👉 https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-mention/src/mentionui.ts#L557 but this need to be prototyped and well tested.

📃 Other details

  • Browser: …
  • OS: …
  • First affected CKEditor version: …
  • Installed CKEditor plugins: …

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@pszczesniak pszczesniak added type:bug This issue reports a buggy (incorrect) behavior. domain:ui/ux This issue reports a problem related to UI or UX. package:mention squad:features Issue to be handled by the Features team. labels Jul 26, 2023
@oleq
Copy link
Member

oleq commented Aug 8, 2023

Potential solution is to pass the DOM element domConverter.viewRangeToDom( viewRange ) from this line 👉 https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-mention/src/mentionui.ts#L557 but this need to be prototyped and well tested.

I would still pass a first rect out of Rect.getDomRangeRects( domConverter.viewRangeToDom( viewRange ) ) there. But what needs to be changed is Rect#isVisible() to use commonAncestorContainer in case of a DOMRange set as Rect#_source. It should then work like for Rects created out of DOM elements.

@scofalik
Copy link
Contributor

Do I understand correctly that this affects all contextual balloon UI elements, including table and image toolbar, and in these cases we should hide them as well?

@oleq
Copy link
Member

oleq commented Jan 4, 2024

I checked this and the problem is exclusive to the mentions feature these days. Improvements were made to the balloon positioning recently that addressed similar problems in balloon toolbars etc.

Screen.Recording.2024-01-04.at.10.58.39.mov

The problem comes down to my recent comment: the positioning logic seems to ignore the fact that a DOMRect of a DOM Range (and a DOM Range itself) can have a "parent" with overflow.

@iXiaoChuan
Copy link

iXiaoChuan commented Jan 5, 2024

I encountered the same problem, is there any solution for this problem now? Which branch can I merge, or which part of code can I merge to solve my problem?

@oleq Many thanks.

@Witoso
Copy link
Member

Witoso commented Jan 5, 2024

This is an open issue, no patch was prepared for it yet.

@iXiaoChuan
Copy link

This is an open issue, no patch was prepared for it yet.

I saw some similar situations in #14755 , it seems to have been solved and has been merged into the master branch, I thought it might be helpful to solve this problem. If not, I will wait for a solution, the related code logic is too complicated for me.

Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:mention squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

5 participants