[button] Ensure that Enter/Space triggers click events.

Previously we have applied specific duck tape in a bunch
of places to prevent "keydown" event handlers on parent
elements from eating up Enter/Space events and thus not
firing click events on buttons as expected. With this CL
we put the logic solely into the `<devtools-button>`
component (together with a test) to ensure not every use
site has to deal with this, and it's less easy to miss.

Fixed: 373168872, 372411090
Change-Id: I00c183547a7f7f2e20a95467687885db463280ca
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5982917
Auto-Submit: Benedikt Meurer <[email protected]>
Commit-Queue: Kim-Anh Tran <[email protected]>
Reviewed-by: Kim-Anh Tran <[email protected]>

Author: bmeurer

front_end/panels/issues/components/HideIssuesMenu.ts

@@ -43,7 +43,7 @@ export class HideIssuesMenu extends HTMLElement {
     this.#shadow.adoptedStyleSheets = [hideIssuesMenuStyles];
   }
 
-  onMenuOpen(event: MouseEvent): void {
+  onMenuOpen(event: Event): void {
     event.stopPropagation();
     const buttonElement = this.#shadow.querySelector('devtools-button');
     const contextMenu = new UI.ContextMenu.ContextMenu(event, {
@@ -55,14 +55,6 @@ export class HideIssuesMenu extends HTMLElement {
     void contextMenu.show();
   }
 
-  onKeydown(event: KeyboardEvent): void {
-    if (event.key === 'Enter' || event.key === 'Space') {
-      // Make sure we don't propagate 'Enter' or 'Space' key events to parents,
-      // so that these get turned into 'click' events properly.
-      event.stopImmediatePropagation();
-    }
-  }
-
   #render(): void {
     // Disabled until https://crbug.com/1079231 is fixed.
     // clang-format off
@@ -71,8 +63,7 @@ export class HideIssuesMenu extends HTMLElement {
       .data=${{variant: Buttons.Button.Variant.ICON,iconName: 'dots-vertical', title: i18nString(UIStrings.tooltipTitle)} as Buttons.Button.ButtonData}
       .jslogContext=${'hide-issues'}
       class="hide-issues-menu-btn"
-      @click=${this.onMenuOpen}
-      @keydown=${this.onKeydown}></devtools-button>
+      @click=${this.onMenuOpen}></devtools-button>
     `, this.#shadow, {host: this});
   }
 }

front_end/ui/components/buttons/Button.test.ts

@@ -166,6 +166,20 @@ describe('Button', () => {
     assert.isFalse(innerButton.classList.contains('small'));
   });
 
+  it('prevents only "keydown" events for Enter and Space to bubble up', () => {
+    const button = renderButton({variant: Buttons.Button.Variant.PRIMARY});
+    const onKeydown = sinon.spy();
+    button.addEventListener('keydown', onKeydown);
+
+    const innerButton = button.shadowRoot!.querySelector('button') as HTMLButtonElement;
+    dispatchKeyDownEvent(innerButton, {bubbles: true, composed: true, key: 'Enter'});
+    dispatchKeyDownEvent(innerButton, {bubbles: true, composed: true, key: ' '});
+    dispatchKeyDownEvent(innerButton, {bubbles: true, composed: true, key: 'x'});
+
+    assert.isTrue(onKeydown.calledOnce);
+    assert.strictEqual(onKeydown.getCall(0).args[0].key, 'x');
+  });
+
   describe('in forms', () => {
     function renderForm(data: Buttons.Button.ButtonData = {
       variant: Buttons.Button.Variant.PRIMARY,

front_end/ui/components/buttons/Button.ts

@@ -300,6 +300,28 @@ export class Button extends HTMLElement {
     }
   }
 
+  /**
+   * Handles "keydown" events on the internal `<button>` element.
+   *
+   * This callback stops propagation of "keydown" events for Enter and Space
+   * originating from the `<button>` element, to ensure that this custom element
+   * can safely be used within parent elements (such as the `TreeOutline`) that
+   * do have "keydown" handlers as well.
+   *
+   * Without this special logic, the Enter and Space events would be
+   * consumed by parent elements, and no "click" event would be generated from
+   * this button.
+   *
+   * @param event the "keydown" event.
+   * @see https://crbug.com/373168872
+   */
+  #onKeydown(event: KeyboardEvent): void {
+    if (event.key !== 'Enter' && event.key !== ' ') {
+      return;
+    }
+    event.stopPropagation();
+  }
+
   #isToolbarVariant(): boolean {
     return this.#props.variant === Variant.TOOLBAR || this.#props.variant === Variant.PRIMARY_TOOLBAR;
   }
@@ -362,10 +384,11 @@ export class Button extends HTMLElement {
     LitHtml.render(
       html`
         <button title=${ifDefined(this.#props.title)}
-          .disabled=${this.#props.disabled}
-          class=${classMap(classes)}
-          aria-pressed=${ifDefined(this.#props.toggled)}
-          jslog=${ifDefined(jslog)}
+                .disabled=${this.#props.disabled}
+                class=${classMap(classes)}
+                aria-pressed=${ifDefined(this.#props.toggled)}
+                jslog=${ifDefined(jslog)}
+                @keydown=${this.#onKeydown}
         >${hasIcon
             ? html`
                 <devtools-icon name=${ifDefined(this.#props.toggled ? this.#props.toggledIconName : this.#props.iconName || this.#props.iconUrl)}>

front_end/ui/legacy/TabbedPane.ts

@@ -1214,7 +1214,6 @@ export class TabbedPaneTab {
       tabElement.classList.add('measuring');
     } else {
       tabElement.addEventListener('click', this.tabClicked.bind(this), false);
-      tabElement.addEventListener('keydown', this.tabKeyDown.bind(this), false);
       tabElement.addEventListener('auxclick', this.tabClicked.bind(this), false);
       tabElement.addEventListener('mousedown', this.tabMouseDown.bind(this), false);
       tabElement.addEventListener('mouseup', this.tabMouseUp.bind(this), false);
@@ -1265,19 +1264,6 @@ export class TabbedPaneTab {
         element?.parentElement?.classList.contains('tabbed-pane-close-button') || false;
   }
 
-  private tabKeyDown(ev: Event): void {
-    const event = ev as KeyboardEvent;
-    switch (event.key) {
-      case 'Enter':
-      case ' ':
-        if (this.isCloseIconClicked(event.target as HTMLElement)) {
-          this.closeTabs([this.id]);
-          ev.consume(true);
-          return;
-        }
-    }
-  }
-
   private tabClicked(ev: Event): void {
     const event = (ev as MouseEvent);
     const middleButton = event.button === 1;

front_end/ui/legacy/UIUtils.ts

@@ -1100,13 +1100,6 @@ export function createTextButton(text: string, clickHandler?: ((arg0: Event) =>
   button.variant = opts?.variant ? opts.variant : Buttons.Button.Variant.OUTLINED;
   if (clickHandler) {
     button.addEventListener('click', clickHandler);
-    button.addEventListener('keydown', (event: KeyboardEvent): void => {
-      if (event.key === 'Enter' || event.key === 'Space') {
-        // Make sure we don't propagate 'Enter' or 'Space' key events to parents,
-        // so that these get turned into 'click' events properly.
-        event.stopImmediatePropagation();
-      }
-    });
   }
   if (opts?.jslogContext) {
     button.setAttribute('jslog', `${VisualLogging.action().track({click: true}).context(opts.jslogContext)}`);