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

Table props dialog displayed offscreen (while table cell dialog works fine) #6181

Closed
Reinmar opened this issue Feb 3, 2020 · 4 comments · Fixed by ckeditor/ckeditor5-utils#329
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. intro Good first ticket. package:table type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Reinmar
Copy link
Member

Reinmar commented Feb 3, 2020

Feb-03-2020 16-43-29


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@Reinmar Reinmar added type:bug This issue reports a buggy (incorrect) behavior. status:confirmed package:table domain:ui/ux This issue reports a problem related to UI or UX. labels Feb 3, 2020
@Reinmar Reinmar added this to the iteration 29 milestone Feb 4, 2020
@Reinmar
Copy link
Member Author

Reinmar commented Feb 4, 2020

Let's give this thing 2h and see if we see a solution.

@panr
Copy link
Contributor

panr commented Mar 27, 2020

This is weird... I've been seeing this for a very long time, but right now I can't reproduce it... 😅
Do we have established steps for this issue?

EDIT: Ok, nevermind. Got it. The table has to be near to the bottom of the editor.

@oleq
Copy link
Member

oleq commented Mar 27, 2020

I think the problem is around here https://github.com/ckeditor/ckeditor5-utils/blob/master/src/dom/position.js#L185-L200.

When there's both limiter and fitInViewport, the algorithm intersects the limiter with the viewport and tries to find the position that fits best in that intersection. And then this actually makes sense:
image

Give it just a little bit more space below the widget, and suddenly the opposite position is better:

image

I think this logic is just a little naive. I think we should separately check

  • the intersection( limiter, viewport )
  • but also if the element fits in the viewport.

For instance, we could loop over the positions and sort them according to the biggest intersection( elementRect, viewport ). Then, on those sorted positions, we could do the logic that checks intersection( limiter, viewport ).

It's a 2-D optimization problem. Picture it as the X-axis representing the element/viewport intersection and the Y-axis representing the limiter/viewport intersection. We want to find the (x,y) point in that 2-D space that will create the rectangle of the biggest area [ (0,0), (x,y) ].

@oleq
Copy link
Member

oleq commented Mar 30, 2020

On the second thought, maybe it's not a plain 2-D optimization after all. We may need to prioritize if fitting in viewport is more important than fitting into the editable (or vice versa). Research...

@niegowski niegowski self-assigned this Mar 30, 2020
oleq added a commit to ckeditor/ckeditor5-utils that referenced this issue Apr 1, 2020
Fix: The `getOptimalPosition()` helper should prefer positions that fit inside the viewport even though there are some others that fit better into the limiter. Closes ckeditor/ckeditor5#6181.
mlewand pushed a commit that referenced this issue May 1, 2020
Fix: The `getOptimalPosition()` helper should prefer positions that fit inside the viewport even though there are some others that fit better into the limiter. Closes #6181.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. intro Good first ticket. package:table type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants