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

Link balloon does not update its position on external changes #4788

Closed
oskarwrobel opened this issue Apr 24, 2017 · 6 comments · Fixed by ckeditor/ckeditor5-link#114
Closed
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:link type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@oskarwrobel
Copy link
Contributor

It doesn't work only for the balloon of the new link. It works fine with balloon attached to existing link element.

apr-24-2017 19-06-54

A follow-up of https://github.com/ckeditor/ckeditor5-ui/issues/198.

@oskarwrobel oskarwrobel changed the title Link balloon does not update its position on document external changes Link balloon does not update its position on external changes Apr 24, 2017
@oleq
Copy link
Member

oleq commented Apr 25, 2017

I suppose it may be a very tricky problem.

The first problem is that the balloon must be re-positioned to something on each render, which is the selection (and the ClientRect of a native Range). As the content changes, we must figure out how to preserve that range, to keep track and pin the balloon in the right place.

The second problem is what to do when the range the balloon was attached to does not make a sense anymore. Like when the external change deletes the paragraphs. Should we simply close the balloon then? Because if yes, then it will mean the user loses their content, which may not be a very good UX. I see no other solution, though 🤔

@Reinmar
Copy link
Member

Reinmar commented Apr 25, 2017

I don't see a problem here.

We have two separate mechanisms here:

  1. Condition to close/open the balloon.
  2. Repositioning the balloon on every possible content / environment change.

These two mechanisms should be implemented independently. It makes it easier to think about the implementation and behaviour (at least for me).

The first point – events on which the balloon should be opened or closed:

  • open: click on a link, Ctrl+K, click on the link button,
  • close: press Esc, submit or cancel, click somewhere else.

These are the actions (I might've forgotten some). Note that all of them are intentional – they are user actions, not background actions. This ensures that the user will not be surprised with disappearing balloon (or that it will happen as infrequently as possible – see the next paragraph).

Now, in between these two actions, we need to constantly reposition the balloon. We need to do that on environment changes (when content doesn't change) like scrolling and on content changes (collaboration).

To what target rect we should be positioning? I can see that initially the balloon is positioned to the link element itself. This is fine. This can be maintained as long as long as a link element can be found in the current selection. Keep in mind that the user's selection is never lost – engine's model and view will maintain the selection infinitely – even, despite any possible collab changes.

The only thing you don't know is where this selection will end. Due to collab changes the originally selected content can be removed (so selection gets collapsed) or a link in which the selection was might be removed or split. All this may happen. The link should, IMO gracefully handle these changes. So:

  • If the user selection is still inside a link (assuming it was before) – use that link. Edge case is s that it may be a different link than before – I think we can just ignore it. It happens. It's collab editing. If 2 users are working on the same piece of content there will be such situations.
  • If the selection was in a link but isn't now – the link feature should simply position to the user selection now. Also, the changes should be applied to that selection (regardless whether it's empty or not).
  • etc, etc.

tl;dr – implement each mechanism separately, handle changes gracefully, keep the least state possible (e.g. don't try to memorise the original link element – it may change!).

@Reinmar
Copy link
Member

Reinmar commented Apr 25, 2017

Let's hope I didn't miss something ;)

@oleq
Copy link
Member

oleq commented Apr 25, 2017

To what target rect we should be positioning? I can see that initially the balloon is positioned to the link element itself. This is fine. This can be maintained as long as long as a link element can be found in the current selection.

What if this is a new link scenario? There's no link in the model/view then.

@oskarwrobel
Copy link
Contributor Author

oskarwrobel commented Apr 25, 2017

I see 2 major scenarios here:

  1. Creating new link - balloon is attached to the selection and should be repositioned as long as selection is non-collapsed user doesn't close this balloon (doesn't work).
  2. Editing existing link - balloon should be attached to the link element as long as collapsed selection is inside this element (it works fine already).

@Reinmar
Copy link
Member

Reinmar commented Apr 25, 2017

What if this is a new link scenario? There's no link in the model/view then.

So, initially you was positioning to the caret, right? Upon some collab changes, the caret will be moved around the content. In 99.9% scenarios – it will end up in some other place where the link can be created – no problem there. So just let's reposition to that caret and that's it.

If the caret ended up in a place where link can't be created... this may happen if there were some serious changes done. This is the edge case where closing the balloon would be reasonable.

@oleq oleq self-assigned this Apr 25, 2017
oleq referenced this issue in ckeditor/ckeditor5-link Apr 27, 2017
oleq referenced this issue in ckeditor/ckeditor5-link Apr 27, 2017
oskarwrobel referenced this issue in ckeditor/ckeditor5-link Apr 27, 2017
Fix: Link balloon should update its position upon external document changes. Closes #113.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-link Oct 9, 2019
@mlewand mlewand added this to the iteration 10 milestone Oct 9, 2019
@mlewand mlewand added domain:ui/ux This issue reports a problem related to UI or UX. status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:link labels Oct 9, 2019
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. package:link type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
4 participants