uiCmd shouldn't confuse up the shortcut with the display of the shortcut

(re: #1272, re: #1261, re: #1255)

Windows/linux should not replace '⌘Z' with 'Ctrl+Z', it should just replace it
with '^Z' and then the `uiCmd.display` should handle how to present that to the user.

This change simplifies the `uiCmd` code, and avoids the 'Ctrl+` stuff.

It also changes `uiTooltip` to work with an array of keys, using
d3-selection.each and `uiCmd.display` to render each one.
Previous code was attempting to split the keycombo into chars, causing #1272

I still need to make sure that `tooltip.keys()` is being called consistently,
with an Array of keys - we do this sometimes but not everywhere.

Also, in 01a16a9, the '+' to zoom in broke, because it is a shifted key.
This commit fixes that issue also.

modules/ui/cmd.js

@@ -1,43 +1,30 @@
 import { utilDetect } from '../util/detect';
 
 
-// Translate a MacOS key command into the appropriate Windows/Linux equivalent.
-// For example, ⌘Z -> Ctrl+Z
-export let uiCmd = function(code) {
+// Throughout Rapid we specify key combos in MacOS style.
+// This helper converts a key combo to the key combo for the system the user is on,
+// - on MacOS, no change
+// - on Windows/Linux, convert Command to Control, for example, ⌘Z -> ⌃Z
+export let uiCmd = function(combo) {
   const detected = utilDetect();
 
   if (detected.os === 'mac') {
-    return code;
+    return combo;
   }
 
   if (detected.os === 'win') {
-    if (code === '⌘⇧Z') return 'Ctrl+Y';
+    if (combo === '⌘⇧Z') return '⌃Y';  // special handling for Redo on Windows
   }
 
-  const replacements = {
-    '⌘': 'Ctrl',
-    '⇧': 'Shift',
-    '⌥': 'Alt',
-    '⌫': 'Backspace',
-    '⌦': 'Delete'
-  };
-
-  let result = '';
-  for (let i = 0; i < code.length; i++) {
-    if (code[i] in replacements) {
-      result += replacements[code[i]] + (i < code.length - 1 ? '+' : '');
-    } else {
-      result += code[i];
-    }
-  }
-
-  return result;
+  return combo.replace('⌘', '⌃');
 };
 
 
-// return a display-focused string for a given keyboard code
-uiCmd.display = function(context, code) {
-  if (code.length !== 1) return code;
+// Return a display-focused string for a given key character.
+// On Mac, we include the symbols, on other systems, we only include the word.
+// For example, '⌘' -> '⌘ Cmd'
+uiCmd.display = function(context, char) {
+  if (char.length !== 1) return char;  // Ignore if multiple chars, like "F11"
 
   const l10n = context.systems.l10n;
   const detected = utilDetect();
@@ -58,5 +45,5 @@ uiCmd.display = function(context, code) {
     '☰': mac ? '☰ ' + l10n.t('shortcuts.key.menu')  : l10n.t('shortcuts.key.menu'),
   };
 
-  return replacements[code] || code;
+  return replacements[char] || char;
 };

modules/ui/shortcuts.js

@@ -139,7 +139,7 @@ export function uiShortcuts(context) {
             .selectAll('kbd.modifier')
             .data(function (d) {
                 if (detected.os === 'win' && d.text === 'shortcuts.editing.commands.redo') {
-                    return ['⌘'];
+                    return ['⌃'];
                 } else if (detected.os !== 'mac' && d.text === 'shortcuts.browsing.display_options.fullscreen') {
                     return [];
                 } else {
@@ -153,11 +153,11 @@ export function uiShortcuts(context) {
                 selection
                     .append('kbd')
                     .attr('class', 'modifier')
-                    .html(function (d) { return uiCmd.display(context, d); });
+                    .text(d => uiCmd.display(context, d));
 
                 selection
                     .append('span')
-                    .html('+');
+                    .text('+');
             });
 
 

modules/ui/tooltip.js

@@ -1,7 +1,10 @@
+import { select as d3_select } from 'd3-selection';
+
 import { utilFunctor } from '../util/util';
 import { uiPopover } from './popover';
 import { uiCmd } from './cmd';
 
+
 export function uiTooltip(context) {
     const l10n = context.systems.l10n;
     var tooltip = uiPopover(context, 'tooltip').displayType('hover');
@@ -72,32 +75,39 @@ export function uiTooltip(context) {
                 .html(text);
 
             var keyhintWrap = selection
-                .selectAll('.keyhint-wrap')
-                .data(keys && keys.length ? [0] : []);
+                .selectAll('.NOT-keyhint-wrap')
+                .data(keys?.length ? [0] : []);
 
             keyhintWrap.exit()
                 .remove();
 
             var keyhintWrapEnter = keyhintWrap.enter()
                 .append('div')
-                .attr('class', 'keyhint-wrap');
+                .attr('class', 'NOT-keyhint-wrap');
 
             keyhintWrapEnter
                 .append('span')
-                .html(l10n.tHtml('tooltip_keyhint'));
+                .text(l10n.t('tooltip_keyhint'));  // "Shortcut:"
 
             keyhintWrap = keyhintWrapEnter.merge(keyhintWrap);
 
             keyhintWrap.selectAll('kbd.shortcut')
-                .data(keys && keys.length ? keys : [])
+                .data(keys?.length ? keys : [])
                 .enter()
-                .append('span')
-                .html(d => {
-                    const keysArray = d.split('');
-                    return keysArray.map(key => `<kbd class="shortcut">${uiCmd.display(context, key)}</kbd>`)
-                    .join('<span>+</span>');
-                })
-                .style('white-space', 'nowrap');
+                .each((d, i, nodes) => {
+                    const selection = d3_select(nodes[i]);
+
+                    selection
+                        .append('kbd')
+                        .attr('class', 'shortcut')
+                        .text(d => uiCmd.display(context, d));
+
+                    if (i < keys.length - 1) {
+                        selection
+                            .append('span')
+                            .text('+');
+                    }
+                });
         };
     });
 

modules/ui/zoom.js

@@ -8,47 +8,45 @@ import { utilKeybinding } from '../util/keybinding';
 
 export function uiZoom(context) {
   const l10n = context.systems.l10n;
+  const map = context.systems.map;
+  const ui = context.systems.ui;
 
   const zooms = [{
     id: 'zoom-in',
     icon: 'rapid-icon-plus',
     title: l10n.t('zoom.in'),
     action: zoomIn,
-    isDisabled: () => !context.systems.map.canZoomIn(),
+    isDisabled: () => !map.canZoomIn(),
     disabledTitle: l10n.t('zoom.disabled.in'),
     key: '+'
   }, {
     id: 'zoom-out',
     icon: 'rapid-icon-minus',
     title: l10n.t('zoom.out'),
     action: zoomOut,
-    isDisabled: () => !context.systems.map.canZoomOut(),
+    isDisabled: () => !map.canZoomOut(),
     disabledTitle: l10n.t('zoom.disabled.out'),
     key: '-'
   }];
 
   function zoomIn(d3_event) {
-    if (d3_event.shiftKey) return;
     d3_event.preventDefault();
-    context.systems.map.zoomIn();
+    map.zoomIn();
   }
 
   function zoomOut(d3_event) {
-    if (d3_event.shiftKey) return;
     d3_event.preventDefault();
-    context.systems.map.zoomOut();
+    map.zoomOut();
   }
 
   function zoomInFurther(d3_event) {
-    if (d3_event.shiftKey) return;
     d3_event.preventDefault();
-    context.systems.map.zoomInFurther();
+    map.zoomInFurther();
   }
 
   function zoomOutFurther(d3_event) {
-    if (d3_event.shiftKey) return;
     d3_event.preventDefault();
-    context.systems.map.zoomOutFurther();
+    map.zoomOutFurther();
   }
 
   return function render(selection) {
@@ -69,7 +67,7 @@ export function uiZoom(context) {
         if (!d.isDisabled()) {
           d.action(d3_event);
         } else if (_lastPointerUpType === 'touch' || _lastPointerUpType === 'pen') {
-          context.systems.ui.flash
+          ui.flash
             .duration(2000)
             .iconName(`#${d.icon}`)
             .iconClass('disabled')
@@ -107,6 +105,6 @@ export function uiZoom(context) {
 
     updateButtonStates();
 
-    context.systems.map.on('draw', updateButtonStates);
+    map.on('draw', updateButtonStates);
   };
 }

modules/util/keybinding.js

@@ -204,11 +204,11 @@ export function utilKeybinding(namespace) {
 utilKeybinding.modifierCodes = {
     // Shift key, ⇧
     '⇧': 16, shift: 16,
-    // CTRL key, on Mac: ⌃
+    // Control key, ⌃
     '⌃': 17, ctrl: 17,
-    // ALT key, on Mac: ⌥ (Alt)
+    // Alt key, on Mac: '⌥ Option'
     '⌥': 18, alt: 18, option: 18,
-    // META, on Mac: ⌘ (CMD), on Windows (Win), on Linux (Super)
+    // Meta key, on Mac: '⌘ Command', on Windows 'Win', on Linux 'Super'
     '⌘': 91, meta: 91, cmd: 91, 'super': 91, win: 91
 };
 
@@ -219,7 +219,7 @@ utilKeybinding.modifierProperties = {
     91: 'metaKey'
 };
 
-utilKeybinding.plusKeys = ['plus', 'ffplus', '=', 'ffequals', '≠', '±'];
+utilKeybinding.plusKeys = ['+', 'plus', 'ffplus', '=', 'ffequals', '≠', '±'];
 utilKeybinding.minusKeys = ['_', '-', 'ffminus', 'dash', '–', '—'];
 
 utilKeybinding.keys = {

test/spec/ui/cmd.js

@@ -37,30 +37,30 @@ describe('uiCmd', () => {
   it('changes keys to linux versions', () => {
     _userAgent = 'Linux';
     Rapid.utilDetect(true);  // force redetection
-    expect(Rapid.uiCmd('⌘⌫')).to.eql('Ctrl+Backspace');
-    expect(Rapid.uiCmd('⌘A')).to.eql('Ctrl+A');
-    expect(Rapid.uiCmd('⇧A')).to.eql('Shift+A');
-    expect(Rapid.uiCmd('⌘⇧A')).to.eql('Ctrl+Shift+A');
-    expect(Rapid.uiCmd('⌘⇧Z')).to.eql('Ctrl+Shift+Z');
+    expect(Rapid.uiCmd('⌘⌫')).to.eql('⌃⌫');
+    expect(Rapid.uiCmd('⌘A')).to.eql('⌃A');
+    expect(Rapid.uiCmd('⇧A')).to.eql('⇧A');
+    expect(Rapid.uiCmd('⌘⇧A')).to.eql('⌃⇧A');
+    expect(Rapid.uiCmd('⌘⇧Z')).to.eql('⌃⇧Z');
   });
 
 
   it('changes keys to win versions', () => {
     _userAgent = 'Win';
     Rapid.utilDetect(true);  // force redetection
-    expect(Rapid.uiCmd('⌘⌫')).to.eql('Ctrl+Backspace');
-    expect(Rapid.uiCmd('⌘A')).to.eql('Ctrl+A');
-    expect(Rapid.uiCmd('⇧A')).to.eql('Shift+A');
-    expect(Rapid.uiCmd('⌘⇧A')).to.eql('Ctrl+Shift+A');
-    expect(Rapid.uiCmd('⌘⇧Z')).to.eql('Ctrl+Y');  // special case
+    expect(Rapid.uiCmd('⌘⌫')).to.eql('⌃⌫');
+    expect(Rapid.uiCmd('⌘A')).to.eql('⌃A');
+    expect(Rapid.uiCmd('⇧A')).to.eql('⇧A');
+    expect(Rapid.uiCmd('⌘⇧A')).to.eql('⌃⇧A');
+    expect(Rapid.uiCmd('⌘⇧Z')).to.eql('⌃Y');  // special case
   });
 
 
   it('handles multi-character keys', () => {
     _userAgent = 'Win';
     Rapid.utilDetect(true);  // force redetection
     expect(Rapid.uiCmd('f11')).to.eql('f11');
-    expect(Rapid.uiCmd('⌘plus')).to.eql('Ctrl+plus');
+    expect(Rapid.uiCmd('⌘plus')).to.eql('⌃plus');
   });
 
 });