Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

The autocompletion panel should prefer the south positon #4643

Closed
Reinmar opened this issue Apr 4, 2019 · 14 comments · Fixed by ckeditor/ckeditor5-mention#52
Closed

The autocompletion panel should prefer the south positon #4643

Reinmar opened this issue Apr 4, 2019 · 14 comments · Fixed by ckeditor/ckeditor5-mention#52
Assignees
Labels
package:mention type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Apr 4, 2019

Instead, it prefers the north position:

image

@Reinmar
Copy link
Member Author

Reinmar commented Apr 4, 2019

cc @dkonopka @oleq For me it was intuitive that the panel should display below the text. So does happen e.g. here on GH. But in some apps, where it sticks to the editing field and not the caret (e.g. Slack), it's displayed above. I consider this a different case, but wanted to verify it with you.

@oleq
Copy link
Member

oleq commented Apr 4, 2019

The positioning algorithm has the limiter defined as editable element.

It chooses one of the available positions which fits the panel within the editable rectangle the most. If none of the positions fits perfectly into the editable, the one with the greatest intersection area is chosen. Hence the "north" position.

We can remove the custom limiter restriction so then the positions are

  • matched against the greatest intersection area with the viewport
  • considered in the order of preference

@jodator Why the limiter in the first place, BTW? Does it have a specific role, or fixes a specific case?

@jodator
Copy link
Contributor

jodator commented Apr 4, 2019

I was struggling with consistent behavior of the positioning algorithm and during some test it turned out to be working well. Later the problem (stick to the firstly chosen position) was solved by reading the panelView.position IIRC. The limiter make sense also to position the mention panel with the consideration of the editing area.

@jodator jodator self-assigned this Apr 4, 2019
@jodator
Copy link
Contributor

jodator commented Apr 4, 2019

considered in the order of preference

I'd remove the limiter and probably this should sort out the problem.

@jodator
Copy link
Contributor

jodator commented Apr 10, 2019

I was debugging why the balloon is choosing the north position where there's clearly a place to the south of cursor?

Because it does better fit in the default limiter (document.body):

Selection_297

So using the default limiter (document.body) should play along with features that shrinks the document body (looking at you ckeditor5 inspector) but will produce "wrong" results for short content.

@oleq Are we OK with this? Using the default limiter (document.body) on manual test would probably need some fixes in manual tests.

@oleq
Copy link
Member

oleq commented Apr 19, 2019

So using the default limiter (document.body) should play along with features that shrinks the document body (looking at you ckeditor5 inspector) but will produce "wrong" results for short content.

Well... a short body (shorter than the viewport) and the editor being the last child (or so) and inserting around at the bottom of the editable does sound like an edge case to me. Developers can easily fix that by expanding <body> using CSS (html, body{ min-height:100%; }). Besides, there's no harm in displaying to the top in that case. The feature remains 100% useful.

As for the inspector and other floating elements that could clash with the panel, AFAIR there's no API that would help us resolve this issue out–of–the–box. We would need to exclude those elements from the area available for the panel to position itself and, in fact, we have an issue for that.

oleq referenced this issue in ckeditor/ckeditor5-mention Apr 24, 2019
Other: The mention UI panel should prefer positions below the caret rather than above it for a more natural UX. Closes #37.
@Reinmar
Copy link
Member Author

Reinmar commented May 28, 2019

Did ckeditor/ckeditor5-mention#52 actually change anything? I don't see any change in the behaviour.

@jodator
Copy link
Contributor

jodator commented May 28, 2019

It's done inline with this comment: ckeditor/ckeditor5-mention#52 (comment) & ckeditor/ckeditor5-mention#52 (comment).
The mention UI prefers south position and is constrained within editing area. So the issue as such was considered invalid.

@Reinmar
Copy link
Member Author

Reinmar commented May 28, 2019

Please show me on screenshots what has changed because I can't figure that out myself. I mean – a case in which the panel will show up in a different position than previously.

@jodator
Copy link
Contributor

jodator commented May 28, 2019

It depends on the editor size, the body size and the position of the content.

The logic behind it doesn't changed as of the comments I've posted - we are not forcing the south position in every case - ie when the editor body is short because we prefer to constrain the UI withing editor (editing area) then overlaping content.

So most interesting static cases (first open):

  1. Big editor, mention inside text (prefer south)

Selection_013

  1. Narrow editor - height of editing area < mention ui height (prefer south)

Selection_016

  1. Big editor - end of content (prefer editing area)
    Selection_015

Now the dynamic behavior (first open, then typing):

  1. Big editor, mention inside text (prefer south - stays south
    Peek 2019-05-28 16-55
    )

  2. Narrow editor (starts south - stays south)
    Peek 2019-05-28 16-56

  3. Big editor - end of content (starts north
    Peek 2019-05-28 16-57

  • stays north)
  1. Similar as 3 but starts with smaller UI (mention ui fits south) - starts south - stays south
    Peek 2019-05-28 16-59

This is for manual tests. For documention pages the behavior might be slightly different due to different sizes of the editor and pages AFAICS. So the calculations of "best" area might be dependent on the content around the editor (I've added some text in the body so the issue from that comment wouldn't affect screencasts.

@Reinmar
Copy link
Member Author

Reinmar commented May 29, 2019

Interesting – I couldn't get the same results in the documentation. This panel chooses the north position unless there's space in the editor below the caret.

Actually... it looks broken. Why does it pick the north here:

image

When it picks the south here (when there's still not enough space):

image

It doesn't seem to work reliably.

@jodator
Copy link
Contributor

jodator commented May 29, 2019

Well I'd argue that it does ;)

The docs have different CSS styles and the rendered line is bigger. I've added logging to the getBestPosition() which calculates which position from a set is the best given the criteria. For mentions we provide limiter - in this case an editing area (as other features) and the ContextualPanel (or Balloon I don't remember which now) sets the fitInViewport to true. So the best position is that which fits most in the viewport and in the editing area.

Now for the same content look how it calculates the best position:
Selection_019

On the screenshot above I've logged each step of internal positions.each() call (I've also logged the return condition but is always falsy as it not fully displayed in the limiter).

As you can see positions are evaluated: caret_se > caret_sw > caret_ne > caret_nw (hence the "prefer south" is valid here).

Now for each position the algorithm checks how mention UI will be contained in VP viewport & LIM limiter area - editing area.

In the docs the editor have different line heights and it turns out that caret_ne is slightly better contained in the limiter area then caret_se by 316 square pixels (~3.6%) which is marginal but is bigger.

@Reinmar
Copy link
Member Author

Reinmar commented May 29, 2019

Hm... I think I realized what caused my confusion – I thought that we consider only the editable area for the limiter. But it seems that the toolbar is also considered as a part of the preferred area... which is a bit counterintuitive for me.

The other thing which I didn't get is that we compare areas and choose the biggest one. I thought that it works differently – try to use the first position, if not acceptable (doesn't fit), check the next one. What's important is that in the screenshots I posted above the panel doesn't fit in the limiter regardless of its placement. I'd expect that if it doesn't fit, we prefer S. But sometimes it chooses S and sometimes N because, as you explained, we compare only areas.

The current solution behaves kinda like an unstable sorting algorithm. Sometimes you'll get this, sometimes that. If you don't know the algorithm, you don't understand as a user why you got something else than a second before for nearly the same initial situation.

@jodator
Copy link
Contributor

jodator commented May 29, 2019

Hm... I think I realized what caused my confusion – I thought that we consider only the editable area for the limiter. But it seems that the toolbar is also considered as a part of the preferred area... which is a bit counterintuitive for me.

No. We consider the editing area - not whole editor with toolbar: https://github.com/ckeditor/ckeditor5-mention/blob/711ebbac96d05153b175d4d47964903ba4c536a6/src/mentionui.js#L530-L540

The editing area in the docs is bigger then in manual tests:

editorheights

The other thing which I didn't get is that we compare areas and choose the biggest one. I thought that it works differently – try to use the first position,

The algorithm works this way only if the whole panel is contained withing limiter. In here it does not so we evaluate all positions to find the best - see the "is fully displayed: false" in the logs.

On your screens shots you have editors of different height (3 lines vs 4 lines). So on the second one it chooses south as it is better contained within editing area the other positions (namely north). On the first one it chooses north because of slightly (3.63% as calculated above) bigger area of fit in editing area.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-mention Oct 9, 2019
@mlewand mlewand added this to the iteration 24 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:mention labels Oct 9, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
package:mention type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants