Improvements to getOptimalPosition to handle cases with scrollable ancestors

[oleq]

Suggested merge commit message (convention)

Fix (utils): Balloon panels should not sticks out of the visible part of the editor while scrolling.

Closes #5328.
Closes #14536.
Closes #14667.
Closes #13307.
Closes #10846.
Closes #5499.
Closes #2584.
Closes #5354.

---

Additional information

The getOptimalPosition family tree:

https://www.figma.com/file/KBYn1Jv4jbxxul5bwiylfZ/getOptimalPosition-family-tree?type=whiteboard&node-id=0%3A1&t=WFmuNW74Z3beCAWQ-1

TODOS

Streamline StickyPanelView logic

ATM it uses getScrollableAncestors() and getElementsIntersectionRect(). I think it should depend on Rect#getVisible() instead.

Then we can ditch API that is no longer needed.

• getScrollableAncestors()
• getElementsIntersectionRect()

If there's no way to move the logic on top of Rect#getVisible(), these helpers should move to StickyPanelView as a private API. They're unlisted in docs anyway, so this should not cause breaking changes.

Addressed in 4a77bf9

viewportStickyNorth engaging when not necessary

I think defaultPositions.viewportStickyNorth should be used by WidgetToolbarRepository only. Despite recent changes aimed to keep it in check 199ea15, it still engages in some cases.

Probably addressed in 2d19f70.

viewportStickyNorth acting up in case of long tables

This has something to do with

ckeditor5/packages/ckeditor5-ui/src/panel/balloon/balloonpanelview.ts

Lines 1159 to 1161 in c14d3f1

	if ( targetRect.top > boundaryRect.top && targetRect.bottom < boundaryRect.bottom ) {
	return null;
	}

Addressed in 2d19f70.

BlockToolbar button misaligned when the window is scrolled

We ditched getOptimalPosition() for button (not the panel) positioning because there's nothing to choose from, really. We used a simple substitute but we didn't account for the fact that getOptimalPosition() returns position: absolute-friendly coordinates.

Fixed in cf5e63a.

[pszczesniak]

After this improvements will work we can take a closer look at those two issues:

• Sticky panel's left position does not consider scrollable ancestors' horizontal scroll #14570 
• Mention balloon panel sticks out of the limiter element while scrolling. #14662

[dufipl]

I have one scenario detected by our E2E tests which is a regression compared to the master.

Simplified steps:

1. Open http://localhost:8125/ckeditor5/tests/manual/all-features.html
2. editor.setData('<p><a href="[www.wikipedia.org](http://www.wikipedia.org/)">foo bar</a></p>')
3. Open the link edit balloon and press Save

Result

ckeditorerror.ts:90 Uncaught TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.
    at At.getVisible (rect.ts:408:181)
    at Ft (position.ts:172:48)
    at dd.zt [as _getOptimalPosition] (position.ts:127:31)
    at dd.attachTo (balloonpanelview.ts:172:50)
    at dd._startPinning (balloonpanelview.ts:255:14)
    at dd.pin (balloonpanelview.ts:229:14)
    at Zd._showView (contextualballoon.ts:366:19)
    at Zd.remove (contextualballoon.ts:210:22)
    at Ib._removeFormView (linkui.ts:312:27)
    at Ib._hideUI (linkui.ts:374:14)

[pszczesniak]

This issue #14755 (comment) fixed with 👉 c13d551

[charlttsie]

"This link has no url" message in the link balloon after saving changes

Steps

1. Open e.g. http://localhost:8125/ckeditor5/tests/manual/all-features.html
2. Add a link
3. Click on the link and edit it
4. Press enter

Result
Balloon with a "this link has no url" shows up:

link-actual.mp4

[dufipl]

We confirm that both issues ☝️
#14755 (comment)
#14755 (comment)
are fixed by c13d551

[oleq]

I think this code should at least warn if !parent. Using getVisible() with an abstract rect that has no clear _source such as DOM range or DOM element makes little sense. It's always fully visible. But doing so in the code may produce confusing results that are very hard to debug.

FYI: I tried to throw here to see how many tests will fail and there are 30+ in utils+ui. They are mostly caused by places like this one where we mock stuff instead of using some real elements

ckeditor5/packages/ckeditor5-ui/tests/panel/balloon/balloonpanelview.js

Lines 1351 to 1358 in d8672c3

	targetRect = new Rect( {
	top: 200,
	bottom: 400,
	left: 50,
	right: 100,
	width: 0,
	height: 0
	} );

Anyway, this is not a blocker but IMO makes for a serious follow-up.

[pszczesniak]

Agree, noticed it also... well it's caused cause getVisible will now works only real DOM elements, and finding intersection between two Rect's should only be done using getIntersection.

[pszczesniak]

Follow-up 👉 #14964

[oleq]

I think we're ready for a merge 👍