fix(animations): make sure query works with shadow dom elements by dario-piotrowicz · Pull Request #46488 · angular/angular

dario-piotrowicz · 2022-06-24T12:37:38Z

fix the query function not taking into consideration element inside
components using the shadowDom view encapsulation

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

The query function does not check elements inside shadow roots (which are created with the SahdowDom view encapsulation)

For example:

What is the new behavior?

Elements inside shadow roots are also being considered

For example:

Does this PR introduce a breaking change?

Yes
No

Other information

There is no equivalent of querySelector that also takes shadowRoot elements into account so I needed to implement it myself, this is of course bound to be much less performant than the native querySelector functions, but I think that it should be ok for the use case, still something to keep in mind
I made sure to keep the ordering of query the same as the querySelector one (which according to MDN is DFS) so the behavior should be the exact same as before
In my implementation I used recursion which I believe is the most natural and clean way to accomplish this, I don't expect it being able to blow the stack at all, but if there are some concerns please let me know and I can do an iterative version (which would likely be more complex and less clear and maintainable)

dario-piotrowicz · 2022-06-24T12:38:56Z

packages/animations/browser/src/render/transition_animation_engine.ts

@crisbeto as promised here 🙂

JoostK

I left some suggestions to reduce memory pressure.

JoostK · 2022-06-24T12:44:27Z

packages/animations/browser/src/render/shared.ts

This generates quite a bit of garbage (intermediate arrays) which can easily be avoided if we just iterate of element.children and then element.shadowRoot.children (have two loops instead of one).

good point, code wise it will not be as nice but you're totally right

I did that via a single look over the two arrays, may look a bit ugly, please let me know what you think

(the alternative would be to either have duplicated code or define a local function to extract the logic, please let me know if those alternatives seem better to you)

packages/animations/browser/src/render/shared.ts

JoostK · 2022-06-24T12:50:01Z

packages/animations/browser/src/render/shared.ts

This function can likely become a top-level function? It doesn't capture anything from the _query arrow block.

well, it sets the starting element to true, I would not want that to be done by the caller (as it would also change the API), I could have the isStartElement flag defaulting to true, but I find it more clear/clean this way, the inner function is encapsulated in the outer which exposes the correct interface without the caller needing, or even being able to provide the values that it should not care about

do you disagree?

Oh I was suggesting to keep _query as is, but to move queryFn out to a top-level function (non-exported). That way the _query function still dictates the initial call into queryFn.

ah ok, yeah that makes sense 🙂👍

fix the query function not taking into consideration element inside components using the shadowDom view encapsulation

address JoostK comments

dario-piotrowicz · 2022-06-24T17:41:32Z

@JoostK thanks a lot for the awesome suggestions! 😃

I replied to all of them (and applied most), please have another look 🙂

About the accumulator, I am not sure if you had in mind something different but I am returning it and re-assigning it (this should not add overhead, let me know if this is not ok)

JoostK

I only realize now that this completely replaces querySelectorAll with a "manual" DOM walk. We'd really have to see how this affects performance, as I can imagine that querySelectorAll is able to leverage optimizations that we won't be able to leverage now.

If performance does take a hit, an alternative might be to walk the DOM using a TreeWalker with NodeFilter.SHOW_ELEMENT filter. I don't know how that interacts with shadow roots, perhaps the NodeFilter.SHOW_DOCUMENT_FRAGMENT filter also affects this (would need some experimentation, I don't know how the TreeWalker API behaves exactly).

JoostK · 2022-06-24T18:07:12Z

packages/animations/browser/src/render/shared.ts

Oh I was suggesting to keep _query as is, but to move queryFn out to a top-level function (non-exported). That way the _query function still dictates the initial call into queryFn.

JoostK · 2022-06-24T18:11:31Z

packages/animations/browser/src/render/shared.ts

+      const childEls = Array.from(element.children);
+      const shadowChildEls = element.shadowRoot ? Array.from(element.shadowRoot.children) : [];


HTMLCollection is iterable (hence you can pass it into Array.from) and there shouldn't be a need to copy it to an array; it can be accessed just like an array or using for..of syntax.

I have been having an issue with this, basically it turns out that Universal uses domino for the DOM interaction, so children normally is an HTMLCollection but it is a ChildrenCollection in Universal (and in some of the Angular's tests as well, that's how I've found the issue)

ChildrenCollection has a different interface and it's not iterable, so unfortunately I will have to add here some code to handle both 😓

Oh interesting! I was looking for when HTMLCollection became iterable and I think it was with ES2015, but couldn't really find a definitive answer for this. You can always index into them with the item method and an indexed loop.

cool, thanks, I'll try to look into that 🙂 (as I am really not familiar with domino), one thing I would like to try is to extract the data from either HTMLCollection or ChildrenCollection into an iterable of Element so that they can be treated in the same way, anyways I will need to investigate/play a bit around it 🙂

I just checked using material and the query actually seem to be working fine there.... I wonder, probably the animations are not kicked in in SSR and this is an issue only with the tests then 🤔 🤷‍♂️

anyways at this point I feel like I can put this on hold and try to look in the performance thing first 🤔

JoostK · 2022-06-24T18:15:53Z

packages/animations/browser/src/render/shared.ts

+          return matched;
+        }
+      }
+      return matched;


nit: no need to return matched really, given that it is the input parameter. It would mean that the initial call has to become:

const matched = []; queryFn(element, selector, multi, matched, true); return matched;

Looking at it more, I think you'll want to have a boolean return value that indicates whether the query can terminate (as an alternative to the !multi && matched.length condition). You can then write the loop as

for (const el of element.children) { if (queryFn(el, selector, multi, matched, false)) { return true; } }

and then it doesn't really hurt to duplicate that loop.

I am not sure, I really don't think that providing the result of functions through the inputs is a nice pattern

We are sort of already cheating by modifying the array, but that is sort of ok because of the performance gain I think, regarding not returning the array I am not sure if that would provide much gain performance wise since retruning and assigning it are based on the array reference, so they shouldn't be problematic.

If we then also need to add a boolean flag in the return it does seem to me to increase the readability of the solution without much gain.

So yeah from my point of view I think you're right that returning the value is not really necessary and can be avoided, but I think that avoiding it just makes the code less readable and I can't see too much gain in doing so 🤔

(By the way, I know that using inputs as outputs is done in other places, for example in the animations package, this is very often used to collect errors, but regardless, I do personally think is a pattern to be avoided if possible)

My primary motivation to not return the array is that it's slightly misleading, similar to how Array.prototype.sort is misleading: the return value would make me believe that it returns an owned array which is distinct from the input, but that is not the case here.

As far as "collecting" results into an array that is passed as parameter, I believe that is a pretty typical pattern. It's probably more appropriate if the function wasn't named queryFn, but rather collectQueryMatches; then it's much clearer from just looking at the name how the function gathers results.

You do have a point in the fact that it is slightly misleading

I am still not a big fan of providing the return via an input variable, but I get your point (and it's not like we can clone the array in the function as that would be too expensive 😓) so ok, I'll update the function and its name 🙂👍

Regarding the boolean, I am not sure, it seems like !multi && matched.length still works fine, do you see some issue there?

Now that you've extracted the loop out into collectQueryMatchesOnChildren the boolean return is a bit less helpful.

I do think I prefer it like this though, but this may be personal and may be getting into stylistic preference.

function collectQueryMatches( element: Element, selector: string, multi: boolean, matched: Element[], isStartElement: boolean): boolean { const matches = isStartElement ? false : element.matches(selector); if (matches) { matched.push(element); if (!multi) { return true; } } for (const el of element.children) { if (collectQueryMatches(el, selector, multi, matched, false)) { return true; } } if (element.shadowRoot) { for (const el of element.shadowRoot.children) { if (collectQueryMatches(el, selector, multi, matched, false)) { return true; } } } return false; }

The bigger thing to discover is the perf impact of this overall approach; if an entirely different approach is needed then this is all moot anyway.

I don't hate your version, I always try to avoid code duplication if possible and that's why I would probably go with my version, but I am happy to compromise and apply yours 🙂 (it's also probably slightly more efficient since there are less function calls involved)

Anyways yeah the performance thing is the biggest issue, I will look into it soon (I sort of wanted to address the other small issues first)

Ah by the way, I need to add as any as Iterable<Element> to make the for-ofs work 😓

dario-piotrowicz · 2022-06-24T21:09:18Z

I only realize now that this completely replaces querySelectorAll with a "manual" DOM walk. We'd really have to see how this affects performance, as I can imagine that querySelectorAll is able to leverage optimizations that we won't be able to leverage now.

If performance does take a hit, an alternative might be to walk the DOM using a TreeWalker with NodeFilter.SHOW_ELEMENT filter. I don't know how that interacts with shadow roots, perhaps the NodeFilter.SHOW_DOCUMENT_FRAGMENT filter also affects this (would need some experimentation, I don't know how the TreeWalker API behaves exactly).

Yeah I totally agree, I did mention this in the PR's description, 100% this is going to be much much less performant than the native querySelector function.

My guess is that it shouldn't be too bad, but if you do apply a query in the root of a page full of components this could indeed cause some issues, do you think this can be checked with a presubmit or some sort of automated way?

(Regarding the TreeWalker that's an awesome suggestion, I wasn't actually familiar with that, do you think that would be more performant?)

JoostK · 2022-06-24T21:22:09Z

My guess is that it shouldn't be too bad, but if you do apply a query in the root of a page full of components this could indeed cause some issues, do you think this can be checked with a presubmit or some sort of automated way?

I'm afraid the answer is no, but you can create a synthetic benchmark with various DOM tree sizes/depths, shadow roots present or not, etc, and then compare the various options. It is very hard to create realistic benchmark setups though and it's hard to avoid bias towards some scenario, but at least it should give an idea how the various approaches behave (my concern about performance is only speculative at the moment, I do not have any data to back it up).

(Regarding the TreeWalker that's an awesome suggestion, I wasn't actually familiar with that, do you think that would be more performant?)

I have no clue of the performance characteristics of either approach (querySelectorAll vs recursively walking children vs TreeWalker)

address JoostK newer comments and improve code

get rid or collectQueryMatchesOnChildren function

dario-piotrowicz · 2022-06-27T20:07:14Z

So @JoostK regarding the performance I have added some console.times in the code and run it again the material website

One experiment was to add a trigger at the root of the application which query would basically go through the entire dom (in the material page I tested, the button one, there are almost 700 dom elements).

With that query I could see some different in numbers, but quite small, the new query took ~4.3 ms vs the querySelectorAll which took ~0.4 ms.

Then I went through other pages and usually the 2 query versions do take a similar amount of time (with the new one being slightly slower), with some spikes here and there for some reason.

You can see the process here:

(the first log is the root query experiment, and after that I just clicked around trying to see what animates)

I put the results in (kinda) excel and this is the result I got:

As you can see the means are very similar but there can be quite slower queries with the new implementation, although always in the order of a few milliseconds.

This is a first benchmarking tests I though of, it is a bit up in the air (I did not plan what to test, etc...) but it does show some results (I can do other tests if you want, and if you have suggestions please let me know 🙂), what do you think of them?

JoostK · 2022-06-28T16:57:37Z

Thanks for sharing the results! The 10x initial regression seems bad, but I wonder if it's due to a cold VM. There are a couple more significant outliers that are a bit worrying though. Do you happen to have CPU profiles as well?

dario-piotrowicz · 2022-06-28T17:21:03Z

Thanks for sharing the results! The 10x initial regression seems bad, but I wonder if it's due to a cold VM. There are a couple more significant outliers that are a bit worrying though. Do you happen to have CPU profiles as well?

I don't have the CPU profiles I can make them though if you want (by the way, if it makes any difference my machine is an old i5 quad core running ubuntu....)

Regarding the first query, I doubt that that is related to anything like cold VM, this is how I run that:

Basically it runs after 5 seconds and so it runs then all is pretty much stable

The big different is that this query is executed from the root of the application so the dom tree that gets queries is much bigger than how it usually is (around ~700 elements). That is sort of a stress test, I really doubt that people use animations like that, but I think it is still a sort-of possibility so it should be handled anyways.

Even thought there is a 10x difference, both numbers are so small anyways that it should not be that bad, don't you think?

JoostK · 2022-06-28T17:30:08Z

By "cold VM" I mostly mean non-optimized code paths; the VM needs to gather some statistics before being able to obtain the best level of optimizations and this is needed per code path; only once a code path has been run sufficiently many times would more optimum code be generated. So even though there is a 5s wait time, the quering logic might still be cold if it hasn't been exercised before.

4ms of a render cycle is about 25% of the frame budget, so it's quite a bit IMO. I realise those are the worst case figures you've observed for a full DOM scan, but a 10x regression is significant.

dario-piotrowicz · 2022-06-28T17:51:10Z

By "cold VM" I mostly mean non-optimized code paths; the VM needs to gather some statistics before being able to obtain the best level of optimizations and this is needed per code path; only once a code path has been run sufficiently many times would more optimum code be generated. So even though there is a 5s wait time, the quering logic might still be cold if it hasn't been exercised before.

4ms of a render cycle is about 25% of the frame budget, so it's quite a bit IMO. I realise those are the worst case figures you've observed for a full DOM scan, but a 10x regression is significant.

Thanks for the explanation, I did misunderstood what you meant 🙂

Unfortunately anyway no, the difference is around ~10x even if that query code was already run

As you can see here:

dario-piotrowicz · 2022-06-28T17:58:54Z

I do understand what you mean about it being significant, anyways this is functionality which simply does not work.

I can try with TreeWalker but I feel that anything will always be much much slower than the querySelector functions anyways.

On the other hand I am not even sure how many people use the shadowDom and how many use it alongside animations.

So I am not sure this may not even be worth it, but if not maybe this should be properly communicated to devs.

(Like for example putting in the docs that the animations are not guaranteed to work alongside the shadowDom or something).

What do you think? 🤔

I would also be curious to know what @jessicajaniuk thinks of it 🙂

JoostK · 2022-06-28T19:33:29Z

I just looked at the Blink source code, and they do indeed implement a fast path for class-only queries:

https://github.com/chromium/chromium/blob/6e1c92f807d788fba18dcde72564a026963f57bb/third_party/blink/renderer/core/css/selector_query.cc#L227-L235

It still does a full traversal though just like we do "manually" now. A CPU profile might indicate where the bottleneck occurs; either in walking the DOM (in which case TreeWalker might help) or in selector matching (in which case you can see if a class-only optimization as done by Blink makes any difference, although I don't find it very appealing to replicate such optimization so this would primarily be an experiment)

crisbeto · 2022-06-28T19:41:34Z

I got lost in the discussion at some point, but I just wanted to throw this module out there https://github.com/salesforce/kagekiri. It does querySelector in the shadow DOM which is what we're basically doing here.

JoostK · 2022-06-28T19:49:42Z

Ah, interesting, I've never seen that lib. I wonder now if we need support for CSS selectors that span across shadow roots; I doubt that the animations engine needs it (it only queries for simple class names, I believe) but the main question is whether using query in animations should pierce shadow roots. I don't think it should (as I'd expect it to behave like native querySelectorAll) but then I'm not sure what the implications are for use-cases which use shadow DOM.

dario-piotrowicz · 2022-06-28T21:19:30Z

@crisbeto thank you so very much, I'll have a look and see how that lib fares 🙂

Anyways it seems like a barely maintained library which has only a handful of developers, I think it would be a bit too risky to include it as a dependency in the angular core lib, wouldn't it?

@JoostK regarding your comment about piercing or not shadow doms, if an application were to use shadowDom view encapsulation I can definitely see this hindering quite a bit the use of the animations, but again, I am not sure how much shadowDom is actually used

The other thing that I would like to mention/ask, is what the emulated view encapsulation is supposed to be, is it assumed to be its own thing? or should it be a view encapsulation which is as close as possible to the real shadowDom one?
Are there other instances in which we want to pierce through the emulated view encapsulation but not the shadow dom one?
(What I believe, which may be wrong, is that the emulated one should behave as close as possible to the native shadowDom one, and having different behaviors between the two should be carefully documented as it can also quite likely confuse developers)

Add a section regarding component view encapsulations in the complex animation sequences guide to let developers know how animations work in regard to view encapsulations. This section is used to both add information to the guide but also to take a stand on how the animations should behave in regard to the shadow dom view encapsulation. This relates to PR angular#46488 which by trying to make sure the query function works with shadow dom elements proved that having the animations implementation work with shadow dom would increase the complexity of the code and would have a negative impact on performance, such facts alongide the small adoption of shadow dom by users influenced the decision to keep the current functionalities and simply discourage the use of animations and shadow dom components.

dario-piotrowicz · 2022-07-07T15:49:38Z

@JoostK and @crisbeto Thank you so very much for having a look at the PR 🙂

I had a chat with @jessicajaniuk and we concluded that it is probably not worth increasing the complexity of the code and decrease performance in order to have the animations work with shadow dom but it is better to simply say that the animations implementation hasn't been designed to work with shadow dom and discourage users from using both together.

So I added a section in the docs about this: #46738

Please let me know what you think, hopefully you agree with the decision 🙂

…#46738) Add a section regarding component view encapsulations in the complex animation sequences guide to let developers know how animations work in regard to view encapsulations. This section is used to both add information to the guide but also to take a stand on how the animations should behave in regard to the shadow dom view encapsulation. This relates to PR #46488 which by trying to make sure the query function works with shadow dom elements proved that having the animations implementation work with shadow dom would increase the complexity of the code and would have a negative impact on performance, such facts alongide the small adoption of shadow dom by users influenced the decision to keep the current functionalities and simply discourage the use of animations and shadow dom components. PR Close #46738

angular-automatic-lock-bot · 2022-08-07T16:06:06Z

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.}

pullapprove bot requested a review from jessicajaniuk June 24, 2022 12:37

dario-piotrowicz commented Jun 24, 2022

View reviewed changes

JoostK reviewed Jun 24, 2022

View reviewed changes

dario-piotrowicz added 2 commits June 24, 2022 18:16

fix(animations): make sure query works with shadow dom elements

8e08ae7

fix the query function not taking into consideration element inside components using the shadowDom view encapsulation

fixup! fix(animations): make sure query works with shadow dom elements

7186b9f

address JoostK comments

dario-piotrowicz force-pushed the animations-query-shadow-dom branch from 06658f7 to 7186b9f Compare June 24, 2022 17:41

dario-piotrowicz requested a review from JoostK June 24, 2022 17:41

JoostK reviewed Jun 24, 2022

View reviewed changes

dylhunn added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime core: CSS encapsulation target: minor This PR is targeted for the next minor release labels Jun 24, 2022

ngbot bot modified the milestone: Backlog Jun 24, 2022

dario-piotrowicz added 2 commits June 26, 2022 20:09

fixup! fix(animations): make sure query works with shadow dom elements

1b89263

address JoostK newer comments and improve code

fixup! fix(animations): make sure query works with shadow dom elements

0c0f540

get rid or collectQueryMatchesOnChildren function

dario-piotrowicz mentioned this pull request Jul 7, 2022

docs(animations): add section about animations and view encapsulation #46738

Closed

14 tasks

dario-piotrowicz closed this Jul 7, 2022

dario-piotrowicz deleted the animations-query-shadow-dom branch July 7, 2022 15:51

angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 7, 2022

		const childEls = Array.from(element.children);
		const shadowChildEls = element.shadowRoot ? Array.from(element.shadowRoot.children) : [];

Conversation

dario-piotrowicz commented Jun 24, 2022

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

dario-piotrowicz Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz commented Jun 24, 2022

Uh oh!

JoostK left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Jun 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Jun 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz commented Jun 24, 2022

Uh oh!

JoostK commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dario-piotrowicz commented Jun 27, 2022

Uh oh!

dario-piotrowicz Jun 24, 2022 •

edited

Loading

dario-piotrowicz Jun 24, 2022 •

edited

Loading

JoostK left a comment •

edited

Loading

dario-piotrowicz Jun 26, 2022 •

edited

Loading

dario-piotrowicz Jun 26, 2022 •

edited

Loading

JoostK commented Jun 24, 2022 •

edited

Loading

dario-piotrowicz commented Jun 28, 2022 •

edited

Loading