Block toolbar buttons do not scroll with editor content if block toolbar is added manually

[Mati365]

Suggested merge commit message (convention)

Fix (ui): Block toolbar button no longer remains fixed in the same position while scrolling the editable content. Closes #5460.

---

Additional information

Closes: ckeditor/ckeditor5-builder#676

1. It closes this builder issue: ckeditor/ckeditor5-builder#676.
2. I added manual tests in ui called scrollable-blocktoolbar

Screens

Before

block-toolbar-button-scroll-before-fix-2024-07-08_14.45.06.mp4

After

block-toolbar-button-scroll-fix-2024-07-08_14.41.10.mp4

[godai78]

Appears to be working correctly now.

[Reinmar]

It should also close this issue.

Should or does? 😄 If it does, please add it to "Closes" so we know that this PR should also be tested against that issue.

As for manual testing:

• Am I right that this PR requires quite a lot of testing in probably not the most obvious HTML+CSS setups? Scrollable containers, scrollable page, vertical/horizontal scrolls, different ways of applying these styles. How and where did you test it? I can't see any new manual test but if there was one on which this issue is clearly reproducible, I guess we'd know about it. So, am I right that we're missing a manual test where such issues are reproducible (both of the closed ones)? If so, please create one.
• In order to enable the QA team to look into this, providing the manual tests as well as potential directions to what to explore may help.

[Reinmar]

One more thing for the QA team: Could you test this solution on a larger page with a lot of content and see if there's some impact on performance? Testing it in some sample in the docs might be a good idea, but demos on ckeditor.com might be even better.

[Mati365]

@Reinmar

Should or does? 😄 If it does, please add it to "Closes" so we know that this PR should also be tested against that issue.

I updated description. I'm not sure if we should add it to closes commit message because that issue belongs to internal builder repo, so I placed it in Additional Information.

As for manual testing: [...]

Ok, it's only about manually adjust height of block toolbar container. I'll add manual test to cover that.

[Reinmar]

And one more thing for clarity: This PR adds "clipping" of the block toolbar icon. So, when it reaches the edge of a scrollable container, we start to cut off the part that overflows its vertical boundaries. This is a very interesting approach. It wasn't necessarily in the scope of this issue, but seems fine.

@oleq, I think this deserves your attention :) Question I'd have to you is: do you see any significant risks (apart from performance) in this approach?

[Reinmar]

OK, I found the first issue. A clipped button is not clickable:

You cannot click it now. It does not react to hover.

Meaning – it's kind of useless and confusing this way.

Which makes the clipping a bit pointless :( In my opinion, it'd be better, to be honest, to just hide the button once it cannot be clicked. Unless it's caused by this line and may be slightly improved.

PS. I'll add that I started checking "clickability" because I was curious whether this button does not actually get pointer events when it's invisible (and floating somewhere over surrounding content).

[Mati365]

@Reinmar It was intended to be clickable, so this line seems broken somehow. Thanks for pointing out, I'll check that.

[Mati365]

@Reinmar Indeed, this line was wrong. It was only about inversion of compare. I slightly renamed this method and made it more obvious. Should be working properly.

One thing besides that - I forgot about A11y. I adjusted PR to change isEnabled state of button to mark button outside of viewport as aria-hidden. It'll make it a bit more screen reader friendly.

[lkszzajac]

With this setup of editor elements:

<div style="max-height: 300px; max-width: 300px; overflow: auto;">
	<div id="editor-scrollable">
	...
	</div>
</div>

the issue is still reproducible. I suspect that there might be setups where the "bounding" element is not the editor itself but something higher up in the element tree, is it possible to calculate the viewport in these cases?

[Mati365]

@lkszzajac Nice catch! I managed to fix that in a recent commit. Can you take a look? I added manual test to cover this case.

[Mati365]

Tested successfully by QA. I added few more tests and assertion to prevent performance issue.

[niegowski]

I'd suggest movin the manual tests to the UI package as it does not require rebuilding the build and also is not related to the build itself but the feature provided by the UI package.

[Mati365]

@niegowski I applied your CR suggestions.

1. I moved manual tests to the UI
2. Fixed incorrect comments
3. Removed maxDepth checks in the closest scrollable parent
4. Removed not used _domEmitter

After removal of maxDepth, I discovered edge case related to calculation of closest scrollable parent. It was returning html element as scrollable for editor containers placed within position: absolute element. While it's true in theory, it was buggy in our case. position: absolute container might have bigger height than the whole html element, which was causing incorrect clipping of the button because the button was clipped to the viewport of html instead of the bigger parent. I fixed that. Manual and units tests should cover that.

Can you check again?

[Mati365]

I'll merge it after QA retest.

[charlttsie]

We have re-tested the PR with @marcelmroz and @kubaklatt, and it looks good. The initial issue doesn't reproduce anymore and we haven't found any regressions either 👍

[Mati365]

@charlttsie Cool! I'll merge it.