From da7e362bcedce556bc3e69d02d85324eae2058c1 Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Thu, 9 Jan 2020 21:48:16 -0800 Subject: [PATCH] =?UTF-8?q?refactor(ivy):=20delete=20`=C9=B5=C9=B5elementH?= =?UTF-8?q?ostAttrs`=20instruction=20(#34717)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR Close #34717 --- packages/core/src/render3/component.ts | 21 ++- .../core/src/render3/instructions/element.ts | 91 ++---------- .../src/render3/instructions/lview_debug.ts | 51 +++---- .../core/src/render3/instructions/shared.ts | 140 +++++++++--------- packages/core/src/render3/interfaces/node.ts | 13 ++ .../core/src/render3/node_selector_matcher.ts | 59 +++++--- packages/core/src/render3/state.ts | 8 - .../core/src/render3/styling/class_differ.ts | 2 +- packages/core/src/render3/util/attrs_utils.ts | 1 - .../core/test/acceptance/host_binding_spec.ts | 24 +++ .../cyclic_import/bundle.golden_symbols.json | 22 +-- .../hello_world/bundle.golden_symbols.json | 11 +- .../bundling/todo/bundle.golden_symbols.json | 19 ++- .../render3/node_selector_matcher_spec.ts | 22 +-- 14 files changed, 219 insertions(+), 265 deletions(-) diff --git a/packages/core/src/render3/component.ts b/packages/core/src/render3/component.ts index 4ce22e6ab4..c9e55e66a1 100644 --- a/packages/core/src/render3/component.ts +++ b/packages/core/src/render3/component.ts @@ -17,13 +17,15 @@ import {assertComponentType} from './assert'; import {getComponentDef} from './definition'; import {diPublicInInjector, getOrCreateNodeInjectorForNode} from './di'; import {registerPostOrderHooks} from './hooks'; -import {CLEAN_PROMISE, addHostBindingsToExpandoInstructions, addToViewTree, createLView, createTView, getOrCreateTComponentView, getOrCreateTNode, growHostVarsSpace, initNodeFlags, instantiateRootComponent, invokeHostBindingsInCreationMode, locateHostElement, markAsComponentHost, refreshView, renderView} from './instructions/shared'; +import {CLEAN_PROMISE, addHostBindingsToExpandoInstructions, addToViewTree, createLView, createTView, getOrCreateTComponentView, getOrCreateTNode, growHostVarsSpace, initTNodeFlags, instantiateRootComponent, invokeHostBindingsInCreationMode, locateHostElement, markAsComponentHost, refreshView, renderInitialStyling, renderView} from './instructions/shared'; +import {registerInitialStylingOnTNode} from './instructions/styling'; import {ComponentDef, ComponentType, RenderFlags} from './interfaces/definition'; import {TElementNode, TNode, TNodeType} from './interfaces/node'; import {PlayerHandler} from './interfaces/player'; import {RElement, Renderer3, RendererFactory3, domRendererFactory3} from './interfaces/renderer'; import {CONTEXT, HEADER_OFFSET, LView, LViewFlags, RootContext, RootContextFlags, TVIEW, TViewType} from './interfaces/view'; import {enterView, getPreviousOrParentTNode, incrementActiveDirectiveId, leaveView, setActiveHostElement} from './state'; +import {setUpAttributes} from './util/attrs_utils'; import {publishDefaultGlobalUtils} from './util/global_utils'; import {defaultScheduler, stringifyForError} from './util/misc_utils'; import {getRootContext} from './util/view_traversal_utils'; @@ -171,6 +173,14 @@ export function createRootComponentView( ngDevMode && assertDataInRange(rootView, 0 + HEADER_OFFSET); rootView[0 + HEADER_OFFSET] = rNode; const tNode: TElementNode = getOrCreateTNode(tView, null, 0, TNodeType.Element, null, null); + const mergedAttrs = tNode.mergedAttrs = def.hostAttrs; + if (mergedAttrs !== null) { + registerInitialStylingOnTNode(tNode, mergedAttrs, 0); + if (rNode !== null) { + setUpAttributes(renderer, rNode, mergedAttrs); + renderInitialStyling(renderer, rNode, tNode, false); + } + } const componentView = createLView( rootView, getOrCreateTComponentView(def), null, def.onPush ? LViewFlags.Dirty : LViewFlags.CheckAlways, rootView[HEADER_OFFSET], tNode, @@ -179,7 +189,7 @@ export function createRootComponentView( if (tView.firstCreatePass) { diPublicInInjector(getOrCreateNodeInjectorForNode(tNode, rootView), tView, def.type); markAsComponentHost(tView, tNode); - initNodeFlags(tNode, rootView.length, 1); + initTNodeFlags(tNode, rootView.length, 1); } addToViewTree(rootView, componentView); @@ -216,7 +226,8 @@ export function createRootComponent( // part of the `hostAttrs`. // The check for componentDef.hostBindings is wrong since now some directives may not // have componentDef.hostBindings but they still need to process hostVars and hostAttrs - if (tView.firstCreatePass && (componentDef.hostBindings || componentDef.hostAttrs !== null)) { + if (tView.firstCreatePass && + (componentDef.hostBindings !== null || componentDef.hostAttrs !== null)) { const elementIndex = rootTNode.index - HEADER_OFFSET; setActiveHostElement(elementIndex); incrementActiveDirectiveId(); @@ -225,9 +236,7 @@ export function createRootComponent( addHostBindingsToExpandoInstructions(rootTView, componentDef); growHostVarsSpace(rootTView, rootLView, componentDef.hostVars); - const expando = tView.expandoInstructions !; - invokeHostBindingsInCreationMode( - componentDef, expando, component, rootTNode, tView.firstCreatePass); + invokeHostBindingsInCreationMode(componentDef, component, rootTNode); setActiveHostElement(null); } diff --git a/packages/core/src/render3/instructions/element.ts b/packages/core/src/render3/instructions/element.ts index c15f4fbae5..599f45eb91 100644 --- a/packages/core/src/render3/instructions/element.ts +++ b/packages/core/src/render3/instructions/element.ts @@ -7,7 +7,7 @@ */ import {assertDataInRange, assertDefined, assertEqual} from '../../util/assert'; -import {assertHasParent} from '../assert'; +import {assertFirstCreatePass, assertHasParent} from '../assert'; import {attachPatchData} from '../context_discovery'; import {registerPostOrderHooks} from '../hooks'; import {TAttributes, TElementNode, TNode, TNodeFlags, TNodeType} from '../interfaces/node'; @@ -28,20 +28,21 @@ import {registerInitialStylingOnTNode} from './styling'; function elementStartFirstCreatePass( index: number, tView: TView, lView: LView, native: RElement, name: string, attrsIndex?: number | null, localRefsIndex?: number): TElementNode { + ngDevMode && assertFirstCreatePass(tView); ngDevMode && ngDevMode.firstCreatePass++; const tViewConsts = tView.consts; const attrs = getConstant(tViewConsts, attrsIndex); const tNode = getOrCreateTNode(tView, lView[T_HOST], index, TNodeType.Element, name, attrs); - if (attrs !== null) { - registerInitialStylingOnTNode(tNode, attrs, 0); - } - const hasDirectives = resolveDirectives(tView, lView, tNode, getConstant(tViewConsts, localRefsIndex)); ngDevMode && warnAboutUnknownElement(lView, native, tNode, hasDirectives); + if (tNode.mergedAttrs !== null) { + registerInitialStylingOnTNode(tNode, tNode.mergedAttrs, 0); + } + if (tView.queries !== null) { tView.queries.elementStart(tView, tNode); } @@ -83,9 +84,9 @@ export function ɵɵelementStart( tView.data[adjustedIndex] as TElementNode; setPreviousOrParentTNode(tNode, true); - const attrs = tNode.attrs; - if (attrs != null) { - setUpAttributes(renderer, native, attrs); + const mergedAttrs = tNode.mergedAttrs; + if (mergedAttrs !== null) { + setUpAttributes(renderer, native, mergedAttrs); } if ((tNode.flags & TNodeFlags.hasInitialStyling) === TNodeFlags.hasInitialStyling) { renderInitialStyling(renderer, native, tNode, false); @@ -106,7 +107,7 @@ export function ɵɵelementStart( createDirectivesInstances(tView, lView, tNode); executeContentQueries(tView, tNode, lView); } - if (localRefsIndex != null) { + if (localRefsIndex !== null) { saveResolvedLocalsInData(lView, tNode); } } @@ -169,78 +170,6 @@ export function ɵɵelement( ɵɵelementEnd(); } -/** - * Assign static attribute values to a host element. - * - * This instruction will assign static attribute values as well as class and style - * values to an element within the host bindings function. Since attribute values - * can consist of different types of values, the `attrs` array must include the values in - * the following format: - * - * attrs = [ - * // static attributes (like `title`, `name`, `id`...) - * attr1, value1, attr2, value, - * - * // a single namespace value (like `x:id`) - * NAMESPACE_MARKER, namespaceUri1, name1, value1, - * - * // another single namespace value (like `x:name`) - * NAMESPACE_MARKER, namespaceUri2, name2, value2, - * - * // a series of CSS classes that will be applied to the element (no spaces) - * CLASSES_MARKER, class1, class2, class3, - * - * // a series of CSS styles (property + value) that will be applied to the element - * STYLES_MARKER, prop1, value1, prop2, value2 - * ] - * - * All non-class and non-style attributes must be defined at the start of the list - * first before all class and style values are set. When there is a change in value - * type (like when classes and styles are introduced) a marker must be used to separate - * the entries. The marker values themselves are set via entries found in the - * [AttributeMarker] enum. - * - * NOTE: This instruction is meant to used from `hostBindings` function only. - * - * @param directive A directive instance the styling is associated with. - * @param attrs An array of static values (attributes, classes and styles) with the correct marker - * values. - * - * @codeGenApi - */ -export function ɵɵelementHostAttrs(attrs: TAttributes) { - const hostElementIndex = getSelectedIndex(); - const lView = getLView(); - const tView = lView[TVIEW]; - const tNode = getTNode(hostElementIndex, lView); - - // non-element nodes (e.g. ``) are not rendered as actual - // element nodes and adding styles/classes on to them will cause runtime - // errors... - if (tNode.type === TNodeType.Element) { - const native = getNativeByTNode(tNode, lView) as RElement; - // TODO(misko-next): setup attributes need to be moved out of `ɵɵelementHostAttrs` - const lastAttrIndex = setUpAttributes(lView[RENDERER], native, attrs); - if (tView.firstCreatePass) { - const stylingNeedsToBeRendered = registerInitialStylingOnTNode(tNode, attrs, lastAttrIndex); - - // this is only called during the first template pass in the - // event that this current directive assigned initial style/class - // host attribute values to the element. Because initial styling - // values are applied before directives are first rendered (within - // `createElement`) this means that initial styling for any directives - // still needs to be applied. Note that this will only happen during - // the first template pass and not each time a directive applies its - // attribute values to the element. - if (stylingNeedsToBeRendered) { - const renderer = lView[RENDERER]; - // TODO(misko-next): Styling initialization should move out of `ɵɵelementHostAttrs` - renderInitialStyling(renderer, native, tNode, true); - } - } - } -} - function setDirectiveStylingInput( context: TStylingContext | StylingMapArray | null, lView: LView, stylingInputs: (string | number)[], propName: string) { diff --git a/packages/core/src/render3/instructions/lview_debug.ts b/packages/core/src/render3/instructions/lview_debug.ts index 296767fe8a..e6166ee6c3 100644 --- a/packages/core/src/render3/instructions/lview_debug.ts +++ b/packages/core/src/render3/instructions/lview_debug.ts @@ -154,31 +154,32 @@ export const TViewConstructor = class TView implements ITView { export const TNodeConstructor = class TNode implements ITNode { constructor( - public tView_: TView, // - public type: TNodeType, // - public index: number, // - public injectorIndex: number, // - public directiveStart: number, // - public directiveEnd: number, // - public propertyBindings: number[]|null, // - public flags: TNodeFlags, // - public providerIndexes: TNodeProviderIndexes, // - public tagName: string|null, // - public attrs: (string|AttributeMarker|(string|SelectorFlags)[])[]|null, // - public localNames: (string|number)[]|null, // - public initialInputs: (string[]|null)[]|null|undefined, // - public inputs: PropertyAliases|null, // - public outputs: PropertyAliases|null, // - public tViews: ITView|ITView[]|null, // - public next: ITNode|null, // - public projectionNext: ITNode|null, // - public child: ITNode|null, // - public parent: TElementNode|TContainerNode|null, // - public projection: number|(ITNode|RNode[])[]|null, // - public styles: TStylingContext|null, // - public classes: TStylingContext|null, // - public classBindings: TStylingRange, // - public styleBindings: TStylingRange, // + public tView_: TView, // + public type: TNodeType, // + public index: number, // + public injectorIndex: number, // + public directiveStart: number, // + public directiveEnd: number, // + public propertyBindings: number[]|null, // + public flags: TNodeFlags, // + public providerIndexes: TNodeProviderIndexes, // + public tagName: string|null, // + public attrs: (string|AttributeMarker|(string|SelectorFlags)[])[]|null, // + public mergedAttrs: (string|AttributeMarker|(string|SelectorFlags)[])[]|null, // + public localNames: (string|number)[]|null, // + public initialInputs: (string[]|null)[]|null|undefined, // + public inputs: PropertyAliases|null, // + public outputs: PropertyAliases|null, // + public tViews: ITView|ITView[]|null, // + public next: ITNode|null, // + public projectionNext: ITNode|null, // + public child: ITNode|null, // + public parent: TElementNode|TContainerNode|null, // + public projection: number|(ITNode|RNode[])[]|null, // + public styles: TStylingContext|null, // + public classes: TStylingContext|null, // + public classBindings: TStylingRange, // + public styleBindings: TStylingRange, // ) {} get type_(): string { diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 6482c8d83a..733d33a964 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -20,7 +20,7 @@ import {attachPatchData} from '../context_discovery'; import {getFactoryDef} from '../definition'; import {diPublicInInjector, getNodeInjectable, getOrCreateNodeInjectorForNode} from '../di'; import {throwMultipleComponentError} from '../errors'; -import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags, registerPreOrderHooks} from '../hooks'; +import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks'; import {ACTIVE_INDEX, ActiveIndexFlag, CONTAINER_HEADER_OFFSET, LContainer, MOVED_VIEWS} from '../interfaces/container'; import {ComponentDef, ComponentTemplate, DirectiveDef, DirectiveDefListOrFactory, PipeDefListOrFactory, RenderFlags, ViewQueriesFunction} from '../interfaces/definition'; import {INJECTOR_BLOOM_PARENT_SIZE, NodeInjectorFactory} from '../interfaces/injector'; @@ -28,20 +28,18 @@ import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, Pro import {RComment, RElement, RNode, RText, Renderer3, RendererFactory3, isProceduralRenderer} from '../interfaces/renderer'; import {SanitizerFn} from '../interfaces/sanitization'; import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRootView} from '../interfaces/type_checks'; -import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, ExpandoInstructions, FLAGS, HEADER_OFFSET, HOST, INJECTOR, InitPhaseState, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, TData, TVIEW, TView, TViewType, T_HOST} from '../interfaces/view'; +import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, INJECTOR, InitPhaseState, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, TData, TVIEW, TView, TViewType, T_HOST} from '../interfaces/view'; import {assertNodeOfPossibleTypes} from '../node_assert'; import {isNodeMatchingSelectorList} from '../node_selector_matcher'; -import {ActiveElementFlags, enterView, executeElementExitFn, getBindingsEnabled, getCheckNoChangesMode, getIsParent, getPreviousOrParentTNode, getSelectedIndex, hasActiveElementFlag, incrementActiveDirectiveId, leaveView, leaveViewProcessExit, setActiveHostElement, setBindingIndex, setBindingRoot, setCheckNoChangesMode, setCurrentDirectiveDef, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state'; +import {ActiveElementFlags, enterView, executeElementExitFn, getBindingsEnabled, getCheckNoChangesMode, getIsParent, getPreviousOrParentTNode, getSelectedIndex, hasActiveElementFlag, incrementActiveDirectiveId, leaveView, leaveViewProcessExit, setActiveHostElement, setBindingIndex, setBindingRoot, setCheckNoChangesMode, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state'; import {renderStylingMap, writeStylingValueDirectly} from '../styling/bindings'; import {NO_CHANGE} from '../tokens'; -import {isAnimationProp} from '../util/attrs_utils'; +import {isAnimationProp, mergeHostAttrs} from '../util/attrs_utils'; import {INTERPOLATION_DELIMITER, renderStringify, stringifyForError} from '../util/misc_utils'; import {getInitialStylingValue} from '../util/styling_utils'; import {getLViewParent} from '../util/view_traversal_utils'; -import {getComponentLViewByIndex, getNativeByIndex, getNativeByTNode, getTNode, isCreationMode, readPatchedLView, resetPreOrderHookFlags, unwrapRNode, viewAttachedToChangeDetector} from '../util/view_utils'; - +import {getComponentLViewByIndex, getNativeByIndex, getNativeByTNode, getTNode, isCreationMode, readPatchedLView, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils'; import {selectIndexInternal} from './advance'; -import {ɵɵelementHostAttrs} from './element'; import {LCleanup, LViewBlueprint, MatchesArray, TCleanup, TNodeConstructor, TNodeInitialInputs, TNodeLocalNames, TViewComponents, TViewConstructor, attachLContainerDebug, attachLViewDebug, cloneToLViewFromTViewBlueprint, cloneToTViewData} from './lview_debug'; @@ -828,6 +826,7 @@ export function createTNode( 0, // providerIndexes: TNodeProviderIndexes tagName, // tagName: string|null attrs, // attrs: (string|AttributeMarker|(string|SelectorFlags)[])[]|null + null, // mergedAttrs null, // localNames: (string|number)[]|null undefined, // initialInputs: (string[]|null)[]|null|undefined null, // inputs: PropertyAliases|null @@ -854,6 +853,7 @@ export function createTNode( providerIndexes: 0, tagName: tagName, attrs: attrs, + mergedAttrs: null, localNames: null, initialInputs: undefined, inputs: null, @@ -1115,64 +1115,69 @@ export function resolveDirectives( // tsickle. ngDevMode && assertFirstCreatePass(tView); - if (!getBindingsEnabled()) return false; - - const directives: DirectiveDef[]|null = findDirectiveMatches(tView, lView, tNode); - const exportsMap: ({[key: string]: number} | null) = localRefs === null ? null : {'': -1}; let hasDirectives = false; + if (getBindingsEnabled()) { + const directiveDefs: DirectiveDef[]|null = findDirectiveDefMatches(tView, lView, tNode); + const exportsMap: ({[key: string]: number} | null) = localRefs === null ? null : {'': -1}; - if (directives !== null) { - let totalDirectiveHostVars = 0; - hasDirectives = true; - initNodeFlags(tNode, tView.data.length, directives.length); - // When the same token is provided by several directives on the same node, some rules apply in - // the viewEngine: - // - viewProviders have priority over providers - // - the last directive in NgModule.declarations has priority over the previous one - // So to match these rules, the order in which providers are added in the arrays is very - // important. - for (let i = 0; i < directives.length; i++) { - const def = directives[i]; - if (def.providersResolver) def.providersResolver(def); - } - generateExpandoInstructionBlock(tView, tNode, directives.length); - let preOrderHooksFound = false; - let preOrderCheckHooksFound = false; - for (let i = 0; i < directives.length; i++) { - const def = directives[i]; + if (directiveDefs !== null) { + let totalDirectiveHostVars = 0; + hasDirectives = true; + initTNodeFlags(tNode, tView.data.length, directiveDefs.length); + // When the same token is provided by several directives on the same node, some rules apply in + // the viewEngine: + // - viewProviders have priority over providers + // - the last directive in NgModule.declarations has priority over the previous one + // So to match these rules, the order in which providers are added in the arrays is very + // important. + for (let i = 0; i < directiveDefs.length; i++) { + const def = directiveDefs[i]; + if (def.providersResolver) def.providersResolver(def); + } + generateExpandoInstructionBlock(tView, tNode, directiveDefs.length); + let preOrderHooksFound = false; + let preOrderCheckHooksFound = false; + for (let i = 0; i < directiveDefs.length; i++) { + const def = directiveDefs[i]; + // Merge the attrs in the order of matches. This assumes that the first directive is the + // component itself, so that the component has the least priority. + tNode.mergedAttrs = mergeHostAttrs(tNode.mergedAttrs, def.hostAttrs); - baseResolveDirective(tView, lView, def); + baseResolveDirective(tView, lView, def); - saveNameToExportMap(tView.data !.length - 1, def, exportsMap); + saveNameToExportMap(tView.data !.length - 1, def, exportsMap); - if (def.contentQueries !== null) tNode.flags |= TNodeFlags.hasContentQuery; - if (def.hostBindings !== null || def.hostAttrs !== null || def.hostVars !== 0) - tNode.flags |= TNodeFlags.hasHostBindings; + if (def.contentQueries !== null) tNode.flags |= TNodeFlags.hasContentQuery; + if (def.hostBindings !== null || def.hostAttrs !== null || def.hostVars !== 0) + tNode.flags |= TNodeFlags.hasHostBindings; - // Only push a node index into the preOrderHooks array if this is the first - // pre-order hook found on this node. - if (!preOrderHooksFound && (def.onChanges || def.onInit || def.doCheck)) { - // We will push the actual hook function into this array later during dir instantiation. - // We cannot do it now because we must ensure hooks are registered in the same - // order that directives are created (i.e. injection order). - (tView.preOrderHooks || (tView.preOrderHooks = [])).push(tNode.index - HEADER_OFFSET); - preOrderHooksFound = true; + // Only push a node index into the preOrderHooks array if this is the first + // pre-order hook found on this node. + if (!preOrderHooksFound && (def.onChanges || def.onInit || def.doCheck)) { + // We will push the actual hook function into this array later during dir instantiation. + // We cannot do it now because we must ensure hooks are registered in the same + // order that directives are created (i.e. injection order). + (tView.preOrderHooks || (tView.preOrderHooks = [])).push(tNode.index - HEADER_OFFSET); + preOrderHooksFound = true; + } + + if (!preOrderCheckHooksFound && (def.onChanges || def.doCheck)) { + (tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [ + ])).push(tNode.index - HEADER_OFFSET); + preOrderCheckHooksFound = true; + } + + addHostBindingsToExpandoInstructions(tView, def); + totalDirectiveHostVars += def.hostVars; } - if (!preOrderCheckHooksFound && (def.onChanges || def.doCheck)) { - (tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [ - ])).push(tNode.index - HEADER_OFFSET); - preOrderCheckHooksFound = true; - } - - addHostBindingsToExpandoInstructions(tView, def); - totalDirectiveHostVars += def.hostVars; + initializeInputAndOutputAliases(tView, tNode); + growHostVarsSpace(tView, lView, totalDirectiveHostVars); } - - initializeInputAndOutputAliases(tView, tNode); - growHostVarsSpace(tView, lView, totalDirectiveHostVars); + if (exportsMap) cacheMatchingLocalNames(tNode, localRefs, exportsMap); } - if (exportsMap) cacheMatchingLocalNames(tNode, localRefs, exportsMap); + // Merge the template attrs last so that they have the highest priority. + tNode.mergedAttrs = mergeHostAttrs(tNode.mergedAttrs, tNode.attrs); return hasDirectives; } @@ -1187,9 +1192,7 @@ export function addHostBindingsToExpandoInstructions( ngDevMode && assertFirstCreatePass(tView); const expando = tView.expandoInstructions !; // TODO(misko): PERF we are adding `hostBindings` even if there is nothing to add! This is - // suboptimal for performance. See `currentDirectiveIndex` comment in - // `setHostBindingsByExecutingExpandoInstructions` for details. - // TODO(misko): PERF `def.hostBindings` may be null, + // suboptimal for performance. `def.hostBindings` may be null, // but we still need to push null to the array as a placeholder // to ensure the directive counter is incremented (so host // binding functions always line up with the corrective directive). @@ -1277,7 +1280,7 @@ function invokeDirectivesHostBindings(tView: TView, viewData: LView, tNode: TNod // It is important that this be called first before the actual instructions // are run because this way the first directive ID value is not zero. incrementActiveDirectiveId(); - invokeHostBindingsInCreationMode(def, expando, directive, tNode, firstCreatePass); + invokeHostBindingsInCreationMode(def, directive, tNode); } else if (firstCreatePass) { expando.push(null); } @@ -1287,22 +1290,13 @@ function invokeDirectivesHostBindings(tView: TView, viewData: LView, tNode: TNod } } +// TODO(COMMIT): jsdoc export function invokeHostBindingsInCreationMode( - def: DirectiveDef, expando: ExpandoInstructions, directive: any, tNode: TNode, - firstCreatePass: boolean) { - const previousExpandoLength = expando.length; - setCurrentDirectiveDef(def); - const elementIndex = tNode.index - HEADER_OFFSET; - // TODO(misko-next): This is a temporary work around for the fact that we moved the information - // from instruction to declaration. The workaround is to just call the instruction as if it was - // part of the `hostAttrs`. - if (def.hostAttrs !== null) { - ɵɵelementHostAttrs(def.hostAttrs); - } + def: DirectiveDef, directive: any, tNode: TNode) { if (def.hostBindings !== null) { + const elementIndex = tNode.index - HEADER_OFFSET; def.hostBindings !(RenderFlags.Create, directive, elementIndex); } - setCurrentDirectiveDef(null); } /** @@ -1331,7 +1325,7 @@ export function generateExpandoInstructionBlock( * Matches the current node against all available selectors. * If a component is matched (at most one), it is returned in first position in the array. */ -function findDirectiveMatches( +function findDirectiveDefMatches( tView: TView, viewData: LView, tNode: TElementNode | TContainerNode | TElementContainerNode): DirectiveDef[]|null { ngDevMode && assertFirstCreatePass(tView); @@ -1413,7 +1407,7 @@ function saveNameToExportMap( * the directive count to 0, and adding the isComponent flag. * @param index the initial index */ -export function initNodeFlags(tNode: TNode, index: number, numberOfDirectives: number) { +export function initTNodeFlags(tNode: TNode, index: number, numberOfDirectives: number) { ngDevMode && assertNotEqual( numberOfDirectives, tNode.directiveEnd - tNode.directiveStart, 'Reached the max number of directives'); diff --git a/packages/core/src/render3/interfaces/node.ts b/packages/core/src/render3/interfaces/node.ts index 1cb13c84a6..7358cf1bfc 100644 --- a/packages/core/src/render3/interfaces/node.ts +++ b/packages/core/src/render3/interfaces/node.ts @@ -434,6 +434,19 @@ export interface TNode { */ attrs: TAttributes|null; + /** + * Same as `TNode.attrs` but contains merged data across all directive host bindings. + * + * We need to keep `attrs` as unmerged so that it can be used for attribute selectors. + * We merge attrs here so that it can be used in a performant way for initial rendering. + * + * The `attrs` are merged in first pass in following order: + * - Component's `hostAttrs` + * - Directives' `hostAttrs` + * - Template `TNode.attrs` associated with the current `TNode`. + */ + mergedAttrs: TAttributes|null; + /** * A set of local names under which a given element is exported in a template and * visible to queries. An entry in this array can be created for different reasons: diff --git a/packages/core/src/render3/node_selector_matcher.ts b/packages/core/src/render3/node_selector_matcher.ts index f4c9b66a52..6eaf8bca7d 100644 --- a/packages/core/src/render3/node_selector_matcher.ts +++ b/packages/core/src/render3/node_selector_matcher.ts @@ -8,32 +8,52 @@ import '../util/ng_dev_mode'; -import {assertDefined, assertNotEqual} from '../util/assert'; +import {assertDefined, assertEqual, assertNotEqual} from '../util/assert'; import {AttributeMarker, TAttributes, TNode, TNodeType, unusedValueExportToPlacateAjd as unused1} from './interfaces/node'; import {CssSelector, CssSelectorList, SelectorFlags, unusedValueExportToPlacateAjd as unused2} from './interfaces/projection'; +import {classIndexOf} from './styling/class_differ'; import {isNameOnlyAttributeMarker} from './util/attrs_utils'; -import {getInitialStylingValue} from './util/styling_utils'; const unusedValueToPlacateAjd = unused1 + unused2; const NG_TEMPLATE_SELECTOR = 'ng-template'; -function isCssClassMatching(nodeClassAttrVal: string, cssClassToMatch: string): boolean { - const nodeClassesLen = nodeClassAttrVal.length; - // we lowercase the class attribute value to be able to match - // selectors without case-sensitivity - // (selectors are already in lowercase when generated) - const matchIndex = nodeClassAttrVal.toLowerCase().indexOf(cssClassToMatch); - const matchEndIdx = matchIndex + cssClassToMatch.length; - if (matchIndex === -1 // no match - || (matchIndex > 0 && nodeClassAttrVal ![matchIndex - 1] !== ' ') // no space before - || - (matchEndIdx < nodeClassesLen && nodeClassAttrVal ![matchEndIdx] !== ' ')) // no space after - { - return false; +/** + * Search the `TAttributes` to see if it contains `cssClassToMatch` (case insensitive) + * + * @param attrs `TAttributes` to search through. + * @param cssClassToMatch class to match (lowercase) + * @param isProjectionMode Whether or not class matching should look into the attribute `class` in + * addition to the `AttributeMarker.Classes`. + */ +function isCssClassMatching( + attrs: TAttributes, cssClassToMatch: string, isProjectionMode: boolean): boolean { + // TODO(misko): The fact that this function needs to know about `isProjectionMode` seems suspect. + // It is strange to me that sometimes the class information comes in form of `class` attribute + // and sometimes in form of `AttributeMarker.Classes`. Some investigation is needed to determine + // if that is the right behavior. + ngDevMode && + assertEqual( + cssClassToMatch, cssClassToMatch.toLowerCase(), 'Class name expected to be lowercase.'); + let i = 0; + while (i < attrs.length) { + let item = attrs[i++]; + if (isProjectionMode && item === 'class') { + item = attrs[i] as string; + if (classIndexOf(item.toLowerCase(), cssClassToMatch, 0) !== -1) { + return true; + } + } else if (item === AttributeMarker.Classes) { + // We found the classes section. Start searching for the class. + while (i < attrs.length && typeof(item = attrs[i++]) == 'string') { + // while we have strings + if (item.toLowerCase() === cssClassToMatch) return true; + } + return false; + } } - return true; + return false; } /** @@ -106,9 +126,8 @@ export function isNodeMatchingSelector( // special case for matching against classes when a tNode has been instantiated with // class and style values as separate attribute values (e.g. ['title', CLASS, 'foo']) - if ((mode & SelectorFlags.CLASS) && tNode.classes) { - if (!isCssClassMatching( - getInitialStylingValue(tNode.classes), selectorAttrValue as string)) { + if ((mode & SelectorFlags.CLASS) && tNode.attrs !== null) { + if (!isCssClassMatching(tNode.attrs, selectorAttrValue as string, isProjectionMode)) { if (isPositive(mode)) return false; skipToNextSelector = true; } @@ -143,7 +162,7 @@ export function isNodeMatchingSelector( const compareAgainstClassName = mode & SelectorFlags.CLASS ? nodeAttrValue : null; if (compareAgainstClassName && - !isCssClassMatching(compareAgainstClassName, selectorAttrValue as string) || + classIndexOf(compareAgainstClassName, selectorAttrValue as string, 0) !== -1 || mode & SelectorFlags.ATTRIBUTE && selectorAttrValue !== nodeAttrValue) { if (isPositive(mode)) return false; skipToNextSelector = true; diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts index 3746c90c6c..1e51ca2594 100644 --- a/packages/core/src/render3/state.ts +++ b/packages/core/src/render3/state.ts @@ -194,14 +194,6 @@ export function decreaseElementDepthCount() { instructionState.lFrame.elementDepthCount--; } -export function getCurrentDirectiveDef(): DirectiveDef|ComponentDef|null { - return instructionState.lFrame.currentDirectiveDef; -} - -export function setCurrentDirectiveDef(def: DirectiveDef| ComponentDef| null): void { - instructionState.lFrame.currentDirectiveDef = def; -} - export function getBindingsEnabled(): boolean { return instructionState.bindingsEnabled; } diff --git a/packages/core/src/render3/styling/class_differ.ts b/packages/core/src/render3/styling/class_differ.ts index df5d87644d..b106cc7dec 100644 --- a/packages/core/src/render3/styling/class_differ.ts +++ b/packages/core/src/render3/styling/class_differ.ts @@ -113,7 +113,7 @@ export function removeClass(className: string, classToRemove: string): string { * * @param className A string containing classes (whitespace separated) * @param classToToggle A class name to remove or add to the `className` - * @param toggle Weather the resulting `className` should contain or not the `classToToggle` + * @param toggle Whether the resulting `className` should contain or not the `classToToggle` * @returns a new class-list which does not have `classToRemove` */ export function toggleClass(className: string, classToToggle: string, toggle: boolean): string { diff --git a/packages/core/src/render3/util/attrs_utils.ts b/packages/core/src/render3/util/attrs_utils.ts index 3327e3643a..4b6201dfa4 100644 --- a/packages/core/src/render3/util/attrs_utils.ts +++ b/packages/core/src/render3/util/attrs_utils.ts @@ -5,7 +5,6 @@ * 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 {assertEqual} from '../../util/assert'; import {CharCode} from '../../util/char_code'; import {AttributeMarker, TAttributes} from '../interfaces/node'; import {CssSelector} from '../interfaces/projection'; diff --git a/packages/core/test/acceptance/host_binding_spec.ts b/packages/core/test/acceptance/host_binding_spec.ts index 320209c0b3..f6a018dadb 100644 --- a/packages/core/test/acceptance/host_binding_spec.ts +++ b/packages/core/test/acceptance/host_binding_spec.ts @@ -830,6 +830,30 @@ describe('host bindings', () => { expect(hostElement.title).toBe('other title'); }); + it('should merge attributes on host and template', () => { + @Directive({selector: '[dir1]', host: {id: 'dir1'}}) + class MyDir1 { + } + @Directive({selector: '[dir2]', host: {id: 'dir2'}}) + class MyDir2 { + } + + @Component({template: `
`}) + class MyComp { + } + + TestBed.configureTestingModule({declarations: [MyComp, MyDir1, MyDir2]}); + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + const div: HTMLElement = fixture.debugElement.nativeElement.firstChild; + expect(div.id).toEqual( + ivyEnabled ? + // In ivy the correct result is `tmpl` because template has the highest priority. + 'tmpl' : + // In VE the order was simply that of execution and so dir2 would win. + 'dir2'); + }); + onlyInIvy('Host bindings do not get merged in ViewEngine') .it('should work correctly with inherited directives with hostBindings', () => { @Directive({selector: '[superDir]', host: {'[id]': 'id'}}) diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index 145d48ce0d..dfd99b843c 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -191,6 +191,9 @@ { "name": "callHooks" }, + { + "name": "classIndexOf" + }, { "name": "concatString" }, @@ -279,7 +282,7 @@ "name": "findAttrIndexInNode" }, { - "name": "findDirectiveMatches" + "name": "findDirectiveDefMatches" }, { "name": "forceStylesAsString" @@ -407,9 +410,6 @@ { "name": "getStylingMapArray" }, - { - "name": "getTNode" - }, { "name": "growHostVarsSpace" }, @@ -444,7 +444,7 @@ "name": "incrementInitPhaseFlags" }, { - "name": "initNodeFlags" + "name": "initTNodeFlags" }, { "name": "initializeInputAndOutputAliases" @@ -533,6 +533,12 @@ { "name": "matchTemplateAttribute" }, + { + "name": "mergeHostAttribute" + }, + { + "name": "mergeHostAttrs" + }, { "name": "nativeAppendChild" }, @@ -638,9 +644,6 @@ { "name": "setClassName" }, - { - "name": "setCurrentDirectiveDef" - }, { "name": "setCurrentQueryIndex" }, @@ -719,9 +722,6 @@ { "name": "ɵɵelementEnd" }, - { - "name": "ɵɵelementHostAttrs" - }, { "name": "ɵɵelementStart" }, diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index 3233494b17..54896c13b1 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -347,9 +347,6 @@ { "name": "getStylingMapArray" }, - { - "name": "getTNode" - }, { "name": "growHostVarsSpace" }, @@ -372,7 +369,7 @@ "name": "incrementInitPhaseFlags" }, { - "name": "initNodeFlags" + "name": "initTNodeFlags" }, { "name": "insertBloom" @@ -518,9 +515,6 @@ { "name": "setClassName" }, - { - "name": "setCurrentDirectiveDef" - }, { "name": "setCurrentQueryIndex" }, @@ -572,9 +566,6 @@ { "name": "ɵɵdefineComponent" }, - { - "name": "ɵɵelementHostAttrs" - }, { "name": "ɵɵtext" } diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 84f82a8400..8f3e88c04d 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -437,6 +437,9 @@ { "name": "checkNoChangesInternal" }, + { + "name": "classIndexOf" + }, { "name": "cleanUpView" }, @@ -579,7 +582,7 @@ "name": "findAttrIndexInNode" }, { - "name": "findDirectiveMatches" + "name": "findDirectiveDefMatches" }, { "name": "findExistingListener" @@ -867,7 +870,7 @@ "name": "incrementInitPhaseFlags" }, { - "name": "initNodeFlags" + "name": "initTNodeFlags" }, { "name": "initializeInputAndOutputAliases" @@ -1040,6 +1043,12 @@ { "name": "matchTemplateAttribute" }, + { + "name": "mergeHostAttribute" + }, + { + "name": "mergeHostAttrs" + }, { "name": "nativeAppendChild" }, @@ -1217,9 +1226,6 @@ { "name": "setClassName" }, - { - "name": "setCurrentDirectiveDef" - }, { "name": "setCurrentQueryIndex" }, @@ -1382,9 +1388,6 @@ { "name": "ɵɵelementEnd" }, - { - "name": "ɵɵelementHostAttrs" - }, { "name": "ɵɵelementStart" }, diff --git a/packages/core/test/render3/node_selector_matcher_spec.ts b/packages/core/test/render3/node_selector_matcher_spec.ts index 58d8a5e129..42cfa7b0de 100644 --- a/packages/core/test/render3/node_selector_matcher_spec.ts +++ b/packages/core/test/render3/node_selector_matcher_spec.ts @@ -22,7 +22,7 @@ describe('css selector matching', () => { const tNode = (!attrsOrTNode || Array.isArray(attrsOrTNode)) ? createTNode(null !, null, TNodeType.Element, 0, tagName, attrsOrTNode as TAttributes) : (attrsOrTNode as TNode); - return isNodeMatchingSelector(tNode, selector, false); + return isNodeMatchingSelector(tNode, selector, true); } describe('isNodeMatchingSimpleSelector', () => { @@ -322,26 +322,6 @@ describe('css selector matching', () => { //
expect(isMatching('div', ['class', 'foo'], selector)).toBeFalsy(); }); - - it('should match against a class value before and after the styling context is created', - () => { - // selector: 'div.abc' - const selector = ['div', SelectorFlags.CLASS, 'abc']; - const tNode = createTNode(null !, null, TNodeType.Element, 0, 'div', []); - - //
(without attrs or styling context) - expect(isMatching('div', tNode, selector)).toBeFalsy(); - - //
(with attrs but without styling context) - tNode.attrs = ['class', 'abc']; - tNode.classes = null; - expect(isMatching('div', tNode, selector)).toBeTruthy(); - - //
(with styling context but without attrs) - tNode.classes = ['abc', 'abc', true]; - tNode.attrs = null; - expect(isMatching('div', tNode, selector)).toBeTruthy(); - }); }); });