fix(core): exclude class attribute intended for projection matching f…

[JoostK]

…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

if (false) { <div class="test"></div> }

from

defineComponent({
  consts: [['class', 'test'], [AttributeMarker.Classes, 'test']],
  template: function(rf) {
    if (rf & 1) {
      ɵɵtemplate(0, App_Conditional_0_Template, 2, 0, "div", 0)
    }
  }
});

to

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 class 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 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

[JoostK]

This is the primary change to fix the bug.

[JoostK]

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)

[JoostK]

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.

[dylhunn]

@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 ['class', 'test'].

Perhaps this PR resolves the potential issues around class selector matching, so it's not necessary to change the TP behavior?

[JoostK]

However, should we also try to get Template Pipeline to match the old TDB const array? It's pretty surprising to me that TP is not emitting ['class', 'test'].

TP is only using ElementAttributes together with serializeAttributes to obtain the costs array. I haven't looked at TDB (it's gone 😝) but I suspect it differentiates between projection attributes on a template versus element attributes themselves.

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 findAttrIndexInNode I do believe it is the correct approach.

[JoostK]

To elaborate on the risk, it's twofold:

1. the runtime changes may impact existing patterns in an unexpected way, that is not discovered by our testing.
2. there may be additional places in the runtime outside of the reported issue that are affected by the TP's output change.

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)

[pkozlowski-opensource]

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.

[AndrewKushnir]

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']?

[JoostK]

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.

[AndrewKushnir]

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 :)

[JoostK]

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 😄

[pkozlowski-opensource]

TGP and a de-flaked, green one

[pkozlowski-opensource]

LGTM with one nit comment that I would love to see addressed.

[JoostK]

caretaker note: although TGP is green for this PR, it's advised to sync on its own.

[atscott]

This PR was merged into the repository by commit edc1740.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}