feat(#119): add grid prefix support #285

SukkaW · 2022-02-24T13:00:43Z

When finished, this should close #119 once and for all.

Note: ~~It is still WIP~~ Although the feature is implemented, it hasn't been optimized AT ALL, so suggestions, optimizations, discussions are welcome!

SukkaW · 2022-02-24T13:04:43Z

SukkaW · 2022-02-24T18:49:56Z

@thysultan @Andarist

The PR is ready for review. And here is a few things:

Prefixing grid-row-start and grid-row-end requires the knowledge of both declarations, so it is impossible for stylis stateless per declaration processor to prefix them.
I just write the code to make it pass the test cases. It is highly un-optimized, introducing tons of performance overhead and unnecessary byte to the final dist build. Feel free to add your ideas and suggestions!

coveralls · 2022-02-24T19:48:01Z

Pull Request Test Coverage Report for Build d74963977ee5661da6a526faa4b7120ed49de8e8-PR-285

7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 98.966%

Totals
Change from base Build 66582efbbbf02895797f1109cf736de5421ac9d2:	0.04%
Covered Lines:	267
Relevant Lines:	269

💛 - Coveralls

src/Prefixer.js

thysultan · 2022-02-24T19:59:33Z

Prefixing grid-row-start and grid-row-end requires the knowledge of both declarations

Elaborate?

thysultan · 2022-02-24T20:02:47Z

so it is impossible for stylis stateless per declaration processor to prefix them

The prefixer middleware, which has access to the complete state of the css token graph does not have this limitation, the prefix function in Prefixer.js is called by this middleware:

stylis/src/Middleware.js

Line 42 in 9e61e05

export function prefixer (element, index, children, callback) {

SukkaW · 2022-02-24T20:33:08Z

Elaborate?

If grid-row-start and grid-row-end should be prefixed, here is how:

If only grid-row-start exists:
- Prefix with -ms-grid-row. So grid-row-start:3; becomes -ms-grid-row:3;-ms-grid-row:3.
If only grid-row-end exists:
- Prefix with -ms-grid-row-span. So grid-row-end:3; becomes -ms-grid-row-span:3;grid-row-end:3;, and grid-row-end:span 3; becomes -ms-grid-row-span:3;grid-row-end:span 3;
If both of them exists:
- Prefix grid-row-start with -ms-grid-row
- Add -ms-grid-row-span with a value of grid-row-end - grid-row-start
- So grid-row-start:3;grid-row-end:5; becomes -ms-grid-row: 3;grid-row-start:3;-ms-grid-row-span:2;grid-row-end:5;

The same rule applies to column.

I mainly port the features from @robinweser's inline-style-prefixer (He recently switch Fela's prefixer from his own inline-style-prefixer to stylis. Porting the feature helps to keep consistent behavior).

Co-authored-by: thysultan <sultantarimo@me.com>

Andarist · 2022-02-24T20:55:14Z

One concern that I have is that it might potentially increase the output quite a bit for existing users. I wonder if this could be split into 2 plugins somehow? Or should I potentially get ready for copying over the older version of the prefixed into Emotion's codebase? 🤔

robinweser · 2022-02-25T09:33:40Z

Chiming in since I was mentioned in this thread:
Maybe it'd be worth exploring a way to build your own prefixer from browser data or something?
Over at inline-style-prefixer, we export a generateData and createPrefixer function which one can use to create your very own one given a list of browser versions. See https://inline-style-prefixer.js.org/docs/guides/CustomPrefixer.html

SukkaW · 2022-02-25T09:36:10Z

One concern that I have is that it might potentially increase the output quite a bit for existing users. I wonder if this could be split into 2 plugins somehow? Or should I potentially get ready for copying over the older version of the prefixed into Emotion's codebase? 🤔

That's why I said my initial changes are highly unoptimized. And I believe it is possible to reduce the impact down to below 2 KiB or even below 1 KiB, which should then be acceptable.

And currently, the after-gzipped size is only increased by about 200 bytes. That doesn't sound so bad, right?

Use ternary expressions to avoid duplicated statements

Andarist · 2022-02-25T13:04:08Z

And currently, the after-gzipped size is only increased by about 200 bytes. That doesn't sound so bad, right?

I'm not that concerned about the increase of the Stylis' size - but rather about the increased size of the prefixed output. My concern is about the generated CSS.

SukkaW · 2022-02-25T17:09:33Z

I'm not that concerned about the increase of the Stylis' size - but rather about the increased size of the prefixed output. My concern is about the generated CSS.

Only those grid-related declarations will contribute to the increasing final CSS output. So it really depends on how much grid do the developers use in their project currently: if they use none, then the prefixed output will not be affected at all.

So I've done some naive tests here:

Go to some websites
Grab a dump of their entire CSS
Put the CSS into PostCSS's autoprefixer, with the browserlist set to since 2022, IE 10 to make sure only -ms- prefixes get applied.
Compare the size change.

I've checked dev.to (their source code can be found here), a website whose layout is heavily based on the CSS grid. And the CSS size before and after autoprefixer is 444KiB vs 449KiB.

I've also run the test on https://emotion.sh 's doc page by extracting all inlined <style> tags. The size before and after is 34KiB vs 35.2KiB.

thysultan · 2022-02-25T17:18:58Z

src/Prefixer.js

+			return WEBKIT + value + MS + 'flex-item-' + replace(value, /flex-|-self/, '')
+				+ (
+					// center, end, start, stretch
+					match(substr(value, length + 1, sizeof(value) - 1), /^(center|end|start|stretch)$/)


Well, at least it is also how autoprefixer handle align-self:

.example { align-self: stretch }

will become

/* * Prefixed by https://autoprefixer.github.io * PostCSS: v8.3.6, * Autoprefixer: v10.3.1 * Browsers: since 2010 */ .example { -webkit-align-self: stretch; -ms-flex-item-align: stretch; -ms-grid-row-align: stretch; align-self: stretch }

SukkaW · 2022-02-25T19:52:28Z

The prefixer middleware, which has access to the complete state of the css token graph does not have this limitation, the prefix function in Prefixer.js is called by this middleware:

@thysultan Done in #287

robinweser · 2022-02-26T09:38:41Z

@robinweser Kind of wish browserlist in bundlers was like eslint in so much as it would support inline comments to pieces of atomic pieces of code i.e
// ie11
if (a) b(c)
Denoting that the code only exists to make something work on ie11, then this data would make it to the bundler and strip it away from supported browser targets that are not ie11.

Omg, that would be a dream come true!

Especially for something like prefixers where the amount of code and required prefixes differs so much depending on the browser range.

src/Prefixer.js

thysultan · 2022-03-02T19:01:03Z

src/Prefixer.js

@@ -82,12 +98,42 @@ export function prefix (value, length) {
 						return ~indexof(value, 'stretch') ? prefix(replace(value, 'stretch', 'fill-available'), length) + value : value
 				}
 			break
+		// grid-(column|row)


we shouldn't use arrow functions, also possibly more size optimisations, i.e hoisting the regex. I'll submit these and more to this branch this weekend.

src/Prefixer.js

test/Prefixer.js

src/Prefixer.js

thysultan · 2022-03-02T19:05:22Z

src/Prefixer.js

@@ -34,10 +34,17 @@ export function prefix (value, length) {
 			return WEBKIT + value + replace(value, /(\w+).+(:[^]+)/, WEBKIT + 'box-$1$2' + MS + 'flex-$1$2') + value
 		// align-self
 		case 5443:
-			return WEBKIT + value + MS + 'flex-item-' + replace(value, /flex-|-self/, '') + value
+			return WEBKIT + value + MS + 'flex-item-' + replace(value, /flex-|-self/g, '')


TODO: size optimisations. assigning to myself.

SukkaW · 2022-03-02T19:34:32Z

@thysultan Thanks for your review! I will remove whitespace (and possibly other style-related) diff first.

- fix justify-self:flex-start getting grid prefixes.

the following fixes the previous failing test, and uses only 1 regex, is there still space for improvement? will think on it.

SukkaW · 2022-03-11T10:24:17Z

src/Prefixer.js

+		// justify-self
+		case 4200:
+			if (!match(value, /flex-|baseline/)) return MS + 'grid-column-align' + substr(value, length) + value
+			break


@thysultan

I re-checked the implementation of autoprefixer, it only excludes flex-|baseline from being prefixed with grid:

https://github.com/postcss/autoprefixer/blob/3041c7bfced438875d668767432666b10a88ef36/lib/hacks/grid-column-align.js#L7-L9
https://github.com/postcss/autoprefixer/blob/3041c7bfced438875d668767432666b10a88ef36/lib/hacks/grid-row-align.js#L7-L9

Adopting the same behavior allows us to simplify the regexp even further, resulting in an even smaller size.

SukkaW · 2022-03-11T10:26:23Z

src/Prefixer.js

-			return WEBKIT + value + MS + 'flex-item-' + replace(value, /flex-|-self/, '') + value
+			return WEBKIT + value + MS + 'flex-item-' + replace(value, /flex-|-self/g, '') + (!match(value, /flex-|baseline/) ? MS + 'grid-row-' + replace(value, /flex-|-self/g, '') : '') + value


#285 (comment)

@SukkaW Any reason why /flex-|-self/ is converted to /flex-|-self/g and also /align-content|flex-|-self/g, does a test fail because of this, can there be multiple flex- -self notations in the declaration?

align-self:flex-start; should be prefixed as -ms-flex-line-pack:end;, not -ms-flex-line-pack:flex-end;. That's why g is added.

thysultan · 2022-03-19T19:43:58Z

src/Prefixer.js

@@ -82,12 +89,15 @@ export function prefix (value, length) {
 						return ~indexof(value, 'stretch') ? prefix(replace(value, 'stretch', 'fill-available'), length) + value : value
 				}
 			break
+		// grid-(column|row)
+		case 5152: case 5920:
+			return replace(value, /(.+?):(\d+)(\s*\/\s*(span)?\s*(\d+))?(.*)/, function (_, a, b, c, d, e, f) { return (MS + a + ':' + b + f) + (c ? (MS + a + '-span:' + (d ? e : +e - +b)) + f : '') + value })


@SukkaW I updated this, can you review? does it still handle the original cases you had in mind?

Since the test cases passed, so I assume it does.

thysultan · 2022-03-21T19:45:26Z

@SukkaW Thanks for the work you've done on this.

SukkaW mentioned this pull request Feb 24, 2022

autoprefixer for grid in IE11? #119

Closed

feat(prefix): support display grid|inline-grid

ea97843

SukkaW force-pushed the feat-grid branch from 1c3ee10 to ea97843 Compare February 24, 2022 13:08

SukkaW added 6 commits February 24, 2022 21:56

feat(prefix): support grid with justify-self & align-self

44b43cc

feat(prefix): support grid-template-columns

b997ad1

feat(prefix): support grid-template-rows

ed66195

perf: use existed sizeof utility

624e790

feat(prefix): grid-(column|row) with simple value

18001c9

feat(prefix): grid-(column|row) with numeric shorthand

7ba25a5

SukkaW marked this pull request as ready for review February 24, 2022 18:39

SukkaW changed the title ~~[WIP] feat(#119): add grid prefix support~~ feat(#119): add grid prefix support Feb 24, 2022

perf: remove unnecessary utilities

9e61e05

thysultan reviewed Feb 24, 2022

View reviewed changes

src/Prefixer.js Outdated Show resolved Hide resolved

thysultan reviewed Feb 24, 2022

View reviewed changes

src/Prefixer.js Outdated Show resolved Hide resolved

thysultan reviewed Feb 24, 2022

View reviewed changes

src/Prefixer.js Show resolved Hide resolved

thysultan reviewed Feb 24, 2022

View reviewed changes

src/Prefixer.js Outdated Show resolved Hide resolved

perf: use regex over array.indexof

817ff44

Co-authored-by: thysultan <sultantarimo@me.com>

perf: further reduce the size

06af540

Use ternary expressions to avoid duplicated statements

thysultan reviewed Feb 25, 2022

View reviewed changes

SukkaW requested a review from thysultan February 25, 2022 19:10

SukkaW force-pushed the feat-grid branch from f3013ed to 6adc047 Compare February 26, 2022 18:34

refactor(prefix): simplify grid-(column|row)

5e8ea45

SukkaW force-pushed the feat-grid branch from 6adc047 to 5e8ea45 Compare February 26, 2022 18:51