fixup! perf(ivy): apply [style]/[class] bindings directly to style/cl…

…assName

packages/common/src/directives/ng_class.ts

@@ -34,7 +34,7 @@ export const ngClassDirectiveDef__POST_R3__ = ɵɵdefineDirective({
   selectors: null as any,
   hostBindings: function(rf: ɵRenderFlags, ctx: any, elIndex: number) {
     if (rf & ɵRenderFlags.Create) {
-      ɵɵallocHostVars(1);
+      ɵɵallocHostVars(2);
     }
     if (rf & ɵRenderFlags.Update) {
       ɵɵclassMap(ctx.getValue());

packages/common/src/directives/ng_style.ts

@@ -35,7 +35,7 @@ export const ngStyleDirectiveDef__POST_R3__ = ɵɵdefineDirective({
   selectors: null as any,
   hostBindings: function(rf: ɵRenderFlags, ctx: any, elIndex: number) {
     if (rf & ɵRenderFlags.Create) {
-      ɵɵallocHostVars(1);
+      ɵɵallocHostVars(2);
     }
     if (rf & ɵRenderFlags.Update) {
       ɵɵstyleMap(ctx.getValue());

packages/core/src/render3/instructions/styling.ts

@@ -588,5 +588,7 @@ function isHostStyling(): boolean {
 function getAndIncrementBindingIndex(lView: LView, isMapBased: boolean): number {
   // map-based bindings use two slots because the previously constructed
   // className / style value must be compared against.
-  return lView[BINDING_INDEX] += isMapBased ? 2 : 1;
+  const increment = isMapBased ? 2 : 1;
+  const index = lView[BINDING_INDEX] += increment;
+  return index - increment;
 }

packages/core/src/render3/styling/bindings.ts

@@ -10,11 +10,11 @@ import {StyleSanitizeFn, StyleSanitizeMode} from '../../sanitization/style_sanit
 import {ProceduralRenderer3, RElement, Renderer3, RendererStyleFlags3, isProceduralRenderer} from '../interfaces/renderer';
 import {ApplyStylingFn, LStylingData, StylingMapArray, StylingMapArrayIndex, StylingMapsSyncMode, SyncStylingMapsFn, TStylingConfig, TStylingContext, TStylingContextIndex, TStylingContextPropConfigFlags} from '../interfaces/styling';
 import {NO_CHANGE} from '../tokens';
-import {DEFAULT_BINDING_INDEX, DEFAULT_BINDING_VALUE, DEFAULT_GUARD_MASK_VALUE, MAP_BASED_ENTRY_PROP_NAME, TEMPLATE_DIRECTIVE_INDEX, forceStylesAsString, getBindingValue, getConfig, getDefaultValue, getGuardMask, getInitialStylingValue, getMapProp, getMapValue, getProp, getPropValuesStartPosition, getStylingMapArray, getTotalSources, getValue, getValuesCount, hasConfig, hasValueChanged, isContextLocked, isHostStylingActive, isSanitizationRequired, isStylingValueDefined, lockContext, normalizeIntoStylingMap, patchConfig, setDefaultValue, setGuardMask, setMapAsDirty, setValue} from '../util/styling_utils';
+import {DEFAULT_BINDING_INDEX, DEFAULT_BINDING_VALUE, DEFAULT_GUARD_MASK_VALUE, MAP_BASED_ENTRY_PROP_NAME, TEMPLATE_DIRECTIVE_INDEX, concatString, forceStylesAsString, getBindingValue, getConfig, getDefaultValue, getGuardMask, getInitialStylingValue, getMapProp, getMapValue, getProp, getPropValuesStartPosition, getStylingMapArray, getTotalSources, getValue, getValuesCount, hasConfig, hasValueChanged, isContextLocked, isHostStylingActive, isSanitizationRequired, isStylingMapArray, isStylingValueDefined, lockContext, normalizeIntoStylingMap, patchConfig, setDefaultValue, setGuardMask, setMapAsDirty, setValue} from '../util/styling_utils';
 
 import {getStylingState, resetStylingState} from './state';
 
-
+const VALUE_IS_EXTERNALLY_MODIFIED = {};
 
 /**
  * --------
@@ -658,36 +658,65 @@ export function applyStylingMapDirectly(
     bindingIndex: number, value: {[key: string]: any} | string | null, isClassBased: boolean,
     sanitizer?: StyleSanitizeFn | null, forceUpdate?: boolean,
     bindingValueContainsInitial?: boolean): void {
-  const config = getConfig(context);
-  const writeToAttrDirectly = !(config & TStylingConfig.HasPropBindings);
   const oldValue = getValue(data, bindingIndex);
-  if (!writeToAttrDirectly && value !== NO_CHANGE) {
-    value = normalizeIntoStylingMap(oldValue, value, !isClassBased);
-  }
-
   if (forceUpdate || hasValueChanged(oldValue, value)) {
+    const config = getConfig(context);
     const hasInitial = config & TStylingConfig.HasInitialStyling;
+    const initialValue =
+        hasInitial && !bindingValueContainsInitial ? getInitialStylingValue(context) : null;
     setValue(data, bindingIndex, value);
 
+    // the cached value is the last snapshot of the style or class
+    // attribute value and is used in the if statement below to
+    // keep track of internal/external changes.
+    const cachedValueIndex = bindingIndex + 1;
+    let cachedValue = getValue(data, cachedValueIndex);
+    debugger;
+    if (cachedValue === NO_CHANGE) {
+      cachedValue = initialValue;
+    }
+
+    // If a class/style value was modified externally then the styling
+    // fast pass cannot guarantee that the external values are retained.
+    // When this happens, the algorithm will bail out and not write to
+    // the style or className attribute directly.
+    let writeToAttrDirectly = !(config & TStylingConfig.HasPropBindings);
+    if (writeToAttrDirectly &&
+        checkExternalAccessAndFlag(element as HTMLElement, cachedValue, isClassBased)) {
+      writeToAttrDirectly = false;
+      if (oldValue !== VALUE_IS_EXTERNALLY_MODIFIED) {
+        // direct styling will reset the attribute entirely each time,
+        // and, for this reason, if the algorithm decides it cannot
+        // write to the class/style attributes directly then it must
+        // reset all the previous style/class values before it starts
+        // to apply values in the non-direct way.
+        removeDirectStyling(renderer, element, oldValue, isClassBased);
+
+        // this will instruct the algorithm not to apply class or style
+        // values directly anymore.
+        setValue(data, cachedValueIndex, VALUE_IS_EXTERNALLY_MODIFIED);
+      }
+    }
+
     if (writeToAttrDirectly) {
-      const initialValue =
-          hasInitial && !bindingValueContainsInitial ? getInitialStylingValue(context) : null;
+      let valueToApply: string;
       if (isClassBased) {
-        let valueToApply = typeof value === 'string' ? value : objectToClassName(value);
+        valueToApply = typeof value === 'string' ? value : objectToClassName(value);
         if (initialValue !== null) {
-          valueToApply = initialValue + ' ' + valueToApply;
+          valueToApply = concatString(initialValue, valueToApply, ' ');
         }
         setClassName(renderer, element, valueToApply);
       } else {
-        let valueToApply = forceStylesAsString(value as{[key: string]: any}, true);
+        valueToApply = forceStylesAsString(value as{[key: string]: any}, true);
         if (initialValue !== null) {
           valueToApply = initialValue + ';' + valueToApply;
         }
         setStyleAttr(renderer, element, valueToApply);
       }
+      setValue(data, cachedValueIndex, valueToApply);
     } else {
       const applyFn = isClassBased ? setClass : setStyle;
-      const map = value as StylingMapArray;
+      const map = normalizeIntoStylingMap(oldValue, value, !isClassBased);
       const initialStyles = hasInitial ? getStylingMapArray(context) : null;
 
       for (let i = StylingMapArrayIndex.ValuesStartPosition; i < map.length;
@@ -974,3 +1003,54 @@ function objectToClassName(obj: {[key: string]: any} | null): string {
   }
   return str;
 }
+
+function diffNewCustomClasses(
+    className: string, initialClasses: string | null, oldClasses: string | null): string {
+  function replace(p: string) {
+    const exp = new RegExp(`\\s*${p}\\s*`);
+    className = className.replace(exp, '');
+  }
+
+  oldClasses && oldClasses.split(/\s+/).forEach(replace);
+  initialClasses && initialClasses.split(/\s+/).forEach(replace);
+  return className;
+}
+
+/**
+ * Determines whether or not an element style/className value has changed since the last update.
+ *
+ * This function helps Angular determine if a style or class attribute value was
+ * modified by an external plugin or API outside of the style binding code.
+ *
+ * @returns true when the value was modified externally.
+ */
+function checkExternalAccessAndFlag(element: HTMLElement, cachedValue: any, isClassBased: boolean) {
+  // case 1: the value was already flagged as modified
+  if (cachedValue === VALUE_IS_EXTERNALLY_MODIFIED) return true;
+
+  // case 2: check to see if the value differs from the actual DOM value
+  const currentValue =
+      isClassBased ? element.className : (element as HTMLElement).getAttribute('style');
+  return currentValue !== cachedValue;
+}
+
+function removeDirectStyling(
+    renderer: any, element: RElement, values: string | {[key: string]: any} | StylingMapArray,
+    isClassBased: boolean) {
+  let arr: StylingMapArray;
+  if (isStylingMapArray(values)) {
+    arr = values as StylingMapArray;
+  } else {
+    arr = normalizeIntoStylingMap(null, values, !isClassBased);
+  }
+
+  const applyFn = isClassBased ? setClass : setStyle;
+  for (let i = StylingMapArrayIndex.ValuesStartPosition; i < arr.length;
+       i += StylingMapArrayIndex.TupleSize) {
+    const value = getMapValue(arr, i);
+    if (value) {
+      const prop = getMapProp(arr, i);
+      applyFn(renderer, element, prop, false);
+    }
+  }
+}

packages/core/src/render3/styling/styling_debug.ts

@@ -430,7 +430,20 @@ export class NodeStylingDebug implements DebugNodeStyling {
    */
   get values(): {[key: string]: any} {
     const entries: {[key: string]: any} = {};
-    this._mapValues(this._data, (prop: string, value: any) => { entries[prop] = value; });
+    const config = this.config;
+    let data = this._data;
+
+    // the direct pass code doesn't convert [style] or [class] values
+    // into StylingMapArray instances. For this reason, the values
+    // need to be converted ahead of time since the styling debug
+    // relies on context resolution to figure out what styling
+    // values have been added/removed on the element.
+    if (config.allowDirectStyling && config.hasMapBindings) {
+      data = data.concat([]);  // make a copy
+      this._convertMapBindingsToStylingMapArrays(data);
+    }
+
+    this._mapValues(data, (prop: string, value: any) => { entries[prop] = value; });
     return entries;
   }
 

packages/core/src/render3/util/styling_utils.ts

@@ -247,7 +247,7 @@ export function isStylingContext(value: any): boolean {
       typeof value[1] !== 'string';
 }
 
-export function isStylingMapArray(value: TStylingContext | StylingMapArray | null): boolean {
+export function isStylingMapArray(value: any): boolean {
   // the StylingMapArray is in the format of [initial, prop, string, prop, string]
   // and this is the defining value to distinguish between arrays
   return Array.isArray(value) &&
@@ -303,7 +303,10 @@ export function forceStylesAsString(
     for (let i = 0; i < props.length; i++) {
       const prop = props[i];
       const propLabel = hyphenateProps ? hyphenate(prop) : prop;
-      str = concatString(str, `${propLabel}:${styles[prop]}`, ';');
+      const value = styles[prop];
+      if (value !== null) {
+        str = concatString(str, `${propLabel}:${value}`, ';');
+      }
     }
   }
   return str;

packages/core/test/acceptance/i18n_spec.ts

@@ -1268,7 +1268,7 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
     fixture.detectChanges();
     expect(fixture.nativeElement.innerHTML)
         .toEqual(
-            `<div test="" title="début 2 milieu 1 fin" class="foo"> traduction: un <i>email</i><!--ICU 20--> ` +
+            `<div test="" title="début 2 milieu 1 fin" class="foo"> traduction: un <i>email</i><!--ICU 21--> ` +
             `</div><div test="" class="foo"></div>`);
 
     directiveInstances.forEach(instance => instance.klass = 'bar');
@@ -1277,7 +1277,7 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
     fixture.detectChanges();
     expect(fixture.nativeElement.innerHTML)
         .toEqual(
-            `<div test="" title="début 3 milieu 2 fin" class="bar"> traduction: 2 emails<!--ICU 20--> ` +
+            `<div test="" title="début 3 milieu 2 fin" class="bar"> traduction: 2 emails<!--ICU 21--> ` +
             `</div><div test="" class="bar"></div>`);
   });
 

packages/core/test/acceptance/styling_spec.ts

@@ -2309,6 +2309,34 @@ describe('styling', () => {
     expect(fixture.debugElement.nativeElement.innerHTML).toContain('two');
     expect(fixture.debugElement.nativeElement.innerHTML).toContain('three');
   });
+
+  onlyInIvy('only ivy treats [class] in concert with other class bindings')
+      .it('should retain classes added externally', () => {
+        @Component({template: `<div [class]="exp"></div>`})
+        class MyComp {
+          exp = '';
+        }
+
+        const fixture =
+            TestBed.configureTestingModule({declarations: [MyComp]}).createComponent(MyComp);
+        fixture.detectChanges();
+
+        const div = fixture.nativeElement.querySelector('div') !;
+        div.className += ' abc';
+        expect(splitSortJoin(div.className)).toEqual('abc');
+
+        fixture.componentInstance.exp = '1 2 3';
+        fixture.detectChanges();
+
+        expect(splitSortJoin(div.className)).toEqual('1 2 3 abc');
+
+        fixture.componentInstance.exp = '4 5 6 7';
+        fixture.detectChanges();
+
+        expect(splitSortJoin(div.className)).toEqual('4 5 6 7 abc');
+
+        function splitSortJoin(s: string) { return s.split(/\s+/).sort().join(' ').trim(); }
+      });
 });
 
 function assertStyleCounters(countForSet: number, countForRemove: number) {

packages/core/test/bundling/todo/bundle.golden_symbols.json

@@ -596,6 +596,9 @@
   {
     "name": "getActiveDirectiveId"
   },
+  {
+    "name": "getAndIncrementBindingIndex"
+  },
   {
     "name": "getBeforeNodeForView"
   },