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

Fix pinning the balloons to #text nodes #16959

Merged
merged 6 commits into from
Aug 27, 2024
Merged

Fix pinning the balloons to #text nodes #16959

merged 6 commits into from
Aug 27, 2024

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Aug 22, 2024

Suggested merge commit message (convention)

Fix (ui): Restored the ability to pin balloons to text nodes in the DOM tree. Closes #16958 #16889.


Additional information

  1. Introduced in Image contextual toolbar does not disappear after clicking source editing. #16739 , we added visibility check before mounting of tooltips to avoid displaying them on hidden elements (which was causing another issue).
  2. Regression was caused by isVisible function. It was returning false for $text nodes, so it was impossible to pin balloons to such nodes.
  3. I extracted docs example to standalone manual test. Apply this patch to your git repo to test it on your own pin-balloons-demo.patch

Before

before-pin-fix-2024-08-22_08.47.00.mp4

After

after-pin-fix-2024-08-22_08.44.42.mp4

@Witoso
Copy link
Member

Witoso commented Aug 22, 2024

Does it potentially fix this scenario: #16889 as well?

@Mati365
Copy link
Member Author

Mati365 commented Aug 22, 2024

@Witoso Most likely.

@Mati365 Mati365 marked this pull request as ready for review August 22, 2024 07:21
@Mati365 Mati365 requested review from oleq and niegowski August 22, 2024 07:22
Copy link
Contributor

@niegowski niegowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the nodeType property of a DOM node to verify node type.

packages/ckeditor5-utils/src/dom/isvisible.ts Outdated Show resolved Hide resolved
@Mati365 Mati365 requested a review from niegowski August 22, 2024 12:00
@Mati365 Mati365 merged commit 2d74802 into master Aug 27, 2024
8 checks passed
@Mati365 Mati365 deleted the ck/16958 branch August 27, 2024 07:35
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.

Contextual balloon does not work with non-empty selection
3 participants