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

Jump to bottom

Implement CSS Block layout (Display::Block) #474

Merged
merged 85 commits into from
May 19, 2023
Merged

Implement CSS Block layout (Display::Block) #474

merged 85 commits into from
May 19, 2023

Conversation

nicoburns
Copy link
Collaborator

@nicoburns nicoburns commented May 1, 2023 •
edited
Loading

Objective

Tasks

  • Add Display::Block variant to the Display enum
  • Add display: block support to the test generator
  • Minimal naive version of the block layout algorithm
  • Support block nodes with content-based width
  • Support padding on block container
    • Fixed padding
    • Percentage padding
  • Support border on block container
    • Fixed border
    • Percentage border
  • Support scrollbars taking up space
  • Support inset (on relatively positioned items)
    • Fixed inset
    • Percentage inset
  • Support simpler margin cases with no parent-child interactions (on relatively positioned items)
    • Inline-axis fixed margins (+/-)
    • Inline-axis percentage margins (+/-)
    • Inline-axis auto margins
    • Block-axis auto margins (resolve to 0)
    • Block axis fixed (non-collapsing/sibling collapsing) margins (+/-)
    • Block axis percentage (non-collapsing/sibling collapsing) margins (+/-)
  • Support margins collapsing with child margins or through children
    • Block axis collapsing first-child margins
    • Block axis collapsing last-child margins
    • Block axis collapsing zero-height box margins
  • Support hidden children
  • Support absolutely positioned children
  • Support aspect ratio
  • Support computing block container's first baseline
  • Testing
    • Test last-child margins with mixes of positive and negative sizes
    • Test deeper hierarchies
    • Test size being floored by padding+border(+scrollbar gutter?)
    • Test percentage vertical margins and margin collapsing with percentages
    • Test cases where margin collapsing is blocked
      • Blocked by padding
      • Blocked by border
      • Blocked by height/min-height
      • Blocked by aspect-ratio
      • Blocked by overflow
      • Blocked by non-zero measured height
      • Blocked by other display modes
    • Test complex mixes of margin collapse types
    • Test interaction with Flexbox and Grid

Feedback wanted

  • General code review of the block algorithm (src/compute/block.rs).
  • Review of the changes to the signature of the perform_child_layout method (in LayoutTree trait)
  • Review of additions to the SizeAndBaselines (now SizeMarginsAndBaselines struct)
  • Any additional test cases would also be welcome

@nicoburns nicoburns added enhancement New feature or request breaking-change A change that breaks our public interface controversial This work requires a heightened standard of review due to implementation or design complexity labels May 1, 2023
@nicoburns nicoburns marked this pull request as draft May 1, 2023 18:47
@nicoburns nicoburns mentioned this pull request May 1, 2023
Open
@nicoburns nicoburns force-pushed the block-layout branch 4 times, most recently from 690a189 to a5072f8 Compare May 8, 2023 21:32
@nicoburns
Copy link
Collaborator Author

This still needs a bunch of testing, and a couple of minor feature additions. But it's mostly implemented and working, and I'm not anticipating major changes being required, so opening this up for review. See "Feedback wanted" section for recommendations on how to approach reviewing this PR.

TimJentzsch
TimJentzsch reviewed May 11, 2023
View reviewed changes
Copy link
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

I didn't notice any obvious stuff in the algorithm, although I didn't look at it in too much detail.

I mostly just noticed a few copy & paste errors for doc comments.

What do you think about splitting the test_fixtures (and corresponding generated files) into multiple folders?
I think one folder per algorithm and maybe one for mixed tests would be useful to organize things a bit.
I'm not sure how hard that would be to integrate in the generation script though.

src/compute/block.rs Outdated Show resolved Hide resolved
src/compute/block.rs Outdated Show resolved Hide resolved
src/compute/block.rs Outdated Show resolved Hide resolved
src/compute/block.rs Outdated Show resolved Hide resolved
src/compute/block.rs Outdated Show resolved Hide resolved
src/compute/block.rs Show resolved Hide resolved
src/compute/block.rs Outdated Show resolved Hide resolved
@nicoburns nicoburns mentioned this pull request May 15, 2023
Closed
nicoburns and others added 16 commits May 17, 2023 11:31
@nicoburns @TimJentzsch
Apply suggestions from code review
c582986 
Replace mentions of flexbox with block

Co-authored-by: TimJentzsch <TimJentzsch.github.com@timjen.net>
@nicoburns
Update block module documentation
8311333
@nicoburns
Clamp block auto width by min/max width
23d4403
@nicoburns
Don't clamp content size of block nodes by their max height
2a92507
@nicoburns
Add block aspect_ratio tests
a7de646
@nicoburns
Remove baseline alignment support from block layout algorithm
ba72e69 
It turns out that the baseline of a block node is always it's
"fallback baseline" (it's bottom edge). And in Taffy we can represent
this by simply returning `Point::NONE` as the baseline.
@nicoburns
Add tests for block algorithm baseline computation
300ded3
@nicoburns
Add block layout to changelog
8d95cad
@nicoburns
Add block layout to the README
e2de11b
@nicoburns
Improve comment on order field
83094e8
@nicoburns
Add extra block margin collapsing through tests
cb664aa
@nicoburns
Prevent margins collapsing through nodes with overflow-x set to a scroll
6b74908 
container overflow
@nicoburns
Don't allow margins to collapse through line boxes
42584a8
@nicoburns
Add tests for margin collapsing through a node being blocked
77e82c4
@nicoburns
Overflow-x blocking margin collapse fixup
14a4879
@nicoburns
Add tests for margin collapsing with children being blocked
f56c025
@nicoburns nicoburns changed the title WIP: Implement CSS Block layout (Display::Block) Implement CSS Block layout (Display::Block) May 18, 2023
@nicoburns nicoburns marked this pull request as ready for review May 18, 2023 13:08
@nicoburns
Copy link
Collaborator Author

@alice-i-cecile I'm now considering this done pending any changes requested in review. There could of course always be more tests, but this PR already contains 209 which I'm considering a pretty good considering that the block algorithm is relatively simple.

alice-i-cecile
alice-i-cecile approved these changes May 19, 2023
View reviewed changes
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.

Looks good; let's merge this now.

@alice-i-cecile alice-i-cecile merged commit 5443996 into DioxusLabs:main May 19, 2023
@nicoburns nicoburns mentioned this pull request May 19, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimJentzsch TimJentzsch TimJentzsch approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@Weibye Weibye Awaiting requested review from Weibye

Assignees
No one assigned
Labels
breaking-change A change that breaks our public interface controversial This work requires a heightened standard of review due to implementation or design complexity enhancement New feature or request
Projects
None yet
Milestone
0.4
Development

Successfully merging this pull request may close these issues.

Support CSS Block layout (Display::Block)
3 participants
@nicoburns @alice-i-cecile @TimJentzsch