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

Viewport offset aware and sticky positions #10373

Merged
merged 94 commits into from
Sep 12, 2021

Conversation

dawidurbanski
Copy link
Contributor

@dawidurbanski dawidurbanski commented Aug 18, 2021

This PR aims to fix contextual balloons positioning constraints if viewportOffset is specified.

Currently if on a page there are any absolutely positioned UI elements, some balloons can go above or under these elements. See #9892 for more information about it.

There are two subproblems to solve in this case:

  1. Constrain best position finder (getOptimalPosition) to only consider positions within an area limited by the newly added viewportOffset config option.
  2. Add new type of position: sticky. To stick to the top of the constrained viewport for very tall elements and widgets (taller than the editor viewport).

It's heavily inspired by #10239 and it invalidates both #10239 and #10120.

Also, it probably will have to be slightly modified when #10352 lands in master because it independently adds the concept of ui.viewportOffset config property.

Suggested merge commit message (convention)

Type: TODO: I will need help with this message. There are probably a lot of breaking changes here. Closes #9892.


Additional informations

Best test to check it out is:

yarn manual --files=ui/manual/tickets/9892/1.js

@dawidurbanski dawidurbanski changed the title Ck/9892 viewport top offset balloons v3 Viewport offset aware and sticky positions Aug 23, 2021
@dawidurbanski dawidurbanski marked this pull request as ready for review August 23, 2021 08:42
@dawidurbanski dawidurbanski requested a review from oleq August 23, 2021 08:56
@dawidurbanski dawidurbanski reopened this Aug 23, 2021
@dawidurbanski dawidurbanski requested a review from oleq August 27, 2021 10:13
@oleq
Copy link
Member

oleq commented Aug 30, 2021

@ckeditor/qa-team Can you give us a hand with this PR? The impact of these changes is huge: we touched the mechanics of positioning balloons/popups/floatables across the entire project (also CF).

@FilipTokarski
Copy link
Member

FilipTokarski commented Aug 30, 2021

What about cases when toolbar wrapping is disabled? Or when using other toolbar items when table is focused? Currently table toolbar is always on top and covers other balloons, making some options unreachable. Not sure if this is a discussion for this PR, but anyway, such cases might be annoying when editing some bigger content, as the table toolbar is now always visible on scroll.
Examples below:

0_toolbar2.mp4

toolbar3

@LukaszGudel
Copy link
Contributor

Steps:

  1. Add a link somewhere inside the table.
  2. Click on this link to open its balloon with the url.
  3. Use arrow keys to place a caret at the end of the link.

Result:

URL balloon will change to table toolbar, but its position will stay next to the link. This toolbar can be scrolled out of the viewport. Changing the caret position once again with arrow keys will fix the table toolbar placement.

CKE5-link-ballon-viewport.mp4

@dawidurbanski
Copy link
Contributor Author

@FilipTokarski @LukaszGudel Thanks for looking at this. Just to make sure I addressed all the issues you raised:

  1. In this issue you do touch a topic of balloons collisions. It's something we can improve for sure, but we do not care about it at this point. Making collision detection and global balloons auto-positioning mechanism is something we maybe will tackle in the future.

  2. In this issue it's caused by a mechanism that prevents balloons from jumping around if the context has been changed (for example if you are on a link, but now you jumped out of the link and you see text attributes balloon. It should stay more or less in the same place). Because of this the balloon stays inside the cell, and cell related balloons (for example table cell properties) do not get sticky when scrolling (because it makes no sense for the UI). So we decided it's good enough as is.

  3. About this issue. After quick discussion with @oleq we decided it's an edge case we don't want to bother with.

  4. This issue with missing arrow has been fixed.

@ckeditor/qa-team Could you quickly re-test this thing with everything I posted above in mind?

@dawidurbanski dawidurbanski dismissed oleq’s stale review September 7, 2021 10:09

Changes had been made and we have green light from Olek already.

@Mgsy
Copy link
Member

Mgsy commented Sep 8, 2021

LGTM 👍

@Reinmar Reinmar merged commit 84700cd into master Sep 12, 2021
@Reinmar Reinmar deleted the ck/9892-viewport-top-offset-balloons-v3 branch September 12, 2021 21:35
@Reinmar
Copy link
Member

Reinmar commented Sep 12, 2021

Thank you all :) It's been a long ride, but an interesting one. I scanned most of the code changes and they look really nice. I was surprised, but I didn't actually spot breaking changes except one (in the widget package). The changes in utils seem to be backwards compat because the new Position class is compatible with the old typedef and the getOptimalBleble() will work without the new option. Please check my merge commit message and add missing info (with an empty commit to master) if I missed something.

Once again, thank you and congrats 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table toolbar does not respect viewportTopOffset configuration