From 32b234e24b9dad83920b916776eb55adf3a7add7 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 10 Feb 2018 12:14:27 +0100 Subject: [PATCH] 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 #4614. Fixes #9231. Fixes #9634 --- src/demo-app/button/button-demo.html | 2 +- src/demo-app/toolbar/toolbar-demo.html | 43 +++++++++++++----- src/demo-app/toolbar/toolbar-demo.scss | 3 ++ src/lib/button/_button-theme.scss | 60 +++++++++++--------------- src/lib/button/button.scss | 42 +++++++++--------- src/lib/button/button.spec.ts | 3 +- src/lib/button/button.ts | 18 +++----- 7 files changed, 91 insertions(+), 80 deletions(-) diff --git a/src/demo-app/button/button-demo.html b/src/demo-app/button/button-demo.html index 2e260122533f..fcc074bfa143 100644 --- a/src/demo-app/button/button-demo.html +++ b/src/demo-app/button/button-demo.html @@ -1,7 +1,7 @@

Buttons

- + diff --git a/src/demo-app/toolbar/toolbar-demo.html b/src/demo-app/toolbar/toolbar-demo.html index fa7f900c945d..3e2ec07f5cd8 100644 --- a/src/demo-app/toolbar/toolbar-demo.html +++ b/src/demo-app/toolbar/toolbar-demo.html @@ -1,40 +1,61 @@
-

- menu - Default Toolbar + + Default Toolbar - code + + + +

- menu - Primary Toolbar + + Primary Toolbar - code + + +

- menu - Accent Toolbar + + Accent Toolbar - code + + + +

- + First Row Second Row diff --git a/src/demo-app/toolbar/toolbar-demo.scss b/src/demo-app/toolbar/toolbar-demo.scss index a433f45b5cb9..fa679ae89f1f 100644 --- 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 index c3d961da123a..bc21f0c91f33 100644 --- 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,7 +43,7 @@ } // 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); @@ -51,13 +51,13 @@ $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 index 03bcb51a987f..4b3c284bc237 100644 --- 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 index 846f89563dbd..d372b982c36e 100644 --- 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', () => { @@ -259,6 +259,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 index feb8840dbba0..ee52cc5c6229 100644 --- 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); }