Skip to content

Commit

Permalink
refactor(ivy): remove TStylingContext locking in favor of firstUpdate…
Browse files Browse the repository at this point in the history 
…Pass flag (#33521)

This patch removes the need to lock the style and class context
instances to track when bindings can be added. What happens now is
that the `tNode.firstUpdatePass` is used to track when bindings are
registered on the context instances.

PR Close #33521
  • Loading branch information
@matsko @atscott
matsko authored and atscott committed Nov 6, 2019
1 parent 5453c4c commit 3297a76
Show file tree
Hide file tree
Showing 7 changed files with 156 additions and 181 deletions.
56 changes: 30 additions & 26 deletions packages/core/src/render3/instructions/styling.ts
View file
Expand Up @@ -13,14 +13,14 @@ import {AttributeMarker, TAttributes, TNode, TNodeFlags, TNodeType} from '../int
import {RElement} from '../interfaces/renderer';
import {StylingMapArray, StylingMapArrayIndex, TStylingConfig, TStylingContext} from '../interfaces/styling';
import {isDirectiveHost} from '../interfaces/type_checks';
import {LView, RENDERER} from '../interfaces/view';
import {LView, RENDERER, TVIEW, TView} from '../interfaces/view';
import {getActiveDirectiveId, getCheckNoChangesMode, getCurrentStyleSanitizer, getLView, getSelectedIndex, incrementBindingIndex, nextBindingIndex, resetCurrentStyleSanitizer, setCurrentStyleSanitizer, setElementExitFn} from '../state';
import {applyStylingMapDirectly, applyStylingValueDirectly, flushStyling, setClass, setStyle, updateClassViaContext, updateStyleViaContext} from '../styling/bindings';
import {activateStylingMapFeature} from '../styling/map_based_bindings';
import {attachStylingDebugObject} from '../styling/styling_debug';
import {NO_CHANGE} from '../tokens';
import {renderStringify} from '../util/misc_utils';
import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, getValue, hasClassInput, hasStyleInput, hasValueChanged, isContextLocked, isHostStylingActive, isStylingContext, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils';
import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, getValue, hasClassInput, hasStyleInput, hasValueChanged, isHostStylingActive, isStylingContext, isStylingValueDefined, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils';
import {getNativeByTNode, getTNode} from '../util/view_utils';


Expand Down Expand Up @@ -154,17 +154,16 @@ function stylingProp(
let updated = false;

const lView = getLView();
const firstUpdatePass = lView[TVIEW].firstUpdatePass;
const tNode = getTNode(elementIndex, lView);
const native = getNativeByTNode(tNode, lView) as RElement;

const hostBindingsMode = isHostStyling();
const context = isClassBased ? getClassesContext(tNode) : getStylesContext(tNode);
const sanitizer = isClassBased ? null : getCurrentStyleSanitizer();

// we check for this in the instruction code so that the context can be notified
// about prop or map bindings so that the direct apply check can decide earlier
// if it allows for context resolution to be bypassed.
if (!isContextLocked(context, hostBindingsMode)) {
if (firstUpdatePass) {
patchConfig(context, TStylingConfig.HasPropBindings);
}

Expand All @@ -181,7 +180,7 @@ function stylingProp(

// Direct Apply Case: bypass context resolution and apply the
// style/class value directly to the element
if (allowDirectStyling(context, hostBindingsMode)) {
if (allowDirectStyling(context, firstUpdatePass)) {
const sanitizerToUse = isClassBased ? null : sanitizer;
const renderer = getRenderer(tNode, lView);
updated = applyStylingValueDirectly(
Expand All @@ -201,11 +200,11 @@ function stylingProp(
if (isClassBased) {
updated = updateClassViaContext(
context, lView, native, directiveIndex, prop, bindingIndex,
value as string | boolean | null);
value as string | boolean | null, false, firstUpdatePass);
} else {
updated = updateStyleViaContext(
context, lView, native, directiveIndex, prop, bindingIndex,
value as string | SafeValue | null, sanitizer);
value as string | SafeValue | null, sanitizer, false, firstUpdatePass);
}

setElementExitFn(stylingApply);
Expand Down Expand Up @@ -237,6 +236,7 @@ export function ɵɵstyleMap(styles: {[styleName: string]: any} | NO_CHANGE | nu
const index = getSelectedIndex();
const lView = getLView();
const tNode = getTNode(index, lView);
const firstUpdatePass = lView[TVIEW].firstUpdatePass;
const context = getStylesContext(tNode);
const hasDirectiveInput = hasStyleInput(tNode);

Expand All @@ -250,11 +250,12 @@ export function ɵɵstyleMap(styles: {[styleName: string]: any} | NO_CHANGE | nu
// 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() && hasDirectiveInput && styles !== NO_CHANGE) {
updateDirectiveInputValue(context, lView, tNode, bindingIndex, styles, false);
updateDirectiveInputValue(context, lView, tNode, bindingIndex, styles, false, firstUpdatePass);
styles = NO_CHANGE;
}

stylingMap(context, tNode, lView, bindingIndex, styles, false, hasDirectiveInput);
stylingMap(
context, tNode, firstUpdatePass, lView, bindingIndex, styles, false, hasDirectiveInput);
}

/**
Expand Down Expand Up @@ -289,6 +290,7 @@ export function classMapInternal(
elementIndex: number, classes: {[className: string]: any} | NO_CHANGE | string | null): void {
const lView = getLView();
const tNode = getTNode(elementIndex, lView);
const firstUpdatePass = lView[TVIEW].firstUpdatePass;
const context = getClassesContext(tNode);
const hasDirectiveInput = hasClassInput(tNode);

Expand All @@ -302,11 +304,12 @@ export function classMapInternal(
// 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() && hasDirectiveInput && classes !== NO_CHANGE) {
updateDirectiveInputValue(context, lView, tNode, bindingIndex, classes, true);
updateDirectiveInputValue(context, lView, tNode, bindingIndex, classes, true, firstUpdatePass);
classes = NO_CHANGE;
}

stylingMap(context, tNode, lView, bindingIndex, classes, true, hasDirectiveInput);
stylingMap(
context, tNode, firstUpdatePass, lView, bindingIndex, classes, true, hasDirectiveInput);
}

/**
Expand All @@ -316,13 +319,12 @@ export function classMapInternal(
* `[class]` bindings in Angular.
*/
function stylingMap(
context: TStylingContext, tNode: TNode, lView: LView, bindingIndex: number,
value: {[key: string]: any} | string | null, isClassBased: boolean,
context: TStylingContext, tNode: TNode, firstUpdatePass: boolean, lView: LView,
bindingIndex: number, value: {[key: string]: any} | string | null, isClassBased: boolean,
hasDirectiveInput: boolean): void {
const directiveIndex = getActiveDirectiveId();
const native = getNativeByTNode(tNode, lView) as RElement;
const oldValue = getValue(lView, bindingIndex);
const hostBindingsMode = isHostStyling();
const sanitizer = getCurrentStyleSanitizer();
const valueHasChanged = hasValueChanged(oldValue, value);

Expand All @@ -337,13 +339,13 @@ function stylingMap(
// we check for this in the instruction code so that the context can be notified
// about prop or map bindings so that the direct apply check can decide earlier
// if it allows for context resolution to be bypassed.
if (!isContextLocked(context, hostBindingsMode)) {
if (firstUpdatePass) {
patchConfig(context, TStylingConfig.HasMapBindings);
}

// Direct Apply Case: bypass context resolution and apply the
// style/class map values directly to the element
if (allowDirectStyling(context, hostBindingsMode)) {
if (allowDirectStyling(context, firstUpdatePass)) {
const sanitizerToUse = isClassBased ? null : sanitizer;
const renderer = getRenderer(tNode, lView);
applyStylingMapDirectly(
Expand All @@ -367,11 +369,11 @@ function stylingMap(
if (isClassBased) {
updateClassViaContext(
context, lView, native, directiveIndex, null, bindingIndex, stylingMapArr,
valueHasChanged);
valueHasChanged, firstUpdatePass);
} else {
updateStyleViaContext(
context, lView, native, directiveIndex, null, bindingIndex, stylingMapArr, sanitizer,
valueHasChanged);
valueHasChanged, firstUpdatePass);
}

setElementExitFn(stylingApply);
Expand All @@ -396,18 +398,17 @@ function stylingMap(
* depending on the following situations:
*
* - If `oldValue !== newValue`
* - If `newValue` is `null` (but this is skipped if it is during the first update pass--
* which is when the context is not locked yet)
* - If `newValue` is `null` (but this is skipped if it is during the first update pass)
*/
function updateDirectiveInputValue(
context: TStylingContext, lView: LView, tNode: TNode, bindingIndex: number, newValue: any,
isClassBased: boolean): void {
const oldValue = lView[bindingIndex];
if (oldValue !== newValue) {
isClassBased: boolean, firstUpdatePass: boolean): void {
const oldValue = getValue(lView, bindingIndex);
if (hasValueChanged(oldValue, newValue)) {
// even if the value has changed we may not want to emit it to the
// directive input(s) in the event that it is falsy during the
// first update pass.
if (newValue || isContextLocked(context, false)) {
if (isStylingValueDefined(newValue) || !firstUpdatePass) {
const inputName: string = isClassBased ? selectClassBasedInputName(tNode.inputs !) : 'style';
const inputs = tNode.inputs ![inputName] !;
const initialValue = getInitialStylingValue(context);
Expand Down Expand Up @@ -452,6 +453,7 @@ function normalizeStylingDirectiveInputValue(
*/
function stylingApply(): void {
const lView = getLView();
const tView = lView[TVIEW];
const elementIndex = getSelectedIndex();
const tNode = getTNode(elementIndex, lView);
const native = getNativeByTNode(tNode, lView) as RElement;
Expand All @@ -460,7 +462,9 @@ function stylingApply(): void {
const sanitizer = getCurrentStyleSanitizer();
const classesContext = isStylingContext(tNode.classes) ? tNode.classes as TStylingContext : null;
const stylesContext = isStylingContext(tNode.styles) ? tNode.styles as TStylingContext : null;
flushStyling(renderer, lView, classesContext, stylesContext, native, directiveIndex, sanitizer);
flushStyling(
renderer, lView, classesContext, stylesContext, native, directiveIndex, sanitizer,
tView.firstUpdatePass);
resetCurrentStyleSanitizer();
}

Expand Down
42 changes: 10 additions & 32 deletions packages/core/src/render3/interfaces/styling.ts
View file
Expand Up @@ -341,7 +341,7 @@ export const enum TStylingConfig {
/**
* The initial state of the styling context config.
*/
Initial = 0b00000000,
Initial = 0b000000,

/**
* Whether or not there are any directives on this element.
Expand All @@ -359,7 +359,7 @@ export const enum TStylingConfig {
* 3. `<comp>`
* 4. `<comp dir-one>`
*/
HasDirectives = 0b00000001,
HasDirectives = 0b000001,

/**
* Whether or not there are prop-based bindings present.
Expand All @@ -370,7 +370,7 @@ export const enum TStylingConfig {
* 3. `@HostBinding('style.prop') x`
* 4. `@HostBinding('class.prop') x`
*/
HasPropBindings = 0b00000010,
HasPropBindings = 0b000010,

/**
* Whether or not there are map-based bindings present.
Expand All @@ -381,7 +381,7 @@ export const enum TStylingConfig {
* 3. `@HostBinding('style') x`
* 4. `@HostBinding('class') x`
*/
HasMapBindings = 0b00000100,
HasMapBindings = 0b000100,

/**
* Whether or not there are map-based and prop-based bindings present.
Expand All @@ -402,7 +402,7 @@ export const enum TStylingConfig {
* 2. map + prop: `<div [style]="x" [style.prop]>`
* 3. map + map: `<div [style]="x" dir-that-sets-style>`
*/
HasCollisions = 0b00001000,
HasCollisions = 0b001000,

/**
* Whether or not the context contains initial styling values.
Expand All @@ -413,7 +413,7 @@ export const enum TStylingConfig {
* 3. `@Directive({ host: { 'style': 'width:200px' } })`
* 4. `@Directive({ host: { 'class': 'one two three' } })`
*/
HasInitialStyling = 0b000010000,
HasInitialStyling = 0b0010000,

/**
* Whether or not the context contains one or more template bindings.
Expand All @@ -424,7 +424,7 @@ export const enum TStylingConfig {
* 3. `<div [class]="x">`
* 4. `<div [class.name]="x">`
*/
HasTemplateBindings = 0b000100000,
HasTemplateBindings = 0b0100000,

/**
* Whether or not the context contains one or more host bindings.
Expand All @@ -435,35 +435,13 @@ export const enum TStylingConfig {
* 3. `@HostBinding('class') x`
* 4. `@HostBinding('class.name') x`
*/
HasHostBindings = 0b001000000,

/**
* Whether or not the template bindings are allowed to be registered in the context.
*
* This flag is after one or more template-based style/class bindings were
* set and processed for an element. Once the bindings are processed then a call
* to stylingApply is issued and the lock will be put into place.
*
* Note that this is only set once.
*/
TemplateBindingsLocked = 0b010000000,

/**
* Whether or not the host bindings are allowed to be registered in the context.
*
* This flag is after one or more host-based style/class bindings were
* set and processed for an element. Once the bindings are processed then a call
* to stylingApply is issued and the lock will be put into place.
*
* Note that this is only set once.
*/
HostBindingsLocked = 0b100000000,
HasHostBindings = 0b1000000,

/** A Mask of all the configurations */
Mask = 0b111111111,
Mask = 0b1111111,

/** Total amount of configuration bits used */
TotalBits = 9,
TotalBits = 7,
}

/**
Expand Down

0 comments on commit 3297a76

Please sign in to comment.