fix(toolbox): shortcuts appearance fixed, icons became optional (#2188)

* fix(toolbox): shortcuts appearing fixed, icons became optional

* rm only from test

* Update tools.spec.ts

* rm useless tests

* Update CHANGELOG.md

docs/CHANGELOG.md

@@ -14,8 +14,10 @@
 - `Fix` — If a Tool specifies some tags to substitute on paste, all attributes of that tags will be removed before passing them to the tool. Possible XSS vulnerability fixed.
 - `Fix` — Pasting from Microsoft Word to Chrome (Mac OS) fixed. Now if there are no image-tools connected, regular text content will be pasted.
 - `Fix` — Workaround for the HTMLJanitor bug with Tables (https://github.com/guardian/html-janitor/issues/3) added
+- `Fix` — Toolbox shortcuts appearance and execution fixed [#2112](https://github.com/codex-team/editor.js/issues/2112)
 - `Improvement` — *Tools API* — `pasteConfig().tags` now support sanitizing configuration. It allows you to leave some explicitly specified attributes for pasted content.
 - `Improvement` — *CodeStyle* — [CodeX ESLint Config](https://github.com/codex-team/eslint-config) has bee updated. All ESLint/Spelling issues resolved
+- `Improvement` — *ToolsAPI* — The `icon` property of the `toolbox` getter became optional.
 
 ### 2.25.0
 
@@ -25,7 +27,7 @@ Due to that API changes: tool's `toolbox` getter now can return either a single
 
 ### 2.24.4
 
-- `Fix` — Keyboard selection by word [2045](https://github.com/codex-team/editor.js/issues/2045)
+- `Fix` — Keyboard selection by word [#2045](https://github.com/codex-team/editor.js/issues/2045)
 
 ### 2.24.3
 

src/components/ui/toolbox.ts

@@ -70,7 +70,7 @@ export default class Toolbox extends EventsDispatcher<ToolboxEvent> {
   /**
    * Popover instance. There is a util for vertical lists.
    */
-  private popover: Popover;
+  private popover: Popover | undefined;
 
   /**
    * List of Tools available. Some of them will be shown in the Toolbox
@@ -86,7 +86,7 @@ export default class Toolbox extends EventsDispatcher<ToolboxEvent> {
    * Current module HTML Elements
    */
   private nodes: {
-    toolbox: HTMLElement;
+    toolbox: HTMLElement | null;
   } = {
       toolbox: null,
     };
@@ -102,11 +102,6 @@ export default class Toolbox extends EventsDispatcher<ToolboxEvent> {
     };
   }
 
-  /**
-   * Id of listener added used to remove it on destroy()
-   */
-  private clickListenerId: string = null;
-
   /**
    * Toolbox constructor
    *
@@ -150,8 +145,8 @@ export default class Toolbox extends EventsDispatcher<ToolboxEvent> {
   /**
    * Returns true if the Toolbox has the Flipper activated and the Flipper has selected button
    */
-  public hasFocus(): boolean {
+  public hasFocus(): boolean | undefined {
-    return this.popover.hasFocus();
+    return this.popover?.hasFocus();
   }
 
   /**
@@ -165,10 +160,8 @@ export default class Toolbox extends EventsDispatcher<ToolboxEvent> {
       this.nodes.toolbox = null;
     }
 
-    this.api.listeners.offById(this.clickListenerId);
-
     this.removeAllShortcuts();
-    this.popover.off(PopoverEvent.OverlayClicked, this.onOverlayClicked);
+    this.popover?.off(PopoverEvent.OverlayClicked, this.onOverlayClicked);
   }
 
   /**
@@ -189,7 +182,7 @@ export default class Toolbox extends EventsDispatcher<ToolboxEvent> {
       return;
     }
 
-    this.popover.show();
+    this.popover?.show();
     this.opened = true;
     this.emit(ToolboxEvent.Opened);
   }
@@ -198,7 +191,7 @@ export default class Toolbox extends EventsDispatcher<ToolboxEvent> {
    * Close Toolbox
    */
   public close(): void {
-    this.popover.hide();
+    this.popover?.hide();
     this.opened = false;
     this.emit(ToolboxEvent.Closed);
   }
@@ -226,24 +219,17 @@ export default class Toolbox extends EventsDispatcher<ToolboxEvent> {
    */
   @_.cacheable
   private get toolsToBeDisplayed(): BlockTool[] {
-    return Array
+    const result: BlockTool[] = [];
-      .from(this.tools.values())
-      .reduce((result, tool) => {
-        const toolToolboxSettings = tool.toolbox;
 
-        if (toolToolboxSettings) {
+    this.tools.forEach((tool) => {
-          const validToolboxSettings = toolToolboxSettings.filter(item => {
+      const toolToolboxSettings = tool.toolbox;
-            return this.areToolboxSettingsValid(item, tool.name);
-          });
 
-          result.push({
+      if (toolToolboxSettings) {
-            ...tool,
+        result.push(tool);
-            toolbox: validToolboxSettings,
+      }
-          });
+    });
-        }
 
-        return result;
+    return result;
-      }, []);
   }
 
   /**
@@ -267,12 +253,12 @@ export default class Toolbox extends EventsDispatcher<ToolboxEvent> {
     };
 
     return this.toolsToBeDisplayed
-      .reduce((result, tool) => {
+      .reduce<PopoverItem[]>((result, tool) => {
         if (Array.isArray(tool.toolbox)) {
           tool.toolbox.forEach(item => {
             result.push(toPopoverItem(item, tool));
           });
-        } else {
+        } else if (tool.toolbox !== undefined)  {
           result.push(toPopoverItem(tool.toolbox, tool));
         }
 
@@ -280,29 +266,6 @@ export default class Toolbox extends EventsDispatcher<ToolboxEvent> {
       }, []);
   }
 
-  /**
-   * Validates tool's toolbox settings
-   *
-   * @param toolToolboxSettings - item to validate
-   * @param toolName - name of the tool used in console warning if item is not valid
-   */
-  private areToolboxSettingsValid(toolToolboxSettings: ToolboxConfigEntry, toolName: string): boolean {
-    /**
-     * Skip tools that don't pass 'toolbox' property
-     */
-    if (!toolToolboxSettings) {
-      return false;
-    }
-
-    if (toolToolboxSettings && !toolToolboxSettings.icon) {
-      _.log('Toolbar icon is missed. Tool %o skipped', 'warn', toolName);
-
-      return false;
-    }
-
-    return true;
-  }
-
   /**
    * Iterate all tools and enable theirs shortcuts if specified
    */

src/components/utils/popover.ts

@@ -437,11 +437,9 @@ export default class Popover extends EventsDispatcher<PopoverEvent> {
       innerHTML: item.label,
     });
 
-    if (item.icon) {
+    el.appendChild(Dom.make('div', Popover.CSS.itemIcon, {
-      el.appendChild(Dom.make('div', Popover.CSS.itemIcon, {
+      innerHTML: item.icon || item.name.substring(0, 1).toUpperCase(),
-        innerHTML: item.icon,
+    }));
-      }));
-    }
 
     el.appendChild(label);
 

test/cypress/tests/api/tools.spec.ts

@@ -192,83 +192,6 @@ describe('Editor Tools Api', () => {
           });
         });
     });
-
-    it('should not display tool in toolbox if the tool has single toolbox entry configured and it has icon missing', () => {
-      /**
-       * Tool with one of the toolbox entries with icon missing
-       */
-      class TestTool {
-        /**
-         * Returns toolbox config as list of entries one of which has missing icon
-         */
-        public static get toolbox(): ToolboxConfig {
-          return {
-            title: 'Entry 2',
-          };
-        }
-      }
-
-      cy.createEditor({
-        tools: {
-          testTool: TestTool,
-        },
-      }).as('editorInstance');
-
-      cy.get('[data-cy=editorjs]')
-        .get('div.ce-block')
-        .click();
-
-      cy.get('[data-cy=editorjs]')
-        .get('div.ce-toolbar__plus')
-        .click();
-
-      cy.get('[data-cy=editorjs]')
-        .get('div.ce-popover__item[data-item-name=testTool]')
-        .should('not.exist');
-    });
-
-    it('should skip toolbox entries that have no icon', () => {
-      const skippedEntryTitle = 'Entry 2';
-
-      /**
-       * Tool with one of the toolbox entries with icon missing
-       */
-      class TestTool {
-        /**
-         * Returns toolbox config as list of entries one of which has missing icon
-         */
-        public static get toolbox(): ToolboxConfig {
-          return [
-            {
-              title: 'Entry 1',
-              icon: ICON,
-            },
-            {
-              title: skippedEntryTitle,
-            },
-          ];
-        }
-      }
-
-      cy.createEditor({
-        tools: {
-          testTool: TestTool,
-        },
-      }).as('editorInstance');
-
-      cy.get('[data-cy=editorjs]')
-        .get('div.ce-block')
-        .click();
-
-      cy.get('[data-cy=editorjs]')
-        .get('div.ce-toolbar__plus')
-        .click();
-
-      cy.get('[data-cy=editorjs]')
-        .get('div.ce-popover__item[data-item-name=testTool]')
-        .should('have.length', 1)
-        .should('not.contain', skippedEntryTitle);
-    });
   });
 
   context('Tunes — renderSettings()', () => {

test/cypress/tests/block-ids.spec.ts

@@ -2,7 +2,7 @@
 import Header from '@editorjs/header';
 import { nanoid } from 'nanoid';
 
-describe.only('Block ids', () => {
+describe('Block ids', () => {
   beforeEach(function () {
     cy.createEditor({
       tools: {

types/configs/popover.d.ts

@@ -2,16 +2,16 @@
  * Common parameters for both types of popover items: with or without confirmation
  */
 interface PopoverItemBase {
-  /**
-   * Item icon to be appeared near a title
-   */
-  icon: string;
-
   /**
    * Displayed text
    */
   label: string;
 
+  /**
+   * Item icon to be appeared near a title
+   */
+  icon?: string;
+
   /**
    * Additional displayed text
    */