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

Widget toolbar covers editor's sticky toolbar when scrolling #15744

Closed
FilipTokarski opened this issue Jan 24, 2024 · 1 comment · Fixed by #16780
Closed

Widget toolbar covers editor's sticky toolbar when scrolling #15744

FilipTokarski opened this issue Jan 24, 2024 · 1 comment · Fixed by #16780
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:ui squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@FilipTokarski
Copy link
Member

📝 Provide detailed reproduction steps (if any)

  1. Go to demo
  2. Add a table
  3. Focus table and scroll the content down

✔️ Expected result

Table toolbar detects editor's sticky toolbar and moves down to the bottom of the table.

❌ Actual result

Table toolbar does not detect sticky toolbar and covers it. It only moves to the bottom of the table when it reaches top border of the editor (or rather the top website navigation bar). This happens also for other widget toolbars, for example image.

Screen.Recording.2024-01-24.at.09.59.10.mov

❓ Possible solution

If you have ideas, you can list them here. Otherwise, you can delete this section.

📃 Other details

  • Browser: …
  • OS: …
  • First affected CKEditor version: …
  • Installed CKEditor plugins: …

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@FilipTokarski FilipTokarski added type:bug This issue reports a buggy (incorrect) behavior. package:ui domain:ui/ux This issue reports a problem related to UI or UX. support:2 An issue reported by a commercially licensed client. squad:core Issue to be handled by the Core team. labels Jan 24, 2024
@oleq
Copy link
Member

oleq commented Jan 24, 2024

We already have two integrations for Dialog positioning and scrollToTheSelection()

/**
* Provides an integration between the sticky toolbar and {@link module:utils/dom/scroll~scrollViewportToShowTarget}.
* It allows the UI-agnostic engine method to consider the geometry of the
* {@link module:editor-classic/classiceditoruiview~ClassicEditorUIView#stickyPanel} that pins to the
* edge of the viewport and can obscure the user caret after scrolling the window.
*
* @param evt The `scrollToTheSelection` event info.
* @param data The payload carried by the `scrollToTheSelection` event.
* @param originalArgs The original arguments passed to `scrollViewportToShowTarget()` method (see implementation to learn more).
*/
private _handleScrollToTheSelectionWithStickyPanel(
evt: EventInfo<'scrollToTheSelection'>,
data: ViewScrollToTheSelectionEvent[ 'args' ][ 0 ],
originalArgs: ViewScrollToTheSelectionEvent[ 'args' ][ 1 ]
): void {
const stickyPanel = this.view.stickyPanel;
if ( stickyPanel.isSticky ) {
const stickyPanelHeight = new Rect( stickyPanel.element! ).height;
data.viewportOffset.top += stickyPanelHeight;
} else {
const scrollViewportOnPanelGettingSticky = () => {
this.editor.editing.view.scrollToTheSelection( originalArgs );
};
this.listenTo( stickyPanel, 'change:isSticky', scrollViewportOnPanelGettingSticky );
// This works as a post-scroll-fixer because it's impossible predict whether the panel will be sticky after scrolling or not.
// Listen for a short period of time only and if the toolbar does not become sticky very soon, cancel the listener.
setTimeout( () => {
this.stopListening( stickyPanel, 'change:isSticky', scrollViewportOnPanelGettingSticky );
}, 20 );
}
}
/**
* Provides an integration between the sticky toolbar and {@link module:ui/dialog/dialog the Dialog plugin}.
*
* It moves the dialog down to ensure that the
* {@link module:editor-classic/classiceditoruiview~ClassicEditorUIView#stickyPanel sticky panel}
* used by the editor UI will not get obscured by the dialog when the dialog uses one of its automatic positions.
*/
private _initDialogPluginIntegration(): void {
if ( !this.editor.plugins.has( 'Dialog' ) ) {
return;
}
const stickyPanel = this.view.stickyPanel;
const dialogPlugin: Dialog = this.editor.plugins.get( 'Dialog' );
dialogPlugin.on( 'show', () => {
const dialogView = dialogPlugin.view!;
dialogView.on<DialogViewMoveToEvent>( 'moveTo', ( evt, data ) => {
// Engage only when the panel is sticky, and the dialog is using one of default positions.
if ( !stickyPanel.isSticky || dialogView.wasMoved ) {
return;
}
const stickyPanelContentRect = new Rect( stickyPanel.contentPanelElement );
if ( data[ 1 ] < stickyPanelContentRect.bottom + DialogView.defaultOffset ) {
data[ 1 ] = stickyPanelContentRect.bottom + DialogView.defaultOffset;
}
}, { priority: 'high' } );
}, { priority: 'low' } );
}

Maybe there’s a similar way to hook into the ContextualBalloon system?

@Mati365 Mati365 self-assigned this Jul 22, 2024
niegowski added a commit that referenced this issue Aug 5, 2024
Fix (editor-classic): Widget toolbar no longer covers editor's sticky toolbar when scrolling. Closes #15744.
@CKEditorBot CKEditorBot added this to the iteration 77 milestone Aug 5, 2024
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:ui squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants