From c28ad8f59c6513e75ae7b1830d692fede44fb81a Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Thu, 19 Sep 2024 09:30:33 +0200 Subject: [PATCH] Do not reposition block button when nested dropdown is being scrolled. --- packages/ckeditor5-ui/package.json | 1 + .../src/toolbar/block/blocktoolbar.ts | 18 ++++++++++++++- .../tests/manual/blocktoolbar/blocktoolbar.js | 8 +++++-- .../blocktoolbar/scrollable-blocktoolbar.js | 8 +++++-- .../tests/toolbar/block/blocktoolbar.js | 23 +++++++++++++++++++ 5 files changed, 53 insertions(+), 5 deletions(-) diff --git a/packages/ckeditor5-ui/package.json b/packages/ckeditor5-ui/package.json index a16006a7a9f..f9c08fcff0e 100644 --- a/packages/ckeditor5-ui/package.json +++ b/packages/ckeditor5-ui/package.json @@ -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", diff --git a/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.ts b/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.ts index 072f85a8060..634a48fe761 100644 --- a/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.ts +++ b/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.ts @@ -15,6 +15,8 @@ import { } from '@ckeditor/ckeditor5-core'; import { + type EventInfo, + getAncestors, global, Rect, ResizeObserver, @@ -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(); diff --git a/packages/ckeditor5-ui/tests/manual/blocktoolbar/blocktoolbar.js b/packages/ckeditor5-ui/tests/manual/blocktoolbar/blocktoolbar.js index 5b3ee43e998..c8586343569 100644 --- a/packages/ckeditor5-ui/tests/manual/blocktoolbar/blocktoolbar.js +++ b/packages/ckeditor5-ui/tests/manual/blocktoolbar/blocktoolbar.js @@ -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'; @@ -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' ] diff --git a/packages/ckeditor5-ui/tests/manual/blocktoolbar/scrollable-blocktoolbar.js b/packages/ckeditor5-ui/tests/manual/blocktoolbar/scrollable-blocktoolbar.js index 4bcaa04eae3..b5c1cfd6819 100644 --- a/packages/ckeditor5-ui/tests/manual/blocktoolbar/scrollable-blocktoolbar.js +++ b/packages/ckeditor5-ui/tests/manual/blocktoolbar/scrollable-blocktoolbar.js @@ -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'; @@ -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' ] diff --git a/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js b/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js index b0ac0dd103f..ff5500ee0fc 100644 --- a/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js +++ b/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js @@ -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()', () => {