From 96481566ae43d24fc25c346d4123ccf1a9d8f833 Mon Sep 17 00:00:00 2001
From: Elizabeth Mitchell <lizmitchell@google.com>
Date: Fri, 10 Nov 2023 11:29:10 -0800
Subject: [PATCH] refactor(button): move background content outside of inner
 button

Part of a series of changes to support text wrapping and host aria for button.

PiperOrigin-RevId: 581319862
---
 button/internal/_outlined-button.scss  |  7 +++++++
 button/internal/_shared.scss           | 27 +++++++++++-------------
 button/internal/button.ts              | 29 ++++++++++++++++++--------
 button/internal/elevated-button.ts     |  2 +-
 button/internal/filled-button.ts       |  2 +-
 button/internal/filled-tonal-button.ts |  2 +-
 button/internal/outlined-button.ts     |  4 ++--
 chips/internal/chip.ts                 |  1 +
 iconbutton/internal/icon-button.ts     |  2 ++
 9 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/button/internal/_outlined-button.scss b/button/internal/_outlined-button.scss
index 58fb3693d..c0d251a6f 100644
--- a/button/internal/_outlined-button.scss
+++ b/button/internal/_outlined-button.scss
@@ -86,6 +86,13 @@
   }
 
   @media (forced-colors: active) {
+    :host([disabled]) .background {
+      // Only outlined buttons change their border when disabled to distinguish
+      // them from other buttons that add a border for increased visibility in
+      // HCM.
+      border-color: GrayText;
+    }
+
     :host([disabled]) .outline {
       opacity: 1;
     }
diff --git a/button/internal/_shared.scss b/button/internal/_shared.scss
index dc04929ec..7dfee6707 100644
--- a/button/internal/_shared.scss
+++ b/button/internal/_shared.scss
@@ -13,6 +13,10 @@
 
 @mixin styles() {
   :host {
+    border-start-start-radius: var(--_container-shape-start-start);
+    border-start-end-radius: var(--_container-shape-start-end);
+    border-end-start-radius: var(--_container-shape-end-start);
+    border-end-end-radius: var(--_container-shape-end-end);
     box-sizing: border-box;
     cursor: pointer;
     display: inline-flex;
@@ -61,10 +65,7 @@
   }
 
   .button {
-    border-start-start-radius: var(--_container-shape-start-start);
-    border-start-end-radius: var(--_container-shape-start-end);
-    border-end-start-radius: var(--_container-shape-end-start);
-    border-end-end-radius: var(--_container-shape-end-end);
+    border-radius: inherit;
     cursor: inherit;
     display: inline-flex;
     align-items: center;
@@ -101,11 +102,10 @@
     color: var(--_pressed-label-text-color);
   }
 
-  .button::before {
+  .background {
     // Background color. Separate node for disabled opacity styles.
     background-color: var(--_container-color);
     border-radius: inherit;
-    content: '';
     inset: 0;
     position: absolute;
   }
@@ -119,17 +119,20 @@
     opacity: var(--_disabled-label-text-opacity);
   }
 
-  :host([disabled]) .button::before {
+  :host([disabled]) .background {
     background-color: var(--_disabled-container-color);
     opacity: var(--_disabled-container-opacity);
   }
 
   @media (forced-colors: active) {
-    .button::before {
+    .background {
+      // Use CanvasText to increase visibility of buttons when the background
+      // is not rendered. Buttons that use outlines by default should change The
+      // outline color to GrayText when disabled.
       border: 1px solid CanvasText;
     }
 
-    :host([disabled]) .button {
+    :host([disabled]) {
       --_disabled-icon-color: GrayText;
       --_disabled-icon-opacity: 1;
       --_disabled-container-opacity: 1;
@@ -138,12 +141,6 @@
     }
   }
 
-  .button::before,
-  md-elevation,
-  md-ripple {
-    z-index: -1; // Place behind content
-  }
-
   :host([has-icon]:not([trailing-icon])) {
     padding-inline-start: var(--_with-leading-icon-leading-space);
     padding-inline-end: var(--_with-leading-icon-trailing-space);
diff --git a/button/internal/button.ts b/button/internal/button.ts
index 9ca891c51..590c502db 100644
--- a/button/internal/button.ts
+++ b/button/internal/button.ts
@@ -117,17 +117,32 @@ export abstract class Button extends buttonBaseClass implements FormSubmitter {
   }
 
   protected override render() {
-    return this.href ? this.renderLink() : this.renderButton();
+    // Link buttons may not be disabled
+    const isDisabled = this.disabled && !this.href;
+    const buttonOrLink = this.href ? this.renderLink() : this.renderButton();
+    // TODO(b/310046938): due to a limitation in focus ring/ripple, we can't use
+    // the same ID for different elements, so we change the ID instead.
+    const buttonId = this.href ? 'link' : 'button';
+    return html`
+      ${this.renderElevationOrOutline?.()}
+      <div class="background"></div>
+      <md-focus-ring part="focus-ring" for=${buttonId}></md-focus-ring>
+      <md-ripple for=${buttonId} ?disabled="${isDisabled}"></md-ripple>
+      ${buttonOrLink}
+    `;
   }
 
-  protected renderElevation?(): unknown;
-
-  protected renderOutline?(): unknown;
+  // Buttons can override this to add elevation or an outline. Use this and
+  // return `<md-elevation>` (for elevated, filled, and tonal buttons)
+  // or `<div class="outline">` (for outlined buttons).
+  // Text buttons that have neither do not need to implement this.
+  protected renderElevationOrOutline?(): unknown;
 
   private renderButton() {
     // Needed for closure conformance
     const {ariaLabel, ariaHasPopup, ariaExpanded} = this as ARIAMixinStrict;
     return html`<button
+      id="button"
       class="button"
       ?disabled=${this.disabled}
       aria-label="${ariaLabel || nothing}"
@@ -141,6 +156,7 @@ export abstract class Button extends buttonBaseClass implements FormSubmitter {
     // Needed for closure conformance
     const {ariaLabel, ariaHasPopup, ariaExpanded} = this as ARIAMixinStrict;
     return html`<a
+      id="link"
       class="button"
       aria-label="${ariaLabel || nothing}"
       aria-haspopup="${ariaHasPopup || nothing}"
@@ -152,16 +168,11 @@ export abstract class Button extends buttonBaseClass implements FormSubmitter {
   }
 
   private renderContent() {
-    // Link buttons may not be disabled
-    const isDisabled = this.disabled && !this.href;
     const icon = html`<slot
       name="icon"
       @slotchange="${this.handleSlotChange}"></slot>`;
 
     return html`
-      ${this.renderElevation?.()}${this.renderOutline?.()}
-      <md-focus-ring part="focus-ring"></md-focus-ring>
-      <md-ripple ?disabled="${isDisabled}"></md-ripple>
       <span class="touch"></span>
       ${this.trailingIcon ? nothing : icon}
       <span class="label"><slot></slot></span>
diff --git a/button/internal/elevated-button.ts b/button/internal/elevated-button.ts
index 0b8d9684f..72828a564 100644
--- a/button/internal/elevated-button.ts
+++ b/button/internal/elevated-button.ts
@@ -14,7 +14,7 @@ import {Button} from './button.js';
  * An elevated button component.
  */
 export class ElevatedButton extends Button {
-  protected override renderElevation() {
+  protected override renderElevationOrOutline() {
     return html`<md-elevation></md-elevation>`;
   }
 }
diff --git a/button/internal/filled-button.ts b/button/internal/filled-button.ts
index 97d1cd095..a9052a523 100644
--- a/button/internal/filled-button.ts
+++ b/button/internal/filled-button.ts
@@ -14,7 +14,7 @@ import {Button} from './button.js';
  * A filled button component.
  */
 export class FilledButton extends Button {
-  protected override renderElevation() {
+  protected override renderElevationOrOutline() {
     return html`<md-elevation></md-elevation>`;
   }
 }
diff --git a/button/internal/filled-tonal-button.ts b/button/internal/filled-tonal-button.ts
index 7a96ffea4..295cc2996 100644
--- a/button/internal/filled-tonal-button.ts
+++ b/button/internal/filled-tonal-button.ts
@@ -14,7 +14,7 @@ import {Button} from './button.js';
  * A filled tonal button component.
  */
 export class FilledTonalButton extends Button {
-  protected override renderElevation() {
+  protected override renderElevationOrOutline() {
     return html`<md-elevation></md-elevation>`;
   }
 }
diff --git a/button/internal/outlined-button.ts b/button/internal/outlined-button.ts
index ab9c3c4ef..3eaf441a1 100644
--- a/button/internal/outlined-button.ts
+++ b/button/internal/outlined-button.ts
@@ -12,7 +12,7 @@ import {Button} from './button.js';
  * An outlined button component.
  */
 export class OutlinedButton extends Button {
-  protected override renderOutline() {
-    return html`<span class="outline"></span>`;
+  protected override renderElevationOrOutline() {
+    return html`<div class="outline"></div>`;
   }
 }
diff --git a/chips/internal/chip.ts b/chips/internal/chip.ts
index e76524218..0b94afd15 100644
--- a/chips/internal/chip.ts
+++ b/chips/internal/chip.ts
@@ -53,6 +53,7 @@ export abstract class Chip extends LitElement {
 
   /**
    * The `id` of the action the primary focus ring and ripple are for.
+   * TODO(b/310046938): use the same id for both elements
    */
   protected abstract readonly primaryId: string;
 
diff --git a/iconbutton/internal/icon-button.ts b/iconbutton/internal/icon-button.ts
index 20a8b498c..a04ebae8e 100644
--- a/iconbutton/internal/icon-button.ts
+++ b/iconbutton/internal/icon-button.ts
@@ -194,12 +194,14 @@ export class IconButton extends iconButtonBaseClass implements FormSubmitter {
   }
 
   private renderFocusRing() {
+    // TODO(b/310046938): use the same id for both elements
     return html`<md-focus-ring
       part="focus-ring"
       for=${this.href ? 'link' : 'button'}></md-focus-ring>`;
   }
 
   private renderRipple() {
+    // TODO(b/310046938): use the same id for both elements
     return html`<md-ripple
       for=${this.href ? 'link' : nothing}
       ?disabled="${!this.href && this.disabled}"></md-ripple>`;