feat: patch the `balloontoolbar` and `balloonpanel` plugins

[julien]

These changes contain the necessary patch to make ckeditor's balloontoolbar plugin behave (more or less) like Alloy Editor's toolbars.

Default behavior

Patched

[wincent]

Just a product-level question here; are we trying to match the Alloy Editor "triangle side" behavior? Seems like we do have some control over that here. FWIW, the Alloy Editor behavior, if I understand it correctly, is:

• Double-click on a word: triangle is on bottom (floating toolbar is above).
• Click on an image etc: triangle is on bottom (floating toolbar is above).
• Drag over a selection range upwards: triangle is on bottom (floating toolbar is above).
• Drag over a selection range downwards: triangle is on top (floating toolbar is below).

[julien]

@wincent let me double check that (because it's something I modified at the last minute)

Concerning design (@liferay-design) there's nothing defined (in their Figma document, the toolbar is always on top, so it's something I also wanted to clarify with them) but @dsanz suggested we match Alloy Editor's behavior, and I might have done it the other way around.

[dsanz]

Yes, design documentation is not dealing with that detail about toolbar positioning. I suggested to get inspiration from AE behavior (not necessarily 100% faithful) as it looks very reasonable, in the terms expressed by Greg:

• Any single line selection produces text toolbar to be rendered above it, if possible, keeping the toolbar within the editor area (AE does not care about this last bit)
• Multi-line selection produce text toolbar to be rendered close to the selections "last end", so, selecting upwards renders the toolbar "above" the selection whereas selecting downwards renders "below" the selection. This makes toolbar visible when selection is big and requires vertical scroll.
• In all cases, toolbar should not overlap selection, and the "triangle" should point to the selection

We may get some advice/confirmation from @victorvalle, at least to double check this makes sense for him Best

[julien]

I've updated the PR.

Any single line selection produces text toolbar to be rendered above it, if possible, keeping the toolbar within the editor area (AE does not care about this last bit)

Multi-line selection produce text toolbar to be rendered close to the selections "last end", so, selecting upwards renders the toolbar "above" the selection whereas selecting downwards renders "below" the selection. This makes toolbar visible when selection is big and requires vertical scroll.

In all cases, toolbar should not overlap selection, and the "triangle" should point to the selection

Concerning the "triangle" should point to the selection, that's one thing I haven't managed to get although spending time thinking about it, (maybe you guys have an idea). With alloy editor, the triangle is always centered, and because we don't care about the toolbar being out of the "viewport", we don't have to change the triangle's position

In the case of Balloon Editor we would need to somehow "map/normalize" the position of the selection (or the last range of the selection) in the toolbar and that's something I haven't been able to get correctly (it's not about mapping from one range to the other, that's actually not so hard) but the visual result is not really accurate.

I'll let you decide if this is good enough for a first iteration, otherwise I can keep on trying.

[victorvalle]

Please @drakonux take a look at this issue.

[wincent]

I'll let you decide if this is good enough for a first iteration, otherwise I can keep on trying.

Nice progress! I like it. Let's see what @drakonux says.

[julien]

In the case of Balloon Editor we would need to somehow "map/normalize" the position of the selection (or the last range of the selection) in the toolbar and that's something I haven't been able to get correctly (it's not about mapping from one range to the other, that's actually not so hard) but the visual result is not really accurate.

:facepalm: I might be actually making this more complex than it is ... I'm not 100% sure why I went down that rabbit hole instead of actually changing the triangle CSS position property to absolute, and placing it where the selection starts, center or end. I'll give this another try just to confirm.

[dsanz]

I think that for the first iteration, it's ok if triangle just "points" to the selection, in the center of the toolbar. Keeping the bar inside the editing area as much as possible helps to prevent other issues more important than the triangle positioning.

The price to pay is that triangle may be rendered not that close to the selection. Particularly, if the following conditions are met:

• Selection is small, meaning the start/end are close enough
• Selection is close to the left/right border of the editing area
• The toolbar is wide

Note that conditions above can be met both with single-line selection and a two-line selection starting close to the right side, and ending close to the left side. This last case may be a bit tricky when it comes to decide where to place the bar, but as it falls into the "upwards"/"downwards" type of selection, it can in fact be reduced to the single-line case in terms of triangle positioning.

I would just go ahead if we're sure there's a simple to implement solution which accounts for the cases above and is able to position the triangle somewhere in the vertical overlap between the selection and the toolbar.

[drakonux]

I talked with @julien offline but just a gentle reminder.

Keeping the bar inside the editing area as much as possible helps to prevent other issues more important than the triangle positioning

This is a good intention, but we currently have several cases where there is little wide space, such as text areas within sidepanels. The balloon toolbar will be constrained, and the designer would have to reduce the available formatting options or apply an overflow behavior. This isn't necessarily a bad thing because we discourage designers from providing many options, but I would say that it's a product decision.

Edited: IMO I would show the ballon outside of the viewport by default, and with the same logic, we made for tooltips. If the ~tooltip~ ballon can't be shown in the default direction (above), move the triangle to the direction where it's the constrain and, if-else, change to another orientation and so on. That would solve all the corner cases.

[julien]

@drakonux that previous comment made me slightly confused.

If the tooltip can't be shown in the default direction (above), move the triangle to the direction where it's the constrain and, if-else, change to another orientation and so on

Since there isn't any tooltip in our Balloon Editor, I guess you meant to say something else. (Am I right?)

What I understand is that if the toolbar can't be shown inside the viewport, we should find the best placement, which is what @dsanz meant by keeping it inside the viewport (at least that's how I understand it).

As far as the "triangle" is concerned, it's true that it adds some visual value (does it really though?) ... but only if it's placed in the right position.For me, that would be where the user "ends" selecting: if it's an element (like an image, a table or a video) it probably should be centered on top or bellow the element depending on the selection's direction, and it it's text it should be placed on top, bellow or in the center (if you've just selected an entire paragraph or a whole line).

Of course, that's only what I think and I'm not saying it has to be that way but since that's something that I've not been able to achieve, at least for now; I would suggest hiding that triangle until I find the correct solution.

Once again, I'm just trying to find a solution to move this forward and I thought I was going in the right direction according to the Technical Analysis and requirements document, but now I just have more doubts.

[julien]

For anyone following this discussion, here's an update to my previous comment.

@drakonux and I just had a call via Slack.

The good thing is that we're actually aiming for the same thing, and this also goes in the same direction as the "technical analysis and requirements".

I'll resume the best I can.

Concerning the editor's toolbar position:

We would like to constrain it to the editing area (keep it inside the editor's viewport), but what @drakonux had to say (and he'll correct me if I'm wrong) is that this also means that this toolbar can't have an large number of buttons; which is something some teams might want to do. We can of course suggest/recommend this, but we can't impose it.

Not constraining the toolbar's position might lead to situation like this:

Which is something we'd like to avoid. It also seems logical that configuring a toolbar with lots of buttons in a "small" editing area (in a sidebar for example) doesn't make sense.

.

We also think that if the toolbar can't be placed where it should be, then we need to find the best placement (that's what @drakonux was referring to when he mentioned the tooltip behavior).

For example, let's say we select something that's on the "top" of the page:

Instead of seeing something like this

We'd rather want to see this

Another thing @drakonux was suggesting on this call, is that we could provide an option to let the developers decide if they want this behavior, if they don't care about the toolbar being always inside the editing area, then so be it.

And concerning the "triangle", we agree that the ideal behavior is what @dsanz described, or to illustrate with screenshots

Top to bottom selection on an entire paragraph

Bottom to top selection

"Single" selection (For text)

(For images, tables, videos)

both @drakonux and I think that if this is not possible at the moment, for now we could simply hide the triangle (simply not include it).

[wincent]

Just kidding... I think your analysis sounds great. ie. make this behave as nicely as possible in as many edge cases as possible (where "possible" here actually means "pragmatically reasonable").

we could provide an option to let the developers decide if they want this behavior

This is the only aspect of this that I would push back on. Getting this to work well in a range of edge case conditions is going to be hard, and making it configurable is going to multiply that difficulty in the name of "letting developers do stupid things". So I'd leave that for last, and ideally, not even implement it at all.

[julien]

This is the only aspect of this that I would push back on. Getting this to work well in a range of edge case conditions is going to be hard, and making it configurable is going to multiply that difficulty in the name of "letting developers do stupid things". So I'd leave that for last, and ideally, not even implement it at all.

I agree, because that's going to bring more complexity to the table ... I added it because it was mentioned, so thanks for giving your opinion on this.

[dsanz]

I'll do my best to focus the discussion on the two main topics:

Triangle

we agree that the ideal behavior is what @dsanz described, or to illustrate with screenshots

I think there's some sort of agreement about what's the ideal behavior. This does not mean that we want it implemented at any cost.

Of course, that's only what I think and I'm not saying it has to be that way but since that's something that I've not been able to achieve, at least for now; I would suggest hiding that triangle until I find the correct solution.

I'm fine with completely hiding it if there is no straightforward implementation. I don't think it provides much value on its own, so let's move on with what we have and if our tests tell us that behavior is not ok, let's hide it.

Toolbar positioning

What I understand is that if the toolbar can't be shown inside the viewport, we should find the best placement, which is what @dsanz meant by keeping it inside the viewport (at least that's how I understand it).

That's correct Julien. I would summarize this as follows:

• We should take as much as editing space width as possible to render the bar. It does not make any sense to exceed any editor vertical/horizontal border when there is unused space in the editing area. This applies both horizontally and vertically
• If space is not enough, then toolbar will render according to an overflow policy. There are many, and they're different depending on whether we talk about horizontal or vertical overflow. Just to illustrate:
  • Horizontal overflow policy examples:
  • Exceed the editor border: overflow by the side closest to the end of the selection, but keeping it within browser window limits.
  • Shrink the toolbar by placing some buttons in a drop down: this is an old idea we had for the CKEditor general toolbar, not yet implemented. We may borrow some ideas
  • Vertical overflow policy examples:
  • For single-line selections, place it above the selection unless it overflows top border. In that case, place it below.
  • For multiple-line selections, switch the placement to the "other end" if the primary end makes toolbar to be placed outside the editing area, and the other end prevents it.
  • Otherwise, overlap with the selection or exceed bottom

Another thing @drakonux was suggesting on this call, is that we could provide an option to let the developers decide if they want this behavior, if they don't care about the toolbar being always inside the editing area, then so be it.

Default policies shall be part of the core editor behavior. I don't believe this has to be configurable in the sense of offering alternate behavior. I would just implement something reasonable (see below), then create an extension point for those not satisfied with the overflow policy we come up with. That's how far I would go.

Reasonable, for me, means:

• Horizontal overflow policy: exceed
• Vertical overflow policy: single & multiple line cases, exceed bottom

is that this also means that this toolbar can't have an large number of buttons; which is something some teams might want to do. We can of course suggest/recommend this, but we can't impose it.

Our role as component providers is to offer reasonable good defaults. In the end, products using the editor will for sure lead to situations where the toolbar may not fit. Just think on a grid in the content editor with many columns. For cases like these, the sum of some reasonable defaults + our recommendations for the teams using the editor should do. And in the end, there is the option of changing the policies via extension.

[julien]

Triangle

I'm fine with completely hiding it if there is no straightforward implementation. I don't think it provides much value on its own, so let's move on with what we have and if our tests tell us that behavior is not ok, let's hide it.

Great! That's done in the latest commit

Toolbar positioning

Exceed the editor border: overflow by the side closest to the end of the selection, but keeping it within browser window limits.

That's another way to view it: for now the "bounds" I'm taking into account are the bounds of the editable area. I think the screenshots in this PR are OK enough to see how this currently works.

Now to illustrate what I'd like to understand about "browser window limits"; and since this is a bit difficult to explain, he's a (very simplified) image to clear some doubts.

Let's imagine blogs-web (or any editor in DXP actually): would this mean that we could have the toolbar "be outside" of the editor's bounds like in this image (the black rectangle is supposed to represent the balloon toolbar)?

This looks a bit strange to me.

Shrink the toolbar by placing some buttons in a drop down: this is an old idea we had for the CKEditor general toolbar, not yet implemented. We may borrow some ideas

That's a very good idea in terms of UX, but unfortunately, as far as we've seen with @ambrinchaudhary (and already reported) the balloon toolbar does not support buttons that have drop down panels, (like background color or text color plugins for example). So we'll have to patch that too, and it might take some time to make it work. I personally wouldn't bet on that being a possibility for now.

Default policies shall be part of the core editor behavior. I don't believe this has to be configurable in the sense of offering alternate behavior. I would just implement something reasonable (see below), then create an extension point for those not satisfied with the overflow policy we come up with. That's how far I would go.

@wincent gave his point of view on that here, but to resume

Getting this to work well in a range of edge case conditions is going to be hard, and making it configurable is going to multiply that difficulty in the name of "letting developers do stupid things"

I have to say that I agree. On top of that, we should keep in mind that to make this work like we'd like to, we're already patching existing code, and I'd like to avoid more complexity because next time we update CKEditor (or if anything changes in upstream), those patches might be hard to re-apply.

Additionally I can't really see a clear benefit between providing options or an "extension point" (both are probably a bad idea). In the case of "extension points", would that mean something like

If the use provides his "own" positioning function then use that instead of "our" positioning logic

Keeping in mind that the developer/user might not provide the correct code, at which point he'll either ask us for support or say our "extension point" system doesn't work. For me this is just asking for trouble.

[dsanz]

Toolbar positioning

Exceed the editor border: overflow by the side closest to the end of the selection, but keeping it within browser window limits.

That's another way to view it: for now the "bounds" I'm taking into account are the bounds of the editable area. I think the screenshots in this PR are OK enough to see how this currently works.

Now to illustrate what I'd like to understand about "browser window limits"; and since this is a bit difficult to explain, he's a (very simplified) image to clear some doubts.

Let's imagine blogs-web (or any editor in DXP actually): would this mean that we could have the toolbar "be outside" of the editor's bounds like in this image (the black rectangle is supposed to represent the balloon toolbar)?

This looks a bit strange to me.

Oh, I think there is a misunderstanding about when that policy applies. When I mentioned "exceed the editor border" as an overflow policy, I meant to apply it when all editor viewport width is consumed by the toolbar, and not before, like image suggests.

So, firstly, the toolbar should be placed within the editable area, and only when that's not possible, the overflow policy kicks in. That policy determines what to do when there is an overflow, it has nothing to say when there is not. Once there's no way to make the toolbar fit within the editing area, the next thing to worry about is to ensure it's not outside the browser window, yet remains on top of other elements outside the editor.

Shrink the toolbar by placing some buttons in a drop down: this is an old idea we had for the CKEditor general toolbar, not yet implemented. We may borrow some ideas

That's a very good idea in terms of UX, but unfortunately, as far as we've seen with @ambrinchaudhary (and already reported) the balloon toolbar does not support buttons that have drop down panels, (like background color or text color plugins for example). So we'll have to patch that too, and it might take some time to make it work. I personally wouldn't bet on that being a possibility for now.

Shrinking the toolbar is definitely out of the scope of this epic. We may consider this later on depending on user demand and priority. That's why I mentioned the other option "exceed editor borders" as reasonable.

Additionally I can't really see a clear benefit between providing options or an "extension point" (both are probably a bad idea). In the case of "extension points", would that mean something like

If the use provides his "own" positioning function then use that instead of "our" positioning logic

Keeping in mind that the developer/user might not provide the correct code, at which point he'll either ask us for support or say our "extension point" system doesn't work. For me this is just asking for trouble.

I agree with Greg and you about not making this configurable. The extension point notion I had in mind is as you described: giving the chance to the editor user to provide their positioning logic. Still, I have to say that currently, I don't see any value for this. I would consider adding it in case we find out that edge cases require particular solutions. So let's keep it out of scope of this epic too.

[julien]

@dsanz Thanks. I think all the points mentioned are covered by these changes. Did you have a chance to test and evaluate if this was good enough to be merged for a first iteration?

[julien]

I'm still waiting for a reply, but I also wanted to let you know that I've been working on the "plus" button and I wanted to share a .gif

This means that I'll probably have to modify my existing patch (and that will also be the case for many other features).

I also thought I'd share that the easiest way to check this "patched" version, is to clone this repository

[jbalsas]

Hey @julien, just a thought here. Patching these plugins here might have the unintended side effect that they'll stop working or work in unexpected ways in the default usecase (CKEditor).

Did you explore the possibility of creating a different plugin or somehow swapping the implementations so the out of the box behaviour was still present? Something like this could mean that the effect can be achieved directly in DXP instead of in this fork.

[julien]

@jbalsas, yes this is something I've also thought of very recently.

I initially started working on the existing plugins to get a better understanding of how they work, but now that you mention it, I think creating a new plugin that overrides balloontoolbar and balloonpanel (via CKEDITOR.tools.override) might a better solution: as you said, it will guarantee that these plugins keep on working the way they should, and we also avoid problems when updating the ckeditor-dev submodule (which was something I wanted to avoid). So I'll work on that, thanks for the suggestion.

[julien]

Closing this one for now. I'll send a new PR soon without patches, since this is definitely a better approach.