Hide balloon panel while scrolling out of the limiter element.

[pszczesniak]

Suggested merge commit message (convention)

Fix (ui): Should hide the ballon panel when it's scrolled outside the visible area.

Closes #5328.

---

Additional information

Waiting for QA testing ⏳ cksource/qa-internal#176  :green_circle: :point_right: #14528 (comment)

[pszczesniak]

Horizontal scrolling issue is out of the scope of this.

Follow up can be found here.

[mabryl]

QA testing completed and it looks good after the latest round of testing. Summary here: https://cksource.slack.com/archives/C056P7FQ8TY/p1689603459626639?thread_ts=1688474081.871299&cid=C056P7FQ8TY

[oleq]

In your solution

• you added a hiding position to the BalloonPanelView#attachTo() defaults,
• you added a hiding position to the list in widget and image utils responsible for positioning toolbars
• you assumed in getOptimalPosition() that the last available position is the hiding one despite getOptimalPosition() working as a generic helper (also used by DropdownView) and having an open API with the list of positions (also custom ones) to be passed by developers.

This will fail as soon as somebody configures any balloon (or other UI using getOptimalPosition()) with their custom positions and they won't put the hiding one as the last. getOptimalPosition() will pick the last one in the corner cases and instead of getting hidden, the balloon (UI) will move in some unexpected fashion.

---

The idea that comes to my mind is to have getOptimalPosition() return null when the target becomes invisible (that ancestor rect intersection check that you did). Then:

• BalloonPanelView could say "oh, alright then, I'm gonna use my hidden position in this case because balloons cannot be attached to a void"
• DropdownView could say "oh, alright then, I'm gonna use my default position (let's say its south) because you just can't make a dropdown panel disappear like a balloon"
• Integrators UI could say "oh, I'm gonna handle it on my own but the API does not force me to do anything specific, it just gives me options (they hidden position is not hardcoded anywhere)"

[oleq]

It can be used in

ckeditor5/packages/ckeditor5-ui/src/panel/sticky/stickypanelview.ts

Lines 395 to 407 in fece664

	function _getScrollableAncestors( element: HTMLElement ) {
	const scrollableAncestors = [];
	let scrollableAncestor = findClosestScrollableAncestor( element );
	while ( scrollableAncestor && scrollableAncestor !== global.document.body ) {
	scrollableAncestors.push( scrollableAncestor );
	scrollableAncestor = findClosestScrollableAncestor( scrollableAncestor! );
	}
	scrollableAncestors.push( global.document );
	return scrollableAncestors;
	}

right away if implementations do match.

[oleq]

Problably here too

ckeditor5/packages/ckeditor5-ui/src/editorui/poweredby.ts

Lines 350 to 362 in fece664

	else {
	const firstScrollableEditableElementAncestor = findClosestScrollableAncestor( focusedEditableElement );
	if ( firstScrollableEditableElementAncestor ) {
	const firstScrollableEditableElementAncestorRect = new Rect( firstScrollableEditableElementAncestor );
	// The watermark cannot be positioned in this corner because the corner is "not visible enough".
	if ( visibleEditableElementRect.bottom + balloonRect.height / 2 > firstScrollableEditableElementAncestorRect.bottom ) {
	return OFF_THE_SCREEN_POSITION;
	}
	}
	}

. AFAIR Illia was changing something there recently so let's see if it makes sense.

If we don't want all ancestors right away, this helper could be rewritten to generator/iterator protol.

[oleq]

This helper is missing unit tests.

[oleq]

This declarative approach got me worried.

On one hand, it allows integrators to opt-out of the viewportHidden position if they don't want it for some reason.

On the other hand, no such use-case pops into my head. And that would need to be a use-case where the balloon should stay visible despite its target being obscured by scrollable ancestors or just off the viewport.

Maybe if there's no such use-case, this should become a core behavior of getOptimalPosition() (a last-resort position injected to positions passed by the user)? Or even better, a core auto-injected position in BalloonPanelView#attachTo() because getOptimalPosition() is also used by other pieces of UI (e.g. some dropdowns) and making dropdown panel disappear is out of question.

[oleq]

The names of positions were written in such a way that they describe where the balloon will show up. For instance eastArrowWest means the balloon will show up east of its target with the arrow poining west. viewportStickyNorth means that the balloon will be sticky to the north of the viewport. viewportHidden then means...? I think plain hidden would make more sense here.

[oleq]

See my comment in image/ui/utils.ts. I would try injecting this position in L257 whether there were no positions specified by the user (defaults being used) or they used some specific ones (even custom ones).

[oleq]

When would top be undefined? Which use-case is it?

[oleq]

Is this to cover cases when getIntersection() returns null that you ignore by casting as Rect in the last line?

[pszczesniak]

for example:

new Rect( global.document )
------
Rect {top: undefined, right: undefined, bottom: undefined, left: undefined, width: undefined, height: undefined}

[oleq]

You don't know that. getOptimalPosition() gets an arbitrary set of positions. It is not used by balloon system only but also by dropdowns to position their panels.

[oleq]

at() is supported in Safari since Mar 2022 https://caniuse.com/?search=Array.at. Knowing yearly release cycle of MacOS I'm wondering how many "old" installations of MacOS are still in use and how often would this throw. Let's stay on the safe side for a little longer, WDYT?

[oleq]

Get scrollable ancestors will always consider global.document.

ckeditor5/packages/ckeditor5-utils/src/dom/getscrollableancestors.ts

Line 27 in fece664

scrollableAncestors.push( global.document );

If you got it covered while retrieving rects, you wouldn't need to handle this one separately. It would be just another rect to intersect. See the implementation of the sticky panel to learn more

ckeditor5/packages/ckeditor5-ui/src/panel/sticky/stickypanelview.ts

Lines 417 to 429 in fece664

	const scrollableAncestorsRects = scrollableAncestors.map( ancestor => {
	// The document (window) is yet another scrollable ancestor, but cropped by the top offset.
	if ( ancestor instanceof Document ) {
	const windowRect = new Rect( global.window );
	windowRect.top += viewportTopOffset;
	windowRect.height -= viewportTopOffset;
	return windowRect;
	} else {
	return new Rect( ancestor );
	}
	} );

[oleq]

Like I said before, an assumption that the last available position is the one that hides the positioned element is weak.

[pszczesniak]

Different approach was chosen - more generic one 👉 #14630