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

Use taffy for scroll container layouting #731

Merged
merged 1 commit into from
Jan 11, 2025

Conversation

PolyMeilex
Copy link
Contributor

@PolyMeilex PolyMeilex commented Jan 11, 2025

Given this simple layout:

use floem::prelude::*;

fn main() {
    floem::launch(|| {
        v_stack((
            "Header".style(|s| s.background(Color::RED)),
            "Long Content\n"
                .repeat(100)
                .scroll()
                .style(|s| s.flex_grow(1.0).width_full().max_height_full()),
        ))
        .style(|s| s.size_full())
    })
}

The scroll container height will be constrained to v_stack height and will not take into account it's siblings, causing it to overflow:

TREE
└──  FLEX COL [x: 0    y: 0    w: 800  h: 600  content_w: 800  content_h: 1212 border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967301))
    ├──  FLEX ROW [x: 0    y: 0    w: 800  h: 12   content_w: 42   content_h: 12   border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967298))
    │   └──  LEAF [x: 0    y: 0    w: 42   h: 12   content_w: 0    content_h: 0    border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967306))
    └──  FLEX ROW [x: 0    y: 12   w: 800  h: 600  content_w: 77   content_h: 1200 border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967300))
        └──  FLEX ROW [x: 0    y: 0    w: 77   h: 1200 content_w: 77   content_h: 1200 border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967299))
            └──  LEAF [x: 0    y: 0    w: 77   h: 1200 content_w: 0    content_h: 0    border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967307))

The size of the node 4294967300 is 600 rather than expected 600 - 12 (for the header node).

I know that it is possible to workaround this by fiddling with flex basis and min_height (aka. #157) but I believe that the proper way to solve this is to simply let taffy know that this is a scroll container.

It will result in this layout:

TREE
└──  FLEX COL [x: 0    y: 0    w: 800  h: 600  content_w: 800  content_h: 600  border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967301))
    ├──  FLEX ROW [x: 0    y: 0    w: 800  h: 12   content_w: 42   content_h: 12   border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967298))
    │   └──  LEAF [x: 0    y: 0    w: 42   h: 12   content_w: 0    content_h: 0    border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967306))
    └──  FLEX ROW [x: 0    y: 12   w: 800  h: 588  content_w: 77   content_h: 1200 border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967300))
        └──  FLEX ROW [x: 0    y: 0    w: 77   h: 1200 content_w: 77   content_h: 1200 border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967299))
            └──  LEAF [x: 0    y: 0    w: 77   h: 1200 content_w: 0    content_h: 0    border: l:0 r:0 t:0 b:0, padding: l:0 r:0 t:0 b:0] (NodeId(4294967307))

As you can see the 12px of the sibling's height was taken into account, and the the scroll node no longer overflows.

As as side bonus this also eliminates the need to manually constrain the scroll height:

@@ -7,8 +7,8 @@
             "Long Content\n"
                 .repeat(100)
                 .scroll()
-                .style(|s| s.flex_grow(1.0).width_full().max_height_full()),
+                .style(|s| s.flex_grow(1.0).width_full()),
         ))
         .style(|s| s.size_full())
     })
}

This also opens up a possibility of using taffy::Style::scrollbar_width and taffy::Layout::scrollbar_size in the future.

@dzhou121
Copy link
Contributor

Thanks for fixing this!

@dzhou121 dzhou121 merged commit 5ab32a2 into lapce:main Jan 11, 2025
7 checks passed
@PolyMeilex PolyMeilex deleted the use-taffy-for-scroll-layouting branch January 11, 2025 23:26
@jrmoulton
Copy link
Collaborator

jrmoulton commented Jan 12, 2025

Woohoo! Thanks

@PolyMeilex
Copy link
Contributor Author

Love the enthusiasm 😁, thank you both for quick response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants