fast icon indicating copy to clipboard operation
fast copied to clipboard

Fix CSS custom property precedence issue and work around Chromium bug

Open m-akinc opened this issue 1 year ago • 2 comments

Pull Request

📖 Description

This addresses two issues:

  1. The last release of the fast-element-1 branch included a change that exposed a Chromium bug.
  2. Overriding a design token's value in a stylesheet does not always work. A stylesheet containing custom properties for design tokens is added to adoptedStyleSheets alongside other client-defined stylesheets. If a client attempts to override a design token value for an element in CSS, in most cases the order of stylesheets in adoptedStyleSheets won't matter because of specificity-based precedence. But if the client stylesheet overrides the token property using the selector :host, then whichever stylesheet appears later in adoptedStyleSheets will be the one that wins. It seems that the client CSS's override should always take precedence.

This change causes the special stylesheet for design token CSS properties to always be prepended to adoptedStyleSheets so that client stylesheets take precedence when order matters.

🎫 Issues

N/A

👩‍💻 Reviewer Notes

This change was conceived as a workaround to issue 1, but during implementation, it was noted that it solved what seems like an actual bug (issue 2).

📑 Test Plan

New design token test case written that failed before and passes now.

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

⏭ Next Steps

m-akinc avatar Feb 06 '24 00:02 m-akinc

@chrisdholt while we are waiting for reviewers (not sure how long to expect), what is the best way to validate the archives branch as there are no workflows configured to run tests, etc?

rajsite avatar Feb 16 '24 21:02 rajsite

@chrisdholt while we are waiting for reviewers (not sure how long to expect), what is the best way to validate the archives branch as there are no workflows configured to run tests, etc?

Working on credential updates/rotations which is a precursor for ensuring pipelines on CI for archive. What I'll do though is pull down and test.

chrisdholt avatar Feb 16 '24 22:02 chrisdholt

@chrisdholt while we are waiting for reviewers (not sure how long to expect), what is the best way to validate the archives branch as there are no workflows configured to run tests, etc?

Working on credential updates/rotations which is a precursor for ensuring pipelines on CI for archive. What I'll do though is pull down and test.

Merged a fix to run pipelines on the archives branch - I'm going to click the "update branch" button and see what happens.

chrisdholt avatar Mar 06 '24 02:03 chrisdholt

@chrisdholt Thanks for getting that merged! When you get a chance, can you kick off a release of archives/fast-element-1?

m-akinc avatar Mar 29 '24 22:03 m-akinc

@chrisdholt Thanks for getting that merged! When you get a chance, can you kick off a release of archives/fast-element-1?

I'll kick it off over the weekend, I'm going to try and get the site publishing for archive releases.

chrisdholt avatar Mar 29 '24 23:03 chrisdholt

I'll kick it off over the weekend, I'm going to try and get the site publishing for archive releases.

Maybe this weekend? 🙏

m-akinc avatar Apr 05 '24 17:04 m-akinc

I'll kick it off over the weekend, I'm going to try and get the site publishing for archive releases.

Maybe this weekend? 🙏

Need to get a cherry-pick in as well, which I've put up today (toolbar stealing focus) - like to take a look to ensure it doesn't concern your publish? Alternatively I can publish, merge, publish :)

chrisdholt avatar Apr 05 '24 18:04 chrisdholt

Need to get a cherry-pick in as well, which I've put up today (toolbar stealing focus) - like to take a look to ensure it doesn't concern your publish? Alternatively I can publish, merge, publish :)

Took a quick look. 👍 Seems fine, so no need to publish twice. Thanks!

m-akinc avatar Apr 05 '24 18:04 m-akinc