New plugin: splitbutton #1766

engineering-this · 2018-03-08T16:38:53Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

I've added the splitbutton plugin which is extending the menubutton class. Also I've slightly updated the button

Closes #1679.

Comandeer

There is issue with plugins with options that are exclusive (e.g. justify) – menu does not correctly update states.

I'm also wondering about making default button not focusable. It makes this button inaccessible not only via screen readers, but also via keyboard. What's more, in screen reader this button does not have any role at all (it is not even used as a state indicator for the splitbutton, to announce which option is active at the moment). Nearly all other implementations in the wild make both of buttons focusable (e.g. Bootstrap).

The other concern I have is the fact how menu works: it focuses the first element upon opening. However our Styles or Format combos focuses the first selected option. I'm wondering what would be more user-friendly. Speaking in terms of ARIA: menu or listbox?

I also don't like the default styles. For now it's pretty obvious that splitbutton is, in fact, two buttons.

Comandeer · 2018-03-09T14:39:15Z

tests/plugins/splitbutton/integration/manual/balloontoolbar.html

+			basicStyles: evt.editor.ui.create( 'teststylesplit' )
+		} );
+
+		panel.attach( this.editable() );


This seems like a toolbar is broken. Maybe implement following the text?

Comandeer · 2018-03-09T14:43:44Z

tests/plugins/splitbutton/manual/splitbutton.html

+	} );
+
+	// Just hiding buttons that are irrelevant to test.
+	CKEDITOR.instances.editor.on( 'contentDom', function() {


This code breaks ability to focus toolbar via Alt + F10 keystroke, disabling sensible means to test screen reader.

It also uncovers very important issue with the whole implementation: as #678 is not resolved, splitbutton needs other buttons to be added to be able to use their commands. That makes the whole feature de facto not usable at all.

Hej, @mlewand what is your view on this?

For sure we should not workaround this like that. We need a proper fix mentioned by @Comandeer.

I mean, workaround can be placed for the time being, but the issue can not be merged to stable release with such workaround in place.

@engineering-this please, put a reference to #678 anywhere where such workaround is used.

Comandeer · 2018-03-09T15:37:55Z

plugins/splitbutton/plugin.js

+				 * @param {Object} definition The splitButton definition.
+				 */
+				$: function( definition ) {
+					var groups = {},


Whole logic of adding items to the menu could be extracted to separate function(s) to make constructor more readable.

Comandeer · 2018-03-09T15:40:17Z

plugins/button/plugin.js

@@ -302,7 +304,7 @@
 				keydownFn: keydownFn,
 				focusFn: focusFn,
 				clickFn: clickFn,
-				style: CKEDITOR.skin.getIconStyle( iconPath, ( editor.lang.dir == 'rtl' ), overridePath, this.iconOffset ),
+				icon: ( this.noicon ? '' : '<span class="cke_button_icon cke_button__' + iconName + '_icon" style="' + style + '">&nbsp;</span>' ),


I'm wondering if we can't simplify that and instead of new option (noicon) just use iconName.

What's more, if we stick with noicon option, let's be coherent with the rest of the plugin and rename it to noIcon.

There are also no API docs for this new option.

Comandeer · 2018-03-09T15:44:52Z

tests/plugins/splitbutton/splitbutton.js

+};
+
+bender.test( {
+	'test splitbutton in toolbar': function() {


This test name does not describe exactly what the test does.

There are also missing tests for changing the state of menu items.

engineering-this · 2018-03-15T15:03:23Z

@Comandeer

There is issue with plugins with options that are exclusive (e.g. justify) – menu does not correctly update states.

It looks fine with justify on sample
https://ckeditor4-preview.ckeditor.com/t/1679/plugins/splitbutton/samples/splitbutton.html

The other concern I have is the fact how menu works: it focuses the first element upon opening. However our Styles or Format combos focuses the first selected option. I'm wondering what would be more user-friendly. Speaking in terms of ARIA: menu or listbox?

I think we need a bit of discussion here if first item should be initially focused or not?

@mlewand What is your tough on that?

It also uncovers very important issue with the whole implementation: as #678 is not resolved, splitbutton needs other buttons to be added to be able to use their commands. That makes the whole feature de facto not usable at all.

~~Actually it is caused by ACF. I had to set config.allowedContent = true; and everything works. My question here is how should we deal with that?~~

Just added feature for each button.

engineering-this · 2018-03-16T13:42:23Z

After some talking first item will be focused on menu open, just like other menu buttons.

Comandeer

Several issues:

1. TypeError

Procedure

Open http://tests.ckeditor.test:1030/tests/plugins/splitbutton/integration/manual/balloontoolbar
Open split button by clicking arrow and select any option. Do not focus editor before.

Expected

Style is applied to the editor.

Actual

Error is thrown:

plugin.js:60 Uncaught TypeError: Cannot read property 'setStyle' of null
    at CKEDITOR.command.<anonymous> (plugin.js:60)
    at CKEDITOR.command.listenerFirer (event.js:144)
    at CKEDITOR.command.CKEDITOR.event.fire (event.js:290)
    at CKEDITOR.command.setState (command.js:231)
    at Editor.<anonymous> (plugin.js:29)
    at Editor.<anonymous> (style.js:1970)
    at Editor.listenerFirer (event.js:144)
    at Editor.CKEDITOR.event.fire (event.js:290)
    at Editor.CKEDITOR.editor.fire (editor_basic.js:24)
    at Editor.checkSelectionChange (selection.js:270)

2. Focus issues

Procedure

Open http://tests.ckeditor.test:1030/tests/plugins/splitbutton/integration/manual/balloontoolbar
Focus editor.
Select any option from the split button (except the first one).
Press Alt + F10.

Expected

The first button in the toolbar is focused.

Actual

Focus remains inside the editor.

3. Changing buttons

Procedure

Open http://tests.ckeditor.test:1030/tests/plugins/splitbutton/integration/manual/balloontoolbar
Select some text.
Apply several styles from split button options.
Keep clicking split button face.

Expected

Actually we haven't defined any behavior for that, but changing icons in such way seems to be confusing. Microsoft Word has always the same icon in split button face and it changes only its state, e.g. "Numbering" has always the same icon despite which numbering option is selected. The face is used for switching feature on/off, not for representing currently selected option.

Actual

Disabling style hides it icon, showing the icon of previously applied style. It also changes the labelling of the whole button, increasing confusion for screen reader users.

4. Styling of focused option

Procedure

Open http://tests.ckeditor.test:1030/tests/plugins/splitbutton/integration/manual/balloontoolbar
Select some text.
Apply several styles from split button options.
Click arrow to open a list of options.

Expected

Focused option should be distinctive from selected option.

Actual

Focused and selected options have the same styles.

Comandeer · 2018-05-04T12:10:54Z

plugins/button/plugin.js

@@ -286,14 +287,15 @@
 			} else {
 				iconPath = iconName;
 			}
-
+			var style = CKEDITOR.skin.getIconStyle( iconPath, ( editor.lang.dir == 'rtl' ), overridePath, this.iconOffset );


Now we have two style variables (this style and… this.style), which do totally different things. We should probably rename this variable to iconStyle or something like that.

Comandeer · 2018-05-04T12:25:13Z

plugins/button/plugin.js

@@ -406,8 +408,9 @@
 	 * @param {String} definition.command The command to be executed once the button is activated.
 	 * @param {String} definition.toolbar The {@link CKEDITOR.config#toolbarGroups toolbar group} into which
 	 * the button will be added. An optional index value (separated by a comma) determines the button position within the group.
-	 * @param {String} definition.icon The path to a custom icon or icon name registered by another plugin. Custom icon paths
-	 * are supported since the **4.9.0** version.
+	 * @param {String} definition.style The optional inline style that will be applied on button.


Please add that this parameter was added in 4.10.

Comandeer · 2018-05-04T12:25:29Z

plugins/button/plugin.js

-	 * @param {String} definition.icon The path to a custom icon or icon name registered by another plugin. Custom icon paths
-	 * are supported since the **4.9.0** version.
+	 * @param {String} definition.style The optional inline style that will be applied on button.
+	 * @param {String/Boolean} definition.icon The path to a custom icon or icon name registered by another plugin. Custom icon paths


Please add that false value was added in 4.10.

Comandeer · 2018-05-04T15:06:21Z

plugins/splitbutton/plugin.js

+					properties.buttons[ activeButton.id ].hidden = false;
+					previousButton = activeButton;
+				} else {
+					CKEDITOR.document.getById( properties.buttons[ defaultButton.id ]._.id ).removeStyle( 'display' );


This code is the same as code above and similar to code, in which display is set to none. It'd move it to separate, dedicated function.

Comandeer · 2018-05-04T15:19:57Z

plugins/splitbutton/plugin.js

+					defaultButton = properties.buttons[ item.id ];
+				} else {
+					properties.buttons[ item.id ].style = 'display: none;';
+					properties.buttons[ item.id ].hidden = true;


I understand that it's a workaround for #678 to add features by adding buttons and hiding them?

You understand right :)

Then I'd think about making hide method on button or any other toolbar element.

engineering-this · 2018-05-08T16:05:18Z

Actually we haven't defined any behavior for that, but changing icons in such way seems to be confusing. Microsoft Word has always the same icon in split button face and it changes only its state, e.g. "Numbering" has always the same icon despite which numbering option is selected. The face is used for switching feature on/off, not for representing currently selected option.

@Comandeer this was the our idea of split button, to create 'splitbutton' with static face we could just add one button for default behaviour and next to it menu button for options.

Comandeer · 2018-05-10T11:50:40Z

But now the face is not static, it's dynamically changing and I think it's quite confusing to the end user.

engineering-this · 2018-05-10T13:01:03Z

Well, I think we could add option to lock face on default button.

Comandeer

There are no tests for buttons' hiding logic or focus order in toolbar.

I also have very serious doubts around accessibility of the current splitbutton behaviour.

Comandeer · 2018-05-11T12:04:30Z

CHANGES.md

@@ -6,6 +6,7 @@
 New Features:

 * [#1761](https://github.com/ckeditor/ckeditor-dev/issues/1761): [Autolink](https://ckeditor.com/cke4/addon/autolink) plugin supports email links.
+* [#1679](https://github.com/ckeditor/ckeditor-dev/issues/1679): Introduced the Split Button plugin. Also added option to set initial style for button when creating it, and create button without icon.


Please add link to the plugin.

Comandeer · 2018-05-11T12:53:27Z

plugins/splitbutton/plugin.js

+					defaultButton = properties.buttons[ item.id ];
+				} else {
+					properties.buttons[ item.id ].style = 'display: none;';
+					properties.buttons[ item.id ].hidden = true;


Then I'd think about making hide method on button or any other toolbar element.

Comandeer

I don't see any tests for static face. There are also no tests for every skin (moono, moono-lisa, kama).

It is also not possible/very convoluted to create split-button in "Word style", e.g. "Underline" in MS Word 365:

Static face is used to apply last selected underline style.
Attached menu is used to select underline style and apply it to selection.

With current solution every choice in menu must be a separate command.

Comandeer · 2018-05-15T12:24:19Z

plugins/splitbutton/plugin.js

+
+			item.role = 'menuitemcheckbox';
+
+			item.onClick = function() {


What if I want to declare custom onClick and omit command altogether? This code won't allow me to do this. What's more: if we're forcing working only with commands, there should be a way to pass data to the command.

Comandeer

I'm not happy with newest API changes. It seems that there are too many new properties and it becomes too complex.

However splitbuttonunderline test is awesome – it works just like I imagined splitbutton! I'd move it also to samples.

Comandeer · 2018-05-27T17:37:34Z

plugins/button/plugin.js

+		 *
+		 * @since 4.10.0
+		 */
+		hideFromToolbar: function() {


Is it from UI or from toolbar? If from whole UI (e.g. also in balloon toolbar), then simple hide and show seems more appropriate.

Comandeer · 2018-05-27T17:38:55Z

plugins/button/plugin.js

+		 *
+		 * @since 4.10.0
+		 */
+		showInToolbar: function() {


If button is not yet rendered, shouldn't this function remove initial style if it's set to display: none;?

Comandeer · 2018-05-27T18:20:31Z

plugins/splitbutton/plugin.js

+			};
+			item.click = item.onClick;
+
+			editor.getCommand( item.command ).on( 'state', function() {


It's still not possible to bind only click to button, without any command.

There aren't also tests for such case.

Comandeer · 2018-05-27T18:36:59Z

plugins/menu/plugin.js

@@ -188,7 +188,7 @@ CKEDITOR.plugins.add( 'menu', {
 							var item = this.editor.getMenuItem( itemName );

 							if ( item && ( !item.command || this.editor.getCommand( item.command ).state ) ) {
-								item.state = listenerItems[ itemName ];
+								item.state = ( item.stateFn && item.stateFn() ) || listenerItems[ itemName ];


I'm not sure if a better solution won't be to just substitute listener, not add another property directly to menu.

Comandeer

I'm wondering also about one thing: why do we need to render and hide all buttons? Can't we have one button that would act as face and its icon and click actions would be just replaced ad hoc? Is the current solution connected with our convoluted way of registering commands alongside the buttons? If yes, then I really think if #678 isn't blocking.

Comandeer · 2018-06-05T06:37:15Z

CHANGES.md

@@ -6,6 +6,7 @@
 New Features:

 * [#1761](https://github.com/ckeditor/ckeditor-dev/issues/1761): [Autolink](https://ckeditor.com/cke4/addon/autolink) plugin supports email links.
+* [#1679](https://github.com/ckeditor/ckeditor-dev/issues/1679): Introduced the [Split Button](https://ckeditor.com/cke4/addon/splitbutton plugin. Also added option to set initial style for button when creating it, and create button without icon.


Link is not closed properly.

Comandeer · 2018-06-05T07:07:01Z

plugins/button/plugin.js

@@ -302,7 +304,7 @@
 				keydownFn: keydownFn,
 				focusFn: focusFn,
 				clickFn: clickFn,
-				style: CKEDITOR.skin.getIconStyle( iconPath, ( editor.lang.dir == 'rtl' ), overridePath, this.iconOffset ),
+				icon: ( this.icon === false ? '' : '<span class="cke_button_icon cke_button__' + iconName + '_icon" style="' + iconStyle + '">&nbsp;</span>' ),


I'm thinking about less strict condition here: just this.icon to accept all falsy values. Passing '' or null as icon to tell that we don't want icon seems more intuitive than passing false.

Comandeer · 2018-06-05T07:59:22Z

plugins/toolbar/plugin.js

@@ -159,6 +141,29 @@
 							return false;
 					}
 					return true;
+					function getNextItem( item, nextOrPrevious ) {
+						var next;


next is a little unfortunate name for variable that could also hold previous item in toolbar ;)

Comandeer · 2018-06-06T15:57:31Z

tests/plugins/button/button.js

+			btnEl = CKEDITOR.document.getById( btn._.id );
+		assert.isNull( btnEl.findOne( '.cke_button_icon' ) );
+	},
+	'test hiding button from toolbar': function() {


This test is failing as names of methods were changed.

engineering-this · 2018-06-07T11:03:36Z

I'm wondering also about one thing: why do we need to render and hide all buttons? Can't we have one button that would act as face and its icon and click actions would be just replaced ad hoc? Is the current solution connected with our convoluted way of registering commands alongside the buttons? If yes, then I really think if #678 isn't blocking.

@Comandeer: We need to render buttons, otherwise we won't register commands. And if thats necessary then I'm reusing all rendered buttons. It might be possible to remove them after rendering and just switch face onClick, icon and label, but that would just make all the logic even more complex. Imho this would be too haxy.

engineering-this · 2018-11-20T12:04:08Z

Rebased onto latest major.

Comandeer requested changes Mar 9, 2018

View reviewed changes

engineering-this force-pushed the t/1679 branch 2 times, most recently from 2304606 to 960b4ac Compare March 14, 2018 17:32

engineering-this force-pushed the t/1679 branch 2 times, most recently from 8a81e7a to b3720d1 Compare March 16, 2018 13:39

engineering-this requested a review from Comandeer March 16, 2018 13:42

Comandeer requested changes May 4, 2018

View reviewed changes

engineering-this force-pushed the t/1679 branch from b84ae4c to 4ae5465 Compare May 8, 2018 16:13

engineering-this requested a review from Comandeer May 8, 2018 16:18

Comandeer requested changes May 11, 2018

View reviewed changes

engineering-this force-pushed the t/1679 branch from 717e9b9 to c334499 Compare May 15, 2018 08:00

engineering-this requested a review from Comandeer May 15, 2018 09:17

Comandeer requested changes May 15, 2018

View reviewed changes

engineering-this requested a review from Comandeer May 23, 2018 12:50

engineering-this force-pushed the t/1679 branch 2 times, most recently from 68c596d to 653118b Compare May 24, 2018 12:53

Comandeer requested changes May 27, 2018

View reviewed changes

engineering-this requested a review from Comandeer May 29, 2018 09:09

Comandeer requested changes Jun 6, 2018

View reviewed changes

engineering-this force-pushed the t/1679 branch 2 times, most recently from ad170e8 to ff641eb Compare June 7, 2018 10:51

engineering-this requested a review from Comandeer June 7, 2018 11:16

engineering-this force-pushed the t/1679 branch 2 times, most recently from 7a45ee9 to b90be88 Compare June 11, 2018 11:15

engineering-this added 23 commits November 20, 2018 13:03

Updated test description

7df9187

Fixed error

95f0931

Added tc for static face

ccbbc76

Fixed click and onClick properties

f333021

Added test case and fixed tests on IE

9bdce8e

Typo

b5bb305

Added tests for each skin

916d69b

Renamed show and hide methods

a8b16aa

Removed stateFn

a1cd620

Splitbutton now passes itself as context to onMenu

c20ee3a

Custom stateFn changed to onMenu

6663d1b

Changed tests case for stateFn to custom onMenu

2ba4503

Fixed menu items disabled when editor is not focused

4a5c2cb

Added underline style to sample

32259c5

Codestyle reduntand space

56a3fe0

Added -webkit- prefixed style to allowedContent

2c3ccb1

Added extraAllowedContent to sample

663f8f1

Fixed after function name change

c13bc79

Butotn definition.icon has less strict condition for no icon

355639a

Changed some helper and vars naming

09c0e2b

Fixed typo

3162be8

Updated version references to 4.11

57259d6

Added docs entry on Split Button definition properties

f67f714

engineering-this force-pushed the t/1679 branch from c97c489 to f67f714 Compare November 20, 2018 12:03

Code style: corrected indentation.

5f8b00f

Comandeer mentioned this pull request Jul 9, 2019

Unified Paste Handling #3258

Open

f1ames mentioned this pull request Jul 17, 2019

Unify pasting UI #3276

Open

f1ames closed this Mar 20, 2020

f1ames added the pr:frozen ❄ The pull request on which work was suspended due to various reasons. label Mar 20, 2020

New plugin: splitbutton #1766

New plugin: splitbutton #1766

Conversation

engineering-this commented Mar 8, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineering-this commented Mar 15, 2018 • edited Loading

engineering-this commented Mar 16, 2018

Comandeer left a comment

Choose a reason for hiding this comment

1. TypeError

Procedure

Expected

Actual

2. Focus issues

Procedure

Expected

Actual

3. Changing buttons

Procedure

Expected

Actual

4. Styling of focused option

Procedure

Expected

Actual

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineering-this May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineering-this commented May 8, 2018 • edited Loading

Comandeer commented May 10, 2018

engineering-this commented May 10, 2018

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineering-this commented Jun 7, 2018

engineering-this commented Nov 20, 2018

engineering-this commented Mar 15, 2018 •

edited

Loading

engineering-this May 8, 2018 •

edited

Loading

engineering-this commented May 8, 2018 •

edited

Loading