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

Balloon panel sticks out of the limiter element while scrolling #5328

Closed
oskarwrobel opened this issue Mar 31, 2017 · 24 comments · Fixed by #14630 or #14755
Closed

Balloon panel sticks out of the limiter element while scrolling #5328

oskarwrobel opened this issue Mar 31, 2017 · 24 comments · Fixed by #14630 or #14755
Assignees
Labels
package:ui squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@oskarwrobel
Copy link
Contributor

Follow-up of: #5320

mar-31-2017 12-37-33

@oskarwrobel oskarwrobel changed the title Panel sticks out of the container element while scroll Panel sticks out of the limiter element while scroll Mar 31, 2017
@oleq
Copy link
Member

oleq commented Mar 31, 2017

And what is the limiter here?

@oskarwrobel
Copy link
Contributor Author

Editable element.

@oleq
Copy link
Member

oleq commented Mar 31, 2017

https://github.com/ckeditor/ckeditor5-ui/issues/173 should resolve it. Precisely:

Additionally getOptimalPosition() could check all the ancestors of the limiter which have overflow different than visible and intersect all their rects one by one up to window to find the real visible area which is available to position the BalloonPanel. That would, in most cases, make the limiter configuration obsolete.

@oskarwrobel
Copy link
Contributor Author

Problem is not with configuration. The question is what should we do with panel element when target element is out of the visible part of the limiter?

@oskarwrobel
Copy link
Contributor Author

Should we hide it or keep inside limiter bounds?

@oleq
Copy link
Member

oleq commented Mar 31, 2017

That's exactly what I quoted ;-)

getOptimalPosition() will essentially traverse up to the root of the document to check if the limiter isn't restricted by some overflow: * and if so, consider that fact. The result Rect of the limiter will then be an intersection of limiter's Rect and "overflow container's" Rect. That's it.

@fredck
Copy link
Contributor

fredck commented Apr 7, 2017

keep inside limiter bounds

+1

@oleq
Copy link
Member

oleq commented Apr 18, 2017

For the record, this issue is actually more complex than https://github.com/ckeditor/ckeditor5-utils/issues/148 and not covered by the fix.

It is complex because:

  1. As #editable is the limiter, there's no way to move the body collection containing the panel inside of it, so the panel gets cropped by the overflow of the (common) parent.

  2. Because of 1., the only solution is analyzing the geometry of the panel/limiter and taking necessary actions.

  3. It's a nasty business, though: suddenly attachTo() and pin() become responsible for the visibility of the panel, which is controlled by #isVisible attribute.

    1. #isVisible is the interface of the balloon. Features use it to control it. So usually it's bound to some external attribute like isFocused of focusManager (like the contextual toolbar) or something else.

    2. When attachTo() and pin() start touching #isVisible things get complicated. The balloon becomes hidden when it reaches the edge of the limiter.

      But can we show it back again if the geometry allows?

      It's not so simple, because after #isVisible switched to false thanks to attachTo() and pin(), tons of things could happen in the editor. Link/Image/Some toolbar feature may not want to display the balloon any longer because the focus was lost or the selection changed. All those things while the balloon hidden because "off the limiter". The features may actually want it to remain hidden, but there's no way to tell – #isVisible remained false and didn't "record" the demands of the features.

    3. To deal with this issue, there could be two different attributes. #isVisible remaining an interface for the features and, let's call it #_withinLimiter, for the attachTo() and pin() logic. Because bind.if in the Template does not support complex bindings, a simple

      bind.if( 'isVisible', 'ck-balloon-panel_visible' ),

      becomes

      	this.bind( 'hasVisibleClass' ).to( 
      		this, 'isVisible', 
      		this, '_withinLimiter', 
      		( isVisible, _withinLimiter ) => {
      			return isVisible && _withinLimiter;
      		} );
      
      	...
      
      	bind.if( 'hasVisibleClass', 'ck-balloon-panel_visible' ),

      so now the features control #isVisible and the internal logic of the balloon controls #_withinLimiter and the presence of the 'ck-balloon-panel_visible' is controlled by both. It could become part of bind.if API in the future to get rid of that intermediate #hasVisibleClass attribute. Looks good? Yes, but...

    4. The utils and algorithms behind attachTo() and pin() depend on DOM window#getBoundingClientRect method. Long story short, it doesn't work if the element is hidden. So once the balloon gets hidden when it's off the limiter, there's no easy way to display it again, even if it fits, because there's no way to get the geometry of the hidden element – it's controlled by #hasVisibleClass.

      1. To deal with it, we'd need to show the panel with opacity: 0, get its rect and hide it quickly. And again, and again, and again, until it's in the limiter's rect and it's OK to show it permanently to the user. It means lots (hundreds...) of CSS style changes, which is very, very slow.
        • We can optimize the whole thing a little bit by caching panel's dimensions before hiding assuming they won't change. I think it's a good assumption and will work for most of the cases.
        • Throttle/debounce become mandatory.
      2. Alternatively, instead of hiding, we can position the balloon with top: -10000px, left: -10000px so it remains invisible to the user but still it can be analyzed by window#getBoundingClientRect, which means no performance loss. Such panel could steal the focus in some cases, which could totally confuse the user – needs to be checked.
  4. ...and there's still the matter of the scrollbar. window#getBoundingClientRect obtains the rect with the scrollbars, the outermost area of the element. So to avoid situations like this

    image

    we must make the whole system scrollbar–aware.

    1. Again, it's not that simple. Scrollbar width/height can be computed using window#getBoundingClientRect and clientWidth|Height. It's a matter of correcting the rect (width, height, right, bottom) with the difference of #width - #clientWidth.
    2. But now comes the RTL. In RTL, we must correct it to the left (width, height, left, bottom) and to learn what direction is used in the webpage, I'm (almost) sure the window#getComputedStyles is necessary, which means another loss of the performance.
      • Caching the direction by the utility could help. It will fail for the mixed direction content, though. The web page could be in RTL with scrollbars on the left side, but some content us LTR and scrollbars are on the right side (didn't check that). OTOH, it's clearly an edge case.

@oleq oleq changed the title Panel sticks out of the limiter element while scroll Balloon panel sticks out of the limiter element while scrolling May 16, 2017
@Reinmar
Copy link
Member

Reinmar commented May 19, 2017

As #editable is the limiter, there's no way to move the body collection containing the panel inside of it, so the panel gets cropped by the overflow of the (common) parent.

For the future reference – I tried moving the toolbar/balloon to the element which has overflow:hidden and fixed height. It doesn't solve the problem automatically because the balloon is positioned absolutely, which negates the overflow:hidden of its parent.

image

We'd need to rewrite positioning of the balloon using relative of fixed positions (uuueeee...) AFAIR.

@Reinmar
Copy link
Member

Reinmar commented May 19, 2017

Besides, there's one more important problem – the scrolling is captured and blocked by the balloon which makes for a terrible UX. I don't think that it's easy to workaround.

@oleq
Copy link
Member

oleq commented May 25, 2017

Besides, there's one more important problem – the scrolling is captured and blocked by the balloon which makes for a terrible UX. I don't think that it's easy to workaround.

Some examples? Because I don't quite get what you had in mind.

@Reinmar
Copy link
Member

Reinmar commented May 25, 2017

I showed it to you live ;) If you keep the mouse over the balloon the page won't scroll. This was, actually, quite surprising because I didn't know that you could capture scroll (we have similar issues with dropdowns, but we'd like there to capture the scroll).

@oleq
Copy link
Member

oleq commented Mar 12, 2018

The issue came back to us in the document editor, which implements a scrollable editable by default. It's time we did something about that.

image

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-ui Oct 9, 2019
@mlewand mlewand added this to the backlog milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:ui labels Oct 9, 2019
@oleq
Copy link
Member

oleq commented Oct 16, 2019

The issue also appears when h–scrolling some wide content

image

@dungkvy
Copy link

dungkvy commented Feb 17, 2020

I still got this bug. How about the solution that we can hide ck-ballon-panel when scroll or add a backdrop to disable all page elements (restore when clicking outside)?

Screen Shot 2020-02-17 at 10 02 13 PM

@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Jun 29, 2023
oleq added a commit that referenced this issue Jul 25, 2023
…ut-of-the-limiter-generic-approach

Fix (ui,utils): A balloon panel should hide when its target (anchor) becomes invisible due to scrolling or cropping. Closes #5328.

MINOR BREAKING CHANGE (utils): The `getOptimalPosition()` helper will now return `null` (previously first available position) if the positioning criteria cannot be met, for instance, if the `target` is off the visible viewport.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jul 25, 2023
@CKEditorBot CKEditorBot added this to the iteration 65 milestone Jul 25, 2023
@mlewand
Copy link
Contributor

mlewand commented Jul 26, 2023

During final testing phase turned out there's some new regressions:

We don't want to merge the improvement with this regression and the fix requires extra effort and time to think this out. Thus we're delaying this fix to the next release.

@mlewand mlewand reopened this Jul 26, 2023
@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Jul 27, 2023
@mlewand mlewand self-assigned this Aug 2, 2023
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Aug 2, 2023
@mlewand
Copy link
Contributor

mlewand commented Aug 2, 2023

Problems that we have today

ScreenReasonSolutions
Balloon leaking outside of a browser viewport.
  • Sticky position should be shown only if target is not entirely visible in editing viewport.
  • Account for browser viewport (or parent viewport?) to be a limiter too
The balloon overlays the caret.
  • Sticky position should be shown only if target is not entirely visible in editing viewport.
  • Selection edge caret should be considered a blocker.
  • Make a common rectangle of all selected ranges to be a blocker.
    ☝️ This could be too generous,think of long content where you use "select all" feature.

Possible solutions

  • Sticky position should be shown only if target is not entirely visible in editing viewport.
    • To be determined, what does entirely visible mean?
      • Is it enough for target to be overflow only in one edge? (e.g. only top part is outside of editing viewport)
      • Or must it overflow in both edges (top and bottom).
    • Will it affect any other cases?

@mlewand mlewand removed this from the iteration 65 (v39.0.0) milestone Aug 2, 2023
@oleq
Copy link
Member

oleq commented Aug 2, 2023

@mlewand What we need here is a complete understanding of all use cases that we have to address. These two only prove that we're missing the big picture. There could be more.

There are two options used in getOptimalPosition(): limiter and fitInViewport. They are used by different editor features in different ways to satisfy different user stories. #14630 proved that we don't understand these user stories well and we're shooting in the dark. We focused on a manual test with specific corner cases and we missed the others.

Let's aggregate all user stories somewhere first (Figma?). That will help us understand and rethink the concepts of limiters and fitting in the viewport. My gut tells me they don't mean the same thing when we start making sure balloons get hidden when their target gets out of sight.

@oleq
Copy link
Member

oleq commented Aug 3, 2023

Yet another related issue #7388 (comment)

oleq added a commit that referenced this issue Aug 4, 2023
@mlewand mlewand removed their assignment Aug 9, 2023
@pszczesniak pszczesniak added squad:core Issue to be handled by the Core team. and removed squad:features Issue to be handled by the Features team. labels Aug 21, 2023
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Sep 8, 2023
@CKEditorBot CKEditorBot added this to the iteration 67 milestone Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment