fix(core): exclude class attribute intended for projection matching f…#54800
fix(core): exclude class attribute intended for projection matching f…#54800JoostK wants to merge 4 commits intoangular:mainfrom
Conversation
There was a problem hiding this comment.
This is the primary change to fix the bug.
There was a problem hiding this comment.
The below changes remove the class-matching logic from the path that uses findAttrIndexInNode to find a matching attribute, it was buggy (classIndexOf was incorrectly compared against !== -1, whereas it should have been === -1) and the logic is irrelevant because of the isCssClassMatching special-case (hence this was moved to its own else block entirely)
6515821 to
1c6781a
Compare
|
I pushed an additional commit that refactors the logic a bit, which I think makes it more clear but happy to drop it/move to a separate PR. |
|
@JoostK Thank you so much for fixing this on the runtime side! However, should we also try to get Template Pipeline to match the old TDB const array? It's a bit surprising to me that TP is not emitting Perhaps this PR resolves the potential issues around class selector matching, so it's not necessary to change the TP behavior? |
TP is only using I do appreciate how the runtime fix makes things more consistent and reduces code size, so from that perspective I don't think TP needs to revert to the TDB output. That said, I do recognise some risk involved with changing the runtime, but judging by the logic in |
|
To elaborate on the risk, it's twofold:
Both are unknown unknowns and TGP would help confirm 1 is not an issue, but it won't help us find issues for 2 (as they'd been discovered during the rollout of TP earlier) |
|
If we can run a TGP (I can do this during my day tomorrow) I think it would be reasonable to change the output - unless we uncover things. |
There was a problem hiding this comment.
Quick question: this line handles the case when we have class in the static attributes section (i.e. when consts array looks like this: ['class', 'a b']), do we still have this codepath in TP or we always process classes and generate [AttributeMarker.Classes, 'a', 'b']?
There was a problem hiding this comment.
It's still present; I've tried removing this entirely but that causes a bunch of tests to fail. I don't know by heart which ones are affected.
There could potentially be additional cleanup to converge to AttributeMarker.Classes, not sure how feasible that is though.
There was a problem hiding this comment.
Thanks for the reply. I've attempted to do the cleanup a while ago (via #39998), but didn't have a chance to land it due to the lack of time (and other priorities). Just wanted to leave a reference to that PR in case it might be useful somehow :)
There was a problem hiding this comment.
That's an interesting PR, very similar in the changes on the runtime side except for dropping implicit class values entirely. I think you're on to something in that PR 😄
|
TGP and a de-flaked, green one |
pkozlowski-opensource
left a comment
There was a problem hiding this comment.
LGTM with one nit comment that I would love to see addressed.
…rom directive matching
This commit resolves a regression that was introduced when the compiler switched from
`TemplateDefinitionBuilder` (TDB) to the template pipeline (TP) compiler. The TP compiler
has changed the output of
```html
if (false) { <div class="test"></div> }
```
from
```ts
defineComponent({
consts: [['class', 'test'], [AttributeMarker.Classes, 'test']],
template: function(rf) {
if (rf & 1) {
ɵɵtemplate(0, App_Conditional_0_Template, 2, 0, "div", 0)
}
}
});
```
to
```ts
defineComponent({
consts: [[AttributeMarker.Classes, 'test']],
template: function(rf) {
if (rf & 1) {
ɵɵtemplate(0, App_Conditional_0_Template, 2, 0, "div", 0)
}
}
});
```
The last argument to the `ɵɵtemplate` instruction (0 in both compilation outputs) corresponds with
the index in `consts` of the element's attribute's, and we observe how TP has allocated only a single
attribute array for the `div`, where there used to be two `consts` entries with TDB. Consequently,
the `ɵɵtemplate` instruction is now effectively referencing a different attributes array, where the
distinction between the `"class"` attribute vs. the `AttributeMarker.Classes` distinction affects
the behavior: TP's emit causes the runtime to incorrectly match a directive with `selector: '.foo'` to
be instantiated on the `ɵɵtemplate` instruction as if it corresponds with a structural directive!
Instead of changing TP to align with TDB's emit, this commit updates the runtime instead. This uncovered
an inconsistency in selector matching for class names, where there used to be two paths dealing with
class matching:
1. The first check was commented to be a special-case for class matching, implemented in `isCssClassMatching`.
2. The second path was part of the main selector matching algorithm, where `findAttrIndexInNode` was being used
to find the start position in `tNode.attrs` to match the selector's value against.
The second path only considers `AttributeMarker.Classes` values if matching for content projection, OR of the
`TNode` is not an inline template. The special-case in path 1 however does not make that distinction, so it
would consider the `AttributeMarker.Classes` binding as a selector match, incorrectly causing a directive to
match on the `ɵɵtemplate` itself.
The second path was also buggy for class bindings, as the return value of `classIndexOf` was incorrectly
negated: it considered a matching class attribute as non-matching and vice-versa. This bug was not observable
because of another issue, where the class-handling in part 2 was never relevant because of the special-case
in part 1.
This commit separates path 1 entirely from path 2 and removes the buggy class-matching logic in part 2, as
that is entirely handled by path 1 anyway. `isCssClassMatching` is updated to exclude class bindings from
being matched for inline templates.
Fixes angular#54798
…ching from directive matching
a7aa0ea to
36f9f37
Compare
|
caretaker note: although TGP is green for this PR, it's advised to sync on its own. |
…ching from directive matching
The logic in `isCssClassMatching` is only interested in two areas in the attributes: implicit attributes and the `AttributeMarker.Classes` area, with the first area only of interest for projection matching, not directive matching. This commit splits these two searches to make this more apparent.
36f9f37 to
9bf3c1a
Compare
|
This PR was merged into the repository by commit edc1740. |
…54800) The logic in `isCssClassMatching` is only interested in two areas in the attributes: implicit attributes and the `AttributeMarker.Classes` area, with the first area only of interest for projection matching, not directive matching. This commit splits these two searches to make this more apparent. PR Close #54800
…rom directive matching (#54800) This commit resolves a regression that was introduced when the compiler switched from `TemplateDefinitionBuilder` (TDB) to the template pipeline (TP) compiler. The TP compiler has changed the output of ```html if (false) { <div class="test"></div> } ``` from ```ts defineComponent({ consts: [['class', 'test'], [AttributeMarker.Classes, 'test']], template: function(rf) { if (rf & 1) { ɵɵtemplate(0, App_Conditional_0_Template, 2, 0, "div", 0) } } }); ``` to ```ts defineComponent({ consts: [[AttributeMarker.Classes, 'test']], template: function(rf) { if (rf & 1) { ɵɵtemplate(0, App_Conditional_0_Template, 2, 0, "div", 0) } } }); ``` The last argument to the `ɵɵtemplate` instruction (0 in both compilation outputs) corresponds with the index in `consts` of the element's attribute's, and we observe how TP has allocated only a single attribute array for the `div`, where there used to be two `consts` entries with TDB. Consequently, the `ɵɵtemplate` instruction is now effectively referencing a different attributes array, where the distinction between the `"class"` attribute vs. the `AttributeMarker.Classes` distinction affects the behavior: TP's emit causes the runtime to incorrectly match a directive with `selector: '.foo'` to be instantiated on the `ɵɵtemplate` instruction as if it corresponds with a structural directive! Instead of changing TP to align with TDB's emit, this commit updates the runtime instead. This uncovered an inconsistency in selector matching for class names, where there used to be two paths dealing with class matching: 1. The first check was commented to be a special-case for class matching, implemented in `isCssClassMatching`. 2. The second path was part of the main selector matching algorithm, where `findAttrIndexInNode` was being used to find the start position in `tNode.attrs` to match the selector's value against. The second path only considers `AttributeMarker.Classes` values if matching for content projection, OR of the `TNode` is not an inline template. The special-case in path 1 however does not make that distinction, so it would consider the `AttributeMarker.Classes` binding as a selector match, incorrectly causing a directive to match on the `ɵɵtemplate` itself. The second path was also buggy for class bindings, as the return value of `classIndexOf` was incorrectly negated: it considered a matching class attribute as non-matching and vice-versa. This bug was not observable because of another issue, where the class-handling in part 2 was never relevant because of the special-case in part 1. This commit separates path 1 entirely from path 2 and removes the buggy class-matching logic in part 2, as that is entirely handled by path 1 anyway. `isCssClassMatching` is updated to exclude class bindings from being matched for inline templates. Fixes #54798 PR Close #54800
…54800) The logic in `isCssClassMatching` is only interested in two areas in the attributes: implicit attributes and the `AttributeMarker.Classes` area, with the first area only of interest for projection matching, not directive matching. This commit splits these two searches to make this more apparent. PR Close #54800
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…rom directive matching
This commit resolves a regression that was introduced when the compiler switched from
TemplateDefinitionBuilder(TDB) to the template pipeline (TP) compiler. The TP compiler has changed the output ofif (false) { <div class="test"></div> }from
to
The last argument to the
ɵɵtemplateinstruction (0 in both compilation outputs) corresponds with the index inconstsof the element's attribute's, and we observe how TP has allocated only a single attribute array for thediv, where there used to be twoconstsentries with TDB. Consequently, theɵɵtemplateinstruction is now effectively referencing a different attributes array, where the distinction between the"class"attribute vs. theAttributeMarker.Classesdistinction affects the behavior: TP's emit causes the runtime to incorrectly match a directive withselector: '.foo'to be instantiated on theɵɵtemplateinstruction as if it corresponds with a structural directive!Instead of changing TP to align with TDB's emit, this commit updates the runtime instead. This uncovered an inconsistency in selector matching for class names, where there used to be two paths dealing with class matching:
isCssClassMatching.findAttrIndexInNodewas being used to find the start position intNode.attrsto match the selector's value against.The second path only considers
AttributeMarker.Classesvalues if matching for content projection, OR of theTNodeis not an inline template. The special-case in 1. however does not make that distinction, so it would consider theAttributeMarker.Classesbinding as a selector match, incorrectly causing a directive to match on theɵɵtemplateitself.The second path was also buggy for class bindings, as the return value of
classIndexOfwas incorrectly negated: it considered a matching class attribute as non-matching and vice-versa. This bug was not observable because of another issue, where the class-handling in part 2 was never relevant because of the special-case in part 1.This commit separates path 1 entirely from path 2 and removes the buggy class-matching logic in part 2, as that is entirely handled by path 1 anyway.
isCssClassMatchingis updated to exclude class bindings from being matched for inline templates.Fixes #54798