fix(button): theme font color being overwritten

* Currently buttons with a background color (e.g. flat buttons, raised buttons, fabs) receive a font color by the theme. This font color is accidentally being overwritten by the normal button CSS that sets the `color` for every button to `inherit`. This can cause the text to be invisible in a dark theme. From now on, those buttons will no longer inherit the font color accidentally. * Normal buttons, stroked buttons and icon buttons will still inherit the font color, because it's wrong to assume that those buttons are always placed inside of containers with the default background color. Otherwise those buttons would be invisible in some containers with a different background color (e.g. in a themed toolbar). * Removes the SSR check for `hasHostAttributes`. This method is essential for the color of the buttons, and needs to run inside of SSR. (now possible with Domino anyway) * Cleans up the button theme while being at it. Fixes angular#4614. Fixes angular#9231. Fixes angular#9634
devversion · Feb 10, 2018 · f3b930b · f3b930b
1 parent 008ee07
commit f3b930b
Show file tree

Hide file tree

Showing 7 changed files with 115 additions and 80 deletions.
diff --git a/src/demo-app/button/button-demo.html b/src/demo-app/button/button-demo.html
@@ -1,7 +1,7 @@
 <div class="demo-button">
   <h4 class="section-header">Buttons</h4>
   <section>
-    <button mat-button>flat</button>
+    <button mat-button>normal</button>
     <button mat-raised-button>raised</button>
     <button mat-stroked-button>stroked</button>
     <button mat-flat-button>flat</button>

diff --git a/src/demo-app/toolbar/toolbar-demo.html b/src/demo-app/toolbar/toolbar-demo.html
@@ -1,40 +1,61 @@
 <div class="demo-toolbar">
-
   <p>
     <mat-toolbar>
-      <mat-icon class="demo-toolbar-icon">menu</mat-icon>
-      <span>Default Toolbar</span>
+      <button mat-icon-button>
+        <mat-icon>menu</mat-icon>
+      </button>
 
+      <span>Default Toolbar</span>
       <span class="demo-fill-remaining"></span>
 
-      <mat-icon>code</mat-icon>
+      <button mat-button>Text</button>
+      <button mat-icon-button>
+        <mat-icon>code</mat-icon>
+      </button>
+
+      <button mat-icon-button color="warn">
+        <mat-icon>code</mat-icon>
+      </button>
     </mat-toolbar>
   </p>
 
   <p>
     <mat-toolbar color="primary">
-      <mat-icon class="demo-toolbar-icon">menu</mat-icon>
-      <span>Primary Toolbar</span>
+      <button mat-icon-button>
+        <mat-icon>menu</mat-icon>
+      </button>
 
+      <span>Primary Toolbar</span>
       <span class="demo-fill-remaining"></span>
 
-      <mat-icon>code</mat-icon>
+      <button mat-raised-button>Text</button>
+      <button mat-raised-button color="accent">Accent</button>
+      <button mat-stroked-button>Stroked</button>
     </mat-toolbar>
   </p>
 
   <p>
     <mat-toolbar color="accent">
-      <mat-icon class="demo-toolbar-icon">menu</mat-icon>
-      <span>Accent Toolbar</span>
+      <button mat-icon-button>
+        <mat-icon>menu</mat-icon>
+      </button>
 
+      <span>Accent Toolbar</span>
       <span class="demo-fill-remaining"></span>
 
-      <mat-icon>code</mat-icon>
+      <button mat-button>Text</button>
+      <button mat-flat-button>Flat</button>
+      <button mat-mini-fab color="">
+        <mat-icon>done</mat-icon>
+      </button>
+      <button mat-mini-fab color="primary">
+        <mat-icon>done</mat-icon>
+      </button>
     </mat-toolbar>
   </p>
 
   <p>
-    <mat-toolbar color="accent">
+    <mat-toolbar>
       <mat-toolbar-row>First Row</mat-toolbar-row>
       <mat-toolbar-row>Second Row</mat-toolbar-row>
     </mat-toolbar>

diff --git a/src/demo-app/toolbar/toolbar-demo.scss b/src/demo-app/toolbar/toolbar-demo.scss
@@ -9,4 +9,7 @@
     flex: 1 1 auto;
   }
 
+  button {
+    margin: 0 4px;
+  }
 }
diff --git a/src/lib/button/_button-theme.scss b/src/lib/button/_button-theme.scss
@@ -2,7 +2,7 @@
 @import '../core/typography/typography-utils';
 
 // Applies a focus style to an mat-button element for each of the supported palettes.
-@mixin _mat-button-focus-color($theme) {
+@mixin _mat-button-focus-overlay-color($theme) {
   $primary: map-get($theme, primary);
   $accent: map-get($theme, accent);
   $warn: map-get($theme, warn);
@@ -24,7 +24,7 @@
   }
 }
 
-@mixin _mat-button-ripple-color($theme, $hue, $opacity: 0.2) {
+@mixin _mat-button-ripple-color($theme, $hue, $opacity: 0.1) {
   $primary: map-get($theme, primary);
   $accent: map-get($theme, accent);
   $warn: map-get($theme, warn);
@@ -43,21 +43,21 @@
 }
 
 // Applies a property to an mat-button element for each of the supported palettes.
-@mixin _mat-button-theme-color($theme, $property, $color: 'default') {
+@mixin _mat-button-theme-property($theme, $property, $hue) {
   $primary: map-get($theme, primary);
   $accent: map-get($theme, accent);
   $warn: map-get($theme, warn);
   $background: map-get($theme, background);
   $foreground: map-get($theme, foreground);
 
   &.mat-primary {
-    #{$property}: mat-color($primary, $color);
+    #{$property}: mat-color($primary, $hue);
   }
   &.mat-accent {
-    #{$property}: mat-color($accent, $color);
+    #{$property}: mat-color($accent, $hue);
   }
   &.mat-warn {
-    #{$property}: mat-color($warn, $color);
+    #{$property}: mat-color($warn, $hue);
   }
 
   &.mat-primary, &.mat-accent, &.mat-warn, &[disabled] {
@@ -76,51 +76,41 @@
   $foreground: map-get($theme, foreground);
 
   .mat-button, .mat-icon-button, .mat-stroked-button {
+    // Buttons without a background color should inherit the font color. This is necessary to
+    // ensure that the button is readable on custom background colors. It's wrong to always assume
+    // that those buttons are always placed inside of containers with the default background
+    // color of the theme (e.g. themed toolbars).
+    color: inherit;
     background: transparent;
 
-    @include _mat-button-focus-color($theme);
-    @include _mat-button-theme-color($theme, 'color');
-  }
-
-  .mat-raised-button, .mat-fab, .mat-mini-fab {
-    // Default properties when not using any [color] value.
-    color: mat-color($foreground, text);
-    background-color: mat-color($background, raised-button);
-
-    @include _mat-button-theme-color($theme, 'color', default-contrast);
-    @include _mat-button-theme-color($theme, 'background-color');
+    @include _mat-button-theme-property($theme, 'color', default);
+    @include _mat-button-focus-overlay-color($theme);
 
-    // Add ripple effect with contrast color to buttons that don't have a focus overlay.
-    @include _mat-button-ripple-color($theme, default-contrast);
-  }
-
-  // Add ripple effect with default color to flat buttons, which also have a focus overlay.
-  .mat-button {
-    @include _mat-button-ripple-color($theme, default, 0.1);
+    // Setup the ripple color to be based on the color palette. The opacity can be a bit weaker
+    // than for icon-buttons, because normal and stroked buttons have a focus overlay.
+    @include _mat-button-ripple-color($theme, default);
   }
 
-  .mat-flat-button {
-    // Default properties when not using any [color] value.
+  .mat-flat-button, .mat-raised-button, .mat-fab, .mat-mini-fab {
+    // Default font and background color when not using any color palette.
     color: mat-color($foreground, text);
-
     background-color: mat-color($background, raised-button);
-    @include _mat-button-theme-color($theme, 'color', default-contrast);
-    @include _mat-button-theme-color($theme, 'background-color');
 
-    // Add ripple effect with contrast color to buttons that don't have a focus overlay.
+    @include _mat-button-theme-property($theme, 'color', default-contrast);
+    @include _mat-button-theme-property($theme, 'background-color', default);
     @include _mat-button-ripple-color($theme, default-contrast);
   }
 
-  // Add ripple effect with default color to the icon button. Ripple color needs to be stronger
-  // since the icon button doesn't have a focus overlay.
+  // Since icon buttons don't have a focus overlay, the ripple opacity should be the higher
+  // than the default value.
   .mat-icon-button {
-    @include _mat-button-ripple-color($theme, default);
+    @include _mat-button-ripple-color($theme, default, 0.2);
   }
 }
 
 @mixin mat-button-typography($config) {
-  .mat-button, .mat-raised-button, .mat-icon-button, .mat-stroked-button, .mat-flat-button,
-  .mat-fab, .mat-mini-fab {
+  .mat-button, .mat-raised-button, .mat-icon-button, .mat-stroked-button,
+  .mat-flat-button, .mat-fab, .mat-mini-fab {
     font: {
       family: mat-font-family($config, button);
       size: mat-font-size($config, button);

diff --git a/src/lib/button/button.scss b/src/lib/button/button.scss
@@ -67,27 +67,6 @@
   }
 }
 
-// Align icon-buttons correctly inside of standard, fill, and outline form-field appearances.
-.mat-form-field:not(.mat-form-field-appearance-legacy) {
-  .mat-form-field-prefix,
-  .mat-form-field-suffix {
-    .mat-icon-button {
-      display: block;
-      font-size: inherit;
-      width: 2.5em;
-      height: 2.5em;
-    }
-  }
-}
-
-// The text and icon should be vertical aligned inside a button
-.mat-button, .mat-raised-button, .mat-icon-button, .mat-fab, .mat-mini-fab {
-  color: currentColor;
-  .mat-button-wrapper > * {
-    vertical-align: middle;
-  }
-}
-
 // The ripple container should match the bounds of the entire button.
 .mat-button-ripple, .mat-button-focus-overlay {
   @include mat-fill;
@@ -124,6 +103,27 @@
   z-index: 1;
 }
 
+// Elements inside of all type of buttons should be vertical aligned in the middle.
+.mat-button, .mat-flat-button, .mat-stroked-button, .mat-raised-button, .mat-icon-button,
+.mat-fab, .mat-mini-fab {
+  .mat-button-wrapper > * {
+    vertical-align: middle;
+  }
+}
+
+// Align icon-buttons correctly inside of standard, fill, and outline form-field appearances.
+.mat-form-field:not(.mat-form-field-appearance-legacy) {
+  .mat-form-field-prefix,
+  .mat-form-field-suffix {
+    .mat-icon-button {
+      display: block;
+      font-size: inherit;
+      width: 2.5em;
+      height: 2.5em;
+    }
+  }
+}
+
 // Add an outline to make buttons more visible in high contrast mode. Stroked buttons
 // don't need a special look in high-contrast mode, because those already have an outline.
 @include cdk-high-contrast {

diff --git a/src/lib/button/button.spec.ts b/src/lib/button/button.spec.ts
@@ -2,7 +2,7 @@ import {async, ComponentFixture, TestBed} from '@angular/core/testing';
 import {Component, DebugElement} from '@angular/core';
 import {By} from '@angular/platform-browser';
 import {MatButtonModule, MatButton} from './index';
-import {MatRipple} from '@angular/material/core';
+import {MatRipple, ThemePalette} from '@angular/material/core';
 
 
 describe('MatButton', () => {
@@ -41,6 +41,30 @@ describe('MatButton', () => {
     expect(aDebugElement.nativeElement.classList).not.toContain('mat-accent');
   });
 
+  it('should mark buttons without a background color and theme as plain buttons', () => {
+    const fixture = TestBed.createComponent(TestApp);
+    const buttonDebugEl = fixture.debugElement.query(By.css('button'));
+    const anchorDebugEl = fixture.debugElement.query(By.css('a'));
+    const fabDebugEl = fixture.debugElement.query(By.css('[mat-fab]'));
+
+    fixture.detectChanges();
+
+    // Buttons that have no background color and theme palette are considered as plain buttons.
+    expect(buttonDebugEl.nativeElement.classList).toContain('mat-button-plain');
+    expect(anchorDebugEl.nativeElement.classList).toContain('mat-button-plain');
+    expect(fabDebugEl.nativeElement.classList).not.toContain('mat-button-plain');
+
+    fixture.componentInstance.buttonColor = 'primary';
+    fixture.detectChanges();
+
+    // Buttons that have no background color, but use an explicit theme palette, are not
+    // considered as plain buttons.
+    expect(buttonDebugEl.nativeElement.classList).not.toContain('mat-button-plain');
+    expect(anchorDebugEl.nativeElement.classList).not.toContain('mat-button-plain');
+    expect(fabDebugEl.nativeElement.classList).not.toContain('mat-button-plain');
+  });
+
+
   it('should expose the ripple instance', () => {
     const fixture = TestBed.createComponent(TestApp);
     const button = fixture.debugElement.query(By.css('button')).componentInstance as MatButton;
@@ -259,6 +283,7 @@ class TestApp {
   clickCount: number = 0;
   isDisabled: boolean = false;
   rippleDisabled: boolean = false;
+  buttonColor: ThemePalette;
 
   increment() {
     this.clickCount++;

diff --git a/src/lib/button/button.ts b/src/lib/button/button.ts
@@ -86,6 +86,11 @@ export class MatButton extends _MatButtonMixinBase
   @ViewChild(MatRipple) ripple: MatRipple;
 
   constructor(elementRef: ElementRef,
+              /**
+               * @deprecated Platform checks for SSR are no longer needed
+               * @deletion-target 7.0.0
+               */
+              // tslint:disable-next-line:no-unused-variable
               private _platform: Platform,
               private _focusMonitor: FocusMonitor) {
     super(elementRef);
@@ -126,13 +131,6 @@ export class MatButton extends _MatButtonMixinBase
 
   /** Gets whether the button has one of the given attributes. */
   _hasHostAttributes(...attributes: string[]) {
-    // If not on the browser, say that there are none of the attributes present.
-    // Since these only affect how the ripple displays (and ripples only happen on the client),
-    // detecting these attributes isn't necessary when not on the browser.
-    if (!this._platform.isBrowser) {
-      return false;
-    }
-
     return attributes.some(attribute => this._getHostElement().hasAttribute(attribute));
   }
 }
@@ -159,10 +157,8 @@ export class MatButton extends _MatButtonMixinBase
   changeDetection: ChangeDetectionStrategy.OnPush,
 })
 export class MatAnchor extends MatButton {
-  constructor(
-      platform: Platform,
-      focusMonitor: FocusMonitor,
-      elementRef: ElementRef) {
+
+  constructor(platform: Platform, focusMonitor: FocusMonitor, elementRef: ElementRef) {
     super(elementRef, platform, focusMonitor);
   }