From 910fe69377547e9b6bc7d0918801c687b27e86c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Zag=C3=B3rski?= Date: Wed, 10 Aug 2022 15:22:59 +0200 Subject: [PATCH 1/7] Fixed focus issues upon closing block toolbar using a button. --- .../ckeditor5-ui/src/toolbar/block/blocktoolbar.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js b/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js index ecc077b3d9e..0d829c22579 100644 --- a/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js +++ b/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js @@ -271,6 +271,7 @@ export default class BlockToolbar extends Plugin { const editor = this.editor; const t = editor.t; const buttonView = new BlockButtonView( editor.locale ); + const buttonBind = buttonView.bindTemplate; buttonView.set( { label: t( 'Edit block' ), @@ -278,6 +279,15 @@ export default class BlockToolbar extends Plugin { withText: false } ); + buttonView.extendTemplate( { + on: { + mousedown: buttonBind.to( evt => { + // Workaround to #12184, see https://github.com/ckeditor/ckeditor5/issues/12184#issuecomment-1199147964. + evt.preventDefault(); + } ) + } + } ); + // Bind the panelView observable properties to the buttonView. buttonView.bind( 'isOn' ).to( this.panelView, 'isVisible' ); buttonView.bind( 'tooltip' ).to( this.panelView, 'isVisible', isVisible => !isVisible ); From 730b64f7282cf284708d629b0951989a8fbb58b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Zag=C3=B3rski?= Date: Wed, 10 Aug 2022 15:23:31 +0200 Subject: [PATCH 2/7] Unit test added to check mousedown event on buttonView. --- packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js b/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js index 4996e435bb4..ec60dc49562 100644 --- a/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js +++ b/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js @@ -295,6 +295,12 @@ describe( 'BlockToolbar', () => { expect( blockToolbar.buttonView.isVisible ).to.be.false; } ); } ); + + it( 'should prevent the mousedown event', () => { + const ret = blockToolbar.buttonView.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); + + expect( ret ).to.false; + } ); } ); describe( 'allowed elements', () => { From cab69b42c63149b573b5e32219414b8f00e1a11d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Zag=C3=B3rski?= Date: Wed, 10 Aug 2022 15:24:32 +0200 Subject: [PATCH 3/7] Fixed not closing blocktoolbar on click in Safari. --- packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js b/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js index 0d829c22579..c7bb76c2510 100644 --- a/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js +++ b/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js @@ -25,6 +25,7 @@ import normalizeToolbarConfig from '../normalizetoolbarconfig'; import ResizeObserver from '@ckeditor/ckeditor5-utils/src/dom/resizeobserver'; import toUnit from '@ckeditor/ckeditor5-utils/src/dom/tounit'; +import env from '@ckeditor/ckeditor5-utils/src/env'; const toPx = toUnit( 'px' ); @@ -282,6 +283,10 @@ export default class BlockToolbar extends Plugin { buttonView.extendTemplate( { on: { mousedown: buttonBind.to( evt => { + // Workaround to #12115. + if ( env.isSafari ) { + this.toolbarView.focus(); + } // Workaround to #12184, see https://github.com/ckeditor/ckeditor5/issues/12184#issuecomment-1199147964. evt.preventDefault(); } ) From 41dc8686ae9c12cb510e430d28ee138cbd9a6d97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Zag=C3=B3rski?= Date: Wed, 10 Aug 2022 15:24:56 +0200 Subject: [PATCH 4/7] Safari unit tests for mousedown changes on buttonView. --- .../tests/toolbar/block/blocktoolbar.js | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js b/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js index ec60dc49562..847f483fbb2 100644 --- a/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js +++ b/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js @@ -27,6 +27,7 @@ import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import Rect from '@ckeditor/ckeditor5-utils/src/dom/rect'; +import env from '@ckeditor/ckeditor5-utils/src/env'; describe( 'BlockToolbar', () => { let editor, element, blockToolbar; @@ -301,6 +302,32 @@ describe( 'BlockToolbar', () => { expect( ret ).to.false; } ); + + describe( 'in Safari', () => { + let view, stub; + + beforeEach( () => { + stub = testUtils.sinon.stub( env, 'isSafari' ).value( true ); + view = blockToolbar.buttonView; + } ); + + afterEach( () => { + stub.resetBehavior(); + } ); + + it( 'the toolbar is focused', () => { + const spy = sinon.spy( blockToolbar.toolbarView, 'focus' ); + view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); + + expect( spy.callCount ).to.equal( 1 ); + } ); + + it( 'the event is prevented', () => { + const ret = view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); + + expect( ret ).to.false; + } ); + } ); } ); describe( 'allowed elements', () => { From 119a192b4ca92ed7c4f820819b62607fafff7905 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Zag=C3=B3rski?= Date: Tue, 30 Aug 2022 10:50:45 +0200 Subject: [PATCH 5/7] Applied changes after review. --- .../ckeditor5-ui/src/toolbar/block/blocktoolbar.js | 2 +- .../tests/toolbar/block/blocktoolbar.js | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js b/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js index d1f382167b3..7b5b266fd64 100644 --- a/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js +++ b/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js @@ -293,7 +293,7 @@ export default class BlockToolbar extends Plugin { on: { mousedown: buttonBind.to( evt => { // Workaround to #12115. - if ( env.isSafari ) { + if ( env.isSafari && this.panelView.isVisible ) { this.toolbarView.focus(); } // Workaround to #12184, see https://github.com/ckeditor/ckeditor5/issues/12184#issuecomment-1199147964. diff --git a/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js b/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js index ce522848a1e..146e78f6f8c 100644 --- a/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js +++ b/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js @@ -338,24 +338,31 @@ describe( 'BlockToolbar', () => { } ); describe( 'in Safari', () => { - let view, stub; + let view, stub, spy; beforeEach( () => { stub = testUtils.sinon.stub( env, 'isSafari' ).value( true ); view = blockToolbar.buttonView; + spy = sinon.spy( blockToolbar.toolbarView, 'focus' ); } ); afterEach( () => { stub.resetBehavior(); } ); - it( 'the toolbar is focused', () => { - const spy = sinon.spy( blockToolbar.toolbarView, 'focus' ); + it( 'the toolbar is open, and gets focused', () => { + blockToolbar.panelView.isVisible = true; view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); expect( spy.callCount ).to.equal( 1 ); } ); + it( 'the toolbar is closed, and it doesn\'t get focused', () => { + view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); + + expect( spy.callCount ).to.equal( 0 ); + } ); + it( 'the event is prevented', () => { const ret = view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); From 01707ca87e2574e15a6eccaaf8ca31e67695f920 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 29 Aug 2022 17:23:54 +0200 Subject: [PATCH 6/7] Docs. --- .../ckeditor5-ui/src/toolbar/block/blocktoolbar.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js b/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js index 7b5b266fd64..f69aa086c5b 100644 --- a/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js +++ b/packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.js @@ -281,7 +281,7 @@ export default class BlockToolbar extends Plugin { const editor = this.editor; const t = editor.t; const buttonView = new BlockButtonView( editor.locale ); - const buttonBind = buttonView.bindTemplate; + const bind = buttonView.bindTemplate; buttonView.set( { label: t( 'Edit block' ), @@ -289,13 +289,17 @@ export default class BlockToolbar extends Plugin { withText: false } ); + // Note that this piece over here overrides the default mousedown logic in ButtonView + // to make it work with BlockToolbar. See the implementation of the ButtonView class to learn more. buttonView.extendTemplate( { on: { - mousedown: buttonBind.to( evt => { - // Workaround to #12115. + mousedown: bind.to( evt => { + // On Safari we have to force the focus on a button on click as it's the only browser + // that doesn't do that automatically. See #12115. if ( env.isSafari && this.panelView.isVisible ) { this.toolbarView.focus(); } + // Workaround to #12184, see https://github.com/ckeditor/ckeditor5/issues/12184#issuecomment-1199147964. evt.preventDefault(); } ) From 8f0023e469b76011dfed020aa67db74c0db83fe7 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 30 Aug 2022 11:14:20 +0200 Subject: [PATCH 7/7] Tests: code refactoring. --- .../tests/toolbar/block/blocktoolbar.js | 59 ++++++++++--------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js b/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js index 146e78f6f8c..8a464d1247b 100644 --- a/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js +++ b/packages/ckeditor5-ui/tests/toolbar/block/blocktoolbar.js @@ -329,44 +329,49 @@ describe( 'BlockToolbar', () => { const blockToolbar = editor.plugins.get( BlockToolbar ); expect( blockToolbar.buttonView.isVisible ).to.be.false; } ); - } ); - it( 'should prevent the mousedown event', () => { - const ret = blockToolbar.buttonView.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); + describe( 'mousedown event', () => { + // https://github.com/ckeditor/ckeditor5/issues/12184 + it( 'should call preventDefault to avoid stealing the focus', () => { + const ret = blockToolbar.buttonView.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); - expect( ret ).to.false; - } ); + expect( ret ).to.false; + } ); - describe( 'in Safari', () => { - let view, stub, spy; + // https://github.com/ckeditor/ckeditor5/issues/12115 + describe( 'in Safari', () => { + let view, stub, spy; - beforeEach( () => { - stub = testUtils.sinon.stub( env, 'isSafari' ).value( true ); - view = blockToolbar.buttonView; - spy = sinon.spy( blockToolbar.toolbarView, 'focus' ); - } ); + beforeEach( () => { + stub = testUtils.sinon.stub( env, 'isSafari' ).value( true ); + view = blockToolbar.buttonView; + spy = sinon.spy( blockToolbar.toolbarView, 'focus' ); + } ); - afterEach( () => { - stub.resetBehavior(); - } ); + afterEach( () => { + stub.resetBehavior(); + } ); - it( 'the toolbar is open, and gets focused', () => { - blockToolbar.panelView.isVisible = true; - view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); + it( 'should focus the toolbar when it shows up', () => { + blockToolbar.panelView.isVisible = true; + view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); - expect( spy.callCount ).to.equal( 1 ); - } ); + expect( spy.callCount ).to.equal( 1 ); + } ); - it( 'the toolbar is closed, and it doesn\'t get focused', () => { - view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); + it( 'should not focus the toolbar when it hides', () => { + blockToolbar.panelView.isVisible = false; + view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); - expect( spy.callCount ).to.equal( 0 ); - } ); + expect( spy.callCount ).to.equal( 0 ); + } ); - it( 'the event is prevented', () => { - const ret = view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); + it( 'should also preventDefault the event', () => { + const ret = view.element.dispatchEvent( new Event( 'mousedown', { cancelable: true } ) ); - expect( ret ).to.false; + expect( ret ).to.false; + } ); + } ); } ); } ); } );