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

Fix scrolling in dropdowns when block toolbar button is active #17123

Merged
merged 1 commit into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/ckeditor5-ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"@ckeditor/ckeditor5-table": "43.1.0",
"@ckeditor/ckeditor5-typing": "43.1.0",
"@ckeditor/ckeditor5-undo": "43.1.0",
"@ckeditor/ckeditor5-code-block": "43.1.0",
"@types/color-convert": "2.0.0",
"typescript": "5.0.4",
"webpack": "^5.94.0",
Expand Down
18 changes: 17 additions & 1 deletion packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
} from '@ckeditor/ckeditor5-core';

import {
type EventInfo,
getAncestors,
global,
Rect,
ResizeObserver,
Expand Down Expand Up @@ -446,11 +448,25 @@ export default class BlockToolbar extends Plugin {
let pendingAnimationFrame = false;

// Reposition the button on scroll, but do it only once per animation frame to avoid performance issues.
const repositionOnScroll = () => {
const repositionOnScroll = ( evt: EventInfo, domEvt: Event ) => {
if ( pendingAnimationFrame ) {
return;
}

// It makes no sense to reposition the button when the user scrolls the dropdown or any other
// nested scrollable element. The button should be repositioned only when the user scrolls the
// editable or any other scrollable parent of the editable. Leaving it as it is buggy on Chrome
// where scrolling nested scrollables is not properly handled.
// See more: https://github.com/ckeditor/ckeditor5/issues/17067
const editableElement = this._getSelectedEditableElement();

if (
domEvt.target !== global.document &&
!getAncestors( editableElement ).includes( domEvt.target as HTMLElement )
) {
return;
}

pendingAnimationFrame = true;
global.window.requestAnimationFrame( () => {
this._updateButton();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import BalloonEditor from '@ckeditor/ckeditor5-editor-balloon/src/ballooneditor.js';
import Essentials from '@ckeditor/ckeditor5-essentials/src/essentials.js';
import List from '@ckeditor/ckeditor5-list/src/list.js';
import CodeBlock from '@ckeditor/ckeditor5-code-block/src/codeblock.js';
import Image from '@ckeditor/ckeditor5-image/src/image.js';
import ImageCaption from '@ckeditor/ckeditor5-image/src/imagecaption.js';
import { Paragraph, ParagraphButtonUI } from '@ckeditor/ckeditor5-paragraph';
Expand All @@ -17,9 +18,12 @@ import BlockToolbar from '../../../src/toolbar/block/blocktoolbar.js';

BalloonEditor
.create( document.querySelector( '#editor' ), {
plugins: [ Essentials, List, Paragraph, Heading, Image, ImageCaption, HeadingButtonsUI, ParagraphButtonUI, BlockToolbar ],
plugins: [
Essentials, List, Paragraph, Heading, Image, ImageCaption,
HeadingButtonsUI, ParagraphButtonUI, BlockToolbar, CodeBlock
],
blockToolbar: [
'paragraph', 'heading1', 'heading2', 'heading3', 'bulletedList', 'numberedList', 'paragraph',
'paragraph', 'heading1', 'heading2', 'heading3', 'bulletedList', 'numberedList', 'paragraph', 'codeBlock',
'heading1', 'heading2', 'heading3', 'bulletedList', 'numberedList', 'paragraph', 'heading1', 'heading2', 'heading3',
'bulletedList', 'numberedList'
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import Essentials from '@ckeditor/ckeditor5-essentials/src/essentials.js';
import List from '@ckeditor/ckeditor5-list/src/list.js';
import Image from '@ckeditor/ckeditor5-image/src/image.js';
import ImageCaption from '@ckeditor/ckeditor5-image/src/imagecaption.js';
import CodeBlock from '@ckeditor/ckeditor5-code-block/src/codeblock.js';
import { Paragraph, ParagraphButtonUI } from '@ckeditor/ckeditor5-paragraph';
import Heading from '@ckeditor/ckeditor5-heading/src/heading.js';
import HeadingButtonsUI from '@ckeditor/ckeditor5-heading/src/headingbuttonsui.js';
Expand All @@ -26,9 +27,12 @@ createBlockButtonEditor( '#editor-scrollable' ).then( editor => {
function createBlockButtonEditor( element ) {
return BalloonEditor
.create( document.querySelector( element ), {
plugins: [ Essentials, List, Paragraph, Heading, Image, ImageCaption, HeadingButtonsUI, ParagraphButtonUI, BlockToolbar ],
plugins: [
Essentials, List, Paragraph, Heading, Image, ImageCaption,
HeadingButtonsUI, ParagraphButtonUI, BlockToolbar, CodeBlock
],
blockToolbar: [
'paragraph', 'heading1', 'heading2', 'heading3', 'bulletedList', 'numberedList', 'paragraph',
'paragraph', 'heading1', 'heading2', 'heading3', 'bulletedList', 'numberedList', 'paragraph', 'codeBlock',
'heading1', 'heading2', 'heading3', 'bulletedList', 'numberedList', 'paragraph', 'heading1', 'heading2', 'heading3',
'bulletedList', 'numberedList'
]
Expand Down
23 changes: 23 additions & 0 deletions packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,29 @@ describe( 'BlockToolbar', () => {

expect( spy ).to.be.calledOnce;
} );

it( 'should not _call _clipButtonToViewport when event target is not editable ancestor', () => {
const spy = sinon.spy( blockToolbar, '_clipButtonToViewport' );

buttonView.isVisible = true;

document.body.dispatchEvent( new Event( 'scroll' ) );
clock.tick( 100 );
expect( spy ).to.be.called;

spy.resetHistory();

// Create a fake parent element and dispatch scroll event on it.
// It's not a button ancestor so _clipButtonToViewport should not be called.
const evt = new Event( 'scroll' );
const fakeParent = document.createElement( 'div' );

sinon.stub( evt, 'target' ).value( fakeParent );
document.body.dispatchEvent( evt );
clock.tick( 100 );
fakeParent.remove();
expect( spy ).not.to.be.called;
} );
} );

describe( '_clipButtonToViewport()', () => {
Expand Down