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

Addressed issues related to closing dropdowns and "Show more items" menu in the toolbar #12319

Merged
merged 26 commits into from
Sep 1, 2022
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
e4e38e1
Move focus only if it is still in the toolbar.
Dumluregn Jul 29, 2022
dda2a47
Enhance docs.
Dumluregn Aug 8, 2022
0108104
Add unit tests.
Dumluregn Aug 8, 2022
0d8dd30
Merge branch 'master' into ck/12178-grouped-toolbar-focus
Dumluregn Aug 8, 2022
8d77fc0
Merge branch 'master' into ck/12178-grouped-toolbar-focus
oleq Aug 22, 2022
c1e5e3a
Docs.
oleq Aug 22, 2022
20c1037
Apply suggested changes in the tests.
Dumluregn Aug 22, 2022
c4f4fab
Revert the 'global' package removal.
Dumluregn Aug 22, 2022
0f7a561
Added FocusTracker instance to the DropdownView.
oleq Aug 23, 2022
633dead
The dropdown should get closed when its button or one of its panel ch…
oleq Aug 23, 2022
d9c6836
Code refactoring in DropdownView class.
oleq Aug 23, 2022
f3e1006
Refactoring in MediaEmbedUI to align to the latest DropdownView behav…
oleq Aug 23, 2022
b4682b5
Code refactoring.
oleq Aug 23, 2022
06d94d6
Docs.
oleq Aug 23, 2022
53d011e
Merge branch 'master' into ck/12178-grouped-toolbar-focus-v2
oleq Aug 23, 2022
617d892
Adjusted the find and replace dropdown to the latest DropdownView foc…
oleq Aug 24, 2022
d3b70a7
Adjusted the font color dropdowns to the latest DropdownView focus be…
oleq Aug 24, 2022
0bdc528
Adjusted the special characters dropdown to the latest DropdownView f…
oleq Aug 24, 2022
4cee8d2
Adjusted the style dropdown to the latest DropdownView focus behavior.
oleq Aug 24, 2022
dfb60ee
Adjusted the image style dropdowns to the latest DropdownView focus b…
oleq Aug 24, 2022
53a3b9e
Adjusted the table dropdown to the latest DropdownView focus behavior.
oleq Aug 24, 2022
d5cc862
Merge branch 'master' into ck/12178-grouped-toolbar-focus-v2
oleq Aug 31, 2022
bacb8cb
Tests: Fixed unstable dropdown utils focus management tests.
oleq Aug 31, 2022
d096b11
Make sure focusChildOnDropdownOpen() and focusDropdownPanelOnOpen() h…
oleq Aug 31, 2022
d3a4bb7
Clicking the back of the special characters pane should not cause the…
oleq Aug 31, 2022
aa85b9f
Tests: Stabilized MediaEmbedUI tests related to focus tracking so the…
oleq Sep 1, 2022
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
3 changes: 0 additions & 3 deletions packages/ckeditor5-find-and-replace/src/findandreplaceui.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,9 @@ export default class FindAndReplaceUI extends Plugin {

formView.reset();
formView._findInputView.fieldView.select();
formView.focus();

formView.enableCssTransitions();
} else {
formView.focus();

this.fire( 'searchReseted' );
}
}, { priority: 'low' } );
Expand Down
14 changes: 4 additions & 10 deletions packages/ckeditor5-find-and-replace/tests/findandreplaceui.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,15 @@ describe( 'FindAndReplaceUI', () => {
form = dropdown.panelView.children.get( 0 );
findCommand = editor.commands.get( 'find' );
plugin = editor.plugins.get( 'FindAndReplaceUI' );

dropdown.render();
global.document.body.appendChild( dropdown.element );
} );
} );

afterEach( () => {
editorElement.remove();
dropdown.element.remove();

return editor.destroy();
} );
Expand Down Expand Up @@ -122,16 +126,6 @@ describe( 'FindAndReplaceUI', () => {
} );

describe( 'upon dropdown close', () => {
it( 'the form should be focused', () => {
dropdown.isOpen = true;

const spy = sinon.spy( form, 'focus' );

dropdown.isOpen = false;

sinon.assert.calledOnce( spy );
} );

it( 'the #searchReseted event should be emitted', () => {
dropdown.isOpen = true;

Expand Down
6 changes: 3 additions & 3 deletions packages/ckeditor5-font/tests/ui/colortableview.js
Original file line number Diff line number Diff line change
Expand Up @@ -498,14 +498,14 @@ describe( 'ColorTableView', () => {
model = editor.model;

dropdown = editor.ui.componentFactory.create( 'testColor' );
dropdown.render();
global.document.body.appendChild( dropdown.element );

dropdown.isOpen = true;
dropdown.isOpen = false;
dropdown.render();

staticColorsGrid = dropdown.colorTableView.staticColorsGrid;
documentColorsGrid = dropdown.colorTableView.documentColorsGrid;

global.document.body.appendChild( dropdown.element );
} );
} );

Expand Down
7 changes: 7 additions & 0 deletions packages/ckeditor5-image/tests/imagestyle/imagestyleui.js
Original file line number Diff line number Diff line change
Expand Up @@ -233,10 +233,17 @@ describe( 'ImageStyleUI', () => {
dropdowns = [ ...defaultDropdowns, ...customDropdowns ].map( dropdown => {
const view = factory.create( dropdown.name );

view.render();
global.document.body.appendChild( view.element );

return { view, buttonView: view.buttonView, config: dropdown };
} );
} );

afterEach( () => {
dropdowns.forEach( ( { view } ) => view.element.remove() );
} );

it( 'should define the drop-down properties and children properly', () => {
for ( const { config, view, buttonView } of dropdowns ) {
const defaultItem = allStyles.find( style => style.name === config.defaultItem.replace( 'imageStyle:', '' ) );
Expand Down
9 changes: 3 additions & 6 deletions packages/ckeditor5-media-embed/src/mediaembedui.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,17 +93,14 @@ export default class MediaEmbedUI extends Plugin {
dropdown.on( 'submit', () => {
if ( form.isValid() ) {
editor.execute( 'mediaEmbed', form.url );
closeUI();
editor.editing.view.focus();
}
} );

dropdown.on( 'change:isOpen', () => form.resetFormStatus() );
dropdown.on( 'cancel', () => closeUI() );

function closeUI() {
dropdown.on( 'cancel', () => {
editor.editing.view.focus();
dropdown.isOpen = false;
}
} );
}

/**
Expand Down
23 changes: 19 additions & 4 deletions packages/ckeditor5-media-embed/tests/mediaembedui.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/* global setTimeout */

import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
import MediaEmbed from '../src/mediaembed';
import MediaEmbedUI from '../src/mediaembedui';
Expand Down Expand Up @@ -36,10 +38,15 @@ describe( 'MediaEmbedUI', () => {
dropdown = editor.ui.componentFactory.create( 'mediaEmbed' );
button = dropdown.buttonView;
form = dropdown.panelView.children.get( 0 );

dropdown.render();

global.document.body.appendChild( dropdown.element );
} );
} );

afterEach( () => {
dropdown.element.remove();
editorElement.remove();

return editor.destroy();
Expand Down Expand Up @@ -142,7 +149,7 @@ describe( 'MediaEmbedUI', () => {
sinon.assert.calledOnce( spy );
} );

it( 'executes the command and closes the UI (if the form is valid)', () => {
it( 'executes the command and closes the UI (if the form is valid)', done => {
Dumluregn marked this conversation as resolved.
Show resolved Hide resolved
const viewFocusSpy = sinon.spy( editor.editing.view, 'focus' );
const commandSpy = sinon.spy( editor.commands.get( 'mediaEmbed' ), 'execute' );

Expand All @@ -163,7 +170,11 @@ describe( 'MediaEmbedUI', () => {
sinon.assert.calledOnce( commandSpy );
sinon.assert.calledWithExactly( commandSpy, 'https://valid/url' );
sinon.assert.calledOnce( viewFocusSpy );
expect( dropdown.isOpen ).to.be.false;

setTimeout( () => {
expect( dropdown.isOpen ).to.be.false;
done();
}, 0 );
} );
} );

Expand All @@ -178,14 +189,18 @@ describe( 'MediaEmbedUI', () => {
} );

describe( '#cancel event', () => {
it( 'closes the UI', () => {
it( 'closes the UI', done => {
const viewFocusSpy = sinon.spy( editor.editing.view, 'focus' );

dropdown.isOpen = true;
dropdown.fire( 'cancel' );

sinon.assert.calledOnce( viewFocusSpy );
expect( dropdown.isOpen ).to.be.false;

setTimeout( () => {
expect( dropdown.isOpen ).to.be.false;
done();
}, 0 );
} );
} );
} );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,12 @@ export default class SpecialCharactersView extends View {
this.navigationView,
this.gridView,
this.infoView
]
],
attributes: {
// Avoid focus loss when the user clicks the area of the grid that is not a button.
// https://github.com/ckeditor/ckeditor5/pull/12319#issuecomment-1231779819
tabindex: '-1'
}
} );

this.items.add( this.navigationView.groupDropdownView.buttonView );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,14 @@ describe( 'SpecialCharacters', () => {

beforeEach( () => {
dropdown = editor.ui.componentFactory.create( 'specialCharacters' );
dropdown.render();
document.body.appendChild( dropdown.element );

dropdown.isOpen = true; // Dropdown is lazy loaded, so needs to be open to be verified (#6175).
} );

afterEach( () => {
dropdown.element.remove();
dropdown.destroy();
} );

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ describe( 'SpecialCahractersView', () => {
expect( view.items.get( 1 ) ).to.equal( gridView );
expect( view.items.length ).to.equal( 2 );
} );

// https://github.com/ckeditor/ckeditor5/pull/12319#issuecomment-1231779819
it( 'sets tabindex to -1 to avoid focus loss', () => {
expect( view.element.getAttribute( 'tabindex' ) ).to.equal( '-1' );
} );
} );

describe( 'render()', () => {
Expand Down
10 changes: 8 additions & 2 deletions packages/ckeditor5-style/tests/styleui.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,17 @@ describe( 'StyleUI', () => {

beforeEach( () => {
dropdown = editor.ui.componentFactory.create( 'style' );
dropdown.render();

document.body.appendChild( dropdown.element );

command = editor.commands.get( 'style' );
} );

afterEach( () => {
dropdown.element.remove();
} );

it( 'should be registered in the component factory', () => {
expect( editor.ui.componentFactory.has( 'style' ) ).to.be.true;
expect( dropdown ).to.be.instanceOf( DropdownView );
Expand All @@ -70,8 +78,6 @@ describe( 'StyleUI', () => {
} );

it( 'should have a static CSS class', () => {
dropdown.render();

expect( dropdown.element.classList.contains( 'ck-style-dropdown' ) ).to.be.true;
} );

Expand Down
14 changes: 11 additions & 3 deletions packages/ckeditor5-table/tests/tableui.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,17 @@ describe( 'TableUI', () => {

beforeEach( () => {
insertTable = editor.ui.componentFactory.create( 'insertTable' );
insertTable.render();

document.body.appendChild( insertTable.element );

insertTable.isOpen = true; // Dropdown is lazy loaded, so make sure its open (#6193).
} );

afterEach( () => {
insertTable.element.remove();
} );

it( 'should register insertTable button', () => {
expect( insertTable ).to.be.instanceOf( DropdownView );
expect( insertTable.buttonView.label ).to.equal( 'Insert table' );
Expand Down Expand Up @@ -103,11 +111,11 @@ describe( 'TableUI', () => {
beforeEach( () => {
insertTable = editor.ui.componentFactory.create( 'insertTable' );

insertTable.isOpen = true; // Dropdown is lazy loaded (#6193).
insertTable.isOpen = false;

insertTable.render();
document.body.appendChild( insertTable.element );

insertTable.isOpen = true; // Dropdown is lazy loaded (#6193).
insertTable.isOpen = false;
} );

afterEach( () => {
Expand Down
22 changes: 14 additions & 8 deletions packages/ckeditor5-ui/src/dropdown/dropdownview.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import View from '../view';
import KeystrokeHandler from '@ckeditor/ckeditor5-utils/src/keystrokehandler';
import { FocusTracker } from '@ckeditor/ckeditor5-utils';

import '../../theme/components/dropdown/dropdown.css';

Expand Down Expand Up @@ -170,6 +171,14 @@ export default class DropdownView extends View {
*/
this.keystrokes = new KeystrokeHandler();

/**
* Tracks information about the DOM focus in the dropdown.
*
* @readonly
* @member {module:utils/focustracker~FocusTracker}
*/
this.focusTracker = new FocusTracker();

this.setTemplate( {
tag: 'div',

Expand Down Expand Up @@ -241,6 +250,9 @@ export default class DropdownView extends View {
render() {
super.render();

this.focusTracker.add( this.buttonView.element );
this.focusTracker.add( this.panelView.element );

// Toggle the dropdown when its button has been clicked.
this.listenTo( this.buttonView, 'open', () => {
this.isOpen = !this.isOpen;
Expand All @@ -251,11 +263,8 @@ export default class DropdownView extends View {

// Let the dropdown control the position of the panel. The position must
// be updated every time the dropdown is open.
this.on( 'change:isOpen', () => {
if ( !this.isOpen ) {
// If the dropdown was closed, move the focus back to the button (#12125).
this.focus();

this.on( 'change:isOpen', ( evt, name, isOpen ) => {
if ( !isOpen ) {
return;
}

Expand All @@ -271,9 +280,6 @@ export default class DropdownView extends View {
} else {
this.panelView.position = this.panelPosition;
}

// Focus the first item in the dropdown when the dropdown opened
this.panelView.focus();
} );

// Listen for keystrokes coming from within #element.
Expand Down
Loading