-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[core] Force visibility on a few components in ink save print mode #20749
[core] Force visibility on a few components in ink save print mode #20749
Conversation
Hey! @oliviertassinari , |
Details of bundle changes.Comparing: afbe28a...2904d89 Details of page changes
|
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 didn't know this existed. Much appreciated! Just one question about the property.
@@ -39,6 +39,9 @@ export const styles = { | |||
pointerEvents: 'none', // Disable link interactions | |||
cursor: 'default', | |||
}, | |||
'@media print': { | |||
WebkitPrintColorAdjust: 'exact', |
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.
Should we rather switch to color-adjust
which is on standard-track? The result should be the same since jss
should convert this to --webkit-print-color-adjust
.
WebkitPrintColorAdjust: 'exact', | |
colorAdjust: 'exact', |
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.
JSS supports the auto-prefixing for this property but styled-components and emotion don't. Should we stick to the webkit approach for interoperability support between the engines in v5?
Looking at where the prefixer of sc and emotion are implemented, I could find https://github.com/thysultan/stylis.js/blob/master/src/Prefixer.js. For JSS it's in https://github.com/cssinjs/css-vendor/blob/71dc0fd1bb73d9b1b1da836b3a73bfcc67b3f268/src/plugins/color-adjust.js.
I have opened this issue thysultan/stylis#210.
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.
Why does it matter what other libraries do? They don't consume these styles, no?
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.
Works with autoprefixer and fixes this behavior in firefox as well.
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.
@eps1lon I think until a property is standardized it should not be implemented. This can cause issues across certain browsers.
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.
@oliviertassinari it depends. if you use https://github.com/NanoHttpd/nanohttpd in a webview invalid css throws an error and crashes it. ( example for non standard web usages on the css engine )
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 confused this with invalid css stylesheets. For rules it should be fine.
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 think until a property is standardized it should not be implemented.
We use a bunch of css properties that aren't standardized. This doesn't even apply here because color-adjust
is standardized while -webkit-print-color-adjust
isn't. color-adjust
just hasn't made it beyond the "editors draft" stage.
if you use NanoHttpd/nanohttpd in a webview invalid css throws an error and crashes it
That is pretty bold of them. They have to stay on top of any property that is being standardized or works in any browser.
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.
Unknown properties. User agents must ignore a declaration with an unknown property
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.
@j-mendez As a workaround, you can use the theme.overrides
API to remove this CSS prop. But if your aim in https://www.a11ywatch.com/ is to crawl pages, you will face a concerning issue once crawling external pages (it's the wild west).
The small attention to the details that make all the difference when compounded to x100 more like this one ✨. @coktopus Thanks. |
Edit @eps1lon: Closes #20568
Fixes: