Added custom `aria-haspopup` values to UI Button, Rich Combo, Panel Button, Menu Button has now proper values #2079

engineering-this · 2018-06-12T09:48:58Z

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 support for custom aria-haspopup to UI Button plugin, via button.definition.hasPopup. Plugins extending button has now proper values:

Menu Button - menu
Panel Button - listbox

also Rich Combo value is now listbox.

Closes #2072

Comandeer · 2018-06-12T13:53:43Z

plugins/button/plugin.js

@@ -432,6 +433,9 @@
 	 * 		editor.ui.addButton( 'my_button', {
 	 * 			iconHiDpi: 'assets/icons/my_button.hidpi.png'
 	 * 		} )
+	 * @param {String} definition.hasPopup The value of button elements `aria-haspopup` attribute.


I don't like the fact that now we have two variables for button that mean exactly the same. What's more hasArrow didn't even become deprecated. I'd see two ways to go:

Overload hasArrow and add new values to it.

Create hasPopup, but deprecate hasArrow.

If we went with 2., it'd be nice to think about better name. hasPopup indicates that this variable is a boolean, but in fact i's an enum.

But both ways need docs for hasArrow parameter.

Comandeer · 2018-06-12T13:59:25Z

plugins/menubutton/plugin.js

@@ -74,6 +74,8 @@ CKEDITOR.plugins.add( 'menubutton', {

 				this.hasArrow = true;

+				this.hasPopup = 'menu';


This code nicely shows the issue I mentioned: now we have two properties that do exactly the same. If we go with new property, it should be enough to declare it alone, without hasArrow. If we go with overloading hasArrow, then we just need to replace the value ;)

Comandeer · 2018-06-12T14:01:38Z

tests/plugins/button/aria.js

@@ -14,6 +14,9 @@ bender.editor = {
 					label: 'disabled button',
 					modes: {} // This button should be disabled because it does not work in any of modes.
 				} );
+				editor.ui.addButton( 'haspopup_btn', {
+					hasPopup: 'foo'


Why not use some of the real values?

Comandeer · 2018-06-12T14:03:58Z

tests/plugins/toolbar/manual/aria-haspopup.md

@@ -0,0 +1,19 @@
+@bender-tags: 4.8.0, trac16893, bug


What happened to these tags?

Comandeer · 2018-06-12T14:04:02Z

tests/plugins/toolbar/manual/aria-haspopup.md

+
+# Test scenario:
+
+Open web inspector, for each button in menu find it's 'a' element and search for `aria-haspopup` attribute.


This test is totally unfriendly. It'd rather create list, in which all [aria-haspopup] attributes for the buttons will be listed.

Comandeer · 2018-06-12T14:04:23Z

tests/plugins/richcombo/richcombo.js

@@ -31,5 +31,11 @@ bender.test( {
 		assert.isTrue( btnEl.hasClass( 'cke_combo' ), 'check ui type class name' );
 		assert.isTrue( btnEl.hasClass( 'cke_combo__custom_combo' ), 'check named ui type class name' );
 		assert.isTrue( btnEl.hasClass( customCls ), 'check ui item custom class name' );
+	},
+	'test aria-haspopup': function() {


Let's add issue reference.

Comandeer · 2018-06-12T14:04:35Z

tests/plugins/button/aria.js

@@ -53,6 +56,13 @@ bender.test( {
 		assert.areEqual( 'aria label', label.getText(), 'innerText of label doesn\'t match' );
 	},

+	'test aria-haspopup': function() {


Let's add issue reference.

Comandeer · 2018-06-18T13:29:20Z

tests/plugins/toolbar/manual/aria-haspopup.md

@@ -0,0 +1,18 @@
+@bender-tags: 4.10.0, feature, 2072


I don't think that we have any other test with - in its name. Let's rename it to ariahaspopup

Comandeer · 2018-06-18T13:30:19Z

tests/plugins/toolbar/manual/aria-haspopup.html

+</table>
+
+<script>
+	if ( CKEDITOR.env.mobile ) {


After changing test into such form I don't see need to ignore mobile devices – test should be working just fine on mobiles.

Comandeer · 2018-06-18T13:32:05Z

tests/plugins/toolbar/manual/aria-haspopup.html

+	if ( CKEDITOR.env.mobile ) {
+		bender.ignore();
+	}
+	CKEDITOR.replace( 'editor', {


I'd force English language in the test, because my Chrome claims to be Polish.

Comandeer · 2018-06-18T13:41:21Z

tests/plugins/richcombo/richcombo.js

@@ -31,5 +31,12 @@ bender.test( {
 		assert.isTrue( btnEl.hasClass( 'cke_combo' ), 'check ui type class name' );
 		assert.isTrue( btnEl.hasClass( 'cke_combo__custom_combo' ), 'check named ui type class name' );
 		assert.isTrue( btnEl.hasClass( customCls ), 'check ui item custom class name' );
+	},
+	// WAI-ARIA 1.1 has added new values for aria-haspopup property #2072


Please adhere to our comments standard:

// Comment (#1618).

Comandeer · 2018-06-18T13:41:52Z

tests/plugins/button/aria.js

@@ -53,6 +56,14 @@ bender.test( {
 		assert.areEqual( 'aria label', label.getText(), 'innerText of label doesn\'t match' );
 	},

+	// WAI-ARIA 1.1 has added new values for aria-haspopup property #2072


Please adhere to our comments standard:

// Comment (#1618).

Comandeer · 2018-06-18T13:44:37Z

plugins/button/plugin.js

@@ -432,6 +432,8 @@
 	 * 		editor.ui.addButton( 'my_button', {
 	 * 			iconHiDpi: 'assets/icons/my_button.hidpi.png'
 	 * 		} )
+	 * @param {String/Boolean} definition.hasArrow If button should have an arrow, and value of buttons `aria-haspopup` attribute.


It sounds strange. Maybe more like:

If boolean, indicates if button should have a dropdown. If string, acts as a value of button's aria-haspopup attribute.

Comandeer · 2018-06-18T13:45:45Z

plugins/button/plugin.js

@@ -298,7 +298,7 @@
 				title: this.title + ( shortcut ? ' (' + shortcut.display + ')' : '' ),
 				ariaShortcut: shortcut ? editor.lang.common.keyboardShortcut + ' ' + shortcut.aria : '',
 				titleJs: env.gecko && !env.hc ? '' : ( this.title || '' ).replace( "'", '' ),
-				hasArrow: this.hasArrow ? 'true' : 'false',
+				hasArrow: this.hasArrow || false,


Please note that attribute should get 'false' (string with value "false"), not a real boolean. The same is true for true value.

Comandeer · 2018-06-27T15:52:37Z

I changed base branch to next as major is locked now.

…value

Comandeer

LGTM!

engineering-this requested a review from Comandeer June 12, 2018 10:16

Comandeer requested changes Jun 12, 2018

View reviewed changes

engineering-this force-pushed the t/2072 branch 2 times, most recently from c72fd4d to c2a9400 Compare June 13, 2018 12:44

engineering-this requested a review from Comandeer June 13, 2018 12:45

Comandeer requested changes Jun 18, 2018

View reviewed changes

engineering-this requested a review from Comandeer June 19, 2018 08:45

Comandeer changed the base branch from major to next June 27, 2018 15:52

Comandeer force-pushed the t/2072 branch from 90bcf13 to 294f595 Compare June 27, 2018 16:28

engineering-this and others added 13 commits June 27, 2018 18:30

Added manual test

28e54d2

Added unit tests

3b58800

Added test case for aria-haspopup

fd990d6

Added support for custom aria-haspopup value

80ab165

Added changelog entry.

6c86981

Removed hasPopup param and overloaded hasArrow

eb9c3b2

Added comment referencing issue

cbeb3a6

Changed manual test and description

4e8d728

Improved docs entry

c889888

Test renamed, removed ignore on mobiles, forced english language

0c15f67

Fixed comment

296f53a

Changed docs entry, hasArrow property will always have string as its …

53bb46c

…value

Update version tag.

c080019

Comandeer force-pushed the t/2072 branch from 294f595 to c080019 Compare June 27, 2018 16:32

Comandeer approved these changes Jun 27, 2018

View reviewed changes

Comandeer merged commit c5c5e39 into next Jun 27, 2018

Comandeer deleted the t/2072 branch June 27, 2018 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added custom `aria-haspopup` values to UI Button, Rich Combo, Panel Button, Menu Button has now proper values #2079

Added custom `aria-haspopup` values to UI Button, Rich Combo, Panel Button, Menu Button has now proper values #2079

engineering-this commented Jun 12, 2018 •

edited

Loading

Comandeer Jun 12, 2018

Comandeer Jun 12, 2018

Comandeer Jun 12, 2018

Comandeer Jun 12, 2018

Comandeer Jun 12, 2018

Comandeer Jun 12, 2018

Comandeer Jun 12, 2018

Comandeer Jun 18, 2018

Comandeer Jun 18, 2018

Comandeer Jun 18, 2018

Comandeer Jun 18, 2018

Comandeer Jun 18, 2018

Comandeer Jun 18, 2018

Comandeer Jun 18, 2018

Comandeer commented Jun 27, 2018

Comandeer left a comment

		@@ -74,6 +74,8 @@ CKEDITOR.plugins.add( 'menubutton', {

		this.hasArrow = true;

		this.hasPopup = 'menu';


		# Test scenario:

		Open web inspector, for each button in menu find it's 'a' element and search for `aria-haspopup` attribute.

Added custom aria-haspopup values to UI Button, Rich Combo, Panel Button, Menu Button has now proper values #2079

Added custom aria-haspopup values to UI Button, Rich Combo, Panel Button, Menu Button has now proper values #2079

Conversation

engineering-this commented Jun 12, 2018 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

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

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 commented Jun 27, 2018

Comandeer left a comment

Choose a reason for hiding this comment

Added custom `aria-haspopup` values to UI Button, Rich Combo, Panel Button, Menu Button has now proper values #2079

Added custom `aria-haspopup` values to UI Button, Rich Combo, Panel Button, Menu Button has now proper values #2079

engineering-this commented Jun 12, 2018 •

edited

Loading