Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Feb 6, 2024

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

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

⏭ Next Steps

@rajsite
Copy link

rajsite commented Feb 16, 2024

@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
Copy link
Member

@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
Copy link
Member

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

rajsite added a commit to ni/nimble that referenced this pull request Mar 6, 2024
# Pull Request

## 🀨 Rationale

Gets all dependencies up-to-date (as reported by `npm outdated`) except
the following:
- `@angular/*`, `zone.js`, `ng-packagr`, `typescript`: Coupled to
current Angular version
- `@microsoft/fast-foundation` and `@microsoft/fast-react-wrapper`:
Pinned due to microsoft/fast#6906
- `playwright`: Latest version on nuget 1.41.2 not aligned with npm
1.42.1. There are some 1.42 beta releases on nuget so guess something is
going on there. Didn't look into it more.
- `remark-gfm`: Updating it to latest caused storybook build errors and
only recommendation seems to be [avoid upgrading `remark-gfm` to
v4](storybookjs/storybook#24743 (comment)).

## πŸ‘©β€πŸ’» Implementation

Updated and fixed build & lint.

## πŸ§ͺ Testing

Relying on CI.

## βœ… Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@rajsite rajsite mentioned this pull request Mar 6, 2024
1 task
rajsite added a commit to ni/nimble that referenced this pull request Mar 6, 2024
# Pull Request

## 🀨 Rationale

- Updated `fetch-depth: 0` comment from being [needed by
beachball](microsoft/beachball#780) to being
[needed by
chromatic](https://github.com/ni/nimble/actions/runs/8164295790/job/22319288123#step:14:50).
- Updated renovate to make a single PR for npm updates that we can let
update to latest.
  - Angular and it's related packages are ignored
    - `^@angular`, `ng-packagr`, `zone.js`, `typescript`
  - Pinned packages are ignored
- `@microsoft/fast-foundation` and `@microsoft/fast-react-wrapper` due
to [fast issue](microsoft/fast#6906)
- `comlink` as it is partially vendored into workers and needs a
controlled and pinned update
- `remark-gfm` as [updating breaks
mdx](storybookjs/storybook#24743 (comment))
- Removed npm version specific install that was now downgrading the npm
version accidentally.
- Removed file permission workaround [no longer needed to prevent
warnings](actions/upload-pages-artifact#45).
- Updated the beachball patch-package

## πŸ‘©β€πŸ’» Implementation

See above.

## πŸ§ͺ Testing

Relying on CI.

## βœ… Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@chrisdholt chrisdholt merged commit 0c27d02 into microsoft:archives/fast-element-1 Mar 29, 2024
5 checks passed
@m-akinc
Copy link
Contributor Author

m-akinc commented Mar 29, 2024

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

@chrisdholt
Copy link
Member

@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 m-akinc deleted the control-stylesheet-order branch April 1, 2024 18:16
@m-akinc
Copy link
Contributor Author

m-akinc commented Apr 5, 2024

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
Copy link
Member

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
Copy link
Contributor Author

m-akinc commented Apr 5, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants