fix(fast-element1): fix design token updates when attaching/detaching nodes and deleting values (#6960)

# Pull Request

## 📖 Description

This PR addresses a couple design-token-related bugs:
1. Removing/adding a child element does not update the CSS token reflection of that child. E.g.
   - Starting with:
   ```html
   <my-parent>
      <my-child></my-child>
   </my-parent>
   <my-other-parent></my-other-parent>
   ```
   - Define a token `T` (with CSS variable `--T`)
   - Explicitly set `T` to "10" for both `<my-child>` and `<my-parent>`
      - because `<my-child>`'s value is the same as the inherited value, the value is not reflected to CSS on `<my-child>`
   - Explicitly set `T` to "20" for `<my-other-parent>`
   - Reparent `<my-child>` under `<my-other-parent>` (detach, then append)
   - Because the value "10" is explicitly set on `<my-child>`, the value of the CSS variable `--T` should be "10" for it, but it is not. It is "20", which is incorrect.
      - since the value for `<my-child>` differs from `<my-other-parent>`, it should reflect the value to CSS. It doesn't. ❌
2. Deleting a token value from an element should notify affected elements (and update CSS reflection). E.g. 
   - Starting with: `<my-element></my-element>`
   - Define a token `T` (with CSS variable `--T`)
   - Explicitly set `T` to "10" for `<my-element>`
   - Delete `T` from `<my-element>`
   - `<my-element>` should not have any value for `--T`, but it is still defined as "10".

Also:
- Removed `.only` from a combobox test that was apparently submitted on accident
- Now catching errors thrown from derived token evaluation (and logging via `console.error`). Now that we are properly triggering token re-evaluations when detaching elements from the DOM, a derived token may throw an error during evaluation, because it depended on an inherited token value. It seems better to print an error than to let this derail execution.

### 🎫 Issues

N/A

## 👩‍💻 Reviewer Notes

[Stackblitz demo](https://stackblitz.com/edit/typescript-kd4ers?file=index.ts) of bug numbered 1 in description.
[Stackblitz demo](https://stackblitz.com/edit/typescript-hd4ghv?file=index.ts) of bug numbered 2 in description.

## 📑 Test Plan

Added additional test cases.

## ✅ Checklist

### General

<!--- Review the list and put an x in the boxes that apply. -->

- [x] I have included a change request file using `$ yarn change`
- [x] I have added tests for my changes.
- [x] I have tested my changes.
- [ ] I have updated the project documentation to reflect my changes.
- [x] I have read the [CONTRIBUTING](https://github.com/microsoft/fast/blob/master/CONTRIBUTING.md) documentation and followed the [standards](/docs/community/code-of-conduct/#our-standards) for this project.

Author: m-akinc

change/@microsoft-fast-foundation-030341a5-c95f-4516-a637-9e489443ff23.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Fix design token issues involving deleting values and CSS reflection",
+  "packageName": "@microsoft/fast-foundation",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

packages/web-components/fast-foundation/src/design-token/design-token.spec.ts

@@ -194,7 +194,32 @@ describe("A DesignToken", () => {
                 removeElement(ancestor);
             });
 
-            it("should set CSS custom property for element if value stops matching inherited value", async () => {
+            it("should override inherited CSS custom property from ancestor", async () => {
+                const ancestor = addElement();
+                const target = addElement(ancestor);
+                const token = DesignToken.create<number>("test");
+                token.setValueFor(ancestor, 12);
+                token.setValueFor(target, 14);
+                await DOM.nextUpdate();
+                expect(window.getComputedStyle(target).getPropertyValue(token.cssCustomProperty)).to.equal('14');
+                removeElement(ancestor);
+            });
+
+            it("should undefine CSS custom property for element if value deleted", async () => {
+                const ancestor = addElement();
+                const target = addElement(ancestor);
+                const token = DesignToken.create<number>("test");
+                token.setValueFor(ancestor, 12);
+                token.setValueFor(target, 14);
+                await DOM.nextUpdate();
+                expect(window.getComputedStyle(target).getPropertyValue(token.cssCustomProperty)).to.equal('14');
+                token.deleteValueFor(target);
+                await DOM.nextUpdate();
+                expect(window.getComputedStyle(target).getPropertyValue(token.cssCustomProperty)).to.equal('12');
+                removeElement(ancestor);
+            });
+
+            it("should set CSS custom property for element if value stops matching inherited value because value changed on ancestor", async () => {
                 const ancestor = addElement();
                 const target = addElement(ancestor);
                 const token = DesignToken.create<number>("test");
@@ -207,6 +232,56 @@ describe("A DesignToken", () => {
                 expect(window.getComputedStyle(target).getPropertyValue(token.cssCustomProperty)).to.equal('12');
                 removeElement(ancestor);
             });
+
+            it("should set CSS custom property for element if value stops matching inherited value because of reparenting", async () => {
+                const ancestorA = addElement();
+                const ancestorB = addElement();
+                const target = addElement(ancestorA);
+                const token = DesignToken.create<number>("test");
+                token.setValueFor(ancestorA, 12);
+                token.setValueFor(ancestorB, 14);
+                token.setValueFor(target, 12);
+                await DOM.nextUpdate();
+                expect(window.getComputedStyle(target).getPropertyValue(token.cssCustomProperty)).to.equal('12');
+                ancestorA.removeChild(target);
+                ancestorB.appendChild(target);
+                await DOM.nextUpdate();
+                expect(window.getComputedStyle(target).getPropertyValue(token.cssCustomProperty)).to.equal('12');
+                removeElement(ancestorA, ancestorB);
+            });
+
+            it("should set CSS custom property for element if matching value is deleted from ancestor", async () => {
+                const ancestor = addElement();
+                const target = addElement(ancestor);
+                const token = DesignToken.create<number>("test");
+                token.setValueFor(ancestor, 12);
+                token.setValueFor(target, 12);
+                await DOM.nextUpdate();
+                // This is inherited from ancestor's CSS
+                expect(window.getComputedStyle(target).getPropertyValue(token.cssCustomProperty)).to.equal('12');
+                token.deleteValueFor(ancestor);
+                await DOM.nextUpdate();
+                // This should now come from CSS set directly on target
+                expect(window.getComputedStyle(target).getPropertyValue(token.cssCustomProperty)).to.equal('12');
+                removeElement(ancestor);
+            });
+
+            it("should set CSS custom property for element if removed from parent with matching value", async () => {
+                const ancestor = addElement();
+                const target = addElement(ancestor);
+                const token = DesignToken.create<number>("test");
+                token.setValueFor(ancestor, 12);
+                token.setValueFor(target, 12);
+                await DOM.nextUpdate();
+                // This is inherited from ancestor's CSS
+                expect(window.getComputedStyle(target).getPropertyValue(token.cssCustomProperty)).to.equal('12');
+                ancestor.removeChild(target);
+                document.body.append(target);
+                await DOM.nextUpdate();
+                // This should now come from CSS set directly on target
+                expect(window.getComputedStyle(target).getPropertyValue(token.cssCustomProperty)).to.equal('12');
+                removeElement(ancestor);
+            });
         });
         describe("that is not a CSSDesignToken", () => {
             it("should not set a CSS custom property for the element", () => {
@@ -640,7 +715,7 @@ describe("A DesignToken", () => {
             await DOM.nextUpdate();
             expect(handleChange).to.have.been.called.once;
         });
-        it("should notify a subscriber for a token after being appended to a parent with a different token value than the previous context", async () => {
+        it("should notify a subscriber for a derived token after being appended to a parent with a different token value than the previous context", async () => {
             const tokenA = DesignToken.create<number>("token-a");
             const tokenB = DesignToken.create<number>("token-b");
 
@@ -678,6 +753,7 @@ describe("A DesignToken", () => {
 
             expect(tokenA.getValueFor(child)).to.equal(6);
             tokenA.subscribe(subscriber, child);
+
             expect(handleChange).not.to.have.been.called()
             parent.appendChild(child);
 
@@ -760,6 +836,11 @@ describe("A DesignToken", () => {
             tokenA.deleteValueFor(target);
 
             expect(tokenB.getValueFor(target)).to.equal(14);
+
+            // When disconnecting target, it will no longer inherit a value for tokenA,
+            // and updating the value for tokenB will throw an error. So remove the
+            // value for tokenB first to avoid the console error.
+            tokenB.deleteValueFor(target);
             removeElement(parent)
         })
     });
@@ -859,6 +940,11 @@ describe("A DesignToken", () => {
             expect(spy.get(parent)).to.be.false;
             expect(spy.get(target)).to.be.false;
 
+            // Note that elements do not get subscribed for changes in their parent's values
+            // until a token value is gotten for/set on that (child) element.
+            // That is why setting values on ancestors do not trigger value change callbacks
+            // for the descendants in this test.
+
             token.setValueFor(ancestor, 12);
             expect(spy.get(ancestor)).to.be.true;
             expect(spy.get(parent)).to.be.false;
@@ -877,6 +963,37 @@ describe("A DesignToken", () => {
             removeElement(ancestor);
         });
 
+        it("should notify an un-targeted subscriber for each element whose value changes", () => {
+            const ancestor = addElement();
+            const parent = addElement(ancestor);
+            const target = addElement(parent);
+            const token = DesignToken.create<number>("test");
+            const spy = new Map<HTMLElement, boolean>([[ancestor, false], [parent, false], [ target, false ]]);
+
+            // Set/get the values to initialize change notifications for these elements
+            token.setValueFor(ancestor, 10);
+            token.getValueFor(parent);
+            token.getValueFor(target);
+
+            const handleChange = chia.spy((record: DesignTokenChangeRecord<typeof token>) => {
+                spy.set(record.target, true)
+            });
+            const subscriber: DesignTokenSubscriber<typeof token> = {
+                handleChange
+            }
+
+            token.subscribe(subscriber);
+            expect(handleChange).to.not.have.been.called;
+
+            token.setValueFor(parent, 14);
+            expect(spy.get(ancestor)).to.be.false;
+            expect(spy.get(parent)).to.be.true;
+            expect(spy.get(target)).to.be.true;
+            expect(handleChange).to.have.been.called.twice;
+
+            removeElement(ancestor);
+        });
+
         it("should notify a target-subscriber if the value is changed for the provided target", () => {
             const parent = addElement();
             const target = addElement(parent);
@@ -887,6 +1004,8 @@ describe("A DesignToken", () => {
                 handleChange
             }
 
+            // A side effect of subscribing for a specific target is that the target gets initialized
+            // for change notifications, so when we set a value on the parent, it will notify for the target.
             token.subscribe(subscriber, target);
 
             token.setValueFor(parent, 12);
@@ -918,6 +1037,38 @@ describe("A DesignToken", () => {
             removeElement(target);
         });
 
+        it("should notify a subscriber when the value is deleted for an element", () => {
+            const ancestor = addElement();
+            const parent = addElement(ancestor);
+            const target = addElement(parent);
+            const token = DesignToken.create<number>("test");
+            const spy = new Map<HTMLElement, boolean>([[ancestor, false], [parent, false], [ target, false ]]);
+
+            const handleChange = chia.spy((record: DesignTokenChangeRecord<typeof token>) => {
+                spy.set(record.target, true)
+            });
+            const subscriber: DesignTokenSubscriber<typeof token> = {
+                handleChange
+            }
+
+            // Set/get the values to initialize change notifications for these elements
+            token.setValueFor(ancestor, 10);
+            token.setValueFor(parent, 12);
+            token.getValueFor(target);
+
+            token.subscribe(subscriber);
+
+            expect(handleChange).to.not.have.been.called;
+
+            token.deleteValueFor(parent);
+            expect(spy.get(ancestor)).to.be.false;
+            expect(spy.get(parent)).to.be.true;
+            expect(spy.get(target)).to.be.true;
+            expect(handleChange).to.have.been.called.twice;
+
+            removeElement(ancestor);
+        });
+
         it("should infer DesignToken and CSSDesignToken token types on subscription record", () => {
             type AssertCSSDesignToken<T> = T extends CSSDesignToken<any> ? T : never;
             DesignToken.create<number>("css").subscribe({handleChange(record) {

packages/web-components/fast-foundation/src/design-token/design-token.ts

@@ -386,14 +386,18 @@ class DesignTokenBindingObserver<T extends { createCSS?(): string }> {
      * @internal
      */
     public handleChange() {
-        this.node.store.set(
-            this.token,
-
-            (this.observer.observe(
-                this.node.target,
-                defaultExecutionContext
-            ) as unknown) as StaticDesignTokenValue<T>
-        );
+        try {
+            this.node.store.set(
+                this.token,
+
+                (this.observer.observe(
+                    this.node.target,
+                    defaultExecutionContext
+                ) as unknown) as StaticDesignTokenValue<T>
+            );
+        } catch (e) {
+            console.error(e);
+        }
     }
 }
 
@@ -421,6 +425,7 @@ class Store {
 
     delete<T extends { createCSS?(): string }>(token: DesignTokenImpl<T>) {
         this.values.delete(token);
+        Observable.getNotifier(this).notify(token.id);
     }
 
     all() {
@@ -724,6 +729,8 @@ class DesignTokenNode implements Behavior, Subscriber {
                 token,
                 this.bindingObservers.has(token) ? this.getRaw(token) : value
             );
+            // Need to stop reflecting any tokens that can now be inherited
+            child.updateCSSTokenReflection(child.store, token);
         }
     }
 
@@ -739,7 +746,18 @@ class DesignTokenNode implements Behavior, Subscriber {
         }
 
         Observable.getNotifier(this.store).unsubscribe(child);
-        return child.parent === this ? childToParent.delete(child) : false;
+
+        if (child.parent !== this) {
+            return false;
+        }
+        const deleted = childToParent.delete(child);
+
+        for (const [token] of this.store.all()) {
+            child.hydrate(token, child.getRaw(token));
+            // Need to start reflecting any assigned values that were previously inherited
+            child.updateCSSTokenReflection(child.store, token);
+        }
+        return deleted;
     }
 
     /**