RPP: update UI for keyboard navigation shortcut dialog

Classic mode: screen/628PBCtSSGDNjCM
Modern mode: screen/ALKdxWdbJ9p9aVA

Bug: 406008928
Change-Id: Iecbc14632c2c867ef484ba05fbf7c655ecea8055
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6415910
Reviewed-by: Paul Irish <paulirish@chromium.org>
Feels: Paul Irish <paulirish@chromium.org>
Commit-Queue: Jack Franklin <jacktfranklin@chromium.org>

Author: jackfranklin

front_end/panels/recorder/RecorderController.ts

@@ -1117,26 +1117,30 @@ export class RecorderController extends LitElement {
   }
 
   #getShortcutsInfo(): Dialogs.ShortcutDialog.Shortcut[] {
-    const getBindingForAction = (action: Actions.RecorderActions): string[][] => {
+    const getBindingForAction = (action: Actions.RecorderActions): Dialogs.ShortcutDialog.ShortcutPart[][] => {
       const shortcuts = UI.ShortcutRegistry.ShortcutRegistry.instance().shortcutsForAction(action);
-      const shortcutsWithSplitBindings =
-          shortcuts.map(shortcut => shortcut.title().split(/[\s+]+/).map(word => word.trim()));
+      const shortcutsWithSplitBindings = shortcuts.map(shortcut => shortcut.title().split(/[\s+]+/).map(word => {
+        return {key: word.trim()};
+      }));
       return shortcutsWithSplitBindings;
     };
 
     return [
       {
         title: i18nString(UIStrings.startStopRecording),
-        bindings: getBindingForAction(Actions.RecorderActions.START_RECORDING),
+        rows: getBindingForAction(Actions.RecorderActions.START_RECORDING),
       },
       {
         title: i18nString(UIStrings.replayRecording),
-        bindings: getBindingForAction(Actions.RecorderActions.REPLAY_RECORDING),
+        rows: getBindingForAction(Actions.RecorderActions.REPLAY_RECORDING),
+      },
+      {
+        title: i18nString(UIStrings.copyShortcut),
+        rows: Host.Platform.isMac() ? [[{key: '⌘'}, {key: 'C'}]] : [[{key: 'Ctrl'}, {key: 'C'}]]
       },
-      {title: i18nString(UIStrings.copyShortcut), bindings: Host.Platform.isMac() ? [['⌘', 'C']] : [['Ctrl', 'C']]},
       {
         title: i18nString(UIStrings.toggleCode),
-        bindings: getBindingForAction(Actions.RecorderActions.TOGGLE_CODE_VIEW),
+        rows: getBindingForAction(Actions.RecorderActions.TOGGLE_CODE_VIEW),
       },
     ];
   }

front_end/panels/timeline/TimelinePanel.ts

@@ -317,21 +317,13 @@ const UIStrings = {
    */
   backToLiveMetrics: 'Go back to the live metrics page',
   /**
-   * @description Description of the Timeline up/down scroll action that appears in the Performance panel shortcuts dialog.
+   * @description Description of the Timeline zoom keyboard instructions that appear in the shortcuts dialog
    */
-  timelineScrollUpDown: 'Move up/down',
+  timelineZoom: 'Zoom',
   /**
-   * @description Description of the Timeline left/right panning action that appears in the Performance panel shortcuts dialog.
+   * @description Description of the Timeline scrolling & panning instructions that appear in the shortcuts dialog.
    */
-  timelinePanLeftRight: 'Move left/right',
-  /**
-   * @description Description of the Timeline in/out zoom action that appears in the Performance panel shortcuts dialog.
-   */
-  timelineZoomInOut: 'Zoom in/out',
-  /**
-   * @description Description of the Timeline fast in/out zoom action that appears in the Performance panel shortcuts dialog.
-   */
-  timelineFastZoomInOut: 'Fast zoom in/out',
+  timelineScrollPan: 'Scroll & Pan',
   /**
    * @description Title for the Dim 3rd Parties checkbox.
    */
@@ -340,6 +332,10 @@ const UIStrings = {
    * @description Description for the Dim 3rd Parties checkbox tooltip describing how 3rd parties are classified.
    */
   thirdPartiesByThirdPartyWeb: '3rd parties classified by third-party-web',
+  /**
+   * @description Title of the shortcuts dialog shown to the user that lists keyboard shortcuts.
+   */
+  shortcutsDialogTitle: 'Keyboard shortcuts for flamechart'
 } as const;
 const str_ = i18n.i18n.registerUIStrings('panels/timeline/TimelinePanel.ts', UIStrings);
 const i18nString = i18n.i18n.getLocalizedString.bind(undefined, str_);
@@ -481,10 +477,10 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod
    * Navigation radio buttons located in the shortcuts dialog.
    */
   #navigationRadioButtons = document.createElement('form');
-  #modernNavRadioButton =
-      UI.UIUtils.createRadioButton('flamechart-selected-navigation', 'Modern', 'timeline.select-modern-navigation');
-  #classicNavRadioButton =
-      UI.UIUtils.createRadioButton('flamechart-selected-navigation', 'Classic', 'timeline.select-classic-navigation');
+  #modernNavRadioButton = UI.UIUtils.createRadioButton(
+      'flamechart-selected-navigation', 'Modern - normal scrolling', 'timeline.select-modern-navigation');
+  #classicNavRadioButton = UI.UIUtils.createRadioButton(
+      'flamechart-selected-navigation', 'Classic - scroll to zoom', 'timeline.select-classic-navigation');
 
   #onMainEntryHovered: (event: Common.EventTarget.EventTargetEvent<number>) => void;
 
@@ -1180,6 +1176,7 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod
     // need to update the radio buttons selection when the dialog is open.
     this.#shortcutsDialog.addEventListener('click', this.#updateNavigationSettingSelection.bind(this));
     this.#shortcutsDialog.data = {
+      customTitle: i18nString(UIStrings.shortcutsDialogTitle),
       shortcuts: this.#getShortcutsInfo(currentNavSetting === 'classic'),
       open: !userHadShortcutsDialogOpenedOnce && hideTheDialogForTests !== 'true' &&
           !Host.InspectorFrontendHost.isUnderTest(),
@@ -1218,29 +1215,54 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod
   }
 
   #getShortcutsInfo(isNavClassic: boolean): Dialogs.ShortcutDialog.Shortcut[] {
+    const metaKey = Host.Platform.isMac() ? '⌘' : 'Ctrl';
     if (isNavClassic) {
+      // Classic navigation = scroll to zoom.
       return [
-        {title: i18nString(UIStrings.timelineScrollUpDown), bindings: [['Shift', 'Scroll up/down'], ['Shift', '↑/↓']]},
         {
-          title: i18nString(UIStrings.timelinePanLeftRight),
-          bindings: [['Shift', '←/→'], ['Scroll left/right'], ['A/D']]
+          title: i18nString(UIStrings.timelineZoom),
+          rows: [
+            [{key: 'Scroll ↕'}], [{key: 'W'}, {key: 'S'}, {joinText: 'or'}, {key: '+'}, {key: '-'}],
+            {footnote: 'hold shift for fast zoom'}
+          ]
         },
-        {title: i18nString(UIStrings.timelineZoomInOut), bindings: [['Scroll up/down'], ['W/S'], ['+/-']]},
-        {title: i18nString(UIStrings.timelineFastZoomInOut), bindings: [['Shift', 'W/S'], ['Shift', '+/-']]},
+        {
+          title: i18nString(UIStrings.timelineScrollPan),
+          rows: [
+            [{key: 'Shift'}, {joinText: '+'}, {key: 'Scroll ↕'}],
+            [{key: 'Scroll ↔'}, {joinText: 'or'}, {key: 'A'}, {key: 'D'}],
+            [
+              {key: 'Drag'}, {joinText: 'or'}, {key: 'Shift'}, {joinText: '+'}, {key: '↑'}, {key: '↓'}, {key: '←'},
+              {key: '→'}
+            ],
+          ]
+        }
       ];
     }
 
+    // New navigation where scroll = scroll.
     return [
-      {title: i18nString(UIStrings.timelineScrollUpDown), bindings: [['Scroll up/down'], ['Shift', '↑/↓']]},
       {
-        title: i18nString(UIStrings.timelinePanLeftRight),
-        bindings: [['Shift', 'Scroll up/down'], ['Scroll left/right'], ['Shift', '←/→'], ['A/D']],
+        title: i18nString(UIStrings.timelineZoom),
+        rows: [
+          [{key: metaKey}, {joinText: '+'}, {key: 'Scroll ↕'}],
+          [{key: 'W'}, {key: 'S'}, {joinText: 'or'}, {key: '+'}, {key: '-'}], {footnote: ''}
+        ]
       },
       {
-        title: i18nString(UIStrings.timelineZoomInOut),
-        bindings: [[Host.Platform.isMac() ? '⌘' : 'Ctrl', 'Scroll up/down'], ['W/S'], ['+/-']],
-      },
-      {title: i18nString(UIStrings.timelineFastZoomInOut), bindings: [['Shift', 'W/S'], ['Shift', '+/-']]},
+        title: i18nString(UIStrings.timelineScrollPan),
+        rows: [
+          [{key: 'Scroll ↕'}],
+          [
+            {key: 'Shift'}, {joinText: '+'}, {key: 'Scroll ↕'}, {joinText: 'or'}, {key: 'Scroll ↔'}, {joinText: 'or'},
+            {key: 'A'}, {key: 'D'}
+          ],
+          [
+            {key: 'Drag'}, {joinText: 'or'}, {key: 'Shift'}, {joinText: '+'}, {key: '↑'}, {key: '↓'}, {key: '←'},
+            {key: '→'}
+          ],
+        ]
+      }
     ];
   }
 

front_end/ui/components/dialogs/ShortcutDialog.test.ts

@@ -16,7 +16,16 @@ describeWithLocale('ShortcutDialog', () => {
     if (prependedElement) {
       shortcutDialog.prependElement(prependedElement);
     }
-    shortcutDialog.data = {shortcuts: [{title: 'Shortcut Title', bindings: [['Ctrl+E']]}], open};
+    shortcutDialog.data = {
+      shortcuts: [{
+        title: 'Shortcut Title',
+        rows: [
+          [{key: 'Cmd'}, {joinText: '+'}, {key: 'W'}],
+          {footnote: 'close the window'},
+        ]
+      }],
+      open
+    };
     Helpers.renderElementIntoDOM(shortcutDialog);
     await RenderCoordinator.done();
 

front_end/ui/components/dialogs/ShortcutDialog.ts

@@ -7,7 +7,7 @@ import * as i18n from '../../../core/i18n/i18n.js';
 import type * as Platform from '../../../core/platform/platform.js';
 import * as Buttons from '../../../ui/components/buttons/buttons.js';
 import * as ComponentHelpers from '../../../ui/components/helpers/helpers.js';
-import {html, nothing, render} from '../../../ui/lit/lit.js';
+import {html, nothing, render, type TemplateResult} from '../../../ui/lit/lit.js';
 
 import type {ButtonDialogData} from './ButtonDialog.js';
 import shortcutDialogStylesRaw from './shortcutDialog.css.js';
@@ -37,13 +37,20 @@ declare global {
   }
 }
 
+export type ShortcutPart = {
+  key: string,
+}|{joinText: string};
+
+export type ShortcutRow = ShortcutPart[]|{footnote: string};
+
 export interface Shortcut {
   title: string|Platform.UIString.LocalizedString;
-  bindings: string[][];
+  rows: readonly ShortcutRow[];
 }
 export interface ShortcutDialogData {
   shortcuts: Shortcut[];
   open?: boolean;
+  customTitle?: Platform.UIString.LocalizedString;
 }
 
 export class ShortcutDialog extends HTMLElement {
@@ -52,17 +59,29 @@ export class ShortcutDialog extends HTMLElement {
 
   #shortcuts: Shortcut[] = [];
   #openOnRender = false;
+  #customTitle?: Platform.UIString.LocalizedString;
   #prependedElement: HTMLElement|null = null;
 
   connectedCallback(): void {
     this.#shadow.adoptedStyleSheets = [shortcutDialogStyles];
   }
 
+  get data(): ShortcutDialogData {
+    return {
+      shortcuts: this.#shortcuts,
+      open: this.#openOnRender,
+      customTitle: this.#customTitle,
+    };
+  }
+
   set data(data: ShortcutDialogData) {
     this.#shortcuts = data.shortcuts;
     if (data.open) {
       this.#openOnRender = data.open;
     }
+    if (data.customTitle) {
+      this.#customTitle = data.customTitle;
+    }
 
     void ComponentHelpers.ScheduledRender.scheduleRender(this, this.#renderBound);
   }
@@ -71,6 +90,22 @@ export class ShortcutDialog extends HTMLElement {
     this.#prependedElement = element;
   }
 
+  #renderRow(row: ShortcutRow): TemplateResult {
+    if (!Array.isArray(row)) {
+      // If it's not an array it's a footnote, which is the easier case, so
+      // render that and return.
+      return html`<span class="footnote">${row.footnote}</span>`;
+    }
+
+    return html`${row.map(part => {
+      if ('key' in part) {
+        return html`<span class="keybinds-key">${part.key}</span>`;
+      }
+      return html`<span class="keybinds-join-text">${part.joinText}</span>`;
+    })}
+    `;
+  }
+
   #render(): void {
     if (!ComponentHelpers.ScheduledRender.isScheduledRender(this)) {
       throw new Error('Shortcut dialog render was not scheduled');
@@ -82,7 +117,7 @@ export class ShortcutDialog extends HTMLElement {
       <devtools-button-dialog .data=${{
           openOnRender: this.#openOnRender,
           closeButton: true,
-          dialogTitle: i18nString(UIStrings.dialogTitle),
+          dialogTitle: this.#customTitle ?? i18nString(UIStrings.dialogTitle),
           variant: Buttons.Button.Variant.TOOLBAR,
           iconName: 'help',
           iconTitle: i18nString(UIStrings.showShortcutTitle),
@@ -94,16 +129,11 @@ export class ShortcutDialog extends HTMLElement {
               <li class="keybinds-list-item">
                 <div class="keybinds-list-title">${shortcut.title}</div>
                 <div class="shortcuts-for-actions">
-                  ${shortcut.bindings.map(binding => {
-                    return html`
-                    <div class="keys-container">
-                      ${binding.map(key => html`
-                          <span class="keybinds-key">${key}</span>
-                      `)}
-                    </div>
+                  ${shortcut.rows.map(row => {
+                    return html`<div class="row-container">${this.#renderRow(row)}</div>
                   `;
-                    })}
-                  </div>
+                  })}
+                </div>
               </li>`,
           )}
         </ul>

front_end/ui/components/dialogs/shortcutDialog.css

@@ -13,30 +13,45 @@
 }
 
 .keybinds-list-item {
-  display: grid;
+  display: block;
+  min-width: 315px;
   align-items: baseline;
-  justify-content: space-between;
-  grid-template-rows: 1fr;
-  grid-template-columns: 1fr var(--sys-size-11) 1fr;
   border-bottom: var(--sys-size-1) solid var(--sys-color-divider);
   padding: var(--sys-size-4) 0;
+  position: relative;
+
 
   &:last-of-type {
     border-bottom: unset;
   }
 
   .keybinds-list-title {
-    grid-row: 1/1;
-  }
-
-  .shortcuts-for-actions {
-    grid-area: auto / 3 / auto / span 1;
+    /* Put the list title absolute so it is out the flow, this enables the
+     * shortcuts to sit on the same row as the title */
+    position: absolute;
+    top: var(--sys-size-4);
+    left: 0;
+    /* Put the text vertically central so it aligns with the shortcuts shown  */
+    display: flex;
+    align-items: center;
+    height: var(--sys-size-11); /* IMPORTANT: this has to be the same height as .keybinds-key to maintain text alignment */
   }
 }
 
-.keys-container {
+.row-container {
   display: flex;
-  gap: var(--sys-size-3);
+  gap: var(--sys-size-5);
+  align-items: center;
+
+  .keybinds-join-text, .footnote {
+    color: var(--color-text-primary);
+    opacity: 50%;
+  }
+
+  .footnote {
+    display: block;
+    height: 15px;
+  }
 }
 
 .shortcuts-for-actions {
@@ -57,6 +72,10 @@
     font: var(--sys-typescale-body3-regular);
     gap: var(--sys-size-2);
   }
+
+  input[type="radio"] {
+    margin-left: 0;
+  }
 }
 
 .keybinds-key {
@@ -70,5 +89,5 @@
   font: var(--sys-typescale-body5-medium);
   white-space: nowrap;
   border-radius: var(--sys-shape-corner-small);
-  background: var(--sys-color-tonal-container);
+  background: var(--sys-color-base-container);
 }

front_end/ui/components/docs/dialog/shortcut_dialog.ts

@@ -14,11 +14,11 @@ shortcutDialog.data = {
   shortcuts: [
     {
       title: 'First Shortcut Title',
-      bindings: [['Ctrl+E']],
+      rows: [[{key: 'Ctrl'}, {key: 'E'}]],
     },
     {
       title: 'Second Shortcut Title',
-      bindings: [['Ctrl', 'Enter'], ['F8']],
+      rows: [[{key: 'F8'}]],
     },
   ],
 };