List all available hotkeys in the Accessibility Instructions dialog #569

msamsel · 2017-06-27T10:39:17Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

yes

This PR contains

Unit tests
Manual tests

What changes did you make?

Automatically created list with available keystroke in editor. List is used in Accessibility help plugin
Move description from accessibility plugin to proper plugin which register given keystroke.
Add new editor.event keystrokeEntry which allows on modification displayed keystrokes
Add option to Command Definition to provide label and description property which later on will be used by accessibility plugin to present it in a table.
Move method representKeystroke to be available in editor.

close #456

AnnaTomanek · 2017-06-27T10:54:38Z

Please make sure that you remember about the following things:

No translations that are already present can get lost if we intend to use them again (also applies to translations available in other plugins - e.g. if "Paste as plain text" is translated in the plugin, we should copy it). This applies to removing language strings from one plugin, but then adding them back in another.
Reuse existing language strings and translations; also if the same language string is used in different places, move it to the core language file.

msamsel · 2017-06-27T11:15:07Z

@AnnaTomanek Generally entire command section, which was predefined in Accessibility Helper plugin now is removed and replaced by automatically generated table, where Commands label has proper keystroke.
Change introduce possibility, that plugin creator can define inside his plugin some description, which will be automatically used by Accessibility Help plugin.
Earlier such change required modification of a11yHelp plugin.

I'm not sure what is proper way in such case. Should those static translations remain in accessibility language files, even though are not used any longer?

f1ames · 2017-06-30T06:32:12Z

This case is a little tricky, as @AnnaTomanek said we would like to make as little changes as possible in language files (especially when translations are already there) especially when just moving language keys/strings around.

However, as @msamsel pointed out the previous solution basically had predefined values which were used in Commands/Shortcuts section in a11yhelp dialog. In order to display new shortcuts, the a11yhelp plugin needed to be modified. In a proposed solution, shortcut list is generated dynamically based only on information contained in each plugin.
This solution is more flexible and gives an ability (also for community plugins) to register an information about command (label and optional description) which then can be reused to list all commands shortcuts with proper naming and description (e.g. in a help dialog).
It seems like this improvement in flexibility is worth the cost. WDYT @mlewand @AnnaTomanek?

@mlewand I think if we decide to go with such solution it will be good to extract it to a new issue (which will be a new feature introducing possibility for each command to contain/register additional information like command label and description). WDYT?

msamsel · 2017-07-03T13:48:22Z

Generally there wasn't much text to remain. Most of it had to be removed. Commands are now in a table which downloads label for them automatically. It takes values from command label or if there is no command label, then from command ui button label. So most of them should have translations already.

mlewand

While checking lang relations I caught few minor things, marked as a comments.

mlewand · 2017-07-04T13:57:40Z

core/commanddefinition.js

@@ -160,3 +160,17 @@
 * @since 4.6.0
 * @property {Number} fakeKeystroke
 */
+
+/**
+ * Defines optional command label. Property is automatically used by accessibility helper to display it in Command table.


There's no need to say that some property is optional in the description. This should be marked by wrapping prop name within brackets.

mlewand · 2017-07-04T14:01:32Z

plugins/a11yhelp/plugin.js

+	 * @class
+	 * @singleton
+	 */
+	CKEDITOR.plugins.a11yhelp = {


This namespace itself should be marked with @since tag as it wasn't present before.

mlewand · 2017-07-04T14:06:52Z

plugins/magicline/lang/af.js

@@ -4,5 +4,13 @@
 */

 CKEDITOR.plugins.setLang( 'magicline', 'af', {
-	title: 'Voeg paragraaf hier in'
+	title: 'Voeg paragraaf hier in',
+	commandsLabels: {


It would be better other way around, say:

commands: { previousSpace: { label: 'foo', description: 'bar' }, nextSpace: { label: 'foo', description: 'bar' } }

mlewand · 2017-07-05T08:35:25Z

Sure, we need to retain translations whenever possible. Unfortunately in this case it won't be possible, as new translations will be different than the former one - new labels are much shorter/atomic.

Old approach was bad, having redundant text and a knowledge about external plugins in a11yhelp plugin.

Luckily it's set for a major release so there's plenty of time for our translation Contributors to pick it up.

msamsel · 2017-07-05T13:00:46Z

I correct problems mention by @mlewand, add milestone 4.8.0 and change base branch to major.

f1ames

There are few things to polish:

Update language meta files.
Magicline plugin diff looks like whole plugin had changed, did you rearrange the code for some reason or did some bigger changes there?

The one thing I'm not sure about is keystrokeEntry event. It looks a little redundant to me. It is used to give the possibility to alter command label/description. But this
is very specialized event and basically will be used only for this particular case only by Accessibility Help dialog (theoretically, it can be used by similar
plugins too).

Wouldn't it be better to mark commandDefiniton.label, commandDefiniton.description as private properties and then introduce default
getters which can be overridden to change default label/description value?

This also will be simpler to use by other plugins as you don't have to remember to fire an event to get proper command label/description, you just use a getter.

f1ames · 2017-07-26T08:28:59Z

tests/plugins/a11yhelp/manual/commandstable.md

@@ -0,0 +1,15 @@
+@bender-tags: manual, 456, tc, 4.8.0, feature


Please drop tc tag.

f1ames · 2017-07-26T08:34:56Z

tests/plugins/a11yhelp/manual/commandstable.md

+@bender-ui: collapsed
+@bender-ckeditor-plugins: wysiwygarea,a11yhelp
+
+1. Focus editor


Remember to always end sentence with . in lists (or , if it is a verbless sentence - well I wasn't 100% sure myself so here is some explanation) :)

f1ames · 2017-07-26T08:39:14Z

tests/plugins/a11yhelp/manual/commandstable.md

+
+1. Focus editor
+1. Press `Alt + 0`, there should be section **Commands table**
+1. Table should contain at least one keystroke `Alt + 0`


Remember that point which is not an action/step, but rather a result of previous step is an expected behaviour. It can be a subitem like:

Press Alt + 0

There should be section Commands table which contains at least one keystroke Alt + 0.

or

Press Alt + 0

Expected: There should be section Commands table which contains at least one keystroke Alt + 0.

If scenario is short enough, expected results could be combined and placed after it (not in this case IMHO).

f1ames · 2017-07-26T08:40:07Z

tests/plugins/a11yhelp/manual/commandstable.html

+
+	function addCommands() {
+		editor.destroy();
+		editor =CKEDITOR.replace( 'editor1' );


editor =CKEDITOR.replace( 'editor1' ); missing space after =.

f1ames · 2017-07-26T08:47:47Z

core/commanddefinition.js

@@ -160,3 +160,17 @@
 * @since 4.6.0
 * @property {Number} fakeKeystroke
 */
+
+/**
+ * Defines command label. Property is automatically used by accessibility helper to display it in Command table.


I would make this description more generic like:

Defines command label. This property is used by different plugins to display command information (by e.g. Accessibility Help).

Same for description property above.

msamsel · 2017-07-27T13:44:11Z

There was a lot of changes.
I've remake entire solution to use getLabel and getKeyDescription instead of event handling. What also affect on pastetext plugin which requires those modifications.

About magicline, linters report many issues. Most of them was declaring something after usage. Changes are rearranging code to have most of variables declaration at the beginning, also some functions was invoked inside another which also had to be rearranged.

As far as I remember I've made only significant changes here (https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/magicline/plugin.js#L892-L894)
where I add label and keyDescription to code.

f1ames · 2017-07-28T09:08:17Z

plugins/pastetext/plugin.js

@@ -80,6 +90,13 @@
 			editor.on( 'pasteState', function( evt ) {
 				editor.getCommand( commandName ).setState( evt.data );
 			} );
+
+			editor.on( 'keystrokeEntry', function( evt ) {


Please remove all leftovers of keystrokeEntry event (some left also in tests).

f1ames · 2017-07-28T10:12:07Z

core/command.js

@@ -116,6 +116,42 @@ CKEDITOR.command = function( editor, commandDefinition ) {
 		return allowed = editor.activeFilter.checkFeature( this );
 	};

+	/**
+	 * Providing this method will overwrite {@link CKEDITOR.commandDefinition#getLabel}, what allows on better customization


I think the description might be a little bit confusing/misleading.

The main function of this method is just returning the label. So the description should say that it returns command label first.

Providing this method will overwrite {@link CKEDITOR.commandDefinition#getLabel}, what allows on better customization

As for providing this method - this method is already provided, the one which can be provided additionally is CKEDITOR.commandDefinition#getLabel (and this is CKEDITOR.command#getLabel) so this part should go to commandDefintion specs. And the part from commandDefintion about fallback should land here IMHO.

Same for getKeyDescription.

I rearrange it, I hope it will be ok ;D

f1ames · 2017-07-28T10:13:12Z

core/commanddefinition.js

+ * Defines command label. This property is used by different plugins to display command information (by e.g. Accessibility Help).
+ * For geting command's label please use {@link CKEDITOR.commandDefinition#getLabel}, instead of directly reading `label` property.
+ *
+ * @since 4.8.0


Please mark as @private. Same for keyDescription.

I have doubts about it. If those will be marked as @private, then some kind of 'setter' might be required. Users might not see those variables, or be afraid of modifying them. What do you think about that?

Makes sense 👍

f1ames · 2017-07-28T10:14:23Z

core/commanddefinition.js

+
+/**
+ * Defines command label. This property is used by different plugins to display command information (by e.g. Accessibility Help).
+ * For geting command's label please use {@link CKEDITOR.commandDefinition#getLabel}, instead of directly reading `label` property.


For geting command's label please use {@link CKEDITOR.commandDefinition#getLabel}

Here you should link to CKEDITOR.command#getLabel.

f1ames · 2017-07-28T10:16:35Z

dev/langtool/meta/ckeditor.plugin-a11yhelp-dialogs/meta.txt

@@ -100,3 +100,6 @@ openBracket = The label for the "Open Bracket" ([) key.
 backSlash = The label for the "Backslash" (\) key.
 closeBracket = The label for the "Close Bracket" (]) key.
 singleQuote = The label for the "Single Quote" (') key.
+commandsList.sectionName = Label for section, which displays table with command's keystrokes.
+commandsList.command = Label for headr of table column, which provides commands labels (names).


Label for table column header, which contains command labels (names).

?

f1ames · 2017-07-28T10:43:11Z

dev/langtool/meta/ckeditor.plugin-a11yhelp-dialogs/meta.txt

@@ -100,3 +100,6 @@ openBracket = The label for the "Open Bracket" ([) key.
 backSlash = The label for the "Backslash" (\) key.
 closeBracket = The label for the "Close Bracket" (]) key.
 singleQuote = The label for the "Single Quote" (') key.
+commandsList.sectionName = Label for section, which displays table with command's keystrokes.
+commandsList.command = Label for headr of table column, which provides commands labels (names).
+commandsList.keystroke = Label for header of table column, which provides keystroke for executing specific commands.


Label for table column header, which contains keystrokes for executing specific commands.

?

f1ames · 2017-07-28T10:47:15Z

plugins/a11yhelp/dialogs/a11yhelp.js

+		var commandsSectionTpl = '<h1>%1</h1><table>%2</table>',
+			commandRowTpl = '<tr><td>%1</td><td>%2</td></tr>',
+			commandsTbodyTpl = '<tbody>%1</tbody>',
+			commandsTbodyHtml = '',


Which means this variable value is always overwrite somewhere else so it doesn't have to be initialized with any value. Same for commandsTheadHtml.

f1ames · 2017-07-28T11:03:42Z

tests/core/command/command.js

+	},
+
+	// #456
+	'test basic behavor of getLabel': function() {


behavior (missing i)

f1ames · 2017-08-01T09:46:12Z

tests/plugins/a11yhelp/a11yhelp.js

+			} );
+		},
+
+		'test keystrokeEntry event': function() {


There are still some leftovers of keystrokeEntry event in this file and also in tests/plugins/a11yhelp/manual/commandstable manual test (which fails now).

f1ames · 2017-08-01T09:50:45Z

core/command.js

+		} else if ( commandDefinition.label ) {
+			return commandDefinition.label;
+		} else if ( this.uiItems && this.uiItems.length && this.uiItems[ 0 ].label ) {
+			return this.uiItems[ 0 ].label;


What was the reason for this fallback, was it used anywhere before?

This kind of fallback allows on reusing already created labels for buttons without modifying all other plugins.
So steps are to use getLabel method if exist, then label property, and then button's label if exist. So we use translations for button's label such as bold, italic, etc, without waiting for them being translate and just use what we have already.

f1ames · 2017-08-01T09:57:27Z

core/commanddefinition.js

- * 2. If {@link CKEDITOR.commandDefinition#label} atribute exists, it is used to provide label.
- * 3. If {@link CKEDITOR.command#uiItems} is non empty, then label of first array's item is used as label
- * 4. Empty string.
+ * Providing this method will overwrite {@link CKEDITOR.command#getLabel}, what allows on better customization


My previous comment might be misleading a little, sorry for that. You moved the description to the right place as I suggested, but still:

Providing this method will overwrite {@link CKEDITOR.command#getLabel}

Providing this method will not overwrite CKEDITOR.command#getLabel. The provided method will be just used by CKEDITOR.command#getLabel to return a label. I suggest to update the description to something like:

If present, this method will be used to provide command's label. It allows customizing label for a specific command.

Same for getKeyDescription description.

f1ames · 2017-08-01T10:00:42Z

core/commanddefinition.js

+ * Defines command label. This property is used by different plugins to display command information (by e.g. Accessibility Help).
+ * For geting command's label please use {@link CKEDITOR.commandDefinition#getLabel}, instead of directly reading `label` property.
+ *
+ * @since 4.8.0


Makes sense 👍

f1ames

It looks good. Just few small things:

I'm thinking that keyDescription should be named keystrokeDescription WDYT?
Please remove legendEdge language entry as it is not used anymore (if it was not added to meta lang files you just need to regenerate lang files).

I also checked how it works with JAWS and it works the same way as before (except the fact that JAWS announces table content, but it works well). So there are no regressions in this regard.

f1ames · 2017-08-02T08:51:58Z

plugins/a11yhelp/plugin.js

+
+				editor.addCommand( commandName, {
+					exec: function() {
+						CKEDITOR.document.appendStyleSheet( CKEDITOR.getUrl( plugin.path + 'styles/a11yhelp.css' ) );


This stylesheet is appended every time dialog is opened. It should be appended only once. It should be placed in onLoad plugin method.

f1ames · 2017-08-02T09:00:53Z

plugins/a11yhelp/plugin.js

+				coreLang = editor.lang.common.keyboard;
+
+			// CharCode <-> KeyChar.
+			var keyMap = {


Maybe this map could be defined as CKEDITOR.plugins.a11yhelp property? Now, every time the method is called, the map is defined again.

f1ames · 2017-08-02T09:02:09Z

plugins/a11yhelp/styles/a11yhelp.css

+	padding: 0.8em;
+}
+
+.cke_accessibility_legend tbody > tr:nth-child(odd) {


This is not supported in IE8 😞 Maybe we could use .odd class when generating the commands table?

f1ames

Almost there :)

f1ames · 2017-08-03T08:33:38Z

dev/langtool/meta/ckeditor.core/meta.txt

@@ -69,7 +69,7 @@ common.invalidCssLength = Error message displayed to the user when the specified
 common.invalidHtmlLength = Error message displayed to the user when the specified length value is not a valid HTML measurement unit.
 common.invalidInlineStyle = Error message displayed to the user when the specified style value is not a valid CSS declaration.
 common.cssLengthTooltip = Tooltip displayed for the Width and Height fields in the Table Properties dialog window explaining the correct format of these length values.
-common.unavailable =
+common.unavailable = 


Not sure why this lang entry get here?

Me neither. ;) I just use dev/langtool/langtool.sh and it was somehow affected by this.

BTW it was only added space at the end of lien if I see correct comparing to repository: https://github.com/ckeditor/ckeditor-dev/blob/master/dev/langtool/meta/ckeditor.core/meta.txt#L72

So I'm not sure what should with that :) any ideas?

Ok, we decided to left it as it is.

f1ames · 2017-08-03T08:42:37Z

plugins/a11yhelp/plugin.js

 		 */
-		representKeystroke: function( editor, keystroke ) {
+		_renderKeyMap: function( editor ) {


I think we can simplify creating keyMap even more, just creating singleton like method:

_renderKeyMap: ( function() { var keyMapCache; return function( editor ) { if ( !keyMapCache ) { // Here goes the body of the current function. Created keyMap should be assigned to keyMapCache variable. } return keyMapCache; } } )();

And then we can completely drop _keyMap property and just use var keyMap = this._renderKeyMap( editor ); in other places.

Also renderKeyMap is somehow misleading name, this functions does not render anything it creates/generates key map object so it can be e.g. [create/generate/get]keyMap.

msamsel · 2017-08-04T08:17:14Z

Ok this hidden comment is corrected also ;)

f1ames

LGTM! Very solid piece of work 👍

f1ames · 2017-08-04T09:40:47Z

@mlewand, please check if your previous change requests were fulfilled or if you have any more comments.

…l parameter.

… dispaly keystrokes list.

… to be more accurate.

mlewand · 2017-09-01T12:43:46Z

Pushed one commit with some code style adjustments.

mlewand · 2017-09-01T13:19:41Z

This feature should utilize features already available in tools, there's a keystrokeToString method that is perfect candidate.

Currently representKeystroke and _createKeyMap combo duplicates it.

However we can't use keystrokeToString at it's current shape, as it returns a string like "CTRL + X". While it might be tempting to just split it using "+" char, this is not the right thing to do. Instead we should have an underlying method that parsers keycode, and return array of keys in a keystroke, so that it can be joined for presentation.

I have created an issue #856 for that - issue #856 will block this pull request.

AnnaTomanek changed the title ~~List all avaialble hotkeys in Accessibility dialog~~ List all available hotkeys in the Accessibility Instructions dialog Jun 27, 2017

msamsel force-pushed the t/456 branch 2 times, most recently from 3e1b561 to a446035 Compare June 27, 2017 13:59

f1ames self-requested a review June 28, 2017 10:49

mlewand self-assigned this Jun 30, 2017

mlewand reviewed Jul 5, 2017

View reviewed changes

mlewand removed their assignment Jul 5, 2017

msamsel force-pushed the t/456 branch from a446035 to f9fe252 Compare July 5, 2017 12:20

msamsel changed the base branch from master to major July 5, 2017 12:55

msamsel force-pushed the t/456 branch from f9fe252 to a6f773f Compare July 5, 2017 12:57

msamsel added this to the 4.8.0 milestone Jul 5, 2017

f1ames suggested changes Jul 26, 2017

View reviewed changes

msamsel force-pushed the t/456 branch from a6f773f to 85d50e2 Compare July 27, 2017 13:31

f1ames suggested changes Jul 28, 2017

View reviewed changes

f1ames suggested changes Aug 1, 2017

View reviewed changes

f1ames suggested changes Aug 2, 2017

View reviewed changes

msamsel force-pushed the t/456 branch from 33cb0f4 to 1008ba9 Compare August 2, 2017 15:07

f1ames suggested changes Aug 3, 2017

View reviewed changes

f1ames self-requested a review August 4, 2017 09:38

f1ames approved these changes Aug 4, 2017

View reviewed changes

f1ames requested a review from mlewand August 4, 2017 09:40

Mateusz Samsel and others added 19 commits August 31, 2017 15:42

Fixes without languages

b6b5ee5

Fixing language stuff

70ade35

Rewording.

6bbadcc

Remake createKeyMap method

f03a394

Rewording.

1cda5e0

Update lang files.

30b3a38

Update lang files for magicline

9dd64d3

Fixes after review.

dfae905

Adding nice css and extend represent keystroke method wit new optiona…

44c3107

…l parameter.

Lang changes for new keystrok tab name.

f4db845

Redesign keystroke list. Move table to separate tab. Add keystroke to…

02f9667

… dispaly keystrokes list.

Another lang file update. Remake headers on tabs and rename variables…

aed7853

… to be more accurate.

Some bigger changes which use renamed variables from lang files.

c0bb990

Minor review fixes.

eaed3c1

Updating docs.

08ed660

Hot fixes for unit test.

b1ee9e1

Added a manual test for language bug.

1253e0e

Added unit test for multilang support.

c0cb660

Review lang changes.

c58b489

mlewand mentioned this pull request Aug 31, 2017

Simplify find/replace dialog using tabId #850

Closed

Review fixes

cffe785

msamsel force-pushed the t/456 branch from 45045c9 to cffe785 Compare September 1, 2017 09:49

Mateusz Samsel and others added 3 commits September 1, 2017 11:57

Lang meta.txt update.

e25b02b

Minor corrections to a11yhelp.representKeystroke API docs.

57fbb5b

Minor code refactroing in a11yhelp plugin.

8f82f0a

mlewand removed this from the 4.8.0 milestone Nov 22, 2017

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

f1ames closed this Jan 16, 2020

		@@ -0,0 +1,15 @@
		@bender-tags: manual, 456, tc, 4.8.0, feature

List all available hotkeys in the Accessibility Instructions dialog #569

List all available hotkeys in the Accessibility Instructions dialog #569

Conversation

msamsel commented Jun 27, 2017 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

AnnaTomanek commented Jun 27, 2017 • edited Loading

msamsel commented Jun 27, 2017 • edited Loading

f1ames commented Jun 30, 2017

msamsel commented Jul 3, 2017

mlewand 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

mlewand commented Jul 5, 2017

msamsel commented Jul 5, 2017

f1ames 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

msamsel commented Jul 27, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames 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

f1ames 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

msamsel commented Aug 4, 2017

f1ames left a comment

Choose a reason for hiding this comment

f1ames commented Aug 4, 2017 • edited Loading

mlewand commented Sep 1, 2017

mlewand commented Sep 1, 2017

msamsel commented Jun 27, 2017 •

edited

Loading

AnnaTomanek commented Jun 27, 2017 •

edited

Loading

msamsel commented Jun 27, 2017 •

edited

Loading

f1ames commented Aug 4, 2017 •

edited

Loading