[WNMGDS-3009][WNMGDS-3010] Add Angular example project and fix Angular web component content issue #3275

pwolfert · 2024-10-11T21:23:41Z

Summary

Angular example project

I took the basic Angular project off of stackblitz.com and added some our web components. Because I lack modern Angular experience, I'm looking for feedback and help to...

Make sure this is a realistic and helpful example of an Angular project
Implement event handling so it's more interactive and exercises the different features of our web components

Fix for Angular providing content to web components

In this Angular project, we're trying to provide inner HTML content to certain web components like the ds-alert and ds-button by Angular's standard methods like putting the content in a template like this:

<ds-alert heading="You've loaded the web-components example">
  <p>
    This is an example of a success alert. If you want to see an error alert, click the
    button below.
  </p>
  <ds-button>Break things</ds-button>
</ds-alert>

However, what we found is that when the CustomElement instances (web components) tried to read their this.innerHTML to get that content, it was blank—just an empty string. That's the bug. That's what our Angular product teams were experiencing. They had a fix, which was to supply the content through an innerHTML attribute (a special Angular binding), but this method is cumbersome and not ideal.

It turns out that Angular wasn't failing to supply the content to the CustomElement; it was just supplying it late. We already had a MutationObserver that was looking out for changes to the inner content, and it was indeed finding those late arrivals. However, our renderPreactComponent wasn't set up to respond well to this "it's blank—psych!—no it's not" behavior.

How to test

Update dependencies with yarn install
From the examples/angular directory, run yarn start

Checklist

Prefixed the PR title with the Jira ticket number as [WNMGDS-####] Title or [NO-TICKET] if this is unticketed work.
Selected appropriate Type (only one) label for this PR, if it is a breaking change, label should only be Type: Breaking
Selected appropriate Impacts, multiple can be selected.
Selected appropriate release milestone

These components show the current issue, which is that our custom elements don't have any initial `innerHTML`, which causes various symptoms. For the month picker, we don't get the default checked state or the disabled months. For the alert content, we don't get any of the alert content, but we get the header because it was defined with an attribute.

When Angular renders the web component at first, it doesn't have any innerHTML, but then it inserts the content later and comes in under the watchful eye of our MutationObserver. This might be a bad assumption, but the code I've added just now assumes the same kind of thing that we assumed before that if we're getting mutations to the root content, it means that we want to replace the previous content and re-render everything. Instead of the mutation kicking off the `renderPreactComponent` function and trying to read the `innerHTML` that isn't really settled with the new content, it will take the new content from the mutations and use that instead as the new content to render. For this to work, we're not taking into account previously defined template elements. I have to think about this a bit more, but I think after our changes to make the web components more stateful, we don't actually need to keep the old template around...that could be wrong. I need to think about it more.

It was the `correctly caches children when moved in the DOM` test, and it caught that when a component was moved in the DOM it ended up with nested templates. The fix for that is to bring back the old logic of only creating a template if one doesn't already exist WHEN we're not dealing with inner content mutations.

This reverts commit 32237c1. Apparently we can't both make this unit test happy and have working Angular examples. I'm going to have to think about this scenario some more. If I bring back the code that will look for previous templates when `addedNodes` doesn't exist, nested web components don't work right in the Angular examples. Try checking out 32237c1 and running the `examples/angular` project.

pwolfert · 2024-10-17T00:26:16Z

Regarding c578c7c, the only place I see a problem without that "fix" is with the Astro example. The nested button inside the alert gets essentially a double-render that puts a button inside the button. This is a similar symptom to what we had before we implemented the template memory thing (using a template to store the original inner content and falling back to that when we need to "re-hydrate" the web component). The problem doesn't show up in any of the other nested components like the accordions or the choice checked children, and it doesn't seem to be a problem in any of the other example projects or inside Storybook. Perhaps Astro is doing something special to the DOM?

I think next I'll try actually removing the template if it is empty so even the empty template doesn't get duplicated inside the new template

where we not only create a new template if the previous one was empty, but we remove the empty template before creating the new one so that it doesn't make its way into the content of the new one. Before this fix, the Angular-rendered alert example with a nested button would end up looking like this in the DOM: ```html <ds-button> <button type="button" class="ds-c-button ds-c-button--alternate ds-c-button--solid" > <template><span></span></template> Break things </button> <template> <span> <template><span></span></template> Break things </span> </template> </ds-button> ``` The above HTML has an empty content inside the actual button content, and the top-level template for the `ds-button` element has a another template inside of it! After this fix, it looks as we expect it: ```html <ds-button> <button type="button" class="ds-c-button ds-c-button--alternate ds-c-button--solid" > Break things </button> <template><span>Break things</span></template> </ds-button> ```

pwolfert · 2024-10-23T22:32:44Z

In our last pairing session, we started putting dynamic content into the <ds-alert> body, like this:

<ds-alert heading="Alert with dynamic content">
  <p>Count: {{count}}</p>
  <ds-button>Increment</ds-button>
</ds-alert>

but we found that the web component content did not update on its own when the Angular variable count changed. If we could possibly get the Angular component itself to re-render its template, it might cause the root elements of the already rendered web component to change and trigger its MutationObserver. However, it doesn't seem to be doing that. We did find that if we accessed the ds-alert element instance within an Angular function and manually called that CustomElement's renderPreactComponent() function, it would re-render with the updated dynamic content. For instance, if the alert text said, "Count: 0" before and we incremented it by 1, re-rendering would result in it successfully showing "Count: 1". There's conceivably some sort of wrapper we could create for our web components in Angular that could detect changes and manually call our renderPreactComponent function. A similar solution would be to allow a framework to pass some sort of publisher that the web component can subscribe to for changes.

Another problem we noticed that we didn't come up with any solution for was that nested components can't have Angular bindings. If a web component has various kinds of bindings defined through the Angular template, but that web component is inside another web component, the rendering of the top-level web component clobbers all of that. When one of our web components renders, it reads in its inner HTML content through a <template> element that then gets converted to Preact VirtualDOM elements. In that process, it's basically making clones of everything, but it doesn't retain any of its event bindings or other meta-data. This seems like a fundamental difference in how Preact works vs Angular, and I really don't know what the solution is here. The only comfort we have is in the fact that you can use the browser native APIs like element.addEventListener to bind events even if you can't use Angular's way.

Ideally if we have to have an Angular wrapper, we could detect changes made by Angular and trigger a re-render, but nothing has seemed to work. This is the only thing so far that has worked.

pwolfert · 2024-11-05T22:15:11Z

A note on event binding: The only place it doesn't seem to work is in nested web components—because the original element that had the binding gets clobbered by the parent web component rendering. I don't see a way around that in Angular, and it could be problematic for container-like components like the dialog, drawers, and accordions.

tamara-corbalt · 2024-11-06T17:23:35Z

I implemented the following polling code at the end of the renderPreactComponent function:

  let previousContentSnapshot = template.content.cloneNode(true);

  const hasContentChanged = () => {
    const currentSnapshot = template.content.cloneNode(true);
    const isDifferent = !currentSnapshot.isEqualNode(previousContentSnapshot);

    if (isDifferent) {
      // console.log('Template content modified!', currentSnapshot);
      previousContentSnapshot = currentSnapshot;
      this.renderPreactComponent([...template.content.firstChild.childNodes]);
    }
  }

  if ((this as any).__pollInterval) {
    clearInterval((this as any).__pollInterval)
  }
  (this as any).__pollInterval = setInterval(hasContentChanged, 500);

It works, but I imagine it's not terribly performant. Every web component on the page will be comparing DOM nodes every half second. If we really did want to use this, we could put it in an Angular wrapper so at least we only get the performance hit in Angular.

Hey @pwolfert, thanks for this detailed explanation!
I have a question about the polling code and the idea of using an Angular wrapper. My assumption is that to isolate the polling to Angular situations, we’d move this logic into a directive in our Angular project. My question is, could we try an event-driven approach instead of using a set interval to check for changes in the template snapshot? For example, could we subscribe to an observable that detects changes in the Angular environment and then call hasContentChanged only when needed?

Just a heads up, I’m not totally confident I understand all the context here.

pwolfert · 2024-11-06T17:32:34Z

@tamara-corbalt, being able to detect changes would be vastly superior, but that is the very thing I can't seem to do. Take a look at the comments for this commit. I've tried many different things but haven't been able to solve that problem yet. I dislike the polling solution too, but I haven't been able to come up with anything else that works. Everyone should feel free to experiment with this and try to detect when Angular makes changes!

pwolfert · 2024-11-11T19:07:53Z

I want to put it down in writing here because this is a pretty good record of these problems we're working through:

I've said recently that we likely wouldn't have some of the same problems if we were using the Shadow DOM in our components, but I also couldn't articulate on the spot the accessibility reasons we chose to not use the Shadow DOM in the first place. This article is likely one of the ones we referenced when coming to this decision, and I think it does a great job explaining the problem and the work that has been done to try to find solutions. We also didn't make the decision in a vacuum; accessibility specialists were in agreement with us that introducing a Shadow DOM would create accessibility problems. If the team were to want to move towards using Shadow DOMs, there'd need to be a pretty extensive research phase that includes working with product teams to find out how they use ARIA within their applications beyond what we've mentioned in our own guidance in order to make sure the design system solutions can support all accessibility needs.

One of the problems we're facing right now with Angular is that the web component render process loses event bindings on nested web components. I don't think that would actually be fixed by switching to the Shadow DOM, because the events that Angular binds are on the input elements and not the output elements. The elements provided to web components as content are not the same exact elements that are rendered in the final product. I'd be curious to see some research into whether other web component frameworks do support that scenario. Does Lit allow Angular to bind a click event listener to an interactive custom element inside a container-like custom element and not lose it?

pwolfert · 2024-11-11T22:47:46Z

I'd be curious to see some research into whether other web component frameworks do support that scenario. Does Lit allow Angular to bind a click event listener to an interactive custom element inside a container-like custom element and not lose it?

Update: Angular bindings do work with Lit because the "input" elements using slots are preserved in the Shadow DOM "output" (which is rendered in a <slot> element).

tamara-corbalt · 2024-11-12T15:09:49Z

Hi @pwolfert, I’m just trying to briefly synthesize this for myself and check my understanding.

Basically we're saying that using the shadow DOM would improve change detection by creating a clear boundary within which we could reliably observe changes, eliminating the need to poll or scan the entire DOM over and over.

However, this doesn't solve the problem of angular’s event bindings on nested elements because angular's bindings are applied to the original input elements in the DOM. When these elements are handed over to Preact’s runtime—whether they’re in the shadow DOM or part of <template> innerHTML—Preact’s Virtual DOM processing takes over and re-renders those elements as new instances in the final output, which is where angular’s bindings are lost.

From your last comment, we might be able to preserve angular bindings on nested elements with Lit, specifically through its use of slots within the shadow DOM. If we were to explore this option, it sounds like we would first need to conduct more research on two key areas: the ARIA concerns related to using the shadow DOM and the potential challenges with managing global styles.

pwolfert · 2024-11-12T17:50:08Z

@tamara-corbalt, that's exactly it! Well said.

pwolfert · 2024-11-12T19:01:56Z

@tamara-corbalt, @jack-ryan-nava-pbc, and @kim-cmsds,

As per the discussion we had in the planning meeting, I've broken off the more experimental solutions into what will be a new pull request for a follow-up ticket. I've cleaned up this current PR to be 1) the creation of the new Angular example (WNMGDS-3009) and 2) the simpler fix for static content only (WNMGDS-3010).

This is now ready for review.

Thanks!

packages/design-system/src/components/web-components/preactement/define.ts

jack-ryan-nava-pbc · 2024-11-13T18:02:33Z

examples/angular/src/main.html

Just noting for myself that this file will need to be manually updated when we call apply-example-templates

jack-ryan-nava-pbc

Minor comments and questions. Looks good! Changes to define don't seem to impact existing web components, which is great. Nice work!!!

jack-ryan-nava-pbc · 2024-11-13T18:03:39Z

examples/angular/src/main.html

+
+      <ds-alert
+        heading="{{alertHeading}}"
+        [attr.variation]="alertVariation ?? null"


Nit: does it make sense to coalesce a null value to null?

It's the only way I can keep Angular from passing an empty string to that prop. That actually highlights an issue that I didn't want to try to fix in this PR but that should probably be discussed: It's actually not very easy for a downstream app to have the variation as a variable and pass it on, because the only way someone can make an info alert is if this is undefined. Our web component wrapper doesn't treat an empty string as undefined, either, which means the only way to pass on the lack of a variation is to not set the attribute in the first place. Two possible fixes:

Just change the ds-alert wrapper to interpret empty or nullish values as undefined when passing it on to the Alert React component or

Change the varations that the Alert React component accepts to include something like "info" as one of the options.

Ah ok. Is a possible issue with option 2 that it would become a breaking change? In cases where users have no variation expressed they would have to supply an option?

Interestingly there's an open PR (#3301) that is asking to expose 'informational' (or the Alert's default type)

I was thinking about that. It's not a bad idea. Maybe info is a good abbreviation for it, and we might need to take a look at all the places where it's used and change some actual code and not just the types in order to merge that.

@jack-ryan-nava-pbc, regarding it being a breaking change, I actually think we could make it a completely backwards-compatible change. Leaving it off would still default to info like it always has.

...s/design-system/src/components/web-components/preactement/__snapshots__/define.test.tsx.snap

packages/design-system/src/components/web-components/preactement/define.ts

…r web component content issue (#3275) * Add a basic Angular example that imports the design system styles * Configure it to allow web components and add a couple simple ones * Copy and paste the other wc examples without the scripts * Update version after patch * Add some debug logging * Strip the example down to just a few components These components show the current issue, which is that our custom elements don't have any initial `innerHTML`, which causes various symptoms. For the month picker, we don't get the default checked state or the disabled months. For the alert content, we don't get any of the alert content, but we get the header because it was defined with an attribute. * I think this fixes it! When Angular renders the web component at first, it doesn't have any innerHTML, but then it inserts the content later and comes in under the watchful eye of our MutationObserver. This might be a bad assumption, but the code I've added just now assumes the same kind of thing that we assumed before that if we're getting mutations to the root content, it means that we want to replace the previous content and re-render everything. Instead of the mutation kicking off the `renderPreactComponent` function and trying to read the `innerHTML` that isn't really settled with the new content, it will take the new content from the mutations and use that instead as the new content to render. For this to work, we're not taking into account previously defined template elements. I have to think about this a bit more, but I think after our changes to make the web components more stateful, we don't actually need to keep the old template around...that could be wrong. I need to think about it more. * Get rid of debug logging * A unit test in `define.test.tsx` caught an issue It was the `correctly caches children when moved in the DOM` test, and it caught that when a component was moved in the DOM it ended up with nested templates. The fix for that is to bring back the old logic of only creating a template if one doesn't already exist WHEN we're not dealing with inner content mutations. * Add some web component examples back in * Revert "A unit test in `define.test.tsx` caught an issue" This reverts commit 32237c1. Apparently we can't both make this unit test happy and have working Angular examples. I'm going to have to think about this scenario some more. If I bring back the code that will look for previous templates when `addedNodes` doesn't exist, nested web components don't work right in the Angular examples. Try checking out 32237c1 and running the `examples/angular` project. * Fix c578c7c (see note) I think next I'll try actually removing the template if it is empty so even the empty template doesn't get duplicated inside the new template * This is a better version of the previous fix where we not only create a new template if the previous one was empty, but we remove the empty template before creating the new one so that it doesn't make its way into the content of the new one. Before this fix, the Angular-rendered alert example with a nested button would end up looking like this in the DOM: ```html <ds-button> <button type="button" class="ds-c-button ds-c-button--alternate ds-c-button--solid" > <template><span></span></template> Break things </button> <template> <span> <template><span></span></template> Break things </span> </template> </ds-button> ``` The above HTML has an empty content inside the actual button content, and the top-level template for the `ds-button` element has a another template inside of it! After this fix, it looks as we expect it: ```html <ds-button> <button type="button" class="ds-c-button ds-c-button--alternate ds-c-button--solid" > Break things </button> <template><span>Break things</span></template> </ds-button> ``` * Edited the comment to try to make it clearer * Update our nvm node version for testing * Fix bad ds version after bump * Move the template content into a separate HTML file—thanks, Michael! * Update lockfile (automatically removing bad entries) * An ugly solution: polling for changes to the template The way I got to this solution is that I observed that Angular was actually making changes to the web component's `template` element contents whenever Angular was re-rendering (probably because of [this line](https://github.com/CMSgov/design-system/blob/pwolfert/angular-example/packages/design-system/src/components/web-components/preactement/define.ts#L393)). The problem is that I can't use a MutationObserver on template content because it's only a document fragment and not part of the DOM. Polling for changes works, but I don't think this is a production-level solution. * Remove the polling part of my last commit I just wanted to get that code in version control, but I don't think this is the long-term solution * Polling now working in the Angular wrapper instead of the design system Ideally if we have to have an Angular wrapper, we could detect changes made by Angular and trigger a re-render, but nothing has seemed to work. This is the only thing so far that has worked. * Looks like custom event binding works even without the wrapper * Remove experimental solutions for ticket 3041 * Add new comment and remove old one * Add a README for the project * Add some more unit tests around this functionality * Using `flatMap` is more succinct and readible here * Get rid of debug logs

pwolfert added 6 commits October 16, 2024 13:45

Add a basic Angular example that imports the design system styles

0be80e6

Configure it to allow web components and add a couple simple ones

a98c3f2

Copy and paste the other wc examples without the scripts

32ed206

Update version after patch

94abfcd

Add some debug logging

63c5ccd

pwolfert force-pushed the pwolfert/angular-example branch from 150d8ea to 368bda6 Compare October 16, 2024 21:41

pwolfert added 5 commits October 16, 2024 16:30

Get rid of debug logging

3ee3909

Add some web component examples back in

08e3465

pwolfert added 3 commits October 17, 2024 06:24

Fix c578c7c (see note)

d5e149d

I think next I'll try actually removing the template if it is empty so even the empty template doesn't get duplicated inside the new template

Edited the comment to try to make it clearer

01a3b53

pwolfert changed the title ~~[NO-TICKET] Add Angular example project~~ [WNMGDS-3009][WNMGDS-3010] Add Angular example project Oct 17, 2024

pwolfert added this to the 12.0.0-beta.0 milestone Oct 17, 2024

pwolfert added Type: Fixed Indicates an unexpected problem or unintended behavior Impacts: Core Impacts the core DS primarily, changes may occur in other themes as well. labels Oct 17, 2024

pwolfert changed the title ~~[WNMGDS-3009][WNMGDS-3010] Add Angular example project~~ [WNMGDS-3009][WNMGDS-3010] Add Angular example project and fix Angular web component content issue Oct 17, 2024

pwolfert marked this pull request as ready for review October 17, 2024 16:51

pwolfert added 6 commits October 18, 2024 10:28

Update our nvm node version for testing

8331a0d

Merge branch 'main' into pwolfert/angular-example

705b189

Merge branch 'main' into pwolfert/angular-example

9621534

Fix bad ds version after bump

a05f04f

Move the template content into a separate HTML file—thanks, Michael!

1b48360

Merge branch 'main' into pwolfert/angular-example

8813141

Update lockfile (automatically removing bad entries)

31031f7

kim-cmsds modified the milestones: 12.0.0-beta.0, 12.1.0 Nov 5, 2024

Polling now working in the Angular wrapper instead of the design system

f415f9a

Ideally if we have to have an Angular wrapper, we could detect changes made by Angular and trigger a re-render, but nothing has seemed to work. This is the only thing so far that has worked.

pwolfert force-pushed the pwolfert/angular-example branch from fda72bf to f415f9a Compare November 5, 2024 21:38

Looks like custom event binding works even without the wrapper

f865fcc

pwolfert added 4 commits November 12, 2024 10:48

Remove experimental solutions for ticket 3041

b95d4c8

Add new comment and remove old one

ba3303d

Add a README for the project

6c45378

Merge branch 'main' into pwolfert/angular-example

6072561

tamara-corbalt reviewed Nov 12, 2024

View reviewed changes

packages/design-system/src/components/web-components/preactement/define.ts Outdated Show resolved Hide resolved

pwolfert added 3 commits November 12, 2024 15:17

Add some more unit tests around this functionality

e158c7e

Using flatMap is more succinct and readible here

7156190

Get rid of debug logs

2f472ed

tamara-corbalt approved these changes Nov 13, 2024

View reviewed changes

Merge branch 'main' into pwolfert/angular-example

a2a8596

jack-ryan-nava-pbc reviewed Nov 13, 2024

View reviewed changes

jack-ryan-nava-pbc approved these changes Nov 13, 2024

View reviewed changes

kim-cmsds approved these changes Nov 13, 2024

View reviewed changes

pwolfert merged commit 2f157e9 into main Nov 13, 2024
1 check passed

pwolfert deleted the pwolfert/angular-example branch November 13, 2024 23:56

pwolfert mentioned this pull request Nov 20, 2024

[WNMGDS-3041] Fix Angular event binding on nested web components #3327

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WNMGDS-3009][WNMGDS-3010] Add Angular example project and fix Angular web component content issue #3275

[WNMGDS-3009][WNMGDS-3010] Add Angular example project and fix Angular web component content issue #3275

pwolfert commented Oct 11, 2024 •

edited

Loading

pwolfert commented Oct 17, 2024 •

edited

Loading

pwolfert commented Oct 23, 2024

pwolfert commented Nov 5, 2024

tamara-corbalt commented Nov 6, 2024

pwolfert commented Nov 6, 2024 •

edited

Loading

pwolfert commented Nov 11, 2024

pwolfert commented Nov 11, 2024

tamara-corbalt commented Nov 12, 2024 •

edited

Loading

pwolfert commented Nov 12, 2024 •

edited

Loading

pwolfert commented Nov 12, 2024

jack-ryan-nava-pbc Nov 13, 2024

jack-ryan-nava-pbc left a comment

jack-ryan-nava-pbc Nov 13, 2024

pwolfert Nov 13, 2024

jack-ryan-nava-pbc Nov 13, 2024

kim-cmsds Nov 13, 2024

pwolfert Nov 13, 2024

pwolfert Nov 13, 2024

[WNMGDS-3009][WNMGDS-3010] Add Angular example project and fix Angular web component content issue #3275

[WNMGDS-3009][WNMGDS-3010] Add Angular example project and fix Angular web component content issue #3275

Conversation

pwolfert commented Oct 11, 2024 • edited Loading

Summary

Angular example project

Fix for Angular providing content to web components

How to test

Checklist

pwolfert commented Oct 17, 2024 • edited Loading

pwolfert commented Oct 23, 2024

pwolfert commented Nov 5, 2024

tamara-corbalt commented Nov 6, 2024

pwolfert commented Nov 6, 2024 • edited Loading

pwolfert commented Nov 11, 2024

pwolfert commented Nov 11, 2024

tamara-corbalt commented Nov 12, 2024 • edited Loading

pwolfert commented Nov 12, 2024 • edited Loading

pwolfert commented Nov 12, 2024

jack-ryan-nava-pbc Nov 13, 2024

Choose a reason for hiding this comment

jack-ryan-nava-pbc left a comment

Choose a reason for hiding this comment

jack-ryan-nava-pbc Nov 13, 2024

Choose a reason for hiding this comment

pwolfert Nov 13, 2024

Choose a reason for hiding this comment

jack-ryan-nava-pbc Nov 13, 2024

Choose a reason for hiding this comment

kim-cmsds Nov 13, 2024

Choose a reason for hiding this comment

pwolfert Nov 13, 2024

Choose a reason for hiding this comment

pwolfert Nov 13, 2024

Choose a reason for hiding this comment

pwolfert commented Oct 11, 2024 •

edited

Loading

pwolfert commented Oct 17, 2024 •

edited

Loading

pwolfert commented Nov 6, 2024 •

edited

Loading

tamara-corbalt commented Nov 12, 2024 •

edited

Loading

pwolfert commented Nov 12, 2024 •

edited

Loading