Repository navigation
refactor(compiler): disallow unsupported bindings and attributes on ng-content and ng-template - #71887
refactor(compiler): disallow unsupported bindings and attributes on ng-content and ng-template#71887JeanMeche wants to merge 1 commit into
Conversation
angular-robot
Bot
added
the
area: compiler
b7a8b82 to
f3dfcfe
Compare
f3dfcfe to
e99d42b
Compare
JeanMeche
changed the title
fdcecd6 to
af14208
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dbf45c7 to
363c2ef
Compare
JeanMeche
left a comment
JeanMeche
left a comment
There was a problem hiding this comment.
Nice improvement! Splitting the validation between r3_template_transform.ts (for syntactic errors that don't depend on component metadata: [attr.foo] on <ng-template>, property/event/reference bindings and selection/selector typos on <ng-content>) and the TCB (TcbContentAttributesOp for static attributes on <ng-content> that don't match a parent component's content projection slot) fits cleanly into the existing architecture and works across both ngtsc and ngp.
Extracting validateNgContentElement and validateNoAttributeBindings also keeps visitElement much easier to read.
I left a few comments on edge cases around multi-class attributes / comma-separated slot selectors, namespaced attributes, legacy .d.ts metadata, and reporting multiple invalid bindings.
| const attributes = new Set<TextAttribute>(); | ||
| for (const attr of child.attributes) { | ||
| const withoutAttr = removeAttributeFromSelector(selector, attr.name); | ||
| if (findProjectionSlot(matcher, ngContentSelectors, withoutAttr) !== slot) { |
There was a problem hiding this comment.
Two subtle edge cases with checking each attribute by removing it individually (withoutAttr = removeAttributeFromSelector(selector, attr.name)):
- Multiple classes in
class="...": BecauseremoveAttributeFromSelectorstrips the entireclassattribute and allselector.classNamesat once (lines 271–275), if a parent has<ng-content select=".used" />and a child writes<ng-content class="used unused" />, removingclasschanges the matched slot so the wholeTextAttributeis marked as used—allowing the redundantunusedclass token through without a diagnostic. - Comma-separated / redundant selectors on the same slot: If a parent slot uses a selector list like
<ng-content select="[slot], .foo" />and a child writes<ng-content slot class="foo" />, removingslotalone still matches slot0via.foo, and removingclassalone still matches slot0via[slot]. Because neither attribute changesslotwhen removed on its own, both attributes will be flagged as unsupported even though at least one is required to match the slot.
| function createContentCssSelector(node: Content): CssSelector { | ||
| const selector = new CssSelector(); | ||
| for (const attr of node.attributes) { | ||
| const name = splitNsName(attr.name)[1]; |
There was a problem hiding this comment.
splitNsName(attr.name)[1] strips the namespace prefix and adds the unprefixed name to CssSelector. However, at runtime Angular's projection matcher (findAttrIndexInNode in packages/core/src/render3/node_selector_matcher.ts) explicitly skips AttributeMarker.NamespaceURI attributes altogether. If <ng-content> has a namespaced attribute (e.g. xlink:title), createContentCssSelector will match a parent's [title] slot even though it won't match at runtime. Consider skipping attributes where splitNsName(attr.name)[0] !== null.
| if ( | ||
| meta === undefined || | ||
| meta.ngContentSelectors === null || | ||
| meta.ngContentSelectors.length === 0 |
There was a problem hiding this comment.
In ngtsc's DtsMetadataReader (packages/compiler-cli/src/ngtsc/metadata/src/dts.ts), a .d.ts component with no <ng-content> slots (NgContentSelectors = never) has ngContentSelectors: [], whereas ngContentSelectors: null indicates a legacy .d.ts declaration (typeArguments.length <= 6) where projection slots were not recorded in the type definition.
Returning early here when meta.ngContentSelectors === null treats unknown slots the same as zero slots, which could produce false-positive diagnostics if <ng-content someAttr> is projected into a component from an older .d.ts.
| references: t.Reference[], | ||
| attributes: t.TextAttribute[], | ||
| ): void { | ||
| if (boundEvents.length > 0) { |
There was a problem hiding this comment.
Nit: boundEvents, parsedProperties, and references only report the first item ([0]), whereas attributes below (and validateNoAttributeBindings) loop over every item. Iterating over all elements in boundEvents, parsedProperties, and references would report all unsupported bindings on <ng-content> in one pass rather than requiring the user to fix them one at a time.
12b8bb1 to
6999080
Compare
…g-content and ng-template
Virtual elements such as <ng-content> and <ng-template> do not support
certain bindings and attributes:
- <ng-template> does not support attribute bindings ([attr.foo]).
- <ng-content> does not support property bindings ([id]), event bindings ((click)),
or template references (#ref).
- <ng-content> only supports an allowlist of static attributes ('select',
'ngProjectAs', 'ngSkipHydration', 'ngNonBindable', 'ngPreserveWhitespaces',
and i18n attributes 'i18n', 'i18n-*'), as well as static attributes that
select a content projection slot of a parent component when re-projecting
<ng-content>. If 'selection' or 'selector' is encountered on <ng-content>,
a helpful 'Did you mean select?' suggestion is provided.
Syntactic binding and reference checks are enforced during template
transformation (r3_template_transform.ts), while static attribute checks on
<ng-content> are performed during TCB generation (TcbContentAttributesOp) so
that parent component projection slots can be taken into account across both
ngtsc and ngp.
Also removes legacy compliance tests in r3_view_compiler_template that asserted
unsupported attribute and class bindings on ng-template degraded to property
bindings, and fixes tests and examples where invalid attributes were used on
<ng-content>.
6999080 to
797374e
Compare
This PR extends compiler and template type-checking validation for virtual elements to
<ng-content>and<ng-template>:r3_template_transform.ts):<ng-template>does not support attribute bindings ([attr.foo]).<ng-content>does not support property bindings ([id]), event bindings ((click)), or template references (#ref).TcbContentAttributesOpinngtscandngp):<ng-content>enforces an allowlist for static attributes:select(case-insensitive)ngProjectAsngSkipHydrationngPreserveWhitespaces<ng-content>when re-projecting into a parent component if the attribute selects a specific content projection slot of that parent component.selectionorselectoris used on<ng-content>(and does not match a parent projection slot), a helpful suggestion is provided:'selection'/'selector' is not supported on ng-content. Did you mean 'select'?.Also removes legacy compliance tests in
r3_view_compiler_templatethat asserted unsupported attribute and class bindings on<ng-template>degraded to property bindings, and fixes invalid attributes found in existing tests and examples whereslot="foo",selector="...", orclass="..."were mistakenly used on<ng-content>.