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

Support CSS Grid #204

Closed
87 tasks done
Tracked by #345
nicoburns opened this issue Jul 22, 2022 · 24 comments · Fixed by #205
Closed
87 tasks done
Tracked by #345

Support CSS Grid #204

nicoburns opened this issue Jul 22, 2022 · 24 comments · Fixed by #205
Labels
enhancement New feature or request

Comments

@nicoburns
Copy link
Collaborator

nicoburns commented Jul 22, 2022

As per #28 (comment), we would like to support CSS Grid in Taffy. This is a modified version of research I did on supporting this in Stretch (vislyhq/stretch#63 (comment)). Which I thought I would copy and update here as a starting point.

Status (22nd Dec 2022)

The implementation is largely feature complete, and a pre-release version of Taffy containing the CSS Grid implementation has been released. There is now a lot of testing and validating to be done before it is ready for a stable release.

Blocking questions / issues

  • How should Grid interact with intrinsically sized leaf nodes. The spec seems to define child nodes as having both a min-size (e.g. text node that wraps at all opportunities) and max-size (e.g. a text node that doesn't wrap at all). Possibly these would need to be added to nodes as additional "measure functions"? Or possibly they could both be assumed to be equal to the one measure functions at first? Possibly min can be obtained by passing the MeasureFunc width: 0, height: 0, and max can be obtained by passing the MeasureFunc width: undefined, height: undefined? But perhaps it would be better to redefine the MeasureFunc to "sizing constraints" (https://www.w3.org/TR/css-sizing-3/#constraints) in addition a definite size constraint if one is available? https://www.w3.org/TR/css-sizing-3/#intrinsic-contribution
    Plan is to update MeasureFunc to take the sizing constraint as parameter (which it can then use or ignore as it sees fit).
  • The interaction between nested Grid and Flexbox nodes. The comment at https://github.com/DioxusLabs/taffy/blob/main/src/flexbox.rs#L463-L466 suggests that the Flex implementation could be made more spec compliant by passing min-content/max-content constraints into to the top (presumably of the compute_internal function. I feel like this may be a prerequisite to getting Grid and Flex to play nicely with each other in indefinitely sized contexts. Tracking issue here: Fix min_max computation when resolving flexible lengths to follow the spec #122
    Plan is not to worry about integration at first, but it's likely that we'll adjust the Flexbox implementation to take sizing constraints as input as suggested above when we are ready to integrate.

Update: Both of these solutions were implemented as part of #246. Grid's integration with other layout modes is now unblocked.

Useful Resources:

MVP Implementation Plan

  • Extend the Style and Geometry sections of Stretch to include definitions for CSS Grid styles, while ensuring that the existing Flex implementation continues to work:

    • fr units This has been implemented as the Flex variant of the new TrackSizingFunction enum rather than being added to Dimension as it only applies to grid templates, and it turns out there are also a bunch of other options (like minmax) that apply there but not elsewhere.
    • display: grid
    • grid-template-columns/grid-template-rows
    • grid-auto-rows/grid-auto-columns/grid-auto-flow
    • grid-column / grid-row/ grid-row-start/grid-row-end/grid-column-start/grid-column-end
    • row-gap/column-gap (note: gap shorthand implemented instead)
    • justify-items / justify-self (justify-content / align-content / align-items / align-self already implemented for flexbox). Note: *-items and *-self properties align grid items (children) within their cell. *-content properties align cells (rows/columns) within the grid container. I think that all are set to stretch to fit by default.

    We can probably ignore the shorthand properties for now, as their functionality is covered by the longhand equivalents above.

  • Resolve the explicit grid:

    • Count the number of tracks in the "explicit grid". This is the rows and columns as defined by grid-template-columns/grid-template-rows (Sections 7.1-7.4 of the spec).
    • Extend gentest infrastructure to support repeat() syntax
    • Implement resolving fixed repetition explicit tracks repeat(integer) Grid: Add support for repeat() with an integer repetition count #342
    • Implement resolving auto-generated explicit tracks repeat(auto-fill)
    • Implement collapsing tracks for repeat(auto-fit)

    (Not strictly necessary features like named grid lines/areas defined by grid-template-areas will be left until later.)

  • Implement grid placement. Matching Grid Items (child nodes of the grid) to Grid Areas. This may include generating extra rows and columns that form the "implicit grid" if any Grid Items are placed outside of the bounds of the "explicit grid". (note: the "implicit grid" is the whole grid, not just the extra bit) (Sections 7.5-8.5 of the spec)

  • Resolve the track sizing functions for all grid tracks (explicit and implicit).

  • Implement the Grid Sizing Algorithm to determine the size of each Grid Area. This mostly consists of the Track Sizing Algorithm, iterated a few times. A Grid Track is a row or column of the grid. Gutters sizes should also be resolved here, although this is relatively simple. (Section 11 of the spec).

    • Resolve size of fixed size tracks (11.4. "Initialize Track Sizes" and 11.6. "Maximise Tracks")
    • Resolve size of flexible tracks (with no children affecting size) (11.7 "Expand Flexible Tracks")
    • Resolve size of intrinsically sized tracks (with no children affecting size) (11.8. "Stretch auto Tracks")
    • Allow children of span 1 to affect track size (11.5. "Resolve Intrinsic Track Sizes" Step 2)
    • Allow children with span > 1 to affect track size (11.5. "Resolve Intrinsic Track Sizes" Step 3 and 4)
    • Support for fit-content() track sizing functions
      • Fixed points fit-content arguments
      • Percentage fit-content arguments
  • Resolve Grid Item (node) sizes and positions, by summing the size of the track and gutters in their grid area (in each axis), and taking into accounts spacing and alignment properties (Section 10 of the spec).

    • Align grid track using align_content and justify_content
    • Align items within their grid area using align_items/align_self/justify_items/justify_self
  • Resolve sizes and positions of absolutely positioned elements (Section 9 of the spec)

  • Support for baseline alignment Grid: baseline alignment support #344

Code quality tasks:

  • Fix all warnings
  • Fix clippy lints
  • Convert track sizing function enums to use LengthPercentage rather than Dimension
  • Implement existing style helper functions (auto, zero, points) for grid styles
  • Add new style helpers:
    • percent
    • min_content
    • max_content
    • flex
    • minmax
    • repeat
    • track (line?)
    • span
  • Refactor code that deals with axes (make consistent)
  • Evaluate use of grid crate
  • Update release notes to reflect the changes in this branch
    • Updates to alignment styles
    • CSS Grid layout mode
  • Add feature flag for CSS grid feature (experimental_grid?)

Correctness fixes:

To test thoroughly

Performance related tasks:

Further tasks

Grid related further tasks

  • Support for named grid lines
  • Support for grid-template-areas and grid-area
  • Support for CSS Grid Level 2 (subgrids)

Non-grid further tasks

  • Support for min-content/max-content/fit-content sizes (widths/heights)
  • Add sub-directory support to gentests to separate grid and flexbox tests.
@nicoburns nicoburns added the enhancement New feature or request label Jul 22, 2022
@alice-i-cecile
Copy link
Collaborator

The interaction between nested Grid and Flexbox nodes

For an MVP, I'm totally fine to punt on this, and forbid mixed layouts. In fact, I'd probably prefer that, to reduce the complexity in the initial PR (which will be extremely high already).

@nicoburns
Copy link
Collaborator Author

nicoburns commented Jul 23, 2022

Style Properties

Implementation status and priorities for implementing

Y = Supported in spec and implemented in Taffy
N = Supported in spec but not implemented in Taffy
- = Not applicable to layout mode
1-5 = Priorities for a phased implementation of CSS Grid

Property Flex Grid Description
Layout Mode
display Y 1 What layout strategy should be used?
Position
position_type Y 2 Absolute vs. in-flow position
position Y 2 How should the position of this element be tweaked relative to the layout defined?
Item size
size Y 1 The nominal height and width of item
min_size Y 1 The minimum height and width of the item
max_size Y 1 The maximum height and width of the item
aspect_ratio Y 3 The preferred aspect ratio (calculated as width divided by height)
padding Y 1 How large should the padding be on each side?
border Y 1 How large should the border be on each side?
Item spacing
margin Y 1 How large should the margin be on each side?
gap Y 1 The size of the vertical and horizontal gaps between flex items / grid rows
Alignment
align_items Y 3 How should items be aligned relative to the cross axis?
align_self Y 3 Should this item violate the cross axis alignment specified by its parent's [AlignItems]?
align_content Y 3 How should content contained within this item be aligned relative to the cross axis?
justify_items - 3 How should items be aligned relative to the main axis?
justify_self - 3 Should this item violate the main axis alignment specified by its parent's [AlignItems]?
justify_content Y 3 How should content contained within this item be aligned relative to the main axis?
Flexbox
flex_direction Y - Which direction does the main axis flow in?
flex_wrap Y - Should elements wrap, or stay in a single line?
flex_basis Y - Sets the initial main axis size of the item
flex_grow Y - The relative rate at which this item grows when it is expanding to fill space
flex_shrink Y - The relative rate at which this item shrinks when it is contracting to fit into space
CSS Grid (Container)
grid-template-columns - 1
grid-template-rows - 1
grid-template-areas - 4
grid-auto-rows - 1
grid-auto-columns - 1
grid-auto-flow - 1
CSS Grid (Child)
grid-row-start - 1
grid-row-end - 1
grid-column-start - 1
grid-column-end - 1
grid-column - 1
grid-row - 1
grid-area - 4

@alice-i-cecile
Copy link
Collaborator

Super interesting. Do you think we should split the FlexboxLayout (rename pending to FlexboxStyle) struct into several substructs then so we can reuse this? Or do we want a GridStyle struct that doesn't share data at all?

@nicoburns
Copy link
Collaborator Author

Do you think we should split the FlexboxLayout (rename pending to FlexboxStyle) struct into several substructs then so we can reuse this? Or do we want a GridStyle struct that doesn't share data at all?

I'm not quite sure. I think we'll want to think about what's efficient in terms of number of allocations vs. size of structs with properties potentially not being used. It could possibly make sense to have it an enum based on display with separate structs for each display mode. Or perhaps we could store each part in a separate SlotMap with Option in a main struct? Also worth considering that Flexbox and Grid share a lot because they're both CSS. But other layout modes may not share nearly so much. For the time being I was planning to punt on this question and just add everything into the FlexboxLayout/FlexboxStyle struct. That should be sufficient for unblocking implementation of the actual algorithm.

A couple of other complications:

  • It's possible to have named grid regions (created with grid-template-areas then referenced with grid-area). These use user-supplied identifiers (strings). I suspect we'd want some kind of string interning for this (can we expect callers of the library to pre-intern strings before passing to us?)

  • It's theoretically possible to have arbitrarily many grid rows and columns (in practice I can't imagine anybody would want more than about 20 - although there's bound to be someone). So we will either need to use a Vec, or limit the number and use something like an ArrayVec (but this would still end up being quite a large ArrayVec)

@alice-i-cecile
Copy link
Collaborator

It's possible to have named grid regions (created with grid-template-areas then referenced with grid-area). These use user-supplied identifiers (strings). I suspect we'd want some kind of string interning for this (can we expect callers of the library to pre-intern strings before passing to us?)

Bevy has a Name component that would fit well here 🤔 I'd punt on this for later for sure though.

It's theoretically possible to have arbitrarily many grid rows and columns (in practice I can't imagine anybody would want more than about 20 - although there's bound to be someone). So we will either need to use a Vec, or limit the number and use something like an ArrayVec (but this would still end up being quite a large ArrayVec)

IMO this is a great fit for something like a SmallVec: it's stack-allocated until it gets too large, then it swaps to the heap.

@nicoburns
Copy link
Collaborator Author

Update: I have implemented support for CSS grid style properties in the gentest scripts (test_helper.js and main.rs). It is now possible to generate valid (they compile) tests from HTML fixtures in the same way as is currently done for flexbox. I haven't yet created a large set of tests, and I don't intend to do this until we have a decent implementation of the algorithm itself, as I think 100s of failing tests will be more of a distraction than helpful at first.

One note: CSS Grid has some extra values for some of the alignment properties that aren't supported in Flexbox. For example, both Flexbox and Grid use justify-content, but stretch is a valid value for Grid but not Flexbox. We'll need to work out what we want to do in these cases. My instinct is just to add the value for all layout modes, and silently ignore values that are invalid for the specified layout mode (by setting them back to the default value). Which is basically what CSS itself does, and in this case would also potentially allow us to merge the JustifyContent enum with the AlignContent enum. But we could go down a route of having separate per-layout mode enums.

For now, I've left these properties as-is. And I also haven't yet added the two missing alignment properties (see above). I have added the gap property, as reading through the spec again (/playing around in chrome), this seems fairly core to sizing grid tracks (rows/columns) properly when the size is dependent on the content size of a child that spans more than one track (the gutter size is included in the child's size, so the columns don't have to be quite as wide as they would need to be without a gutter).

@alice-i-cecile
Copy link
Collaborator

But we could go down a route of having separate per-layout mode enums.

I have a pretty strong preference for this. I don't think there's much value in reusing the enums, and I'd like to try to get nice type safety wherever we can.

@Weibye
Copy link
Collaborator

Weibye commented Jul 31, 2022

I agree with alice here. When we start supporting even more algorithms I'd prefer to not add noise from one into the other as it would make it even harder to reason about

@nicoburns
Copy link
Collaborator Author

nicoburns commented Dec 6, 2022

It still needs a lot of work – tidying up the code, removing all the debug logs, documenting, improving performance, and likely fixing correctness issues – but this is now working to the point that it would be reasonable for other people to try it and play around with it.

Generated tests are working end-to-end. So that could be a good way to try this out. And if anyone is interested in helping, then coming up with generated tests which don't work with the current implementation would be helpful.

I have also extended the test generator to support generating measure_funcs. This is triggered by adding text to a leaf node. In order to make sure that measurements match correctly, you should only use H (capital h) and &ZeroWidthSpace characters in leaf nodes. This restriction extends to leaf nodes not containing line breaks, so be careful with formatting. An example of this can be found here: https://github.com/DioxusLabs/taffy/blob/22c9f78905c9f1a5523da5bd802b48ede8eae80f/test_fixtures/grid_min_content_maximum_single_item.html

EDIT: I should add that it is absolutely not ready for code review. Some of the older code is in decent shape, but the recently added track sizing code needs a lot of cleaning up!

@nicoburns nicoburns pinned this issue Dec 16, 2022
@nicoburns
Copy link
Collaborator Author

Possible source of test cases here: https://github.com/web-platform-tests/wpt/tree/master/css/css-grid

@nicoburns
Copy link
Collaborator Author

Knew that was going to happen. Reopening so we can continue to track progress.

@nicoburns nicoburns reopened this Dec 22, 2022
@nicoburns
Copy link
Collaborator Author

nicoburns commented Dec 26, 2022

Out of curiosity, can the GP::line(0) condition be reached here: https://github.com/DioxusLabs/taffy/blob/main/src/style/grid.rs#L138 if taking the check on L130 into account (i'm new to Rust, might be a dumb question, in which case I am sorry in advance)

No, I'm pretty sure it can't EDIT: Sorry, yes it can because the if track1 != 0 && track2 != 0 guard means the zero case won't match line L130 even though the pattern on that line does match.

As it happens, all of the resolve_* methods in that impl need completely redoing (see the bullet point starting "More careful handling of grid lines indexes" in the todo list above). This because arithmetic on grid line indexes is invalid unless you use "the number of explicit grid tracks" in the calculation due to how the grid indexes are defined (positive indices count from the left of the explicit grid and negative indices from the right - so in a grid with 2 tracks (and therefore 3 grid lines) in a given axis, grid line 1 and grid line -3 are the same line, and the line before grid line 1 is actually line -4)).

The arithmetic we have currently will work so long as it doesn't end up crossing zero (which will be most of the time), but will fail (placing the item in the wrong place) is it does cross zero.

@adjabaev
Copy link
Contributor

adjabaev commented Jan 3, 2023

Hi!

I was going trough today's commit and came across this
https://github.com/DioxusLabs/taffy/blob/main/src/compute/grid/mod.rs#L300

isn't it supposed to be final_row_counts ?
If not sorry in advance!

@nicoburns
Copy link
Collaborator Author

@adjabaev I believe you are correct. Well spotted! Would you like to submit a PR to fix it?

@adjabaev
Copy link
Contributor

adjabaev commented Jan 3, 2023

@nicoburns done! I'm glad it helps!! 🎉

@nicoburns
Copy link
Collaborator Author

nicoburns commented Jan 30, 2023

Phew! So it's been a long journey, but the end is in sight! I've just submitted #344 implementing baseline alignment, which means that once the following outstanding PRs have been merged:

... we are feature complete, performant enough for an initial release, and have no outstanding known issues!

The remaining task will be to create some high-level tests that test complex track content-sizing scenarios, and the interaction between grid and flexbox. Although I'm keen not to go overboard with this, as with N grid features and M flexbox features it would be end up being a ridiculous amount of tests if we attempted to test all feature combinations. I suggest we stick to a strategy of mostly "unit style" generated tests that test features in isolation or in combination with a single other feature, along with few carefully crafted integration tests that test a lot of features at once (allowing us to keep the total number of them low as each test is testing many features).

Once that's completed, we can close this issue and start thinking about putting out a stable 0.3.0 release :)

@berkus
Copy link

berkus commented Feb 5, 2023

@nicoburns this is amazing, any way to test it out with current dioxus?

@nicoburns
Copy link
Collaborator Author

@berkus So Dioxus is currently only using Taffy for the TUI platform. All other platforms are rendering in a web view that already has built-in support for Flexbox and CSS Grid layout. Experimental native rendering which also uses Taffy exists in https://github.com/DioxusLabs/blitz. To test Grid support with the TUI or Blitz I believe you would currently need to upgrade the Taffy version yourself.

@triniwiz
Copy link

triniwiz commented Feb 5, 2023

@nicoburns I’ve been experimenting with using taffy for Android and iOS since the initial grid support was merged with hopes to replace NativeScript’s views 😅 and to bring it closer to the web. It’s not perfect but I do have a couple wrappers

@nicoburns
Copy link
Collaborator Author

@triniwiz That's awesome. Would you be willing to contribute the bindings upstream into Taffy?

@triniwiz
Copy link

triniwiz commented Feb 5, 2023

@nicoburns I could would love to try but rather not subject anyone to those crimes committed in that repo 😭.

@nicoburns
Copy link
Collaborator Author

@triniwiz Well contributing upstream could be an opportunity to clean things up ;)

@nicoburns
Copy link
Collaborator Author

The remaining feature PR have been merged. And the testing / bug fixing ones have too. There's always more testing that can be done, but we have to draw the line somewhere and I think it's fair to say that we have a working CSS Grid implementation at this point.

Closing this issue as completed. Further work can continue in new issues if need be.

@jkelleyrtp
Copy link
Member

I'll spend some time working with @Demonthos to get it wired up into blitz! Amazing! Thanks for all the hard work here.

@nicoburns nicoburns unpinned this issue Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants