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

Fix sizing of flexbox nodes under a min-content constraint #291

Merged
merged 9 commits into from
Dec 24, 2022

Conversation

nicoburns
Copy link
Collaborator

Objective

Nesting a flexbox row inside a grid column with a min-content width was giving incorrect results.

Context

This was discovered in testing Grid/Flexbox interaction, although I believe it was a pre-existing bug in the Flexbox implementation that was exposed by the Grid implementation implementing min-content.

Changes made

  • Compute flex children's "minimum size" according to the spec
  • Updated cache implementation to avoid thrashing the cache with the new implementation (which happens because we now sometimes need to size a node under both a min-content and a max-content constraint). This bumps the cache size from 4 to 5. I think a similar change would be required for decent Grid perf anyway (we just haven't started measuring this yet).
  • Added tests to cover failing cases.
  • Updated measure.rs test values (these were incorrectly enforcing our previous buggy implementation - I've verified the new values against Chrome).

@nicoburns nicoburns added the bug Something isn't working label Dec 24, 2022
Copy link
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a release notes entry :) I suspect you're right about the need to expand the cache anyways.

I also think we should try to improve the clarity of the cache_slot code.

// Compute cache slot
let has_known_width = known_dimensions.width.is_some();
let has_known_height = known_dimensions.height.is_some();
let cache_slot = if has_known_width && has_known_height {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is hard to follow, and seems likely to result in surprising bugs if altered unsuspectingly. Is there a clearer or more robust approach we can use? It feels like a match statement may help as a stopgap change.

Failing that, more comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic itself is good (for the time being at least), but I agree that it could do with some explaining / documenting. I remember trying to understand the caching implementation when I first started working on Taffy, and it made no sense to me at all.

@nicoburns
Copy link
Collaborator Author

Needs a release notes entry :)

I always forget those!

@nicoburns
Copy link
Collaborator Author

@alice-i-cecile Release notes added, and I've moved the cache slot logic to a function and added documentation to the function. Let me know if it makes any more sense now.

Copy link
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better! May still want to refactor the caching code later, but this is a great base for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants