From 1281530925986f6836f99fad62f7ce5806f8322b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksander=20Nowodzi=C5=84ski?= Date: Wed, 2 Oct 2024 16:48:26 +0200 Subject: [PATCH] Code refactoring and misc tests. --- .../menu/dropdownmenunestedmenuview.ts | 3 +-- .../dropdown/menu/dropdownmenurootlistview.ts | 4 +-- .../menu/dropdownmenunestedmenuview.js | 9 +++++++ packages/ckeditor5-ui/tests/dropdown/utils.js | 10 ++++++++ .../ckeditor5-ui/tests/editorui/editorui.js | 6 ++--- .../tests/toolbar/balloon/balloontoolbar.js | 25 ++++++++++++------- .../ckeditor5-ui/tests/toolbar/toolbarview.js | 18 +++++++++---- 7 files changed, 53 insertions(+), 22 deletions(-) diff --git a/packages/ckeditor5-ui/src/dropdown/menu/dropdownmenunestedmenuview.ts b/packages/ckeditor5-ui/src/dropdown/menu/dropdownmenunestedmenuview.ts index 36b7820c87f..d227a2e54a1 100644 --- a/packages/ckeditor5-ui/src/dropdown/menu/dropdownmenunestedmenuview.ts +++ b/packages/ckeditor5-ui/src/dropdown/menu/dropdownmenunestedmenuview.ts @@ -151,8 +151,6 @@ export default class DropdownMenuNestedMenuView extends View implements Focusabl this.listView = new DropdownMenuListView( locale ); this.listView.bind( 'ariaLabel' ).to( this.buttonView, 'label' ); - this.focusTracker.add( this.listView ); - this.panelView.content.add( this.listView ); const bind = this.bindTemplate; @@ -212,6 +210,7 @@ export default class DropdownMenuNestedMenuView extends View implements Focusabl this.focusTracker.add( this.buttonView.element! ); this.focusTracker.add( this.panelView.element! ); + this.focusTracker.add( this.listView ); // Listen for keystrokes coming from within #element. this.keystrokes.listenTo( this.element! ); diff --git a/packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts b/packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts index 56af4671ca8..54fbc0e4568 100644 --- a/packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts +++ b/packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts @@ -16,7 +16,7 @@ import { DropdownRootMenuBehaviors } from './dropdownmenubehaviors.js'; import type BodyCollection from '../../editorui/bodycollection.js'; import type { DropdownMenuDefinition } from './utils.js'; -import { type Locale, type BaseEvent, type FocusTracker } from '@ckeditor/ckeditor5-utils'; +import { type Locale, type BaseEvent } from '@ckeditor/ckeditor5-utils'; /** * Creates and manages a multi-level menu UI structure, suitable to be used inside dropdown components. @@ -85,8 +85,6 @@ export default class DropdownMenuRootListView extends DropdownMenuListView { */ declare public menuPanelClass: string | undefined; - declare public focusTracker: FocusTracker; - /** * The definitions object used to create the whole menu structure. */ diff --git a/packages/ckeditor5-ui/tests/dropdown/menu/dropdownmenunestedmenuview.js b/packages/ckeditor5-ui/tests/dropdown/menu/dropdownmenunestedmenuview.js index e608341d89c..d5d54d9559c 100644 --- a/packages/ckeditor5-ui/tests/dropdown/menu/dropdownmenunestedmenuview.js +++ b/packages/ckeditor5-ui/tests/dropdown/menu/dropdownmenunestedmenuview.js @@ -141,6 +141,15 @@ describe( 'DropdownMenuNestedMenuView', () => { sinon.assert.calledWithExactly( focusTrackerAddSpy.secondCall, menuView.panelView.element ); } ); + // https://github.com/cksource/ckeditor5-commercial/issues/6633 + it( 'should add the #listView to the focus tracker to allow for linking focus trackers and sharing state of nested menus', () => { + const focusTrackerAddSpy = sinon.spy( menuView.focusTracker, 'add' ); + + menuView.render(); + + sinon.assert.calledWithExactly( focusTrackerAddSpy.thirdCall, menuView.listView ); + } ); + it( 'should start listening to keystrokes', () => { const keystrokeHandlerAddSpy = sinon.spy( menuView.keystrokes, 'listenTo' ); diff --git a/packages/ckeditor5-ui/tests/dropdown/utils.js b/packages/ckeditor5-ui/tests/dropdown/utils.js index 9438ac1796c..10a6a845b6c 100644 --- a/packages/ckeditor5-ui/tests/dropdown/utils.js +++ b/packages/ckeditor5-ui/tests/dropdown/utils.js @@ -1391,6 +1391,16 @@ describe( 'utils', () => { expect( dropdownView.menuView.render.calledOnce ).to.be.true; } ); + it( 'should add the menu view to dropdown\'s focus tracker to allow for linking focus trackers and keeping track of the focus ' + + 'when it goes to sub-menus in other DOM sub-trees', + () => { + const addSpy = sinon.spy( dropdownView.focusTracker, 'add' ); + + addMenuToDropdown( dropdownView, body, definition ); + + sinon.assert.calledWithExactly( addSpy, dropdownView.menuView ); + } ); + it( 'should focus dropdown menu view after dropdown is opened', () => { addMenuToDropdown( dropdownView, body, definition ); diff --git a/packages/ckeditor5-ui/tests/editorui/editorui.js b/packages/ckeditor5-ui/tests/editorui/editorui.js index 9f4cfc53fd9..f36012fe6d8 100644 --- a/packages/ckeditor5-ui/tests/editorui/editorui.js +++ b/packages/ckeditor5-ui/tests/editorui/editorui.js @@ -530,13 +530,13 @@ describe( 'EditorUI', () => { } ); describe( 'for a ToolbarView that has already been rendered', () => { - it( 'adds ToolbarView#element to the EditorUI#focusTracker', () => { + it( 'adds ToolbarView to the EditorUI#focusTracker', () => { const spy = testUtils.sinon.spy( ui.focusTracker, 'add' ); toolbar.render(); ui.addToolbar( toolbar ); - sinon.assert.calledOnce( spy ); + sinon.assert.calledOnceWithExactly( spy, toolbar ); } ); it( 'adds ToolbarView#element to Editor#keystokeHandler', () => { @@ -559,7 +559,7 @@ describe( 'EditorUI', () => { await new Promise( resolve => { toolbar.once( 'render', () => { sinon.assert.calledOnce( spy ); - sinon.assert.calledOnce( spy2 ); + sinon.assert.calledOnceWithExactly( spy2, toolbar ); resolve(); } ); diff --git a/packages/ckeditor5-ui/tests/toolbar/balloon/balloontoolbar.js b/packages/ckeditor5-ui/tests/toolbar/balloon/balloontoolbar.js index f8163797510..729a727e81b 100644 --- a/packages/ckeditor5-ui/tests/toolbar/balloon/balloontoolbar.js +++ b/packages/ckeditor5-ui/tests/toolbar/balloon/balloontoolbar.js @@ -239,6 +239,13 @@ describe( 'BalloonToolbar', () => { expect( balloonToolbar.focusTracker.isFocused ).to.true; } ); + // https://github.com/cksource/ckeditor5-commercial/issues/6633 + it( 'should track the ToolbarView instance (not just its element) to allow using complex toolbar items scattered across DOM ' + + 'sub-trees and keep track of the focus', + () => { + expect( balloonToolbar.focusTracker.externalViews ).to.include( balloonToolbar.toolbarView ); + } ); + it( 'it should track the focus of the toolbarView#element', () => { expect( balloonToolbar.focusTracker.isFocused ).to.false; @@ -898,12 +905,12 @@ describe( 'BalloonToolbar', () => { const clock = sinon.useFakeTimers(); expect( balloonToolbar.focusTracker.isFocused ).to.false; - expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 3 ); + expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 4 ); editor.addRoot( 'dynamicRoot' ); // Check if newly added editable is tracked in focus tracker. - expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 4 ); + expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 5 ); // Check if element is added to focus tracker. const editableElement = editor.ui.getEditableElement( 'dynamicRoot' ); @@ -927,18 +934,18 @@ describe( 'BalloonToolbar', () => { const clock = sinon.useFakeTimers(); expect( balloonToolbar.focusTracker.isFocused ).to.false; - expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 3 ); + expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 4 ); editor.addRoot( 'dynamicRoot' ); const editableElement = editor.ui.getEditableElement( 'dynamicRoot' ); // Check if newly added editable is tracked in focus tracker. - expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 4 ); + expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 5 ); editor.detachRoot( 'dynamicRoot' ); // Check if element is removed from focus tracker. - expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 3 ); + expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 4 ); // Focus is no longer tracked. editableElement.focus(); @@ -955,19 +962,19 @@ describe( 'BalloonToolbar', () => { addEditableOnRootAdd = false; expect( balloonToolbar.focusTracker.isFocused ).to.false; - expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 3 ); + expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 4 ); editor.addRoot( 'dynamicRoot' ); const root = editor.model.document.getRoot( 'dynamicRoot' ); // Editable is not yet attached - expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 3 ); + expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 4 ); // Focus is no longer tracked. const editableElement = editor.createEditable( root ); global.document.body.appendChild( editableElement ); - expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 4 ); + expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 5 ); // Lets test focus editableElement.focus(); @@ -977,7 +984,7 @@ describe( 'BalloonToolbar', () => { // Detach editable element editor.detachEditable( root ); - expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 3 ); + expect( balloonToolbar.focusTracker.elements.length ).to.be.equal( 4 ); editableElement.remove(); clock.restore(); diff --git a/packages/ckeditor5-ui/tests/toolbar/toolbarview.js b/packages/ckeditor5-ui/tests/toolbar/toolbarview.js index b0737f1e0fd..ebc37908f98 100644 --- a/packages/ckeditor5-ui/tests/toolbar/toolbarview.js +++ b/packages/ckeditor5-ui/tests/toolbar/toolbarview.js @@ -256,28 +256,36 @@ describe( 'ToolbarView', () => { view.render(); - sinon.assert.calledOnce( spyAdd ); + sinon.assert.calledOnceWithExactly( spyAdd, view.element ); sinon.assert.notCalled( spyRemove ); view.destroy(); } ); - it( 'registers #items in #focusTracker', () => { + // https://github.com/cksource/ckeditor5-commercial/issues/6633 + it( 'registers #items in #focusTracker as View instances (not just DOM elements) to alow for complex Views scattered across ' + + 'multiple DOM sub-trees', + () => { const view = new ToolbarView( locale ); const spyAdd = sinon.spy( view.focusTracker, 'add' ); const spyRemove = sinon.spy( view.focusTracker, 'remove' ); - view.items.add( focusable() ); - view.items.add( focusable() ); + const focusableViewA = focusable(); + const focusableViewB = focusable(); + + view.items.add( focusableViewA ); + view.items.add( focusableViewB ); sinon.assert.notCalled( spyAdd ); view.render(); // 2 for items and 1 for toolbar itself. sinon.assert.calledThrice( spyAdd ); + sinon.assert.calledWithExactly( spyAdd.secondCall, focusableViewA ); + sinon.assert.calledWithExactly( spyAdd.thirdCall, focusableViewB ); view.items.remove( 1 ); - sinon.assert.calledOnce( spyRemove ); + sinon.assert.calledOnceWithExactly( spyRemove, focusableViewB ); view.destroy(); } );