From 3cb1a93b7a0d6df8ba9fecb75fc8767baba07477 Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 23 Aug 2022 13:15:20 +0200 Subject: [PATCH 01/22] Make color picker buttons in table properties focusable. --- .../src/tableproperties/ui/tablepropertiesview.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/ckeditor5-table/src/tableproperties/ui/tablepropertiesview.js b/packages/ckeditor5-table/src/tableproperties/ui/tablepropertiesview.js index 3b22c120297..64ea751e991 100644 --- a/packages/ckeditor5-table/src/tableproperties/ui/tablepropertiesview.js +++ b/packages/ckeditor5-table/src/tableproperties/ui/tablepropertiesview.js @@ -360,8 +360,10 @@ export default class TablePropertiesView extends View { [ this.borderStyleDropdown, this.borderColorInput, + this.borderColorInput.fieldView._dropdownView.buttonView, this.borderWidthInput, this.backgroundInput, + this.backgroundInput.fieldView._dropdownView.buttonView, this.widthInput, this.heightInput, this.alignmentToolbar, From dd5b549ad7aa01e4822f2aad6a5687a0bc9eaa75 Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 23 Aug 2022 16:31:19 +0200 Subject: [PATCH 02/22] Add navigation with (shift) tab keystroke to color input view in table properties. --- .../ckeditor5-table/src/ui/colorinputview.js | 74 ++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-table/src/ui/colorinputview.js b/packages/ckeditor5-table/src/ui/colorinputview.js index 7484d944c1f..ff714fc0073 100644 --- a/packages/ckeditor5-table/src/ui/colorinputview.js +++ b/packages/ckeditor5-table/src/ui/colorinputview.js @@ -7,8 +7,9 @@ * @module table/ui/colorinputview */ -import { View, InputTextView, ButtonView, createDropdown, ColorGridView } from 'ckeditor5/src/ui'; +import { View, InputTextView, ButtonView, createDropdown, ColorGridView, FocusCycler, ViewCollection } from 'ckeditor5/src/ui'; import { icons } from 'ckeditor5/src/core'; +import { FocusTracker, KeystrokeHandler } from 'ckeditor5/src/utils'; import '../../theme/colorinput.css'; @@ -109,6 +110,23 @@ export default class ColorInputView extends View { */ this.options = options; + /** + * Tracks information about the DOM focus in the view. + * + * @readonly + * @member {module:utils/focustracker~FocusTracker} + */ + this.focusTracker = new FocusTracker(); + + /** + * A collection of views that can be focused in the view. + * + * @readonly + * @protected + * @member {module:ui/viewcollection~ViewCollection} + */ + this._focusables = new ViewCollection(); + /** * An instance of the dropdown allowing to select a color from a grid. * @@ -135,6 +153,34 @@ export default class ColorInputView extends View { */ this._stillTyping = false; + /** + * An instance of the {@link module:utils/keystrokehandler~KeystrokeHandler}. + * + * @readonly + * @member {module:utils/keystrokehandler~KeystrokeHandler} + */ + this.keystrokes = new KeystrokeHandler(); + + /** + * Helps cycling over focusable items in the view. + * + * @readonly + * @protected + * @member {module:ui/focuscycler~FocusCycler} + */ + this._focusCycler = new FocusCycler( { + focusables: this._focusables, + focusTracker: this.focusTracker, + keystrokeHandler: this.keystrokes, + actions: { + // Navigate items backwards using the Shift + Tab keystroke. + focusPrevious: 'shift + tab', + + // Navigate items forwards using the Tab key. + focusNext: 'tab' + } + } ); + this.setTemplate( { tag: 'div', attributes: { @@ -156,6 +202,16 @@ export default class ColorInputView extends View { this.on( 'change:value', ( evt, name, inputValue ) => this._setInputValue( inputValue ) ); } + /** + * @inheritDoc + */ + render() { + super.render(); + + // Start listening for the keystrokes coming from the dropdown panel view. + this.keystrokes.listenTo( this._dropdownView.panelView.element ); + } + /** * Focuses the input. */ @@ -163,6 +219,16 @@ export default class ColorInputView extends View { this._inputView.focus(); } + /** + * @inheritDoc + */ + destroy() { + super.destroy(); + + this.focusTracker.destroy(); + this.keystrokes.destroy(); + } + /** * Creates and configures the {@link #_dropdownView}. * @@ -215,6 +281,12 @@ export default class ColorInputView extends View { dropdown.panelView.children.add( colorGrid ); dropdown.bind( 'isEnabled' ).to( this, 'isReadOnly', value => !value ); + this._focusables.add( removeColorButton ); + this._focusables.add( colorGrid ); + + this.focusTracker.add( removeColorButton.element ); + this.focusTracker.add( colorGrid.element ); + return dropdown; } From 4aa2fc800fd1d7a2b1b91b8ee852c5be3a7be6ce Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 23 Aug 2022 16:40:35 +0200 Subject: [PATCH 03/22] Update test for registere focusables in TablePropertiesView. --- .../tests/tableproperties/ui/tablepropertiesview.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/ckeditor5-table/tests/tableproperties/ui/tablepropertiesview.js b/packages/ckeditor5-table/tests/tableproperties/ui/tablepropertiesview.js index 9c2bbce56a8..a05dcf63407 100644 --- a/packages/ckeditor5-table/tests/tableproperties/ui/tablepropertiesview.js +++ b/packages/ckeditor5-table/tests/tableproperties/ui/tablepropertiesview.js @@ -616,8 +616,10 @@ describe( 'table properties', () => { expect( view._focusables.map( f => f ) ).to.have.members( [ view.borderStyleDropdown, view.borderColorInput, + view.borderColorInput.fieldView._dropdownView.buttonView, view.borderWidthInput, view.backgroundInput, + view.backgroundInput.fieldView._dropdownView.buttonView, view.widthInput, view.heightInput, view.alignmentToolbar, From cd4d2553622d9dec34051c74ad8f9196ae51fa3c Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 23 Aug 2022 17:01:17 +0200 Subject: [PATCH 04/22] Tests to check that new class properties do exist. --- .../tests/ui/colorinputview.js | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-table/tests/ui/colorinputview.js b/packages/ckeditor5-table/tests/ui/colorinputview.js index 4f500f7c06a..4bd511bf46b 100644 --- a/packages/ckeditor5-table/tests/ui/colorinputview.js +++ b/packages/ckeditor5-table/tests/ui/colorinputview.js @@ -9,7 +9,8 @@ import ColorInputView from '../../src/ui/colorinputview'; import InputTextView from '@ckeditor/ckeditor5-ui/src/inputtext/inputtextview'; import ColorGridView from '@ckeditor/ckeditor5-ui/src/colorgrid/colorgridview'; import DropdownView from '@ckeditor/ckeditor5-ui/src/dropdown/dropdownview'; -import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview'; +import { ButtonView, FocusCycler, ViewCollection } from '@ckeditor/ckeditor5-ui'; +import { FocusTracker, KeystrokeHandler } from '@ckeditor/ckeditor5-utils'; const DEFAULT_COLORS = [ { @@ -95,6 +96,22 @@ describe( 'ColorInputView', () => { expect( view.isFocused ).to.be.false; } ); + it( 'should have #focusTracker', () => { + expect( view.focusTracker ).to.be.instanceOf( FocusTracker ); + } ); + + it( 'should have #_focusables', () => { + expect( view._focusables ).to.be.instanceOf( ViewCollection ); + } ); + + it( 'should have #keystrokes', () => { + expect( view.keystrokes ).to.be.instanceOf( KeystrokeHandler ); + } ); + + it( 'should have #_focusCycler', () => { + expect( view._focusCycler ).to.be.instanceOf( FocusCycler ); + } ); + describe( 'dropdown', () => { it( 'should be created', () => { expect( view._dropdownView ).to.be.instanceOf( DropdownView ); From 337c8e493c8ace4870e8bc875434ff060c996c40 Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 23 Aug 2022 17:14:16 +0200 Subject: [PATCH 05/22] Test for checking #keystrokes start listening to the proper element. --- .../ckeditor5-table/tests/ui/colorinputview.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/ckeditor5-table/tests/ui/colorinputview.js b/packages/ckeditor5-table/tests/ui/colorinputview.js index 4bd511bf46b..a44f5197c87 100644 --- a/packages/ckeditor5-table/tests/ui/colorinputview.js +++ b/packages/ckeditor5-table/tests/ui/colorinputview.js @@ -512,4 +512,21 @@ describe( 'ColorInputView', () => { sinon.assert.calledOnce( spy ); } ); } ); + + describe( 'render()', () => { + it( 'starts listening for #keystrokes coming from the #element of the panel view in the dropdown view', () => { + const view = new ColorInputView( locale, { + colorDefinitions: DEFAULT_COLORS, + columns: 5 + } ); + + const spy = sinon.spy( view.keystrokes, 'listenTo' ); + + view.render(); + sinon.assert.calledOnce( spy ); + sinon.assert.calledWithExactly( spy, view._dropdownView.panelView.element ); + + view.destroy(); + } ); + } ); } ); From b9b9e829ccce7f96ae224e882bf946b514306215 Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 23 Aug 2022 17:17:59 +0200 Subject: [PATCH 06/22] Tests for destroy(). --- .../ckeditor5-table/tests/ui/colorinputview.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/packages/ckeditor5-table/tests/ui/colorinputview.js b/packages/ckeditor5-table/tests/ui/colorinputview.js index a44f5197c87..f7ce351c3b2 100644 --- a/packages/ckeditor5-table/tests/ui/colorinputview.js +++ b/packages/ckeditor5-table/tests/ui/colorinputview.js @@ -529,4 +529,22 @@ describe( 'ColorInputView', () => { view.destroy(); } ); } ); + + describe( 'destroy()', () => { + it( 'should destroy the FocusTracker instance', () => { + const destroySpy = sinon.spy( view.focusTracker, 'destroy' ); + + view.destroy(); + + sinon.assert.calledOnce( destroySpy ); + } ); + + it( 'should destroy the KeystrokeHandler instance', () => { + const destroySpy = sinon.spy( view.keystrokes, 'destroy' ); + + view.destroy(); + + sinon.assert.calledOnce( destroySpy ); + } ); + } ); } ); From 59d8a0c39cd7168714a000b2e5750acd4a069fd1 Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 23 Aug 2022 17:25:23 +0200 Subject: [PATCH 07/22] Test to check if #_focusables registered panel view children. --- packages/ckeditor5-table/tests/ui/colorinputview.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/ckeditor5-table/tests/ui/colorinputview.js b/packages/ckeditor5-table/tests/ui/colorinputview.js index f7ce351c3b2..1e03f739392 100644 --- a/packages/ckeditor5-table/tests/ui/colorinputview.js +++ b/packages/ckeditor5-table/tests/ui/colorinputview.js @@ -178,6 +178,13 @@ describe( 'ColorInputView', () => { expect( view._dropdownView.panelPosition ).to.equal( 'se' ); } ); } ); + + it( 'should register panelView children in #_focusables', () => { + expect( view._focusables.map( f => f ) ).to.have.members( [ + view._dropdownView.panelView.children.first, + view._dropdownView.panelView.children.last + ] ); + } ); } ); describe( 'color grid', () => { From 7395dde955bddfa3cdebbc73ab3bb341db8df7b0 Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 23 Aug 2022 17:46:34 +0200 Subject: [PATCH 08/22] Test to check if #focusTracker registered panel view children elements. --- packages/ckeditor5-table/tests/ui/colorinputview.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/ckeditor5-table/tests/ui/colorinputview.js b/packages/ckeditor5-table/tests/ui/colorinputview.js index 1e03f739392..6aa94b9ee2d 100644 --- a/packages/ckeditor5-table/tests/ui/colorinputview.js +++ b/packages/ckeditor5-table/tests/ui/colorinputview.js @@ -185,6 +185,11 @@ describe( 'ColorInputView', () => { view._dropdownView.panelView.children.last ] ); } ); + + it( 'should register panelView children elements in #focusTracker', () => { + expect( view.focusTracker._elements ).to.include( view._dropdownView.panelView.children.first.element ); + expect( view.focusTracker._elements ).to.include( view._dropdownView.panelView.children.last.element ); + } ); } ); describe( 'color grid', () => { From c170d9cb9920c25e1e6c6e04ff7fd34a81609a8c Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 23 Aug 2022 18:03:46 +0200 Subject: [PATCH 09/22] Tests for navigation with (shift) tab in ColorInputView dropdown. --- .../tests/ui/colorinputview.js | 67 ++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-table/tests/ui/colorinputview.js b/packages/ckeditor5-table/tests/ui/colorinputview.js index 6aa94b9ee2d..9f81d066a40 100644 --- a/packages/ckeditor5-table/tests/ui/colorinputview.js +++ b/packages/ckeditor5-table/tests/ui/colorinputview.js @@ -10,7 +10,8 @@ import InputTextView from '@ckeditor/ckeditor5-ui/src/inputtext/inputtextview'; import ColorGridView from '@ckeditor/ckeditor5-ui/src/colorgrid/colorgridview'; import DropdownView from '@ckeditor/ckeditor5-ui/src/dropdown/dropdownview'; import { ButtonView, FocusCycler, ViewCollection } from '@ckeditor/ckeditor5-ui'; -import { FocusTracker, KeystrokeHandler } from '@ckeditor/ckeditor5-utils'; +import { FocusTracker, KeystrokeHandler, keyCodes } from '@ckeditor/ckeditor5-utils'; +import { global } from 'ckeditor5/src/utils'; const DEFAULT_COLORS = [ { @@ -513,6 +514,70 @@ describe( 'ColorInputView', () => { } ); } ); } ); + + describe( 'activates keyboard navigation in the color input view', () => { + let view, locale; + + beforeEach( () => { + locale = { t: val => val }; + view = new ColorInputView( locale, { + colorDefinitions: DEFAULT_COLORS, + columns: 5 + } ); + view.render(); + global.document.body.appendChild( view.element ); + } ); + + afterEach( () => { + view.element.remove(); + view.destroy(); + } ); + + it( 'so "tab" focuses the next focusable item', () => { + const keyEvtData = { + keyCode: keyCodes.tab, + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + }; + + view._dropdownView.isOpen = true; + + // Mock the remove color button view is focused. + view.focusTracker.isFocused = true; + view.focusTracker.focusedElement = view._focusables.first.element; + + // Spy the next view which in this case is the color grid view. + const spy = sinon.spy( view._focusables.last, 'focus' ); + + view.keystrokes.press( keyEvtData ); + sinon.assert.calledOnce( keyEvtData.preventDefault ); + sinon.assert.calledOnce( keyEvtData.stopPropagation ); + sinon.assert.calledOnce( spy ); + } ); + + it( 'so "shift + tab" focuses the previous focusable item', () => { + const keyEvtData = { + keyCode: keyCodes.tab, + shiftKey: true, + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + }; + + view._dropdownView.isOpen = true; + + // Mock the remove color button view is focused. + view.focusTracker.isFocused = true; + view.focusTracker.focusedElement = view._focusables.first.element; + + // Spy the previous view which in this case is the color grid view. + const spy = sinon.spy( view._focusables.last, 'focus' ); + + view.keystrokes.press( keyEvtData ); + sinon.assert.calledOnce( keyEvtData.preventDefault ); + sinon.assert.calledOnce( keyEvtData.stopPropagation ); + sinon.assert.calledOnce( spy ); + } ); + } ); } ); describe( 'focus()', () => { From e96cf704f065c8b7d2028ecc0d74663ed29d375e Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Wed, 24 Aug 2022 11:41:25 +0200 Subject: [PATCH 10/22] Tests for arrow keys navigation in color input view in table properties view. --- .../tests/ui/colorinputview.js | 120 ++++++++++++------ 1 file changed, 84 insertions(+), 36 deletions(-) diff --git a/packages/ckeditor5-table/tests/ui/colorinputview.js b/packages/ckeditor5-table/tests/ui/colorinputview.js index 9f81d066a40..e519164eb3b 100644 --- a/packages/ckeditor5-table/tests/ui/colorinputview.js +++ b/packages/ckeditor5-table/tests/ui/colorinputview.js @@ -515,17 +515,19 @@ describe( 'ColorInputView', () => { } ); } ); - describe( 'activates keyboard navigation in the color input view', () => { - let view, locale; + describe( 'keyboard navigation', () => { + let view, locale, colorGridView; beforeEach( () => { locale = { t: val => val }; view = new ColorInputView( locale, { colorDefinitions: DEFAULT_COLORS, - columns: 5 + columns: 2 } ); view.render(); global.document.body.appendChild( view.element ); + + colorGridView = view._dropdownView.panelView.children.last; } ); afterEach( () => { @@ -533,49 +535,95 @@ describe( 'ColorInputView', () => { view.destroy(); } ); - it( 'so "tab" focuses the next focusable item', () => { - const keyEvtData = { - keyCode: keyCodes.tab, - preventDefault: sinon.spy(), - stopPropagation: sinon.spy() - }; + describe( 'activates keyboard navigation in the color input view', () => { + it( 'so "tab" focuses the next focusable item', () => { + const keyEvtData = { + keyCode: keyCodes.tab, + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + }; - view._dropdownView.isOpen = true; + view._dropdownView.isOpen = true; - // Mock the remove color button view is focused. - view.focusTracker.isFocused = true; - view.focusTracker.focusedElement = view._focusables.first.element; + // Mock the remove color button view is focused. + view.focusTracker.isFocused = true; + view.focusTracker.focusedElement = view._focusables.first.element; - // Spy the next view which in this case is the color grid view. - const spy = sinon.spy( view._focusables.last, 'focus' ); + // Spy the next view which in this case is the color grid view. + const spy = sinon.spy( view._focusables.last, 'focus' ); - view.keystrokes.press( keyEvtData ); - sinon.assert.calledOnce( keyEvtData.preventDefault ); - sinon.assert.calledOnce( keyEvtData.stopPropagation ); - sinon.assert.calledOnce( spy ); + view.keystrokes.press( keyEvtData ); + sinon.assert.calledOnce( keyEvtData.preventDefault ); + sinon.assert.calledOnce( keyEvtData.stopPropagation ); + sinon.assert.calledOnce( spy ); + } ); + + it( 'so "shift + tab" focuses the previous focusable item', () => { + const keyEvtData = { + keyCode: keyCodes.tab, + shiftKey: true, + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + }; + + view._dropdownView.isOpen = true; + + // Mock the remove color button view is focused. + view.focusTracker.isFocused = true; + view.focusTracker.focusedElement = view._focusables.first.element; + + // Spy the previous view which in this case is the color grid view. + const spy = sinon.spy( view._focusables.last, 'focus' ); + + view.keystrokes.press( keyEvtData ); + sinon.assert.calledOnce( keyEvtData.preventDefault ); + sinon.assert.calledOnce( keyEvtData.stopPropagation ); + sinon.assert.calledOnce( spy ); + } ); } ); - it( 'so "shift + tab" focuses the previous focusable item', () => { - const keyEvtData = { - keyCode: keyCodes.tab, - shiftKey: true, - preventDefault: sinon.spy(), - stopPropagation: sinon.spy() - }; + describe( 'keyboard navigation in the color input grid', () => { + it( '"arrow right" should focus the next focusable color button', () => { + const keyEvtData = { + keyCode: keyCodes.arrowright, + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + }; - view._dropdownView.isOpen = true; + view._dropdownView.isOpen = true; - // Mock the remove color button view is focused. - view.focusTracker.isFocused = true; - view.focusTracker.focusedElement = view._focusables.first.element; + // Mock the first color button is focused. + colorGridView.focusTracker.isFocused = true; + colorGridView.focusTracker.focusedElement = colorGridView.items.first.element; - // Spy the previous view which in this case is the color grid view. - const spy = sinon.spy( view._focusables.last, 'focus' ); + const spy = sinon.spy( colorGridView.items.get( 1 ), 'focus' ); - view.keystrokes.press( keyEvtData ); - sinon.assert.calledOnce( keyEvtData.preventDefault ); - sinon.assert.calledOnce( keyEvtData.stopPropagation ); - sinon.assert.calledOnce( spy ); + colorGridView.keystrokes.press( keyEvtData ); + sinon.assert.calledOnce( keyEvtData.preventDefault ); + sinon.assert.calledOnce( keyEvtData.stopPropagation ); + sinon.assert.calledOnce( spy ); + } ); + + it( '"arrow down" should focus the focusable color button in the second row', () => { + const keyEvtData = { + keyCode: keyCodes.arrowdown, + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + }; + + view._dropdownView.isOpen = true; + + // Mock the first color button is focused. + colorGridView.focusTracker.isFocused = true; + colorGridView.focusTracker.focusedElement = colorGridView.items.first.element; + + const spy = sinon.spy( colorGridView.items.get( 2 ), 'focus' ); + + colorGridView.keystrokes.press( keyEvtData ); + sinon.assert.calledOnce( keyEvtData.preventDefault ); + sinon.assert.calledOnce( keyEvtData.stopPropagation ); + sinon.assert.calledOnce( spy ); + } ); } ); } ); } ); From 6d16b53723cbdded946a43a163db04fbb17cae9d Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Mon, 29 Aug 2022 20:15:37 +0200 Subject: [PATCH 11/22] Make dropdownView of ColorInputView a public component. --- .../src/tableproperties/ui/tablepropertiesview.js | 4 ++-- packages/ckeditor5-table/src/ui/colorinputview.js | 15 +++++++-------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/ckeditor5-table/src/tableproperties/ui/tablepropertiesview.js b/packages/ckeditor5-table/src/tableproperties/ui/tablepropertiesview.js index 64ea751e991..95b64d179cd 100644 --- a/packages/ckeditor5-table/src/tableproperties/ui/tablepropertiesview.js +++ b/packages/ckeditor5-table/src/tableproperties/ui/tablepropertiesview.js @@ -360,10 +360,10 @@ export default class TablePropertiesView extends View { [ this.borderStyleDropdown, this.borderColorInput, - this.borderColorInput.fieldView._dropdownView.buttonView, + this.borderColorInput.fieldView.dropdownView.buttonView, this.borderWidthInput, this.backgroundInput, - this.backgroundInput.fieldView._dropdownView.buttonView, + this.backgroundInput.fieldView.dropdownView.buttonView, this.widthInput, this.heightInput, this.alignmentToolbar, diff --git a/packages/ckeditor5-table/src/ui/colorinputview.js b/packages/ckeditor5-table/src/ui/colorinputview.js index ff714fc0073..70c9194dcad 100644 --- a/packages/ckeditor5-table/src/ui/colorinputview.js +++ b/packages/ckeditor5-table/src/ui/colorinputview.js @@ -130,10 +130,9 @@ export default class ColorInputView extends View { /** * An instance of the dropdown allowing to select a color from a grid. * - * @protected * @member {module:ui/dropdown/dropdown~DropdownView} */ - this._dropdownView = this._createDropdownView(); + this.dropdownView = this._createDropdownView(); /** * An instance of the input allowing the user to type a color value. @@ -194,7 +193,7 @@ export default class ColorInputView extends View { 'aria-describedby': bind.to( 'ariaDescribedById' ) }, children: [ - this._dropdownView, + this.dropdownView, this._inputView ] } ); @@ -209,7 +208,7 @@ export default class ColorInputView extends View { super.render(); // Start listening for the keystrokes coming from the dropdown panel view. - this.keystrokes.listenTo( this._dropdownView.panelView.element ); + this.keystrokes.listenTo( this.dropdownView.panelView.element ); } /** @@ -230,7 +229,7 @@ export default class ColorInputView extends View { } /** - * Creates and configures the {@link #_dropdownView}. + * Creates and configures the {@link #dropdownView}. * * @private */ @@ -347,7 +346,7 @@ export default class ColorInputView extends View { removeColorButton.label = removeColorButtonLabel; removeColorButton.on( 'execute', () => { this.value = defaultColor; - this._dropdownView.isOpen = false; + this.dropdownView.isOpen = false; this.fire( 'input' ); } ); @@ -355,7 +354,7 @@ export default class ColorInputView extends View { } /** - * Creates and configures the color grid inside the {@link #_dropdownView}. + * Creates and configures the color grid inside the {@link #dropdownView}. * * @private */ @@ -367,7 +366,7 @@ export default class ColorInputView extends View { colorGrid.on( 'execute', ( evtData, data ) => { this.value = data.value; - this._dropdownView.isOpen = false; + this.dropdownView.isOpen = false; this.fire( 'input' ); } ); colorGrid.bind( 'selectedColor' ).to( this, 'value' ); From 790da2585f2a9922ae19e6dc3f7331582284d1b0 Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Mon, 29 Aug 2022 20:28:49 +0200 Subject: [PATCH 12/22] Update tests after making dropdownView of ColorInputView a public component. --- .../tableproperties/ui/tablepropertiesview.js | 8 +-- .../tests/ui/colorinputview.js | 60 +++++++++---------- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/packages/ckeditor5-table/tests/tableproperties/ui/tablepropertiesview.js b/packages/ckeditor5-table/tests/tableproperties/ui/tablepropertiesview.js index a05dcf63407..24cd75fc72e 100644 --- a/packages/ckeditor5-table/tests/tableproperties/ui/tablepropertiesview.js +++ b/packages/ckeditor5-table/tests/tableproperties/ui/tablepropertiesview.js @@ -616,10 +616,10 @@ describe( 'table properties', () => { expect( view._focusables.map( f => f ) ).to.have.members( [ view.borderStyleDropdown, view.borderColorInput, - view.borderColorInput.fieldView._dropdownView.buttonView, + view.borderColorInput.fieldView.dropdownView.buttonView, view.borderWidthInput, view.backgroundInput, - view.backgroundInput.fieldView._dropdownView.buttonView, + view.backgroundInput.fieldView.dropdownView.buttonView, view.widthInput, view.heightInput, view.alignmentToolbar, @@ -812,7 +812,7 @@ describe( 'table properties', () => { it( 'should replace "Remove color" with the "Restore default" label', () => { const { borderColorInput } = view; - const { panelView } = borderColorInput.fieldView._dropdownView; + const { panelView } = borderColorInput.fieldView.dropdownView; expect( panelView.children.first.label ).to.equal( 'Restore default' ); } ); @@ -822,7 +822,7 @@ describe( 'table properties', () => { describe( 'background row', () => { it( 'should replace "Remove color" with the "Restore default" label', () => { const { backgroundInput } = view; - const { panelView } = backgroundInput.fieldView._dropdownView; + const { panelView } = backgroundInput.fieldView.dropdownView; expect( panelView.children.first.label ).to.equal( 'Restore default' ); } ); diff --git a/packages/ckeditor5-table/tests/ui/colorinputview.js b/packages/ckeditor5-table/tests/ui/colorinputview.js index e519164eb3b..51a64981632 100644 --- a/packages/ckeditor5-table/tests/ui/colorinputview.js +++ b/packages/ckeditor5-table/tests/ui/colorinputview.js @@ -45,8 +45,8 @@ describe( 'ColorInputView', () => { view.render(); inputView = view._inputView; - removeColorButton = view._dropdownView.panelView.children.first; - colorGridView = view._dropdownView.panelView.children.last; + removeColorButton = view.dropdownView.panelView.children.first; + colorGridView = view.dropdownView.panelView.children.last; } ); afterEach( () => { @@ -115,29 +115,29 @@ describe( 'ColorInputView', () => { describe( 'dropdown', () => { it( 'should be created', () => { - expect( view._dropdownView ).to.be.instanceOf( DropdownView ); - expect( view._dropdownView.buttonView.element.classList.contains( 'ck-input-color__button' ) ).to.be.true; - expect( view._dropdownView.buttonView.tooltip ).to.be.true; - expect( view._dropdownView.buttonView.label ).to.equal( 'Color picker' ); + expect( view.dropdownView ).to.be.instanceOf( DropdownView ); + expect( view.dropdownView.buttonView.element.classList.contains( 'ck-input-color__button' ) ).to.be.true; + expect( view.dropdownView.buttonView.tooltip ).to.be.true; + expect( view.dropdownView.buttonView.label ).to.equal( 'Color picker' ); } ); it( 'should bind #isEnabled to the view\'s #isReadOnly', () => { view.isReadOnly = false; - expect( view._dropdownView.isEnabled ).to.be.true; + expect( view.dropdownView.isEnabled ).to.be.true; view.isReadOnly = true; - expect( view._dropdownView.isEnabled ).to.be.false; + expect( view.dropdownView.isEnabled ).to.be.false; } ); it( 'should have the color preview', () => { - const preview = view._dropdownView.buttonView.children.first; + const preview = view.dropdownView.buttonView.children.first; expect( preview.element.classList.contains( 'ck' ) ).to.be.true; expect( preview.element.classList.contains( 'ck-input-color__button__preview' ) ).to.be.true; } ); it( 'should display no-color preview when color is not set', () => { - const preview = view._dropdownView.buttonView.children.first; + const preview = view.dropdownView.buttonView.children.first; const noColorPreview = preview.element.firstChild; view.value = 'hsl(0, 0, 50%)'; @@ -150,7 +150,7 @@ describe( 'ColorInputView', () => { } ); it( 'should have the remove color button', () => { - const removeColorButton = view._dropdownView.panelView.children.first; + const removeColorButton = view.dropdownView.panelView.children.first; expect( removeColorButton ).to.be.instanceOf( ButtonView ); expect( removeColorButton.label ).to.equal( 'Remove color' ); @@ -165,7 +165,7 @@ describe( 'ColorInputView', () => { } ); view.render(); - expect( view._dropdownView.panelPosition ).to.equal( 'sw' ); + expect( view.dropdownView.panelPosition ).to.equal( 'sw' ); } ); it( 'should be SouthEast in RTL', () => { @@ -176,20 +176,20 @@ describe( 'ColorInputView', () => { } ); view.render(); - expect( view._dropdownView.panelPosition ).to.equal( 'se' ); + expect( view.dropdownView.panelPosition ).to.equal( 'se' ); } ); } ); it( 'should register panelView children in #_focusables', () => { expect( view._focusables.map( f => f ) ).to.have.members( [ - view._dropdownView.panelView.children.first, - view._dropdownView.panelView.children.last + view.dropdownView.panelView.children.first, + view.dropdownView.panelView.children.last ] ); } ); it( 'should register panelView children elements in #focusTracker', () => { - expect( view.focusTracker._elements ).to.include( view._dropdownView.panelView.children.first.element ); - expect( view.focusTracker._elements ).to.include( view._dropdownView.panelView.children.last.element ); + expect( view.focusTracker._elements ).to.include( view.dropdownView.panelView.children.first.element ); + expect( view.focusTracker._elements ).to.include( view.dropdownView.panelView.children.last.element ); } ); } ); @@ -215,11 +215,11 @@ describe( 'ColorInputView', () => { } ); it( 'should close the dropdown upon ColorTileView#execute', () => { - view._dropdownView.isOpen = true; + view.dropdownView.isOpen = true; colorGridView.items.last.fire( 'execute' ); - expect( view._dropdownView.isOpen ).to.be.false; + expect( view.dropdownView.isOpen ).to.be.false; } ); it( 'should fire the ColorInputView#input event upon ColorTileView#execute', () => { @@ -254,12 +254,12 @@ describe( 'ColorInputView', () => { expect( view.value ).to.equal( '' ); } ); - it( 'should close the #_dropdownView upon #execute', () => { - view._dropdownView.isOpen = true; + it( 'should close the #dropdownView upon #execute', () => { + view.dropdownView.isOpen = true; removeColorButton.fire( 'execute' ); - expect( view._dropdownView.isOpen ).to.be.false; + expect( view.dropdownView.isOpen ).to.be.false; } ); } ); @@ -410,7 +410,7 @@ describe( 'ColorInputView', () => { it( 'should set the template', () => { expect( view.element.classList.contains( 'ck' ) ).to.be.true; expect( view.element.classList.contains( 'ck-input-color' ) ).to.be.true; - expect( view.element.firstChild ).to.equal( view._dropdownView.element ); + expect( view.element.firstChild ).to.equal( view.dropdownView.element ); expect( view.element.lastChild ).to.equal( inputView.element ); } ); @@ -496,7 +496,7 @@ describe( 'ColorInputView', () => { let removeColorButton; beforeEach( () => { - removeColorButton = view._dropdownView.panelView.children.first; + removeColorButton = view.dropdownView.panelView.children.first; } ); it( 'should replace "Remove color" with "Restore default"', () => { @@ -527,7 +527,7 @@ describe( 'ColorInputView', () => { view.render(); global.document.body.appendChild( view.element ); - colorGridView = view._dropdownView.panelView.children.last; + colorGridView = view.dropdownView.panelView.children.last; } ); afterEach( () => { @@ -543,7 +543,7 @@ describe( 'ColorInputView', () => { stopPropagation: sinon.spy() }; - view._dropdownView.isOpen = true; + view.dropdownView.isOpen = true; // Mock the remove color button view is focused. view.focusTracker.isFocused = true; @@ -566,7 +566,7 @@ describe( 'ColorInputView', () => { stopPropagation: sinon.spy() }; - view._dropdownView.isOpen = true; + view.dropdownView.isOpen = true; // Mock the remove color button view is focused. view.focusTracker.isFocused = true; @@ -590,7 +590,7 @@ describe( 'ColorInputView', () => { stopPropagation: sinon.spy() }; - view._dropdownView.isOpen = true; + view.dropdownView.isOpen = true; // Mock the first color button is focused. colorGridView.focusTracker.isFocused = true; @@ -611,7 +611,7 @@ describe( 'ColorInputView', () => { stopPropagation: sinon.spy() }; - view._dropdownView.isOpen = true; + view.dropdownView.isOpen = true; // Mock the first color button is focused. colorGridView.focusTracker.isFocused = true; @@ -649,7 +649,7 @@ describe( 'ColorInputView', () => { view.render(); sinon.assert.calledOnce( spy ); - sinon.assert.calledWithExactly( spy, view._dropdownView.panelView.element ); + sinon.assert.calledWithExactly( spy, view.dropdownView.panelView.element ); view.destroy(); } ); From 27aa168d3e01619dc84885d16f813ec6d2cfa535 Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 30 Aug 2022 11:11:41 +0200 Subject: [PATCH 13/22] Fix css for bottom border of the remove color button in table properties. --- .../theme/ckeditor5-table/colorinput.css | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-theme-lark/theme/ckeditor5-table/colorinput.css b/packages/ckeditor5-theme-lark/theme/ckeditor5-table/colorinput.css index f627c9c6342..b7c95f9f25a 100644 --- a/packages/ckeditor5-theme-lark/theme/ckeditor5-table/colorinput.css +++ b/packages/ckeditor5-theme-lark/theme/ckeditor5-table/colorinput.css @@ -62,12 +62,15 @@ & .ck.ck-input-color__remove-color { width: 100%; - border-bottom: 1px solid var(--ck-color-input-border); padding: calc(var(--ck-spacing-standard) / 2) var(--ck-spacing-standard); border-bottom-left-radius: 0; border-bottom-right-radius: 0; + &:not(:focus) { + border-bottom: 1px solid var(--ck-color-input-border); + } + @mixin ck-dir ltr { border-top-right-radius: 0; } From b0c238f45cc375115e569d3511a7d4f073ff897e Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Tue, 30 Aug 2022 12:05:34 +0200 Subject: [PATCH 14/22] Fix css for left border for color picker button in table properties. --- .../theme/ckeditor5-table/colorinput.css | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-theme-lark/theme/ckeditor5-table/colorinput.css b/packages/ckeditor5-theme-lark/theme/ckeditor5-table/colorinput.css index b7c95f9f25a..547c192bd9e 100644 --- a/packages/ckeditor5-theme-lark/theme/ckeditor5-table/colorinput.css +++ b/packages/ckeditor5-theme-lark/theme/ckeditor5-table/colorinput.css @@ -24,15 +24,21 @@ padding: 0; @mixin ck-dir ltr { - border-left-width: 0; border-top-left-radius: 0; border-bottom-left-radius: 0; + + &:not(:focus) { + border-left: 1px solid transparent; + } } @mixin ck-dir rtl { - border-right-width: 0; border-top-right-radius: 0; border-bottom-right-radius: 0; + + &:not(:focus) { + border-right: 1px solid transparent; + } } &.ck-disabled { From d513ffb7bb5a2a1418fe91412897c4b81226dc8f Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Wed, 31 Aug 2022 11:44:44 +0200 Subject: [PATCH 15/22] Add aria-describedby to the color input fields of table properties. --- .../ckeditor5-table/src/ui/colorinputview.js | 17 ++++++++--------- .../src/utils/ui/table-properties.js | 14 +++++++------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/packages/ckeditor5-table/src/ui/colorinputview.js b/packages/ckeditor5-table/src/ui/colorinputview.js index 70c9194dcad..1d1a3187772 100644 --- a/packages/ckeditor5-table/src/ui/colorinputview.js +++ b/packages/ckeditor5-table/src/ui/colorinputview.js @@ -137,14 +137,13 @@ export default class ColorInputView extends View { /** * An instance of the input allowing the user to type a color value. * - * @protected * @member {module:ui/inputtext/inputtextview~InputTextView} */ - this._inputView = this._createInputTextView(); + this.inputView = this._createInputTextView(); /** * The flag that indicates whether the user is still typing. - * If set to true, it means that the text input field ({@link #_inputView}) still has the focus. + * If set to true, it means that the text input field ({@link #inputView}) still has the focus. * So, we should interrupt the user by replacing the input's value. * * @protected @@ -194,7 +193,7 @@ export default class ColorInputView extends View { }, children: [ this.dropdownView, - this._inputView + this.inputView ] } ); @@ -215,7 +214,7 @@ export default class ColorInputView extends View { * Focuses the input. */ focus() { - this._inputView.focus(); + this.inputView.focus(); } /** @@ -293,7 +292,7 @@ export default class ColorInputView extends View { * Creates and configures an instance of {@link module:ui/inputtext/inputtextview~InputTextView}. * * @private - * @returns {module:ui/inputtext/inputtextview~InputTextView} A configured instance to be set as {@link #_inputView}. + * @returns {module:ui/inputtext/inputtextview~InputTextView} A configured instance to be set as {@link #inputView}. */ _createInputTextView() { const locale = this.locale; @@ -375,7 +374,7 @@ export default class ColorInputView extends View { } /** - * Sets {@link #_inputView}'s value property to the color value or color label, + * Sets {@link #inputView}'s value property to the color value or color label, * if there is one and the user is not typing. * * Handles cases like: @@ -393,9 +392,9 @@ export default class ColorInputView extends View { const mappedColor = this.options.colorDefinitions.find( def => normalizedInputValue === normalizeColor( def.color ) ); if ( mappedColor ) { - this._inputView.value = mappedColor.label; + this.inputView.value = mappedColor.label; } else { - this._inputView.value = inputValue || ''; + this.inputView.value = inputValue || ''; } } } diff --git a/packages/ckeditor5-table/src/utils/ui/table-properties.js b/packages/ckeditor5-table/src/utils/ui/table-properties.js index e36e75deeaf..fcb77d8e66d 100644 --- a/packages/ckeditor5-table/src/utils/ui/table-properties.js +++ b/packages/ckeditor5-table/src/utils/ui/table-properties.js @@ -372,29 +372,29 @@ export const defaultColors = [ */ export function getLabeledColorInputCreator( options ) { return ( labeledFieldView, viewUid, statusUid ) => { - const inputView = new ColorInputView( labeledFieldView.locale, { + const colorInputView = new ColorInputView( labeledFieldView.locale, { colorDefinitions: colorConfigToColorGridDefinitions( options.colorConfig ), columns: options.columns, defaultColorValue: options.defaultColorValue } ); - inputView.set( { + colorInputView.inputView.set( { id: viewUid, ariaDescribedById: statusUid } ); - inputView.bind( 'isReadOnly' ).to( labeledFieldView, 'isEnabled', value => !value ); - inputView.bind( 'hasError' ).to( labeledFieldView, 'errorText', value => !!value ); + colorInputView.bind( 'isReadOnly' ).to( labeledFieldView, 'isEnabled', value => !value ); + colorInputView.bind( 'hasError' ).to( labeledFieldView, 'errorText', value => !!value ); - inputView.on( 'input', () => { + colorInputView.on( 'input', () => { // UX: Make the error text disappear and disable the error indicator as the user // starts fixing the errors. labeledFieldView.errorText = null; } ); - labeledFieldView.bind( 'isEmpty', 'isFocused' ).to( inputView ); + labeledFieldView.bind( 'isEmpty', 'isFocused' ).to( colorInputView ); - return inputView; + return colorInputView; }; } From 4b81359ec6020445631b43688daadee185e7e3db Mon Sep 17 00:00:00 2001 From: Marta Motyczynska Date: Wed, 31 Aug 2022 12:17:36 +0200 Subject: [PATCH 16/22] Simplify the template for ColorInputView. --- .../ckeditor5-table/src/ui/colorinputview.js | 37 +------------------ 1 file changed, 2 insertions(+), 35 deletions(-) diff --git a/packages/ckeditor5-table/src/ui/colorinputview.js b/packages/ckeditor5-table/src/ui/colorinputview.js index 1d1a3187772..7164d860aea 100644 --- a/packages/ckeditor5-table/src/ui/colorinputview.js +++ b/packages/ckeditor5-table/src/ui/colorinputview.js @@ -35,8 +35,6 @@ export default class ColorInputView extends View { constructor( locale, options ) { super( locale ); - const bind = this.bindTemplate; - /** * The value of the input. * @@ -46,14 +44,6 @@ export default class ColorInputView extends View { */ this.set( 'value', '' ); - /** - * The `id` attribute of the input (i.e. to pair with the `