From dcdb433b7d8b4802412593326764dff9ebe2f201 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Tue, 22 Oct 2019 15:18:40 -0700 Subject: [PATCH] perf(ivy): apply [style]/[class] bindings directly to style/className (#33336) This patch ensures that the `[style]` and `[class]` based bindings are directly applied to an element's style and className attributes. This patch optimizes the algorithm so that it... - Doesn't construct an update an instance of `StylingMapArray` for `[style]` and `[class]` bindings - Doesn't apply `[style]` and `[class]` based entries using `classList` and `style` (direct attributes are used instead) - Doesn't split or iterate over all string-based tokens in a string value obtained from a `[class]` binding. This patch speeds up the following cases: - `
` and `
` - `
` and `
` The overall speec increase is by over 5x. PR Close #33336 --- packages/common/src/directives/ng_class.ts | 2 +- packages/common/src/directives/ng_style.ts | 2 +- .../r3_view_compiler_styling_spec.ts | 32 +-- .../src/render3/view/styling_builder.ts | 5 +- .../core/src/render3/instructions/styling.ts | 74 ++++--- packages/core/src/render3/styling/bindings.ts | 203 +++++++++++++++--- .../core/src/render3/styling/styling_debug.ts | 87 +++++++- .../core/src/render3/util/styling_utils.ts | 13 +- packages/core/test/acceptance/i18n_spec.ts | 4 +- packages/core/test/acceptance/styling_spec.ts | 28 +++ .../bundling/todo/bundle.golden_symbols.json | 3 + .../core/test/render3/perf/noop_renderer.ts | 3 + .../styling_next/styling_debug_spec.ts | 7 + 13 files changed, 360 insertions(+), 103 deletions(-) diff --git a/packages/common/src/directives/ng_class.ts b/packages/common/src/directives/ng_class.ts index a160ac95faf86..fe6b8e6ff44cd 100644 --- a/packages/common/src/directives/ng_class.ts +++ b/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()); diff --git a/packages/common/src/directives/ng_style.ts b/packages/common/src/directives/ng_style.ts index 6d728f1b5674f..97670a906ef15 100644 --- a/packages/common/src/directives/ng_style.ts +++ b/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()); diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts index cf8c3d99afa61..5ae7759b70389 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts @@ -436,7 +436,7 @@ describe('compiler compliance: styling', () => { const template = ` … decls: 1, - vars: 2, + vars: 3, template: function MyComponentWithInterpolation_Template(rf, $ctx$) { if (rf & 1) { $r3$.ɵɵelement(0, "div"); @@ -447,7 +447,7 @@ describe('compiler compliance: styling', () => { } … decls: 1, - vars: 3, + vars: 4, template: function MyComponentWithMuchosInterpolation_Template(rf, $ctx$) { if (rf & 1) { $r3$.ɵɵelement(0, "div"); @@ -458,7 +458,7 @@ describe('compiler compliance: styling', () => { } … decls: 1, - vars: 1, + vars: 2, template: function MyComponentWithoutInterpolation_Template(rf, $ctx$) { if (rf & 1) { $r3$.ɵɵelement(0, "div"); @@ -506,7 +506,7 @@ describe('compiler compliance: styling', () => { type: MyComponent, selectors:[["my-component"]], decls: 1, - vars: 4, + vars: 5, consts: [[${AttributeMarker.Styles}, "opacity", "1"]], template: function MyComponent_Template(rf, $ctx$) { if (rf & 1) { @@ -700,7 +700,7 @@ describe('compiler compliance: styling', () => { type: MyComponent, selectors:[["my-component"]], decls: 1, - vars: 4, + vars: 5, consts: [[${AttributeMarker.Classes}, "grape"]], template: function MyComponent_Template(rf, $ctx$) { if (rf & 1) { @@ -863,8 +863,8 @@ describe('compiler compliance: styling', () => { } if (rf & 2) { $r3$.ɵɵstyleSanitizer($r3$.ɵɵdefaultStyleSanitizer); - $r3$.ɵɵstyleMap($r3$.ɵɵpipeBind1(1, 2, $ctx$.myStyleExp)); - $r3$.ɵɵclassMap($r3$.ɵɵpipeBind1(2, 4, $ctx$.myClassExp)); + $r3$.ɵɵstyleMap($r3$.ɵɵpipeBind1(1, 4, $ctx$.myStyleExp)); + $r3$.ɵɵclassMap($r3$.ɵɵpipeBind1(2, 6, $ctx$.myClassExp)); } } `; @@ -916,11 +916,11 @@ describe('compiler compliance: styling', () => { } if (rf & 2) { $r3$.ɵɵstyleSanitizer($r3$.ɵɵdefaultStyleSanitizer); - $r3$.ɵɵstyleMap($r3$.ɵɵpipeBind2(1, 6, $ctx$.myStyleExp, 1000)); + $r3$.ɵɵstyleMap($r3$.ɵɵpipeBind2(1, 8, $ctx$.myStyleExp, 1000)); $r3$.ɵɵclassMap($e2_styling$); - $r3$.ɵɵstyleProp("bar", $r3$.ɵɵpipeBind2(2, 9, $ctx$.barExp, 3000)); - $r3$.ɵɵstyleProp("baz", $r3$.ɵɵpipeBind2(3, 12, $ctx$.bazExp, 4000)); - $r3$.ɵɵclassProp("foo", $r3$.ɵɵpipeBind2(4, 15, $ctx$.fooExp, 2000)); + $r3$.ɵɵstyleProp("bar", $r3$.ɵɵpipeBind2(2, 11, $ctx$.barExp, 3000)); + $r3$.ɵɵstyleProp("baz", $r3$.ɵɵpipeBind2(3, 14, $ctx$.bazExp, 4000)); + $r3$.ɵɵclassProp("foo", $r3$.ɵɵpipeBind2(4, 17, $ctx$.fooExp, 2000)); $r3$.ɵɵadvance(5); $r3$.ɵɵtextInterpolate1(" ", $ctx$.item, ""); } @@ -1018,7 +1018,7 @@ describe('compiler compliance: styling', () => { const template = ` hostBindings: function MyComponent_HostBindings(rf, ctx, elIndex) { if (rf & 1) { - $r3$.ɵɵallocHostVars(4); + $r3$.ɵɵallocHostVars(6); $r3$.ɵɵelementHostAttrs($e0_attrs$); } if (rf & 2) { @@ -1077,7 +1077,7 @@ describe('compiler compliance: styling', () => { const template = ` hostBindings: function MyComponent_HostBindings(rf, ctx, elIndex) { if (rf & 1) { - $r3$.ɵɵallocHostVars(6); + $r3$.ɵɵallocHostVars(8); } if (rf & 2) { $r3$.ɵɵstyleSanitizer($r3$.ɵɵdefaultStyleSanitizer); @@ -1152,7 +1152,7 @@ describe('compiler compliance: styling', () => { const hostBindings = ` hostBindings: function MyComponent_HostBindings(rf, ctx, elIndex) { if (rf & 1) { - $r3$.ɵɵallocHostVars(4); + $r3$.ɵɵallocHostVars(6); } if (rf & 2) { $r3$.ɵɵstyleSanitizer($r3$.ɵɵdefaultStyleSanitizer); @@ -1218,7 +1218,7 @@ describe('compiler compliance: styling', () => { const template = ` function ClassDirective_HostBindings(rf, ctx, elIndex) { if (rf & 1) { - $r3$.ɵɵallocHostVars(1); + $r3$.ɵɵallocHostVars(2); } if (rf & 2) { $r3$.ɵɵclassMap(ctx.myClassMap); @@ -1506,7 +1506,7 @@ describe('compiler compliance: styling', () => { … hostBindings: function MyComponent_HostBindings(rf, ctx, elIndex) { if (rf & 1) { - $r3$.ɵɵallocHostVars(4); + $r3$.ɵɵallocHostVars(6); $r3$.ɵɵelementHostAttrs($_c0$); } if (rf & 2) { diff --git a/packages/compiler/src/render3/view/styling_builder.ts b/packages/compiler/src/render3/view/styling_builder.ts index 8eb0b566d3331..65af1e9171b07 100644 --- a/packages/compiler/src/render3/view/styling_builder.ts +++ b/packages/compiler/src/render3/view/styling_builder.ts @@ -326,7 +326,10 @@ export class StylingBuilder { valueConverter: ValueConverter, isClassBased: boolean, stylingInput: BoundStylingEntry): StylingInstruction { // each styling binding value is stored in the LView - let totalBindingSlotsRequired = 1; + // map-based bindings allocate two slots: one for the + // previous binding value and another for the previous + // className or style attribute value. + let totalBindingSlotsRequired = 2; // these values must be outside of the update block so that they can // be evaluated (the AST visit call) during creation time so that any diff --git a/packages/core/src/render3/instructions/styling.ts b/packages/core/src/render3/instructions/styling.ts index 168de69f36bd0..8526a243ae551 100644 --- a/packages/core/src/render3/instructions/styling.ts +++ b/packages/core/src/render3/instructions/styling.ts @@ -96,7 +96,7 @@ export function stylePropInternal( // in this case we do not need to do anything, but the binding index // still needs to be incremented because all styling binding values // are stored inside of the lView. - const bindingIndex = lView[BINDING_INDEX]++; + const bindingIndex = getAndIncrementBindingIndex(lView, false); const updated = stylingProp(elementIndex, bindingIndex, prop, resolveStylePropValue(value, suffix), false); @@ -130,7 +130,7 @@ export function ɵɵclassProp(className: string, value: boolean | null): void { // in this case we do not need to do anything, but the binding index // still needs to be incremented because all styling binding values // are stored inside of the lView. - const bindingIndex = lView[BINDING_INDEX]++; + const bindingIndex = getAndIncrementBindingIndex(lView, false); const updated = stylingProp(getSelectedIndex(), bindingIndex, className, value, true); if (ngDevMode) { @@ -189,8 +189,7 @@ function stylingProp( const sanitizerToUse = isClassBased ? null : sanitizer; const renderer = getRenderer(tNode, lView); updated = applyStylingValueDirectly( - renderer, context, native, lView, bindingIndex, prop, value, isClassBased, - isClassBased ? setClass : setStyle, sanitizerToUse); + renderer, context, native, lView, bindingIndex, prop, value, isClassBased, sanitizerToUse); if (sanitizerToUse) { // it's important we remove the current style sanitizer once the @@ -243,28 +242,23 @@ export function ɵɵstyleMap(styles: {[styleName: string]: any} | NO_CHANGE | nu const lView = getLView(); const tNode = getTNode(index, lView); const context = getStylesContext(tNode); + const hasDirectiveInput = hasStyleInput(tNode); // if a value is interpolated then it may render a `NO_CHANGE` value. // in this case we do not need to do anything, but the binding index // still needs to be incremented because all styling binding values // are stored inside of the lView. - const bindingIndex = lView[BINDING_INDEX]++; + const bindingIndex = getAndIncrementBindingIndex(lView, true); // inputs are only evaluated from a template binding into a directive, therefore, // there should not be a situation where a directive host bindings function // evaluates the inputs (this should only happen in the template function) - if (!isHostStyling() && hasStyleInput(tNode) && styles !== NO_CHANGE) { + if (!isHostStyling() && hasDirectiveInput && styles !== NO_CHANGE) { updateDirectiveInputValue(context, lView, tNode, bindingIndex, styles, false); styles = NO_CHANGE; } - const updated = stylingMap(index, context, bindingIndex, styles, false); - if (ngDevMode) { - ngDevMode.styleMap++; - if (updated) { - ngDevMode.styleMapCacheMiss++; - } - } + stylingMap(context, tNode, lView, bindingIndex, styles, false, hasDirectiveInput); } /** @@ -300,28 +294,23 @@ export function classMapInternal( const lView = getLView(); const tNode = getTNode(elementIndex, lView); const context = getClassesContext(tNode); + const hasDirectiveInput = hasClassInput(tNode); // if a value is interpolated then it may render a `NO_CHANGE` value. // in this case we do not need to do anything, but the binding index // still needs to be incremented because all styling binding values // are stored inside of the lView. - const bindingIndex = lView[BINDING_INDEX]++; + const bindingIndex = getAndIncrementBindingIndex(lView, true); // inputs are only evaluated from a template binding into a directive, therefore, // there should not be a situation where a directive host bindings function // evaluates the inputs (this should only happen in the template function) - if (!isHostStyling() && hasClassInput(tNode) && classes !== NO_CHANGE) { + if (!isHostStyling() && hasDirectiveInput && classes !== NO_CHANGE) { updateDirectiveInputValue(context, lView, tNode, bindingIndex, classes, true); classes = NO_CHANGE; } - const updated = stylingMap(elementIndex, context, bindingIndex, classes, true); - if (ngDevMode) { - ngDevMode.classMap++; - if (updated) { - ngDevMode.classMapCacheMiss++; - } - } + stylingMap(context, tNode, lView, bindingIndex, classes, true, hasDirectiveInput); } /** @@ -331,13 +320,10 @@ export function classMapInternal( * `[class]` bindings in Angular. */ function stylingMap( - elementIndex: number, context: TStylingContext, bindingIndex: number, - value: {[key: string]: any} | string | null, isClassBased: boolean): boolean { - let updated = false; - - const lView = getLView(); + context: TStylingContext, tNode: TNode, lView: LView, bindingIndex: number, + value: {[key: string]: any} | string | null, isClassBased: boolean, + hasDirectiveInput: boolean): void { const directiveIndex = getActiveDirectiveId(); - const tNode = getTNode(elementIndex, lView); const native = getNativeByTNode(tNode, lView) as RElement; const oldValue = getValue(lView, bindingIndex); const hostBindingsMode = isHostStyling(); @@ -359,17 +345,14 @@ function stylingMap( patchConfig(context, TStylingConfig.HasMapBindings); } - const stylingMapArr = - value === NO_CHANGE ? NO_CHANGE : normalizeIntoStylingMap(oldValue, value, !isClassBased); - // Direct Apply Case: bypass context resolution and apply the // style/class map values directly to the element if (allowDirectStyling(context, hostBindingsMode)) { const sanitizerToUse = isClassBased ? null : sanitizer; const renderer = getRenderer(tNode, lView); - updated = applyStylingMapDirectly( - renderer, context, native, lView, bindingIndex, stylingMapArr as StylingMapArray, - isClassBased, isClassBased ? setClass : setStyle, sanitizerToUse, valueHasChanged); + applyStylingMapDirectly( + renderer, context, native, lView, bindingIndex, value, isClassBased, sanitizerToUse, + valueHasChanged, hasDirectiveInput); if (sanitizerToUse) { // it's important we remove the current style sanitizer once the // element exits, otherwise it will be used by the next styling @@ -377,7 +360,9 @@ function stylingMap( setElementExitFn(stylingApply); } } else { - updated = valueHasChanged; + const stylingMapArr = + value === NO_CHANGE ? NO_CHANGE : normalizeIntoStylingMap(oldValue, value, !isClassBased); + activateStylingMapFeature(); // Context Resolution (or first update) Case: save the map value @@ -396,7 +381,12 @@ function stylingMap( setElementExitFn(stylingApply); } - return updated; + if (ngDevMode) { + isClassBased ? ngDevMode.classMap : ngDevMode.styleMap++; + if (valueHasChanged) { + isClassBased ? ngDevMode.classMapCacheMiss : ngDevMode.styleMapCacheMiss++; + } + } } /** @@ -452,8 +442,8 @@ function normalizeStylingDirectiveInputValue( value = concatString(initialValue, forceClassesAsString(bindingValue)); } else { value = concatString( - initialValue, forceStylesAsString(bindingValue as{[key: string]: any} | null | undefined), - ';'); + initialValue, + forceStylesAsString(bindingValue as{[key: string]: any} | null | undefined, true), ';'); } } return value; @@ -594,3 +584,11 @@ function resolveStylePropValue( function isHostStyling(): boolean { return isHostStylingActive(getActiveDirectiveId()); } + +function getAndIncrementBindingIndex(lView: LView, isMapBased: boolean): number { + // map-based bindings use two slots because the previously constructed + // className / style value must be compared against. + const index = lView[BINDING_INDEX]; + lView[BINDING_INDEX] += isMapBased ? 2 : 1; + return index; +} diff --git a/packages/core/src/render3/styling/bindings.ts b/packages/core/src/render3/styling/bindings.ts index 8d9453e23767f..0ce5ae6084770 100644 --- a/packages/core/src/render3/styling/bindings.ts +++ b/packages/core/src/render3/styling/bindings.ts @@ -7,14 +7,15 @@ */ import {SafeValue, unwrapSafeValue} from '../../sanitization/bypass'; import {StyleSanitizeFn, StyleSanitizeMode} from '../../sanitization/style_sanitizer'; +import {global} from '../../util/global'; 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, getBindingValue, getConfig, getDefaultValue, getGuardMask, getInitialStylingValue, getMapProp, getMapValue, getProp, getPropValuesStartPosition, getStylingMapArray, getTotalSources, getValue, getValuesCount, hasConfig, hasValueChanged, isContextLocked, isHostStylingActive, isSanitizationRequired, isStylingValueDefined, lockContext, 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 = {}; /** * -------- @@ -655,44 +656,99 @@ export function applyStylingViaContext( */ export function applyStylingMapDirectly( renderer: any, context: TStylingContext, element: RElement, data: LStylingData, - bindingIndex: number, map: StylingMapArray, isClassBased: boolean, applyFn: ApplyStylingFn, - sanitizer?: StyleSanitizeFn | null, forceUpdate?: boolean): boolean { - if (forceUpdate || hasValueChanged(data[bindingIndex], map)) { - setValue(data, bindingIndex, map); - const initialStyles = - hasConfig(context, TStylingConfig.HasInitialStyling) ? getStylingMapArray(context) : null; - - for (let i = StylingMapArrayIndex.ValuesStartPosition; i < map.length; - i += StylingMapArrayIndex.TupleSize) { - const prop = getMapProp(map, i); - const value = getMapValue(map, i); + bindingIndex: number, value: {[key: string]: any} | string | null, isClassBased: boolean, + sanitizer?: StyleSanitizeFn | null, forceUpdate?: boolean, + bindingValueContainsInitial?: boolean): void { + const oldValue = getValue(data, bindingIndex); + 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); - // case 1: apply the map value (if it exists) - let applied = - applyStylingValue(renderer, element, prop, value, applyFn, bindingIndex, sanitizer); + // 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); + if (cachedValue === NO_CHANGE) { + cachedValue = initialValue; + } + cachedValue = typeof cachedValue !== 'string' ? '' : cachedValue; + + // 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 && + checkIfExternallyModified(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. + removeStylingValues(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); + } + } - // case 2: apply the initial value (if it exists) - if (!applied && initialStyles) { - applied = findAndApplyMapValue( - renderer, element, applyFn, initialStyles, prop, bindingIndex, sanitizer); + if (writeToAttrDirectly) { + let valueToApply: string; + if (isClassBased) { + valueToApply = typeof value === 'string' ? value : objectToClassName(value); + if (initialValue !== null) { + valueToApply = concatString(initialValue, valueToApply, ' '); + } + setClassName(renderer, element, valueToApply); + } else { + valueToApply = forceStylesAsString(value as{[key: string]: any}, true); + if (initialValue !== null) { + valueToApply = initialValue + ';' + valueToApply; + } + setStyleAttr(renderer, element, valueToApply); } + setValue(data, cachedValueIndex, valueToApply || null); + } else { + const applyFn = isClassBased ? setClass : setStyle; + const map = normalizeIntoStylingMap(oldValue, value, !isClassBased); + const initialStyles = hasInitial ? getStylingMapArray(context) : null; + + for (let i = StylingMapArrayIndex.ValuesStartPosition; i < map.length; + i += StylingMapArrayIndex.TupleSize) { + const prop = getMapProp(map, i); + const value = getMapValue(map, i); + + // case 1: apply the map value (if it exists) + let applied = + applyStylingValue(renderer, element, prop, value, applyFn, bindingIndex, sanitizer); + + // case 2: apply the initial value (if it exists) + if (!applied && initialStyles) { + applied = findAndApplyMapValue( + renderer, element, applyFn, initialStyles, prop, bindingIndex, sanitizer); + } - // default case: apply `null` to remove the value - if (!applied) { - applyFn(renderer, element, prop, null, bindingIndex); + // default case: apply `null` to remove the value + if (!applied) { + applyFn(renderer, element, prop, null, bindingIndex); + } } - } - const state = getStylingState(element, TEMPLATE_DIRECTIVE_INDEX); - if (isClassBased) { - state.lastDirectClassMap = map; - } else { - state.lastDirectStyleMap = map; + const state = getStylingState(element, TEMPLATE_DIRECTIVE_INDEX); + if (isClassBased) { + state.lastDirectClassMap = map; + } else { + state.lastDirectStyleMap = map; + } } - - return true; } - return false; } /** @@ -727,11 +783,12 @@ export function applyStylingMapDirectly( */ export function applyStylingValueDirectly( renderer: any, context: TStylingContext, element: RElement, data: LStylingData, - bindingIndex: number, prop: string, value: any, isClassBased: boolean, applyFn: ApplyStylingFn, + bindingIndex: number, prop: string, value: any, isClassBased: boolean, sanitizer?: StyleSanitizeFn | null): boolean { let applied = false; if (hasValueChanged(data[bindingIndex], value)) { setValue(data, bindingIndex, value); + const applyFn = isClassBased ? setClass : setStyle; // case 1: apply the provided value (if it exists) applied = applyStylingValue(renderer, element, prop, value, applyFn, bindingIndex, sanitizer); @@ -888,6 +945,26 @@ export const setClass: ApplyStylingFn = } }; +export const setClassName = (renderer: Renderer3 | null, native: RElement, className: string) => { + if (renderer !== null) { + if (isProceduralRenderer(renderer)) { + renderer.setAttribute(native, 'class', className); + } else { + native.className = className; + } + } +}; + +export const setStyleAttr = (renderer: Renderer3 | null, native: RElement, value: string) => { + if (renderer !== null) { + if (isProceduralRenderer(renderer)) { + renderer.setAttribute(native, 'style', value); + } else { + native.setAttribute('style', value); + } + } +}; + /** * Iterates over all provided styling entries and renders them on the element. * @@ -914,3 +991,63 @@ export function renderStylingMap( } } } + +function objectToClassName(obj: {[key: string]: any} | null): string { + let str = ''; + if (obj) { + for (let key in obj) { + const value = obj[key]; + if (value) { + str += (str.length ? ' ' : '') + key; + } + } + } + return str; +} + +/** + * 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. This + * means any JS code that adds/removes class/style values on an element outside + * of Angular's styling binding algorithm. + * + * @returns true when the value was modified externally. + */ +function checkIfExternallyModified(element: HTMLElement, cachedValue: any, isClassBased: boolean) { + // this means it was checked before and there is no reason + // to compare the style/class values again. Either that or + // web workers are being used. + if (global.Node === 'undefined' || cachedValue === VALUE_IS_EXTERNALLY_MODIFIED) return true; + + // comparing the DOM value against the cached value is the best way to + // see if something has changed. + const currentValue = + (isClassBased ? element.className : (element.style && element.style.cssText)) || ''; + return currentValue !== (cachedValue || ''); +} + +/** + * Removes provided styling values from the element + */ +function removeStylingValues( + 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); + } + } +} diff --git a/packages/core/src/render3/styling/styling_debug.ts b/packages/core/src/render3/styling/styling_debug.ts index 7b9e108c597e8..1369d411b750b 100644 --- a/packages/core/src/render3/styling/styling_debug.ts +++ b/packages/core/src/render3/styling/styling_debug.ts @@ -5,12 +5,13 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ +import {createProxy} from '../../debug/proxy'; import {StyleSanitizeFn} from '../../sanitization/style_sanitizer'; import {RElement} from '../interfaces/renderer'; import {ApplyStylingFn, LStylingData, TStylingConfig, TStylingContext, TStylingContextIndex} from '../interfaces/styling'; import {getCurrentStyleSanitizer} from '../state'; import {attachDebugObject} from '../util/debug_utils'; -import {MAP_BASED_ENTRY_PROP_NAME, TEMPLATE_DIRECTIVE_INDEX, allowDirectStyling as _allowDirectStyling, getBindingValue, getDefaultValue, getGuardMask, getProp, getPropValuesStartPosition, getValuesCount, hasConfig, isContextLocked, isSanitizationRequired, isStylingContext} from '../util/styling_utils'; +import {MAP_BASED_ENTRY_PROP_NAME, TEMPLATE_DIRECTIVE_INDEX, allowDirectStyling as _allowDirectStyling, getBindingValue, getDefaultValue, getGuardMask, getProp, getPropValuesStartPosition, getValue, getValuesCount, hasConfig, isContextLocked, isSanitizationRequired, isStylingContext, normalizeIntoStylingMap, setValue} from '../util/styling_utils'; import {applyStylingViaContext} from './bindings'; import {activateStylingMapFeature} from './map_based_bindings'; @@ -374,10 +375,52 @@ export class NodeStylingDebug implements DebugNodeStyling { */ get summary(): {[key: string]: DebugNodeStylingEntry} { const entries: {[key: string]: DebugNodeStylingEntry} = {}; - this._mapValues((prop: string, value: any, bindingIndex: number | null) => { + const config = this.config; + const isClassBased = this._isClassBased; + + 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, bindingIndex: number | null) => { entries[prop] = {prop, value, bindingIndex}; }); - return entries; + + // because the styling algorithm runs into two different + // modes: direct and context-resolution, the output of the entries + // object is different because the removed values are not + // saved between updates. For this reason a proxy is created + // so that the behavior is the same when examining values + // that are no longer active on the element. + return createProxy({ + get(target: {}, prop: string): DebugNodeStylingEntry{ + let value: DebugNodeStylingEntry = entries[prop]; if (!value) { + value = { + prop, + value: isClassBased ? false : null, + bindingIndex: null, + }; + } return value; + }, + set(target: {}, prop: string, value: any) { return false; }, + ownKeys() { return Object.keys(entries); }, + getOwnPropertyDescriptor(k: any) { + // we use a special property descriptor here so that enumeration operations + // such as `Object.keys` will work on this proxy. + return { + enumerable: true, + configurable: true, + }; + }, + }); } get config() { return buildConfig(this.context.context); } @@ -387,11 +430,41 @@ export class NodeStylingDebug implements DebugNodeStyling { */ get values(): {[key: string]: any} { const entries: {[key: string]: any} = {}; - this._mapValues((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; } - private _mapValues(fn: (prop: string, value: string|null, bindingIndex: number|null) => any) { + private _convertMapBindingsToStylingMapArrays(data: LStylingData) { + const context = this.context.context; + const limit = getPropValuesStartPosition(context); + for (let i = + TStylingContextIndex.ValuesStartPosition + TStylingContextIndex.BindingsStartOffset; + i < limit; i++) { + const bindingIndex = context[i] as number; + const bindingValue = bindingIndex !== 0 ? getValue(data, bindingIndex) : null; + if (bindingValue && !Array.isArray(bindingValue)) { + const stylingMapArray = normalizeIntoStylingMap(null, bindingValue, !this._isClassBased); + setValue(data, bindingIndex, stylingMapArray); + } + } + } + + private _mapValues( + data: LStylingData, + fn: (prop: string, value: string|null, bindingIndex: number|null) => any) { // there is no need to store/track an element instance. The // element is only used when the styling algorithm attempts to // style the value (and we mock out the stylingApplyFn anyway). @@ -409,11 +482,11 @@ export class NodeStylingDebug implements DebugNodeStyling { // run the template bindings applyStylingViaContext( - this.context.context, null, mockElement, this._data, true, mapFn, sanitizer, false); + this.context.context, null, mockElement, data, true, mapFn, sanitizer, false); // and also the host bindings applyStylingViaContext( - this.context.context, null, mockElement, this._data, true, mapFn, sanitizer, true); + this.context.context, null, mockElement, data, true, mapFn, sanitizer, true); } } diff --git a/packages/core/src/render3/util/styling_utils.ts b/packages/core/src/render3/util/styling_utils.ts index d48019dd7e1ca..1d6e3c9bcb353 100644 --- a/packages/core/src/render3/util/styling_utils.ts +++ b/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) && @@ -295,13 +295,18 @@ export function forceClassesAsString(classes: string | {[key: string]: any} | nu return (classes as string) || ''; } -export function forceStylesAsString(styles: {[key: string]: any} | null | undefined): string { +export function forceStylesAsString( + styles: {[key: string]: any} | null | undefined, hyphenateProps: boolean): string { let str = ''; if (styles) { const props = Object.keys(styles); for (let i = 0; i < props.length; i++) { const prop = props[i]; - str = concatString(str, `${prop}:${styles[prop]}`, ';'); + const propLabel = hyphenateProps ? hyphenate(prop) : prop; + const value = styles[prop]; + if (value !== null) { + str = concatString(str, `${propLabel}:${value}`, ';'); + } } } return str; @@ -463,4 +468,4 @@ export function splitOnWhitespace(text: string): string[]|null { // `input('class') + classMap()` instructions. export function selectClassBasedInputName(inputs: PropertyAliases): string { return inputs.hasOwnProperty('class') ? 'class' : 'className'; -} \ No newline at end of file +} diff --git a/packages/core/test/acceptance/i18n_spec.ts b/packages/core/test/acceptance/i18n_spec.ts index 344384e02264d..16fd89ffd5f29 100644 --- a/packages/core/test/acceptance/i18n_spec.ts +++ b/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( - `
traduction: un email ` + + `
traduction: un email ` + `
`); directiveInstances.forEach(instance => instance.klass = 'bar'); @@ -1277,7 +1277,7 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => { fixture.detectChanges(); expect(fixture.nativeElement.innerHTML) .toEqual( - `
traduction: 2 emails ` + + `
traduction: 2 emails ` + `
`); }); diff --git a/packages/core/test/acceptance/styling_spec.ts b/packages/core/test/acceptance/styling_spec.ts index c8248228140d1..a2ce244bf272d 100644 --- a/packages/core/test/acceptance/styling_spec.ts +++ b/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: `
`}) + 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) { diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index b512676080640..3a2ba04028dbd 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -608,6 +608,9 @@ { "name": "getActiveDirectiveId" }, + { + "name": "getAndIncrementBindingIndex" + }, { "name": "getBeforeNodeForView" }, diff --git a/packages/core/test/render3/perf/noop_renderer.ts b/packages/core/test/render3/perf/noop_renderer.ts index fbc597444499f..c39f21e3105c2 100644 --- a/packages/core/test/render3/perf/noop_renderer.ts +++ b/packages/core/test/render3/perf/noop_renderer.ts @@ -49,6 +49,9 @@ export class NoopRenderer implements ProceduralRenderer3 { export class NoopRendererFactory implements RendererFactory3 { createRenderer(hostElement: RElement|null, rendererType: null): Renderer3 { + if (typeof global !== 'undefined') { + (global as any).Node = WebWorkerRenderNode; + } return new NoopRenderer(); } } diff --git a/packages/core/test/render3/styling_next/styling_debug_spec.ts b/packages/core/test/render3/styling_next/styling_debug_spec.ts index f689316d18e64..5c1497750a67c 100644 --- a/packages/core/test/render3/styling_next/styling_debug_spec.ts +++ b/packages/core/test/render3/styling_next/styling_debug_spec.ts @@ -13,6 +13,8 @@ describe('styling debugging tools', () => { describe('NodeStylingDebug', () => { it('should list out each of the values in the context paired together with the provided data', () => { + if (isIE()) return; + const debug = makeContextWithDebug(false); const context = debug.context; const data: any[] = []; @@ -67,3 +69,8 @@ function makeContextWithDebug(isClassBased: boolean) { const ctx = allocTStylingContext(null, false); return attachStylingDebugObject(ctx, isClassBased); } + +function isIE() { + // note that this only applies to older IEs (not edge) + return typeof window !== 'undefined' && (window as any).document['documentMode'] ? true : false; +}