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

Rethink the contextual balloon API #5439

Closed
Reinmar opened this issue Dec 18, 2017 · 12 comments
Closed

Rethink the contextual balloon API #5439

Reinmar opened this issue Dec 18, 2017 · 12 comments
Labels
package:ui resolution:expired This issue was closed due to lack of feedback. status:discussion type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@Reinmar
Copy link
Member

Reinmar commented Dec 18, 2017

There's so much wrong with this API now that I won't be able to describe all this here, so here goes a photo of my notes which kinda covers it all.

tl;dr is that when working on the highlight feature we realised that there are cases when two features may want to display their balloons at the same time. Right now we can't handle this reliably (display some merge of them) because we have a simple stack of balloons.

The problem comes from the fact that when we designed this API a year ago we still knew very little about all the requirements and use cases. Now, we see things like these conflicting features, real-time collaboration scenarios in which you need to carefully reposition the panel, many user actions that we need to handle (touch!), complexity of interaction (series of balloons – image toolbar => link info => link edit and back...), etc.

img_2869

(click the image – it's not horizontal, really)

@Reinmar
Copy link
Member Author

Reinmar commented Dec 18, 2017

Our current idea for the API is that features should not listen to any editor events directly because we're not able then to handle input from those features inside the ctx balloon. E.g. we'd not able to synchronise the Link feature (which listens to click) and Highlight (which might listen to a throttled selectionchange) because those events are not synchronous.

Additionally, there are so many ways in which the user can interact with the editor that it's just hard to understand them. And then, some of the events we have (like Selection#change:range) are tricky to use because they can be triggered by the user or by the system (collab editing).

In other words – it seems that the ctx balloon needs to unload features from all their current duties and it will then also be possible to implement more features.

@Reinmar
Copy link
Member Author

Reinmar commented Dec 18, 2017

@vladikoff
Copy link

cc @cedricium

@oleq
Copy link
Member

oleq commented Jun 27, 2018

I guess EditorUI#update could be the first step to some better balloon architecture. At least, it centralizes the model<->UI interaction, which makes easier for us to implement a contextual balloon with a single entry point for other features to listen to. It probably means that prioritizing the access to the balloon and stacking should also be easier.

@oleq
Copy link
Member

oleq commented Jul 25, 2018

In a F2F we agreed that:

  • The new architecture is needed because managing a constellation of features accessing the same view container (ContextualBalloon) using asynchronous event listeners is getting out of hand,
  • EditorUI#update (throttled) will be one of the key pieces of the new architecture, feeding the information to the ContextualBalloon about the state of the editor,
  • The ContextualBalloon will respond when the state of the editor changes (selection, focus) and query other plugins offering them a possibility to inject their views into the balloon.
  • The precise implementation is not clear yet because the system is too complex to anticipate all pitfalls,
  • One of the implementation ideas could be as follows:
    1. ContextualBalloon fires an #update event when the state of the editor has changed.
    2. Listeners in plugins like LinkUI or BalloonToolbar listen to that event.
    3. The plugins respond in the event, adding or removing their views from/to the ContextualBalloon or repositioning the balloon.
    4. The response works like a batch. ContextualBalloon resolves a batch (e.g. using a very low priority listener), settling the final state of its stack. The batch may have multiple "adds" and "removes".
    5. When adding a job to the batch (e.g. "add XView" or "remove YView") the plugins use priorities to resolve conflicts when two views could show up for a given editor state (e.g. a link in a table cell).
      • The system of priorities could be organized in named, pre-defined groups.
      • UI attached to the inline content would share the lowest priorities (e.g. 0-10), UI of the objects could operate at some other level (e.g. 10-50), etc.
    6. Some views in the ContextualBalloon's stack could be position setters. Once added, they would determine the position of the successive views on top of them, ensuring a fluid navigation for the users.
      • The BalloonToolbar would definitely be one of them,
      • Navigating BalloonToolbar->LinkActionsView->LinkFormView->LinkActionsView->... would use the position as initially determined by the BalloonToolbar (e.g. attached to the end of the selection, the tip of the balloon "north").
        • BTW, what if the successive views won't fit into the viewport? The toolbar could be small but the LinkActionsView could be huge and space may not be sufficient to display it. I guess the position inheritance system should be more like "use the position of the ancestor if possible; if not, use your own but at least make sure the tip of the balloon points toward the same spot"
    7. Since the update of the ContextualBalloon works like a batch (views are removed first, then one of the highest priority is added), the situation where switching the context (e.g. focusing a table when editing a link) should never end up with a table toolbar attached to the link because the link view or the underlying BalloonToolbar will be removed first, so no position setter will control the stack.
    8. To handle triggers other than EditorUI#update the LinkUI plugin could also either:
      • Tell ContextualBalloon to include view.document#click in ContextualBalloon#update,
      • Use the view.document#click listener internally to activate the feature, making it display the UI only upon the next ContextualBalloon#update,
      • TBH, it's hard for me to imagine how should that work but I'm pretty sure since everything is centralized around the ContextualBalloon#update event, some mechanism must be available to extend it to additional triggers

So this is what we have ATM:

image

And this is (more or less) what we could implement:

image

cc @pjasiun @oskarwrobel @Reinmar

@pjasiun
Copy link

pjasiun commented Dec 18, 2018

There is one more case to consider: what if 2 plugins add panels to the same element and both should be shown? An example might be spellchecking on the link. We can not make one cover the other and I do not see any option that one panel knows about the other.

@jodator
Copy link
Contributor

jodator commented Dec 18, 2018

add panels to the same element and both should be shown?

Recent case when one should be shown: https://github.com/ckeditor/ckeditor5-widget/issues/60.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-ui Oct 9, 2019
@mlewand mlewand added this to the nice-to-have milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:task This issue reports a chore (non-production change) and other types of "todos". package:ui labels Oct 9, 2019
@pjasiun
Copy link

pjasiun commented Oct 11, 2019

Working on comments and balloon rotator we realized that there are far more problems with the balloon system then we though. I am sure that @oskarwrobel we be able to write more about it, but I can name a few.

We should reconsider scenarios for balloon

There are more cases then we thought at the beginning.

There are cases when one toolbar should hide when the other appears - like inline toolbar -> link toolbar.

There are cases when two toolbars should be displayed together: for instance link toolbar and comments toolbar, it is hard to decide which one is more important. There is no navigation between these two plugins. They know nothing about each other. Right now we use rotator for such cases.

There are also mixed cases: for instance, image toolbar which lets you create a new comment thread attached to the image and inline comments which are also displayed in the balloon. Such a case is tricky because, on the one hand, there is a "comments" icon in the toolbar, it is possible to create a new comment thread and switch, from the image toolbar to the comments thread panel. But on the other, one image might have multiple comment threads, so a rotator is needed anyway.

Synchronise balloons life cycles

One of the problems, we met, working on the balloon system, was the fact that right now, each plugin defines its own rules when its balloon should appear or hide. They are not only attached to different events but also do not handle the same user actions in the same way. For instance, some balloons appear only on click in the text (like link), some also on keyboard actions (like inline toolbar), some debounce keyboard actions, some do not. This is something what should be unified to avoid randomly jumping balloons.

Synchronise balloon positioning

For some balloons, the default position is below the element (like for the balloon toolbar, when text is selected from left to right), for some, the default position is above the element (like for widget toolbar). This is a problem for comments balloon which can be attached to both text or widget and can be navigated from the regular toolbar. There should be some mechanism that helps us to keep the position of the toolbar. But I am not sure if we will be able to unify it.

Balloons outside the editable area

Implementing narrow sidebar we met a problem with integrating a toolbar that is attached to the element which is not a part of the editable area. I hope @oskarwrobel will be able to tell better about issues we had, but I remember that it was tricky how to make you one balloon visible and how to handle focus properly.

@oleq
Copy link
Member

oleq commented Mar 24, 2020

Related case #6443.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added resolution:expired This issue was closed due to lack of feedback. and removed status:stale labels Jan 15, 2024
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ui resolution:expired This issue was closed due to lack of feedback. status:discussion type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

8 participants