Skip to content

Commit

Permalink
Merge pull request #17206 from ckeditor/cc/6633-v2a
Browse files Browse the repository at this point in the history
Fix (ui): The dropdown menu component should not cause an editor blur if used in a `BalloonToolbar` while the user hovers a nested menu. Closes #17277.

Feature (utils): Made `FocusTracker` extendable with other `FocusTracker` instances to allow logical focus tracking across separate DOM sub-trees (see #17277)
  • Loading branch information
oleq authored Oct 22, 2024
2 parents 6c16073 + c106fdf commit 8ca7301
Show file tree
Hide file tree
Showing 17 changed files with 1,344 additions and 159 deletions.
3 changes: 2 additions & 1 deletion docs/_snippets/framework/ui/ui-toolbar-text.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
const locale = new Locale();

const text = new View();
text.element = document.createTextNode( 'Toolbar text' );
text.element = document.createElement( 'span' );
text.element.innerHTML = 'Toolbar text';

const toolbarText = new ToolbarView( locale );
toolbarText.items.add( text );
Expand Down
3 changes: 2 additions & 1 deletion packages/ckeditor5-theme-lark/tests/manual/theme.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ class TextView extends View {
constructor() {
super();

this.element = document.createTextNode( 'Sample text' );
this.element = document.createElement( 'span' );
this.element.innerHTML = 'Sample text';
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,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! );
Expand Down
21 changes: 19 additions & 2 deletions packages/ckeditor5-ui/src/dropdown/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
global,
priorities,
logWarning,
type FocusTracker,
type Collection,
type Locale,
type ObservableChangeEvent
Expand Down Expand Up @@ -198,6 +199,7 @@ export function addMenuToDropdown(
ariaLabel?: string;
} = {} ): void {
dropdownView.menuView = new DropdownMenuRootListView( dropdownView.locale!, body, definition );
dropdownView.focusTracker.add( dropdownView.menuView );

if ( dropdownView.isOpen ) {
addMenuToOpenDropdown( dropdownView, options );
Expand Down Expand Up @@ -231,7 +233,7 @@ function addMenuToOpenDropdown(
// Nested menu panels are added to body collection, so they are not children of the `dropdownView` from DOM perspective.
// Add these panels to `dropdownView` focus tracker, so they are treated like part of the `dropdownView` for focus-related purposes.
for ( const menu of dropdownMenuRootListView.menus ) {
dropdownView.focusTracker.add( menu.panelView.element! );
dropdownView.focusTracker.add( menu );
}

dropdownMenuRootListView.ariaLabel = options.ariaLabel || t( 'Dropdown menu' );
Expand Down Expand Up @@ -360,6 +362,7 @@ function addToolbarToOpenDropdown(
}

dropdownView.panelView.children.add( toolbarView );
dropdownView.focusTracker.add( toolbarView );
toolbarView.items.delegate( 'execute' ).to( dropdownView );
}

Expand Down Expand Up @@ -537,11 +540,25 @@ function closeDropdownOnClickOutside( dropdownView: DropdownView ) {
},
contextElements: () => [
dropdownView.element!,
...dropdownView.focusTracker.elements
// Include all elements connected to the dropdown's focus tracker, but exclude those that are direct children
// of DropdownView#element. They would be identified as descendants of #element anyway upon clicking and would
// not contribute to the logic.
...getFocusTrackerTreeElements( dropdownView.focusTracker ).filter( element => !dropdownView.element!.contains( element ) )
]
} );
}

/**
* Returns all DOM elements connected to a DropdownView's focus tracker, either directly (same DOM sub-tree)
* or indirectly (external views registered in the focus tracker).
*/
function getFocusTrackerTreeElements( focusTracker: FocusTracker ): Array<Element> {
return [
...focusTracker.elements,
...focusTracker.externalViews.flatMap( view => getFocusTrackerTreeElements( view.focusTracker ) )
];
}

/**
* Adds a behavior to a dropdownView that closes the dropdown view on "execute" event.
*/
Expand Down
4 changes: 2 additions & 2 deletions packages/ckeditor5-ui/src/editorui/editorui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -306,11 +306,11 @@ export default abstract class EditorUI extends /* #__PURE__ */ ObservableMixin()
*/
public addToolbar( toolbarView: ToolbarView, options: FocusableToolbarOptions = {} ): void {
if ( toolbarView.isRendered ) {
this.focusTracker.add( toolbarView.element! );
this.focusTracker.add( toolbarView );
this.editor.keystrokes.listenTo( toolbarView.element! );
} else {
toolbarView.once<UIViewRenderEvent>( 'render', () => {
this.focusTracker.add( toolbarView.element! );
this.focusTracker.add( toolbarView );
this.editor.keystrokes.listenTo( toolbarView.element! );
} );
}
Expand Down
2 changes: 1 addition & 1 deletion packages/ckeditor5-ui/src/search/text/searchtextview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ export default class SearchTextView<
const stopPropagation = ( data: Event ) => data.stopPropagation();

for ( const focusableChild of this.focusableChildren ) {
this.focusTracker.add( focusableChild.element as Element );
this.focusTracker.add( focusableChild.element as HTMLElement );
}

// Start listening for the keystrokes coming from #element.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export default class BalloonToolbar extends Plugin {

// Track focusable elements in the toolbar and the editable elements.
this._trackFocusableEditableElements();
this.focusTracker.add( this.toolbarView.element! );
this.focusTracker.add( this.toolbarView );

// Register the toolbar so it becomes available for Alt+F10 and Esc navigation.
editor.ui.addToolbar( this.toolbarView, {
Expand Down
6 changes: 3 additions & 3 deletions packages/ckeditor5-ui/src/toolbar/toolbarview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,15 +260,15 @@ export default class ToolbarView extends View implements DropdownPanelFocusable

// Children added before rendering should be known to the #focusTracker.
for ( const item of this.items ) {
this.focusTracker.add( item.element! );
this.focusTracker.add( item );
}

this.items.on<CollectionAddEvent<View>>( 'add', ( evt, item ) => {
this.focusTracker.add( item.element! );
this.focusTracker.add( item );
} );

this.items.on<CollectionRemoveEvent<View>>( 'remove', ( evt, item ) => {
this.focusTracker.remove( item.element! );
this.focusTracker.remove( item );
} );

// Start listening for the keystrokes coming from #element.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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' );

Expand Down
63 changes: 62 additions & 1 deletion packages/ckeditor5-ui/tests/dropdown/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
/* globals document, Event, console */

import { assertBinding } from '@ckeditor/ckeditor5-utils/tests/_utils/utils.js';
import { global, keyCodes, Locale } from '@ckeditor/ckeditor5-utils';
import { FocusTracker, global, keyCodes, Locale } from '@ckeditor/ckeditor5-utils';
import Collection from '@ckeditor/ckeditor5-utils/src/collection.js';
import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils.js';

Expand Down Expand Up @@ -193,6 +193,42 @@ describe( 'utils', () => {

documentElement.remove();
} );

it( 'considers DOM elements in different DOM sub-trees but connected via focus trackers', () => {
// <body>
// <DropdownView#element />
// <child-view />
// <secondary-child-view />
// </body>
const childView = new View();
const secondaryChildView = new View();

childView.setTemplate( { tag: 'child-view' } );
secondaryChildView.setTemplate( { tag: 'secondary-child-view' } );

childView.focusTracker = new FocusTracker();

childView.render();
secondaryChildView.render();
document.body.appendChild( childView.element );
document.body.appendChild( secondaryChildView.element );

// DropdownView#focusTracker -> child-view#focusTracker -> secondary-child-view#focusTracker
dropdownView.focusTracker.add( childView );
childView.focusTracker.add( secondaryChildView );

dropdownView.isOpen = true;

secondaryChildView.element.dispatchEvent( new Event( 'mousedown', {
bubbles: true
} ) );

// External view's element is logically connected to the dropdown view's element via focus tracker.
expect( dropdownView.isOpen ).to.be.true;

childView.element.remove();
secondaryChildView.element.remove();
} );
} );

describe( 'closeDropdownOnExecute()', () => {
Expand Down Expand Up @@ -690,6 +726,15 @@ describe( 'utils', () => {
dropdownView.element.remove();
} );

// https://github.com/cksource/ckeditor5-commercial/issues/6633
it( 'should add the ToolbarView instance of dropdown\'s focus tracker to allow for using toolbar items distributed ' +
'across the DOM sub-trees', () => {
// Lazy load.
dropdownView.isOpen = true;

expect( dropdownView.focusTracker.externalViews ).to.include( dropdownView.toolbarView );
} );

it( 'focuses active item upon dropdown opening', () => {
buttons[ 0 ].isOn = true;

Expand Down Expand Up @@ -1391,6 +1436,22 @@ describe( 'utils', () => {
expect( dropdownView.menuView.render.calledOnce ).to.be.true;
} );

// https://github.com/cksource/ckeditor5-commercial/issues/6633
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 );

dropdownView.isOpen = true;

sinon.assert.calledThrice( addSpy );
sinon.assert.calledWithExactly( addSpy.firstCall, dropdownView.menuView );
sinon.assert.calledWithExactly( addSpy.secondCall, dropdownView.menuView.menus[ 0 ] );
sinon.assert.calledWithExactly( addSpy.thirdCall, dropdownView.menuView.menus[ 1 ] );
} );

it( 'should focus dropdown menu view after dropdown is opened', () => {
addMenuToDropdown( dropdownView, body, definition );

Expand Down
10 changes: 5 additions & 5 deletions packages/ckeditor5-ui/tests/editorui/editorui.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ describe( 'EditorUI', () => {
} );

it( 'should reset editables array', () => {
ui.setEditableElement( 'foo', {} );
ui.setEditableElement( 'bar', {} );
ui.setEditableElement( 'foo', document.createElement( 'div' ) );
ui.setEditableElement( 'bar', document.createElement( 'div' ) );

expect( [ ...ui.getEditableElementsNames() ] ).to.deep.equal( [ 'foo', 'bar' ] );

Expand Down Expand Up @@ -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', () => {
Expand All @@ -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();
} );
Expand Down
Loading

0 comments on commit 8ca7301

Please sign in to comment.