fix(core): handle !important in style property value (#39603)

* Fixes that the Ivy styling logic wasn't accounting for `!important` in the property value. * Fixes that the default DOM renderer only sets `!important` on a property with a dash in its name. * Accounts for the `flags` parameter of `setStyle` in the server renderer. Fixes #35323. PR Close #39603
angular · Nov 12, 2020 · 978f081 · 978f081
1 parent 888f0e4
commit 978f081
Show file tree

Hide file tree

Showing 5 changed files with 174 additions and 8 deletions.
diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json
@@ -12,7 +12,7 @@
     "master": {
       "uncompressed": {
         "runtime-es2015": 3037,
-        "main-es2015": 448922,
+        "main-es2015": 449483,
         "polyfills-es2015": 52415
       }
     }

diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts
@@ -987,6 +987,13 @@ function applyContainer(
   }
 }
 
+// TODO(misko): Can't import RendererStyleFlags2.DashCase as it causes imports to be resolved
+// in different order which causes failures. Duplicating for now.
+const enum TempRendererStyleFlags2 {
+  Important = 1 << 0,
+  DashCase = 1 << 1
+}
+
 /**
  * Writes class/style to element.
  *
@@ -1019,9 +1026,7 @@ export function applyStyling(
       }
     }
   } else {
-    // TODO(misko): Can't import RendererStyleFlags2.DashCase as it causes imports to be resolved
-    // in different order which causes failures. Using direct constant as workaround for now.
-    const flags = prop.indexOf('-') == -1 ? undefined : 2 /* RendererStyleFlags2.DashCase */;
+    let flags = prop.indexOf('-') === -1 ? undefined : TempRendererStyleFlags2.DashCase as number;
     if (value == null /** || value === undefined */) {
       ngDevMode && ngDevMode.rendererRemoveStyle++;
       if (isProcedural) {
@@ -1030,12 +1035,22 @@ export function applyStyling(
         (rNode as HTMLElement).style.removeProperty(prop);
       }
     } else {
+      // A value is important if it ends with `!important`. The style
+      // parser strips any semicolons at the end of the value.
+      const isImportant = typeof value === 'string' ? value.endsWith('!important') : false;
+
+      if (isImportant) {
+        // !important has to be stripped from the value for it to be valid.
+        value = value.slice(0, -10);
+        flags! |= TempRendererStyleFlags2.Important;
+      }
+
       ngDevMode && ngDevMode.rendererSetStyle++;
       if (isProcedural) {
         (renderer as Renderer2).setStyle(rNode, prop, value, flags);
       } else {
         ngDevMode && assertDefined((rNode as HTMLElement).style, 'HTMLElement expected');
-        (rNode as HTMLElement).style.setProperty(prop, value);
+        (rNode as HTMLElement).style.setProperty(prop, value, isImportant ? 'important' : '');
       }
     }
   }

diff --git a/packages/core/test/acceptance/styling_spec.ts b/packages/core/test/acceptance/styling_spec.ts
@@ -12,6 +12,7 @@ import {ngDevModeResetPerfCounters} from '@angular/core/src/util/ng_dev_mode';
 import {TestBed} from '@angular/core/testing';
 import {getElementClasses, getElementStyles, getSortedClassName, getSortedStyle} from '@angular/core/testing/src/styling';
 import {By, DomSanitizer, SafeStyle} from '@angular/platform-browser';
+import {browserDetection} from '@angular/platform-browser/testing/src/browser_util';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
 import {ivyEnabled, modifiedInIvy, onlyInIvy} from '@angular/private/testing';
 
@@ -801,6 +802,154 @@ describe('styling', () => {
     });
   });
 
+  onlyInIvy('ViewEngine only supports !important when set through the `style` attribute')
+      .it('should set !important on a single property', () => {
+        // We go through `CSSStyleDeclaration.setProperty` to set `!important`. Values set through
+        // `setProperty` aren't propagated to the `style` attribute on IE11, even though the style
+        // is applied, so we have to skip the test there since we don't have a way of asserting.
+        if (browserDetection.isIE) {
+          return;
+        }
+
+        @Component({template: '<div [style.width]="width"></div>'})
+        class Cmp {
+          width!: string;
+        }
+
+        TestBed.configureTestingModule({declarations: [Cmp]});
+        const fixture = TestBed.createComponent(Cmp);
+        fixture.componentInstance.width = '50px !important';
+        fixture.detectChanges();
+        const html = fixture.nativeElement.innerHTML;
+
+        // We have to check the `style` attribute, because `element.style.prop` doesn't include
+        // `!important`. Use a regex, because the different renderers produce different whitespace.
+        expect(html).toMatch(/style=["|']width:\s*50px\s*!important/);
+
+        onlyInIvy('perf counters').expectPerfCounters({
+          rendererSetStyle: 1,
+          tNode: 2,
+        });
+      });
+
+  onlyInIvy('ViewEngine only supports !important when set through the `style` attribute')
+      .it('should set !important that is not preceded by a space', () => {
+        // We go through `CSSStyleDeclaration.setProperty` to set `!important`. Values set through
+        // `setProperty` aren't propagated to the `style` attribute on IE11, even though the style
+        // is applied, so we have to skip the test there since we don't have a way of asserting.
+        if (browserDetection.isIE) {
+          return;
+        }
+
+        @Component({template: '<div [style.width]="width"></div>'})
+        class Cmp {
+          width!: string;
+        }
+
+        TestBed.configureTestingModule({declarations: [Cmp]});
+        const fixture = TestBed.createComponent(Cmp);
+        fixture.componentInstance.width = '50px!important';
+        fixture.detectChanges();
+        const html = fixture.nativeElement.innerHTML;
+
+        // We have to check the `style` attribute, because `element.style.prop` doesn't include
+        // `!important`. Use a regex, because the different renderers produce different whitespace.
+        expect(html).toMatch(/style=["|']width:\s*50px\s*!important/);
+
+        onlyInIvy('perf counters').expectPerfCounters({
+          rendererSetStyle: 1,
+          tNode: 2,
+        });
+      });
+
+  onlyInIvy('ViewEngine only supports !important when set through the `style` attribute')
+      .it('should set !important on a dash-case property', () => {
+        // We go through `CSSStyleDeclaration.setProperty` to set `!important`. Values set through
+        // `setProperty` aren't propagated to the `style` attribute on IE11, even though the style
+        // is applied, so we have to skip the test there since we don't have a way of asserting.
+        if (browserDetection.isIE) {
+          return;
+        }
+
+        @Component({template: '<div [style.margin-right]="marginRight"></div>'})
+        class Cmp {
+          marginRight!: string;
+        }
+
+        TestBed.configureTestingModule({declarations: [Cmp]});
+        const fixture = TestBed.createComponent(Cmp);
+        fixture.componentInstance.marginRight = '5px !important';
+        fixture.detectChanges();
+        const html = fixture.nativeElement.innerHTML;
+
+        // We have to check the `style` attribute, because `element.style.prop` doesn't include
+        // `!important`. Use a regex, because the different renderers produce different whitespace.
+        expect(html).toMatch(/style=["|']margin-right:\s*5px\s*!important/);
+
+        onlyInIvy('perf counters').expectPerfCounters({
+          rendererSetStyle: 1,
+          tNode: 2,
+        });
+      });
+
+  it('should set !important on multiple properties', () => {
+    // We go through `CSSStyleDeclaration.setProperty` to set `!important`. Values set through
+    // `setProperty` aren't propagated to the `style` attribute on IE11, even though the style is
+    // applied, so we have to skip the test there since we don't have a way of asserting.
+    if (browserDetection.isIE) {
+      return;
+    }
+
+    @Component({template: '<div [style]="styles"></div>'})
+    class Cmp {
+      styles!: string;
+    }
+
+    TestBed.configureTestingModule({declarations: [Cmp]});
+    const fixture = TestBed.createComponent(Cmp);
+    fixture.componentInstance.styles = 'height: 25px !important; width: 50px !important;';
+    fixture.detectChanges();
+    const html = fixture.nativeElement.innerHTML;
+
+    // We have to check the `style` attribute, because `element.style.prop` doesn't include
+    // `!important`. Use a regex, because the different renderers produce different whitespace.
+    expect(html).toMatch(/style=["|']height:\s*25px\s*!important;\s*width:\s*50px\s*!important/);
+
+    onlyInIvy('perf counters').expectPerfCounters({
+      rendererSetStyle: 2,
+      tNode: 2,
+    });
+  });
+
+  it('should set !important if some properties are !important and other are not', () => {
+    // We go through `CSSStyleDeclaration.setProperty` to set `!important`. Values set through
+    // `setProperty` aren't propagated to the `style` attribute on IE11, even though the style is
+    // applied, so we have to skip the test there since we don't have a way of asserting.
+    if (browserDetection.isIE) {
+      return;
+    }
+
+    @Component({template: '<div [style]="styles"></div>'})
+    class Cmp {
+      styles!: string;
+    }
+
+    TestBed.configureTestingModule({declarations: [Cmp]});
+    const fixture = TestBed.createComponent(Cmp);
+    fixture.componentInstance.styles = 'height: 25px; width: 50px !important;';
+    fixture.detectChanges();
+    const html = fixture.nativeElement.innerHTML;
+
+    // We have to check the `style` attribute, because `element.style.prop` doesn't include
+    // `!important`. Use a regex, because the different renderers produce different whitespace.
+    expect(html).toMatch(/style=["|']height:\s*25px;\s*width:\s*50px\s*!important/);
+
+    onlyInIvy('perf counters').expectPerfCounters({
+      rendererSetStyle: 2,
+      tNode: 2,
+    });
+  });
+
   it('should not write to the native element if a directive shadows the class input', () => {
     // This ex is a bit contrived. In real apps, you might have a shared class that is extended
     // both by components with host elements and by directives on template nodes. In that case, the

diff --git a/packages/platform-browser/src/dom/dom_renderer.ts b/packages/platform-browser/src/dom/dom_renderer.ts
@@ -234,9 +234,8 @@ class DefaultDomRenderer2 implements Renderer2 {
   }
 
   setStyle(el: any, style: string, value: any, flags: RendererStyleFlags2): void {
-    if (flags & RendererStyleFlags2.DashCase) {
-      el.style.setProperty(
-          style, value, !!(flags & RendererStyleFlags2.Important) ? 'important' : '');
+    if (flags & (RendererStyleFlags2.DashCase | RendererStyleFlags2.Important)) {
+      el.style.setProperty(style, value, flags & RendererStyleFlags2.Important ? 'important' : '');
     } else {
       el.style[style] = value;
     }

diff --git a/packages/platform-server/src/server_renderer.ts b/packages/platform-server/src/server_renderer.ts
@@ -160,6 +160,9 @@ class DefaultServerRenderer2 implements Renderer2 {
   setStyle(el: any, style: string, value: any, flags: RendererStyleFlags2): void {
     style = style.replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase();
     const styleMap = _readStyleAttribute(el);
+    if (flags & RendererStyleFlags2.Important) {
+      value += ' !important';
+    }
     styleMap[style] = value == null ? '' : value;
     _writeStyleAttribute(el, styleMap);
   }