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

[m-akinc]

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:

   <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 of bug numbered 1 in description. Stackblitz demo of bug numbered 2 in description.

📑 Test Plan

Added additional test cases.

✅ Checklist

General

• [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 documentation and followed the standards for this project.

[m-akinc]

@chrisdholt No rush, but it's been a couple weeks since I posted this, so I thought I'd give you a ping.

[m-akinc]

@janechu is there anything I can do to help get this in?

[janechu]

@janechu is there anything I can do to help get this in?

LGTM, sorry for the delay!

[m-akinc]

@chrisdholt while we've got some momentum, would be nice to get a review on this. 😊

[m-akinc]

@janechu are you comfortable merging this in? @chrisdholt do you want to review?

[m-akinc]

@chrisdholt I'm still holding on to hope that this will be merged at some point. Should be a nice bug fix for production clients.

[m-akinc]

@chrisdholt @janechu Is the policy now not to take further changes into the archive branch? I don't want to keep pinging you all if it's pointless, but we are still relying on this branch and would like to be able to apply fixes if possible. We actually just identified a memory leak last week related to some design token bindings not getting cleaned up. Ideally, we would put up a PR for that as well.

[janechu]

@m-akinc going forward we won't be accepting new changes to archived branches, but we are in process of cleaning up the current outstanding PRs, doing a final publish, then the guidance will be new changes should get applied only to the current version. Sorry about the delay!

[m-akinc]

Thanks for clarifying, and thanks for merging.