Change viewportTopOffset config. Make viewportTopOffset writable in runtime

[dawidurbanski]

WIP (check additional information)

Suggested merge commit message (convention)

Type: Change viewportTopOffset config. Make viewportTopOffset writable in runtime. Closes #9672.

---

Additional information

I added couple TODO comments to point out some stuff that could be improved in this PR IMO.

@oleq if you would find some time to check this one it would be awesome.

These two manual tests are best to see this in action:

yarn manual --files=editor-classic/manual/tickets/9672/1.js,editor-inline/manual/tickets/9672/1.js

Follow ups on this one:

• Update all our tests to use the new config (ui.viewportOffset.top instead of toolbar.viewportTopOffset) done in d114b6b
• Update all docs snippets to use the new config done in e4429e5
• Update docs to reflect this change done in be2339d
• Update TODOs when discussed done in 39597fd and 83b130c

[oleq]

I think this is the right direction 👍

I read your comments @dawidurbanski but instead of replying under individual lines of code, I decided to fork your solution (ck/9672-modify-viewporTopOffset-in-runtime-v2) with a single commit so it is easier to understand what direction I think would be the best. And besides, unlike GH comments, you can run it straight away 😀

In short,

1. to avoid code duplication, I'm for moving the offset normalization to EditorUI. All editors with the UI inherit from this class so it's a safe spot IMO.
   • this makes the runtime config available for developers under editor.ui.viewportOffset
   • alternatively: this could be moved from EditorUI to EditorUIView. The only drawback is that the view needs to obtain the editor config via constructure which is, um..., an antipattern (our architecture assumes UI views are completely decoupled from the editor), but I guess this would also work. The property would be editor.ui.view.viewportOffset.
   • now that I think about it, EditorUIView could be a better place because EditorUI is a controller (glue) and this whole situation is just pure geometry (presentation). All editors with the UI use EditorUIView at some point. WDYT?
2. speaking of viewportOffset (instead of viewportTopOffset), I think it would be best if we made it future-proof and, as you correctly assumed in your code, used an object with top, bottom, left, right properties. For now, only the top makes sense but this will change one day.
   • the implementation in the twin issue (the one with the "sticky position") allows restricting the viewport Rect from all sides, anyway, so these changes would work together out-of-the-box.
3. moving on, I bound editor.ui.view.stickyPanel.viewportTopOffset (Classic) and InlineEditorUIView.viewportTopOffset (Inline) to EditorUI#viewportOffset.
   • this way, the whole viewport offset situation is transparent to developers. They don't need to worry about which editor type is used and which property should be changed in the runtime. There's a single source of truth for all editors with UI.

[dawidurbanski]

Thanks @oleq !

now that I think about it, EditorUIView could be a better place because EditorUI is a controller (glue) and this whole situation is just pure geometry (presentation). All editors with the UI use EditorUIView at some point. WDYT?

Let's stick to the the EditorUI with this. It felt awkward in the UIView for me, but we may still update this part though.

I rebased your ck/9672-modify-viewporTopOffset-in-runtime-v2 branch to this one so we can continue working here.

I also added some missing pieces, small refactoring, tests, some initial docs, and proper comments in 83b130c commit.

The only thing left now is to update rest of the docs including snippets and it should be job done. I'm going to complete it soon.

[Reinmar]

OK, I'm done with my review. The change works - the top offset is writable in runtime.

There are two things that I'd want to add:

• The migration guide is necessary and it doesn't need to be part of this ticket, but we need to do it. Please open a followup and ping me about it.
• It's a pity that the toolbar doesn't change its position once you update editor.ui.viewportOffset. I also checked if calling editor.ui.update() works but it doesn't. Could you make a quick check whether there's an option to update (recalculate) the position of it on demand. If so, please report a followup ticket too.

[dawidurbanski]

• The migration guide is necessary and it doesn't need to be part of this ticket, but we need to do it. Please open a followup and ping me about it.

Done: #10401

• It's a pity that the toolbar doesn't change its position once you update editor.ui.viewportOffset. I also checked if calling editor.ui.update() works but it doesn't. Could you make a quick check whether there's an option to update (recalculate) the position of it on demand. If so, please report a followup ticket too.

In these two tests:

yarn manual --files=editor-classic/manual/tickets/9672/1.js,editor-inline/manual/tickets/9672/1.js

We actually just set new viewportOffset by clicking on a button outside of the editor. This means we need to focus back in to it after it's done. We do make this step automated by adding editor.editing.view.focus(); on button click after we are done with updating offset to new, random one.

Apparently focusing out and back in is sufficient to reposition the toolbar.

Example:

const newOffset = 200;
document.documentElement.style.setProperty( '--top-offset', `${ newOffset }px` );
editor.ui.viewportOffset = { top: newOffset };
editor.editing.view.focus();

I added an additional, naive case to both of these tests. An another button to start updating top offset randomly in two seconds interval. This way we do keep focused in the editor while offset is being updated.

Indeed in such case editor toolbar does not get updated.

I found we just need to fire layoutChanged event after updating the offset.

Example:

const newOffset = 200;
document.documentElement.style.setProperty( '--top-offset', `${ newOffset }px` );
editor.ui.viewportOffset = { top: newOffset };
editor.editing.view.document.fire( 'layoutChanged' );

In my opinion It's just a matter of proper docs to describe these cases.

I created a ticket for it as well: #10402

[Reinmar]

Indeed in such case editor toolbar does not get updated.

That's the case to worry about. It's clear that re-focusing resolves this issue. And it's good there's even a better workaround with layoutChanged. However, both of these are hacks. The straightforward solution would be if calling editor.ui.update() worked because that method is supposed to be called whenever the editor's UI should be updated due to some external changes. I'll write that in the issue you opened too.

[Reinmar]

FYI: The merge commit message that I went with:

Feature: Introduced `editor.ui.viewportOffset` that allows modifying in runtime the viewport's offset. This value is used by the editor to e.g. position its sticky toolbar and contextual balloons. Additionally, the `config.toolbar.viewportTopOffset` property was moved to `config.ui.viewportOffset` and it now accetps an object. Closes #9672.

BREAKING CHANGE: The `config.toolbar.viewportTopOffset` property was moved to `config.ui.viewportOffset` and it now accetps an object.

[wimleers]

Looks like this also unblocked https://www.drupal.org/project/drupal/issues/3259380 per @lauriii! 🥳