-
-
Notifications
You must be signed in to change notification settings - Fork 77
fix : css-has-pseudo #101
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-has-pseudo #101
Conversation
Maybe we can have a simple test setup running against a semi random chrome version via github workflows? This should tell give us some feedback when updating things. A more comprehensive test can be run from https://github.com/mrhenry/web-tests Browserstack (or something similar) is needed to test a wide range of devices but these services are very flaky and I don't want to slow down development by having a flaky CI.
Update : I have added a small setup to run tests agains Chrome in CI here. Once we have some release of the features here I can see how I can add it to |
…ourageous-patas-monkey-7b2ebb639f
…ourageous-patas-monkey-7b2ebb639f
### Unreleased | ||
|
||
Tracking initial implementation of `:has()` pseudo-class in Safari Technology Preview. | ||
This is a breaking change and affects both the generated CSS and the client side polyfill. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we couple minor examples (but meaningful, not something like .foo but just an input with hover and that sort of thing to quickly identify how's changing? Just an idea, not really a request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add these.
I should also create some markdown file with unresolved issues.
Those are listed in the PR comment, but should remain visible after merge.
Side node :
I added this as Unreleased
and intend to do this for future changes in all plugins.
I hope this makes it easy to do a search in the repo for things that have not been released yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few examples here but do not want to go into too much detail.
Given the complexity of :has
it's difficult to explain something without explaining everything (or at least for me)
## PostCSS CLI | ||
|
||
Add [PostCSS CLI] to your project: | ||
|
||
```bash | ||
npm install postcss-cli --save-dev | ||
``` | ||
|
||
Use [CSS Has Pseudo] in your `postcss.config.js` configuration file: | ||
|
||
```js | ||
const postcssHasPseudo = require('css-has-pseudo'); | ||
|
||
module.exports = { | ||
plugins: [ | ||
postcssHasPseudo(/* pluginOptions */) | ||
] | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add here along these lines, how to integrate with postcss-preset-env? It's trivial to us and we know what's going on but I'm not sure it's clear enough outside of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps better on the README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add this both in the readme in the /experimental/
folder and in the readme of this plugin!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added these
transformObservedItemsThrottled(); | ||
|
||
// observe DOM modifications that affect selectors | ||
if ('MutationObserver' in self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this is required by the requisites, I think we should test this earlier on, and if not just throw and not run.
I don't see (but could be wrong) if we're checking support for :has
on JS before allowing any of this code to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mutation observer is actually not a requirement for this polyfill.
I was surprised by this too.
It is needed to re-calc styles if html changes through client side javascript.
It's better not to throw or do early returns as this polyfill should embrace CSS concepts -> it should degrade very gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know and surprising indeed.
Thing is, as it is right now I think (if Safari was fully compliant) it would run in Safari and it wouldn't be needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The polyfill would still be needed if people did not set preserve : true
.
Since we don't know if they set preserve : true
we have to always run the polyfill :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new option forcePolyfill
.
If this is not set we do feature detection and early return.
We can slim down the amount of options and config later in the experimental track.
Having more is ok now I think :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romainmenke this is astonishing! Great achievement here and I think this is very thorough. I'm really glad that we got some tests for browser too.
Left some comments there which aren't a big deal. Great work :)
This plugin was a bit broken.-> there were just a lot more nuances left to coverI don't know if the spec changed or if it was always a bit broken, or if I broke during the migration.
It does show that we really need to start testing these features in real browsers.-> added thisWe are flying blind here when we make structural changes to the setup/repo's.
fixed :
a > :has(*) ~ b
,a :has(*) b
:has(> b:has(> img))
:has(> b), :has(> c)
:hover
(enabled with an option)This is now a breaking change and will be released as an experimental plugin first to gather feedback.
PostCSS plugin changes :
:has(...)
was encoded)selector encoding :
We need the entire selector to correctly match elements.
Rebuilding it manually client side is error prone.
Encoding the entire selector seemed like the easiest way to resolve this.
Specificity :
Does anyone know if we can safely use
:not(#does-not-exist)
/:not(does-not-exist)
as prefixes to selectors to get correct A/C specificity.Plugin order :
old problem, but larger now that we encode the entire selector
Because we encode the selector there will be bugs caused by plugin order.
I don't currently have a good solution for this.
This can have different results if
postcss-logical
runs before or aftercss-has-pseudo
.We need to investigate if there are ways to always run as the final plugin.
Browser polyfill changes :
querySelector
polyfillThis now uses a polyfill for
querySelector
: queryselector-with-has-pseudo-classI wrote this and it passes all tests from WPT in most browsers : https://mrhenry.github.io/web-tests/#73cfc27f-b188-4c49-af0d-83801d0a7be4
It is not fast as it favours correctness over performance.
There might be ways to optimise some bits but none that I could find :/
Source code can be found here and feedback is welcome!
I wonder if we should give users the option to choose their own
querySelector
polyfill?Now it's bundled, but we can omit it and document that they have to find their own.
MutationObserver
This did not track enough of the DOM to work reliably with client side rendering.
Now most standard html attributes are also tracked with an exposed option for users to track more.
:hover
This is disabled by default but can be enabled.
throttle
transformObservedItems
Because we are now tracking a bunch more mutations and events with a slower polyfill I added a throttle to prevent excessive recalcs.
The previous version also used
requestAnimationFrame
.I added the cancel.
This makes sure we only run
transformObservedItems
once per event loop.tests
I am in process of bringing over all WPT tests for
:has
that concern CSS.At the moment enough have been ported and pass to know that we are on the right track.