From 16e6c0009cd3a3e44e10f662589d42db732eb033 Mon Sep 17 00:00:00 2001 From: Mateusz Samsel Date: Thu, 10 Oct 2019 10:10:32 +0200 Subject: [PATCH 01/10] Add role=presentation to empty table cells in dialog of special char plugin to avoid bug in screen readers. --- plugins/specialchar/dialogs/specialchar.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/specialchar/dialogs/specialchar.js b/plugins/specialchar/dialogs/specialchar.js index ffdcbf95b70..8b05764bf13 100644 --- a/plugins/specialchar/dialogs/specialchar.js +++ b/plugins/specialchar/dialogs/specialchar.js @@ -216,7 +216,7 @@ CKEDITOR.dialog.add( 'specialchar', function( editor ) { charDesc + '' ); } else { - html.push( ' ' ); + html.push( ' ' ); } html.push( '' ); From cb19bc526d8028f50e80f42687098cac6c7f8b01 Mon Sep 17 00:00:00 2001 From: Mateusz Samsel Date: Thu, 10 Oct 2019 11:03:04 +0200 Subject: [PATCH 02/10] Add unit test checking role of empty table cells in specialchar dialog. --- tests/plugins/specialchar/dialog.js | 33 +++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 tests/plugins/specialchar/dialog.js diff --git a/tests/plugins/specialchar/dialog.js b/tests/plugins/specialchar/dialog.js new file mode 100644 index 00000000000..7ae0e6dffd8 --- /dev/null +++ b/tests/plugins/specialchar/dialog.js @@ -0,0 +1,33 @@ +/* bender-tags: specialchar */ +/* bender-ckeditor-plugins: toolbar,wysiwygarea,specialchar */ + +( function() { + 'use strict'; + + bender.editor = true; + + bender.test( { + tearDown: function() { + var dialog; + + while ( ( dialog = CKEDITOR.dialog.getCurrent() ) ) { + dialog.hide(); + } + }, + + 'test specialchardialog should have table cells with role="presentation"': function() { + var bot = this.editorBot; + + bot.dialog( 'specialchar', function( dialog ) { + var tableWithCharacters = dialog.parts.contents.findOne( 'td.cke_dialog_ui_hbox_first table[role="listbox"]' ), + tableCells = tableWithCharacters.find( 'td' ).toArray(), + i; + + for ( i = 0; i < tableCells.length; i++ ) { + assert.areSame( 'presentation', tableCells[ i ].getAttribute( 'role' ), + 'Table cell with index: ' + i + ' should have role="presentation". Instead it has following html: ' + tableCells[ i ].getOuterHtml() ); + } + } ); + } + } ); +} )(); From 8e47d32fd7e32d420e0f75dccaf2621d3c5ce878 Mon Sep 17 00:00:00 2001 From: Mateusz Samsel Date: Thu, 10 Oct 2019 12:28:58 +0200 Subject: [PATCH 03/10] Remove empty table cells as those was read by narrator and voice over anyway. --- plugins/specialchar/dialogs/specialchar.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/plugins/specialchar/dialogs/specialchar.js b/plugins/specialchar/dialogs/specialchar.js index 8b05764bf13..a982d7c28eb 100644 --- a/plugins/specialchar/dialogs/specialchar.js +++ b/plugins/specialchar/dialogs/specialchar.js @@ -215,8 +215,6 @@ CKEDITOR.dialog.add( 'specialchar', function( editor ) { '' + charDesc + '' ); - } else { - html.push( ' ' ); } html.push( '' ); From a11692fdc28f1995cc7c3d90fde5e3832f913c8a Mon Sep 17 00:00:00 2001 From: Mateusz Samsel Date: Thu, 10 Oct 2019 12:48:44 +0200 Subject: [PATCH 04/10] Remove multiple var statements and hoist variable declaration to the beginning of scope, to prevent linter errors. --- plugins/specialchar/dialogs/specialchar.js | 107 +++++++++++---------- 1 file changed, 56 insertions(+), 51 deletions(-) diff --git a/plugins/specialchar/dialogs/specialchar.js b/plugins/specialchar/dialogs/specialchar.js index a982d7c28eb..ba5a2aea124 100644 --- a/plugins/specialchar/dialogs/specialchar.js +++ b/plugins/specialchar/dialogs/specialchar.js @@ -7,69 +7,74 @@ CKEDITOR.dialog.add( 'specialchar', function( editor ) { // Simulate "this" of a dialog for non-dialog events. // @type {CKEDITOR.dialog} var dialog, - lang = editor.lang.specialchar; - - var onChoice = function( evt ) { - var target, value; - if ( evt.data ) - target = evt.data.getTarget(); - else - target = new CKEDITOR.dom.element( evt ); - - if ( target.getName() == 'a' && ( value = target.getChild( 0 ).getHtml() ) ) { - target.removeClass( 'cke_light_background' ); - dialog.hide(); - - // We must use "insertText" here to keep text styled. - var span = editor.document.createElement( 'span' ); - span.setHtml( value ); - editor.insertText( span.getText() ); - } - }; - - var onClick = CKEDITOR.tools.addFunction( onChoice ); + lang = editor.lang.specialchar, + focusedNode, + onChoice, + onClick, + onBlur, + onFocus, + onKeydown; + + + onChoice = function( evt ) { + var target, value; + if ( evt.data ) + target = evt.data.getTarget(); + else + target = new CKEDITOR.dom.element( evt ); + + if ( target.getName() == 'a' && ( value = target.getChild( 0 ).getHtml() ) ) { + target.removeClass( 'cke_light_background' ); + dialog.hide(); + + // We must use "insertText" here to keep text styled. + var span = editor.document.createElement( 'span' ); + span.setHtml( value ); + editor.insertText( span.getText() ); + } + }; - var focusedNode; + onClick = CKEDITOR.tools.addFunction( onChoice ); - var onFocus = function( evt, target ) { - var value; - target = target || evt.data.getTarget(); + onFocus = function( evt, target ) { + var value; + target = target || evt.data.getTarget(); - if ( target.getName() == 'span' ) - target = target.getParent(); + if ( target.getName() == 'span' ) + target = target.getParent(); - if ( target.getName() == 'a' && ( value = target.getChild( 0 ).getHtml() ) ) { - // Trigger blur manually if there is focused node. - if ( focusedNode ) - onBlur( null, focusedNode ); + if ( target.getName() == 'a' && ( value = target.getChild( 0 ).getHtml() ) ) { + // Trigger blur manually if there is focused node. + if ( focusedNode ) + onBlur( null, focusedNode ); - var htmlPreview = dialog.getContentElement( 'info', 'htmlPreview' ).getElement(); + var htmlPreview = dialog.getContentElement( 'info', 'htmlPreview' ).getElement(); - dialog.getContentElement( 'info', 'charPreview' ).getElement().setHtml( value ); - htmlPreview.setHtml( CKEDITOR.tools.htmlEncode( value ) ); - target.getParent().addClass( 'cke_light_background' ); + dialog.getContentElement( 'info', 'charPreview' ).getElement().setHtml( value ); + htmlPreview.setHtml( CKEDITOR.tools.htmlEncode( value ) ); + target.getParent().addClass( 'cke_light_background' ); - // Memorize focused node. - focusedNode = target; - } - }; + // Memorize focused node. + focusedNode = target; + } + }; - var onBlur = function( evt, target ) { - target = target || evt.data.getTarget(); + onBlur = function( evt, target ) { + target = target || evt.data.getTarget(); - if ( target.getName() == 'span' ) - target = target.getParent(); + if ( target.getName() == 'span' ) + target = target.getParent(); - if ( target.getName() == 'a' ) { - dialog.getContentElement( 'info', 'charPreview' ).getElement().setHtml( ' ' ); - dialog.getContentElement( 'info', 'htmlPreview' ).getElement().setHtml( ' ' ); - target.getParent().removeClass( 'cke_light_background' ); + if ( target.getName() == 'a' ) { + dialog.getContentElement( 'info', 'charPreview' ).getElement().setHtml( ' ' ); + dialog.getContentElement( 'info', 'htmlPreview' ).getElement().setHtml( ' ' ); + target.getParent().removeClass( 'cke_light_background' ); - focusedNode = undefined; - } - }; + focusedNode = undefined; + } + }; - var onKeydown = CKEDITOR.tools.addFunction( function( ev ) { + onKeydown = CKEDITOR.tools.addFunction( function( ev ) { ev = new CKEDITOR.dom.event( ev ); // Get an Anchor element. From 67b19f013f1368414fa51301e8223c8d7961708b Mon Sep 17 00:00:00 2001 From: Mateusz Samsel Date: Thu, 10 Oct 2019 13:05:12 +0200 Subject: [PATCH 05/10] Adapt unit test to check if there is no empty cells in specialchar dialog. --- tests/plugins/specialchar/dialog.js | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/plugins/specialchar/dialog.js b/tests/plugins/specialchar/dialog.js index 7ae0e6dffd8..dd98964e081 100644 --- a/tests/plugins/specialchar/dialog.js +++ b/tests/plugins/specialchar/dialog.js @@ -26,6 +26,7 @@ for ( i = 0; i < tableCells.length; i++ ) { assert.areSame( 'presentation', tableCells[ i ].getAttribute( 'role' ), 'Table cell with index: ' + i + ' should have role="presentation". Instead it has following html: ' + tableCells[ i ].getOuterHtml() ); + assert.areNotSame( ' ', tableCells[ i ].getHtml(), 'Table cell with index: ' + i + ' should not be empty.' ); } } ); } From 937a13695cea00d5465d5ed6e6c49b593c443483 Mon Sep 17 00:00:00 2001 From: Mateusz Samsel Date: Thu, 10 Oct 2019 13:05:30 +0200 Subject: [PATCH 06/10] Add manual test. --- .../specialchar/manual/presentationrole.html | 13 +++++++++++++ .../plugins/specialchar/manual/presentationrole.md | 12 ++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 tests/plugins/specialchar/manual/presentationrole.html create mode 100644 tests/plugins/specialchar/manual/presentationrole.md diff --git a/tests/plugins/specialchar/manual/presentationrole.html b/tests/plugins/specialchar/manual/presentationrole.html new file mode 100644 index 00000000000..42e0e15b3e9 --- /dev/null +++ b/tests/plugins/specialchar/manual/presentationrole.html @@ -0,0 +1,13 @@ + +

Classic editor

+
+

I'm CKEditor 4 instance.

+
+ + diff --git a/tests/plugins/specialchar/manual/presentationrole.md b/tests/plugins/specialchar/manual/presentationrole.md new file mode 100644 index 00000000000..8bf20becfb7 --- /dev/null +++ b/tests/plugins/specialchar/manual/presentationrole.md @@ -0,0 +1,12 @@ +@bender-ui: collapsed +@bender-tags: 3544, 4.13.1, bug +@bender-ckeditor-plugins: wysiwygarea, toolbar, specialchar + +1. Open specialchar dialog + +### Expected result: +Empty table cells at the end of character list should have role="presentation". You can inspect element to check it, however, +there are also a css rules which marks `` elements with `role="presentation"` with red dashed border. + +### Unexpected result +Empty table cells at the end of the list don't have presentation role. From ff05e365d7d50412a91272e94fcd6442407c6f68 Mon Sep 17 00:00:00 2001 From: Mateusz Samsel Date: Thu, 10 Oct 2019 13:08:25 +0200 Subject: [PATCH 07/10] Improve template in special char dialog. --- plugins/specialchar/dialogs/specialchar.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/specialchar/dialogs/specialchar.js b/plugins/specialchar/dialogs/specialchar.js index ba5a2aea124..39a2f44c957 100644 --- a/plugins/specialchar/dialogs/specialchar.js +++ b/plugins/specialchar/dialogs/specialchar.js @@ -219,10 +219,10 @@ CKEDITOR.dialog.add( 'specialchar', function( editor ) { '' + '' + charDesc + - '' ); + '' + ); } - html.push( '' ); } html.push( '' ); } From b0c63237a05672405949de5990f2e0657eb7111b Mon Sep 17 00:00:00 2001 From: Mateusz Samsel Date: Thu, 10 Oct 2019 13:58:00 +0200 Subject: [PATCH 08/10] Improve manual test to better reflect real problem. --- .../specialchar/manual/presentationrole.html | 8 +++----- .../specialchar/manual/presentationrole.md | 16 ++++++++++++---- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/tests/plugins/specialchar/manual/presentationrole.html b/tests/plugins/specialchar/manual/presentationrole.html index 42e0e15b3e9..1dc9401d0c5 100644 --- a/tests/plugins/specialchar/manual/presentationrole.html +++ b/tests/plugins/specialchar/manual/presentationrole.html @@ -1,13 +1,11 @@ -

Classic editor

I'm CKEditor 4 instance.

diff --git a/tests/plugins/specialchar/manual/presentationrole.md b/tests/plugins/specialchar/manual/presentationrole.md index 8bf20becfb7..cbe3450bb3b 100644 --- a/tests/plugins/specialchar/manual/presentationrole.md +++ b/tests/plugins/specialchar/manual/presentationrole.md @@ -2,11 +2,19 @@ @bender-tags: 3544, 4.13.1, bug @bender-ckeditor-plugins: wysiwygarea, toolbar, specialchar -1. Open specialchar dialog +1. Open specialchar dialog. +2. Turn on Voice Over on MacOS (`CMD + F5`) or Narrator on Windows (`WIN + CTRL + Enter`). +3. Move selection to the last character. +4. For MacOS: press `CapsLock + Right Arrow`. +5. For Windows: press `Right Arrow`. + +In case that you have problem with performing steps with assistive technology: +1. Open console. +2. Inspect if there exists empty table cells after last character in the dialog. ### Expected result: -Empty table cells at the end of character list should have role="presentation". You can inspect element to check it, however, -there are also a css rules which marks `` elements with `role="presentation"` with red dashed border. +Assisitve technology doesn't detect and read content of empty table cells at the end of special char dialog. +There shouldn't be any empty cells at the end. ### Unexpected result -Empty table cells at the end of the list don't have presentation role. +Empty table cells are added at the end of characters list. From 87031d9bb7a46ed159792a878d41ad622568b563 Mon Sep 17 00:00:00 2001 From: Mateusz Samsel Date: Thu, 10 Oct 2019 14:08:23 +0200 Subject: [PATCH 09/10] Correct unit test name --- tests/plugins/specialchar/dialog.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/plugins/specialchar/dialog.js b/tests/plugins/specialchar/dialog.js index dd98964e081..220fdaae783 100644 --- a/tests/plugins/specialchar/dialog.js +++ b/tests/plugins/specialchar/dialog.js @@ -15,7 +15,7 @@ } }, - 'test specialchardialog should have table cells with role="presentation"': function() { + 'test specialchar dialog should have table cells with role="presentation" and don\'t have empty table cells': function() { var bot = this.editorBot; bot.dialog( 'specialchar', function( dialog ) { From c74ad40fddb388bfb3495b45122c9bb075588c06 Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Mon, 4 Nov 2019 18:05:35 +0100 Subject: [PATCH 10/10] Add changelog entry. [ci skip] --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index d5595e69b67..0025ba78d5b 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,6 +9,7 @@ Fixed Issues: * [#3413](https://github.com/ckeditor/ckeditor4/issues/3413): Fixed: Menu items with labels containing double quotes are rendered incorrectly. * [#3475](https://github.com/ckeditor/ckeditor4/issues/3475): [Firefox] Fixed: Pasting plain text over existing content fails and throws an error. * [#2027](https://github.com/ckeditor/ckeditor4/issues/2027): Fixed: Incorrect email display text after reopening [Link](https://ckeditor.com/cke4/addon/link) dialog for display names starting with `@`. +* [#3544](https://github.com/ckeditor/ckeditor4/issues/3544): Fixed: [Special Characters](https://ckeditor.com/cke4/addon/specialchar) dialog read incorrectly by screen readers due to empty table cells at the end. ## CKEditor 4.13