ViewportTopOffset and BalloonPanelView integration

[oleq]

Suggested merge commit message (convention)

Type: WIP. Closes #9892.

---

Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

[oleq]

@ckeditor/qa-team Could you verify if this solution for #9892 is safe, please? Essentially, it touches all kinds of balloons/popups/toolbars across the editor so there's a chance something went wrong.

Also, for more information, you can ping @dawidurbanski.

[LukaszGudel]

I'm testing this branch and it works great in our manual tests (all-features or tickets/9892/1.html). I've also built docs on this branch and there it looks worse 😕

On page http://127.0.0.1:8080/ckeditor5/29.0.0/features/table.html:

cke5-toolbar-on-long-table.mp4

cke5-toolbar-on-short-table.mp4

Table toolbars are hidden behind the header and never jumps to the bottom of the table.

[dawidurbanski]

I'm on it. Docs have to be reconfigured to use the new viewportTopOffset feature.

[dawidurbanski]

@LukaszGudel I updated table docs on this branch, but this part will probably get another update in the near future (when #9672 will be done).

You can re-check it in the current shape though.

[dawidurbanski]

Also @oleq @Reinmar this PR aims to do couple things at once. One of them is to change toolbar.viewportTopOffset config to ui.viewportOffset.top for all balloons. https://github.com/ckeditor/ckeditor5/pull/10239/files#diff-a35ba47e37e29680ac1908b00da274cdde808b8e8c80fd18e60c86faf7c27781R25

This makes sense because most of them has nothing to do with the toolbar.

So now the question becomes whether we keep both of these options. One for the toolbar and another one for the rest of balloons. Or do we make all balloons and toolbars to use the new ui.viewportOffset.top. Or maybe even a third option, to allow setting a separate value for toolbar with toolbar.viewportTopOffset and if not provided then default to ui.viewportOffset.top

Case can be made for all of them. The fact is that probably in most of cases (if not all of them), the toolbar offset will be matched with all the other UI elements positions offsets.

In case of making it all under ui.viewportOffset.top we probably have to introduce some way of letting people know they now should switch from toolbar.viewportTopOffset to ui.viewportOffset.top.

I was thinking of some nice option for plugins to be able to specify deprecated config properties.

Something like this (pseudo code):

editor.config.addDeprecationNotice( 'toolbar.vieportTopOffset', 'ui.viewportOffset.top' );

And now if someone uses the editor like so:

ClassicEditor
	.create( document.querySelector( '#editor' ), {
		plugins: [ ... ],
		toolbar: {
			items: [ ... ],
			viewportTopOffset: 100
		}
	} )

Will get a console warning similar to this one:

⚠️ The toolbar.vieportTopOffset configuration option is deprecated. It will be removed from future CKEditor versions. Use ui.viewportOffset.top instead.

The deprecation check could be injected in the get method in the main Config class, and more precisely in the _getFromSource method.

[Reinmar]

So now the question becomes whether we keep both of these options. One for the toolbar and another one for the rest of balloons. Or do we make all balloons and toolbars to use the new ui.viewportOffset.top. Or maybe even a third option, to allow setting a separate value for toolbar with toolbar.viewportTopOffset and if not provided then default to ui.viewportOffset.top

Less is usually better. So one option would be my choice.

However, the question to answer is whether it's a realistic and frequent enough use case to have to configure the toolbar independently from the rest of the UI.

My intuition says - no. Hence, still single config. But maybe @oleq has some use cases in his mind.

---

As for what to do when someone defined two properties if we'll agree that only one makes sense: To answer that we need to think about the migration path for integrators. That's the main scenario, to me, in which people will have both defined by mistake. And then, we should indeed log a warning.

Does warning people when they use the old property (but not the new one yet) make sense? I think so too. It depends on whether the old property will work the same way the new one will. If so, warning 90% of the integrators that they need to change their config has a limited sense (limited > zero). If, however, the new property has a broader effect, then warning makes more sense for sure.

Will get a console warning similar to this one:

⚠️ The toolbar.vieportTopOffset configuration option is deprecated. It will be removed from future CKEditor versions. Use ui.viewportOffset.top instead.

The deprecation check could be injected in the get method in the main Config class, and more precisely in the _getFromSource method.

That's not going to work because no one will use get() for that property anymore.

[dawidurbanski]

That's not going to work because no one will use get() for that property anymore.

Oh, yeah. Indeed. Of course we need to hook this up when setting the property in _setToTarget.

warning 90% of the integrators that they need to change their config has a limited sense (limited > zero). If, however, the new property has a broader effect, then warning makes more sense for sure.

I mean, if we plan to remove some property in the future, it obviously has a crucial effect. Or shall we keep supporting the old toolbar.vieportTopOffset forever?

[LukaszGudel]

@dawidurbanski I've checked the docs after your fixes and it works all right in the features/table section. I see two issues tho:

1. We have dozens of other editor demos throughout the docs. Most of them do have the table or image plugins and if I understand correctly, they would also need similar fixes? Right now they are bugged just like on the videos from the ViewportTopOffset and BalloonPanelView integration #10239 (comment). Also, I've noticed that not all of the demos have the sticky toolbar and on some of them (e.g. features/export-pdf.html or CF demos) the toolbar scrolls out of the viewport. How should they be handled?
2. Are we sure about the placement of the table/image toolbar when scrolling on large table/image? It covers the bottom half of the editor toolbar. Imagine that there were 2 more buttons on the left side of the toolbar on this screen:

The table/image toolbar would cover all of the alignment buttons and it would be hard to see which one aligns to the left/right/center. What's more, this image/table toolbar in this case covers the button's labels and it's impossible to read them on hover. I feel like UX in this case could be improved.

[dawidurbanski]

@LukaszGudel

if I understand correctly, they would also need similar fixes?

Well yes. But maybe not. It depends on how are we going to treat the old toolbar.viewportTopOffset option. If we decide we switch completely to ui.viewportOffset.top then I'll go through every single snippet in the docs and I'll update the config. If we decide to still support toolbar.viewportTopOffset for some time, we can hold on to this until we eventually remove it in which case we have to update all docs snippets.

For now it's undecided yet, so I just updated all snippets in a single table feature page just for showcase purposes.

Are we sure about the placement of the table/image toolbar when scrolling on large table/image?

I think we just settled on some lesser evil here with @oleq. The idea here is that it's better than it is currently (so it sticks at least), and we added some offset so even if sticky all buttons in the toolbar are at least clickable. We know it's not ideal, but that's what we came up with at this moment.

[dawidurbanski]

Summary of what has been done and what still needs to be done in this ticket

Done:

• First try: naive implementation of the sticky position tackled in [WIP] Fixed baloon position when viewportTopOffset is set #10120
• Second try: PoC for adding another position viewportStickyNorth by @oleq in ViewportTopOffset and BalloonPanelView integration #10239

In progress:

• Recreating ViewportTopOffset and BalloonPanelView integration #10239 step by step (with adding docs and test as we go) in a2c88fd

Todo:

• Finishing the in progress task, more precisely:
  • in depth review of ViewportTopOffset and BalloonPanelView integration #10239 (@dawidurbanski is already familiar with what is going on there)
  • adding the rest of the logic from ViewportTopOffset and BalloonPanelView integration #10239 to the new branch
  • write/modify tests for all the added and modified methods and functions
  • update / add missing API docs
  • stabilizing the new implementation

[dawidurbanski]

Closed in favour of #10373