microsoft / fast

The adaptive interface system for modern web experiences.
https://www.fast.design
Other
9.23k stars 590 forks source link

Fix CSS custom property precedence issue and work around Chromium bug #6906

Closed m-akinc closed 5 months ago

m-akinc commented 7 months ago

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

⏭ Next Steps

rajsite commented 6 months ago

@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?

chrisdholt commented 6 months ago

@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 commented 6 months ago

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

m-akinc commented 5 months ago

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

chrisdholt commented 5 months ago

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

m-akinc commented 5 months ago

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

Maybe this weekend? πŸ™

chrisdholt commented 5 months ago

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 :)

m-akinc commented 5 months ago

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!