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

[css-grid] Interpolate repeat() #3503

Closed
BorisChiou opened this issue Jan 11, 2019 · 10 comments
Closed

[css-grid] Interpolate repeat() #3503

BorisChiou opened this issue Jan 11, 2019 · 10 comments
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1 Tracked in DoC

Comments

@BorisChiou
Copy link
Contributor

We have a discussion about interpolating track listings (#3201), but the current spec doesn't have clear definition of how to interpolate repeat(). (Or maybe I misread it. Please feel free to close it if there is a duplicated issue or they are well-defined already.)

Example 1:

repeat(2, 2fr 30px)
repeat(2, 3fr 40px)

They use the same repeat count, so doing interpolation between them is ok to me.

Example 2:

repeat(2, 2fr 30px)
repeat(4, 40px)

Their resolved/expanded track lists have the same length, so people may think they are animatable. However, we do interpolation by computed values, so it seems we shouldn't expand the list during interpolation, which means they are not animatable? I'm not sure which behavior is expected.

Example 3:

repeat(auto-fill, 10px)
repeat(auto-fill, 20px)

Even if the keyword is the same it can result in different number of tracks. For both auto-fit/fill, the number of columns isn't known until you do layout since it depends on the container size, item placement and other factors. Therefore, we fallback to discrete in this case. Right?

Thanks.
cc @MatsPalmgren @birtles

@BorisChiou BorisChiou changed the title [css-grid] Interpolating repeat() [css-grid] Interpolate repeat() Jan 11, 2019
@BorisChiou
Copy link
Contributor Author

BorisChiou commented Jan 11, 2019

I will add some test cases to wpt by Firefox Bug 1348519. Please feel free to update them.

@tabatkins
Copy link
Member

Yup, this is unspecified and should be fixed. My intuitions:

repeat(2, 2fr 30px)
repeat(2, 3fr 40px)

This should definitely interpolate. Same N, contained track list is same length and item-wise interpolable, everything's A-OK.

repeat(2, 2fr 30px)
repeat(4, 40px)

I could go either way. Fine with this being interpolable in theory (same lengths once expanded, expanded track list is item-wise interpolable), but I'd also be fine being simple here and saying no if it would cause implementation difficulties. I'm generally in favor of keeping interpolation simple and obvious if possible, which leans me toward saying "no", and requiring authors to write the second as repeat(2, 40px 40px) if they want it to interpolate.

repeat(auto-fill, 10px)
repeat(auto-fill, 20px)

I think these should interpolate, going from 10px to 20px. Yes, the number of tracks can change, but I think that's fine? Track-number determination doesn't happen until used-value time, so interpolation doesn't need to care about it. Leaning on my "should be simple and obvious", these definitely look like they're compatible with each other.


So all in all, the rule I lean towards is "first argument must be identical, second argument must be interpolable as a computed track list; else discrete". So that's yes on Example 1, no on Example 2, and yes on Example 3.

@MatsPalmgren
Copy link

How about:

repeat(2, 2fr 30px)
repeat(4, 40px 40px)

and

repeat(auto-fill, 10px)
repeat(auto-fit, 20px)

It might be convenient to authors if we could treat the first argument to repeat() as just another component that is animatable if the 2nd argument has the same length. My first example would then animate as repeat(2, ...) repeat(3, ...) repeat(4, ...). My second example would animate the first argument discretely while still animating 10px -> 20px as a <length>. (<integer> -> auto-fill/fit would also be discrete)

@tabatkins
Copy link
Member

tabatkins commented Jan 11, 2019

Hm. My first instinct is to say no to both, and require an exact match on the first arg.

But that's not actually a very consistent position; if I'm fine with repeat(auto-fill, 20px) => repeat(auto-fill, 10px), which at used-value time (if we pretend a 40px container) is equivalent to repeat(2, 20px) => repeat(4, 10px), then I should be fine with that 2 => 4 transition directly.

So I think I agree. We can do either of the following:

  • require that the first arg is a number, and identical between start/end; different values, or keyword values, both go discrete
  • allow any values for the first arg: if they've the same value they transition by doing nothing; if they're different numbers they transition as integers; otherwise the whole function transitions discretely

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Interpolate repeat(), and agreed to the following:

  • RESOLVED: Define repeat interpolation using the more restrictive rule in https://github.com/w3c/csswg-drafts/issues/3503#issuecomment-453674839
The full IRC log of that discussion <dael> Topic: Interpolate repeat()
<dael> Github: https://github.com//issues/3503#issuecomment-453674839
<fantasai> https://github.com//issues/3503#issuecomment-453674839
<gregwhitworth> iank_: yes - we can chat about it a little bit, I'll do a phone chat with Alek as well
<dael> TabAtkins: Question was raised by Boris. How are repeat functions interpolated in grid template rows
<fantasai> s/Alek/Aleks/
<dael> TabAtkins: It's retained in computed values bc some can't be resolved at computed value time. Computed value animaiton runs on will have repeat functions. How do they interpolate. This isn't in spec.
<dael> TabAtkins: They have 2 functions. After some discussion in issue and some nice examples there's 2 options that are consistent with how we interpolate
<dael> TabAtkins: First is we're very restrictive. To get smooth interpolation first arg must be a number and track listing must have same number of things. 2, 10px, 10px to 2, 20px, 20px will be smooth. If you have auto fit no smooth
<dael> TabAtkins: Other poss is we allow any values for first. If same transition by doing nothing. If they're numbers and diff they transition as int. If you try and go into to keyword it goes discrete.
<dael> TabAtkins: Still require track listing match with normal transitioning rules. You could not go from a repeat 4, 10 px to repeat 2, 10px, 10px. An auto fit and auto fill we can't match and I don't like special casing that
<dael> TabAtkins: I propose: We require that either the first arg be the same value or both int. Track listings must be interpolable normally
<dael> fantasai: 2 proposal. Main difference is the first one you can only interpolate if it results in same # of tracks. 2nd version we try and follow value interp patterns we have and that allows more possibilities. Disadvantage to that is you're not interpolate between # tracks and therefore position of elements can change.
<dael> fantasai: Do we want as much smooth in value space but have layout discontinuous or do we want to align what's possible to interopolate in layout with value space
<dael> TabAtkins: With caveat that discontinuous layout does happen if you end up in discrete. We do allow int based on grid positions we also can have skipping. THis is the whole grid, but similar in idea
<dael> astearns: I think I would like to have many example is spec whichever we choose
<dael> TabAtkins: Yep
<dael> fantasai: Anyone besides me and TabAtkins have an opinion or want more time?
<dael> astearns: fantasai do you have a preference?
<dael> fantasai: From what I know I'd go with first option. More restrictive.
<dael> fantasai: It seems like interpolating in value space and having things jump around, I'm not sure it would be that great. I don't feel very strongly b/c I don't understand strongly impl for layout system. I'd do whatever Mats thinks is right
<dael> fantasai: We can poke Mats. Any other opinions?
<dael> astearns: Often we will be more restrictive to begin with and then later add smooth interpolation to things that were previously discrete. THat's an argument to more restrictive because we can get to more liberal eventually
<dael> astearns: Other opinions?
<dael> astearns: Am I correct about being able to loosen in the future?
<dael> fantasai: I think you're correct
<dael> astearns: Prop: Define repeat interpolation using the more restrictive rule in https://github.com//issues/3503#issuecomment-453674839
<dael> astearns: Objections?
<dael> RESOLVED: Define repeat interpolation using the more restrictive rule in https://github.com//issues/3503#issuecomment-453674839

@fantasai
Copy link
Collaborator

Alright, fixed in 2e57e1e ! @MatsPalmgren @BorisChiou Let us know if the edits seem satisfactory.

@BorisChiou
Copy link
Contributor Author

Thanks for this. :)

@fantasai fantasai added Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. and removed Commenter Response Pending labels Jan 29, 2019
@birtles
Copy link
Contributor

birtles commented Jan 29, 2019

It would be nice if we didn't refer to interpolation but combination so that it covers addition and accumulation too.

@fantasai
Copy link
Collaborator

fantasai commented Feb 7, 2019

@birtles Done.

@birtles
Copy link
Contributor

birtles commented Feb 7, 2019

Thank you @fantasai!

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 5, 2019
…se cbindgen.

I'm _really_ sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.

This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:

 * Make us preserve repeat() at computed-value time. This is per spec since
   interpolation needs to know about repeat(). Except for subgrid, which did the
   repeat expansion at parse-time and was a bit more annoying (plus it doesn't
   really animate yet so we don't need it to comply with the spec).

 * Tweaks the WPT tests for interpolation to adopt the resolution at:
   w3c/csswg-drafts#3503.

Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.

Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.

This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.

In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.

This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.

Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.

Differential Revision: https://phabricator.services.mozilla.com/D36598

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1519958
gecko-commit: a1eb75e785ded866d66184c8b59b5a5b907a34fb
gecko-integration-branch: autoland
gecko-reviewers: mats, boris
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 6, 2019
…lue time and use cbindgen. r=mats,boris

I'm _really_ sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.

This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:

 * Make us preserve repeat() at computed-value time. This is per spec since
   interpolation needs to know about repeat(). Except for subgrid, which did the
   repeat expansion at parse-time and was a bit more annoying (plus it doesn't
   really animate yet so we don't need it to comply with the spec).

 * Tweaks the WPT tests for interpolation to adopt the resolution at:
   w3c/csswg-drafts#3503.

Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.

Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.

This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.

In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.

This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.

Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.

Differential Revision: https://phabricator.services.mozilla.com/D36598
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 6, 2019
…se cbindgen.

I'm _really_ sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.

This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:

 * Make us preserve repeat() at computed-value time. This is per spec since
   interpolation needs to know about repeat(). Except for subgrid, which did the
   repeat expansion at parse-time and was a bit more annoying (plus it doesn't
   really animate yet so we don't need it to comply with the spec).

 * Tweaks the WPT tests for interpolation to adopt the resolution at:
   w3c/csswg-drafts#3503.

Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.

Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.

This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.

In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.

This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.

Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.

Differential Revision: https://phabricator.services.mozilla.com/D36598

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1519958
gecko-commit: a1eb75e785ded866d66184c8b59b5a5b907a34fb
gecko-integration-branch: autoland
gecko-reviewers: mats, boris
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Aug 6, 2019
…lue time and use cbindgen. r=mats,boris

I'm _really_ sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.

This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:

 * Make us preserve repeat() at computed-value time. This is per spec since
   interpolation needs to know about repeat(). Except for subgrid, which did the
   repeat expansion at parse-time and was a bit more annoying (plus it doesn't
   really animate yet so we don't need it to comply with the spec).

 * Tweaks the WPT tests for interpolation to adopt the resolution at:
   w3c/csswg-drafts#3503.

Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.

Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.

This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.

In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.

This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.

Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.

Differential Revision: https://phabricator.services.mozilla.com/D36598
emilio added a commit to emilio/servo that referenced this issue Aug 15, 2019
…e and use cbindgen.

I'm _really_ sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.

This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:

 * Make us preserve repeat() at computed-value time. This is per spec since
   interpolation needs to know about repeat(). Except for subgrid, which did the
   repeat expansion at parse-time and was a bit more annoying (plus it doesn't
   really animate yet so we don't need it to comply with the spec).

 * Tweaks the WPT tests for interpolation to adopt the resolution at:
   w3c/csswg-drafts#3503.

Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.

Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.

This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.

In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.

This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.

Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.

Differential Revision: https://phabricator.services.mozilla.com/D36598
natechapin pushed a commit to natechapin/wpt that referenced this issue Aug 23, 2019
…se cbindgen.

I'm _really_ sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.

This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:

 * Make us preserve repeat() at computed-value time. This is per spec since
   interpolation needs to know about repeat(). Except for subgrid, which did the
   repeat expansion at parse-time and was a bit more annoying (plus it doesn't
   really animate yet so we don't need it to comply with the spec).

 * Tweaks the WPT tests for interpolation to adopt the resolution at:
   w3c/csswg-drafts#3503.

Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.

Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.

This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.

In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.

This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.

Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.

Differential Revision: https://phabricator.services.mozilla.com/D36598

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1519958
gecko-commit: a1eb75e785ded866d66184c8b59b5a5b907a34fb
gecko-integration-branch: autoland
gecko-reviewers: mats, boris
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…lue time and use cbindgen. r=mats,boris

I'm _really_ sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.

This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:

 * Make us preserve repeat() at computed-value time. This is per spec since
   interpolation needs to know about repeat(). Except for subgrid, which did the
   repeat expansion at parse-time and was a bit more annoying (plus it doesn't
   really animate yet so we don't need it to comply with the spec).

 * Tweaks the WPT tests for interpolation to adopt the resolution at:
   w3c/csswg-drafts#3503.

Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.

Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.

This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.

In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.

This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.

Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.

Differential Revision: https://phabricator.services.mozilla.com/D36598

UltraBlame original commit: a1eb75e785ded866d66184c8b59b5a5b907a34fb
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…lue time and use cbindgen. r=mats,boris

I'm _really_ sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.

This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:

 * Make us preserve repeat() at computed-value time. This is per spec since
   interpolation needs to know about repeat(). Except for subgrid, which did the
   repeat expansion at parse-time and was a bit more annoying (plus it doesn't
   really animate yet so we don't need it to comply with the spec).

 * Tweaks the WPT tests for interpolation to adopt the resolution at:
   w3c/csswg-drafts#3503.

Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.

Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.

This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.

In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.

This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.

Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.

Differential Revision: https://phabricator.services.mozilla.com/D36598

UltraBlame original commit: a1eb75e785ded866d66184c8b59b5a5b907a34fb
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…lue time and use cbindgen. r=mats,boris

I'm _really_ sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.

This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:

 * Make us preserve repeat() at computed-value time. This is per spec since
   interpolation needs to know about repeat(). Except for subgrid, which did the
   repeat expansion at parse-time and was a bit more annoying (plus it doesn't
   really animate yet so we don't need it to comply with the spec).

 * Tweaks the WPT tests for interpolation to adopt the resolution at:
   w3c/csswg-drafts#3503.

Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.

Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.

This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.

In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.

This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.

Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.

Differential Revision: https://phabricator.services.mozilla.com/D36598

UltraBlame original commit: a1eb75e785ded866d66184c8b59b5a5b907a34fb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1 Tracked in DoC
Projects
None yet
Development

No branches or pull requests

6 participants