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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 26, 2020
Merged

[core] Force visibility on a few components in ink save print mode #20749

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
017c1f9
'slider-switch-linnerProgress'
coktopus Apr 25, 2020
03f1d8e
'button-base'
coktopus Apr 25, 2020
2904d89
fix: Fix missing color in firefox
eps1lon Apr 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/material-ui/src/ButtonBase/ButtonBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ export const styles = {
pointerEvents: 'none', // Disable link interactions
cursor: 'default',
},
'@media print': {
WebkitPrintColorAdjust: 'exact',
Copy link
Member

@eps1lon eps1lon Apr 25, 2020 •
edited by oliviertassinari
Loading

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.

Suggested change
WebkitPrintColorAdjust: 'exact',
colorAdjust: 'exact',

Copy link
Member

@oliviertassinari oliviertassinari Apr 25, 2020 •
edited
Loading

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor

@j-mendez j-mendez Apr 27, 2020 •
edited
Loading

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.

Copy link
Contributor

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 )

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member

@oliviertassinari oliviertassinari Apr 27, 2020 •
edited
Loading

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

https://www.w3.org/TR/CSS21/syndata.html

Copy link
Member

@oliviertassinari oliviertassinari Apr 27, 2020 •
edited
Loading

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

},
},
/* Pseudo-class applied to the root element if `disabled={true}`. */
disabled: {},
Expand Down
3 changes: 3 additions & 0 deletions packages/material-ui/src/LinearProgress/LinearProgress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ export const styles = (theme) => {
position: 'relative',
overflow: 'hidden',
height: 4,
'@media print': {
WebkitPrintColorAdjust: 'exact',
},
},
/* Styles applied to the root and bar2 element if `color="primary"`; bar2 if `variant="buffer"`. */
colorPrimary: {
Expand Down
3 changes: 3 additions & 0 deletions packages/material-ui/src/Slider/Slider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ export const styles = (theme) => ({
padding: '0 20px',
},
},
'@media print': {
WebkitPrintColorAdjust: 'exact',
},
},
/* Styles applied to the root element if `color="primary"`. */
colorPrimary: {
Expand Down
3 changes: 3 additions & 0 deletions packages/material-ui/src/Switch/Switch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ export const styles = (theme) => ({
flexShrink: 0,
zIndex: 0, // Reset the stacking context.
verticalAlign: 'middle', // For correct alignment with the text.
'@media print': {
WebkitPrintColorAdjust: 'exact',
},
},
/* Styles applied to the root element if `edge="start"`. */
edgeStart: {
Expand Down