DioxusLabs / taffy

A high performance rust-powered UI layout library
https://docs.rs/taffy
Other
2.04k stars 102 forks source link

Absolute Insets should be resolved against the container size minus border #666

Closed julia-script closed 2 months ago

julia-script commented 3 months ago

Objective

Currently, absolute insets seem to be resolved incorrectly

Context

Right now both relative and absolute insets are resolved against the container size. The common (and probably specified) behavior is to resolve them against container_size - border, and in taffy's case, I'd assume container_size - border - scrollbar_width when applicable

Note: Parent's padding does not seem to affect insets

Here's a codepen example

https://codepen.io/juliascript/pen/abrJvZa

Here's how yoga behaves

https://www.yogalayout.dev/playground?code=DwGQhgng9grgLgAgMZQHYDMCWBzAvAb3xgGcBTAdVICMARU9MGAGzmIC4E4AnGUgXz4A+AFAIEwAHJQAJqQTE4EJqQL4A7pmlwAFhwCMABgMAaBNtI5tcfUdMAHMNOmZU2G6apQusruU079AFYEARExMUkZOQUlFUJRcLE7KGJMOEw0DgByMCpiKCZ4UizjBMS4KDtswIMAUhKy8OV0awQswzqGxLENLV0EAGYTRrFzS1ah0sSBBAB6MPFZqVkRYFnwaHhBIA

Here's the same layout from the Yoga example computed with taffy BEFORE the fix

TREE
└──  FLEX ROW [x: 0    y: 0    w: 100  h: 100  content_w: 145  content_h: 95   border: l:15 r:15 t:15 b:15, padding: l:10 r:10 t:10 b:10] (NodeId(4294967298))
    └──  LEAF [x: 115  y: 65   w: 30   h: 30   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(4294967297))

(x and y ignores parent's border)

Here's the result AFTER the fix

TREE
└──  FLEX ROW [x: 0    y: 0    w: 100  h: 100  content_w: 115  content_h: 80   border: l:15 r:15 t:15 b:15, padding: l:10 r:10 t:10 b:10] (NodeId(4294967298))
    └──  LEAF [x: 85   y: 50   w: 30   h: 30   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(4294967297))

Feedback

Looking at the code, it seems to me that the scrollbar is always assumed to be on the right or bottom, is this assumption correct?

julia-script commented 3 months ago

Hi 👋

Sure, I'll try to take a look at this this week 😄

julia-script commented 2 months ago

Quick check in.

Block layout seems to be working as expected. Grid layout works as expected without the scrollbar. Scrollbar doesn't seem to be accounted for though.

Also, I noticed that auto insets of absolute elements in chrome, firefox and safari are different depending on the layout mode. Flex and block accounts for padding while grid doesn't. Absolute and relative insets seem to behave the same though.

image

https://codepen.io/ju-faria/pen/MWdrOQR

The changes in this PR actually make flex work like grid (not account for padding in auto insets),so I imagine that was one cause for the bug in the first place, auto insets should account for padding while explicit insets shouldn't.

Weird standards, probably legacy madness, but it is what it is 😅. I'll check the test fixtures you mention and address the issues.

Edit:

no, actually the changes in pr do not break auto insets..but block layout seems to be broken in a different way?

Parent has border: 15 and inset: 10

inset = auto

not sure where the block layout numbers comes from

Main

❌ block Point {
    x: 15.0,
    y: 40.0,
}
✅ flex Point {
    x: 25.0,
    y: 25.0,
}
✅ Grid Point {
    x: 15.0,
    y: 15.0,
}

Current

❌ Block Point {
    x: 15.0,
    y: 40.0,
}
✅ Flex Point {
    x: 25.0,
    y: 25.0,
}
✅ Grid Point {
    x: 15.0,
    y: 15.0,
}

top and left = 0

Current

✅ Block Point {
    x: 15.0,
    y: 15.0,
}
✅ Flex Point {
    x: 15.0,
    y: 15.0,
}
✅ Grid Point {
    x: 15.0,
    y: 15.0,
}

Main, top and left = 0

✅ Block Point {
    x: 15.0,
    y: 15.0,
}
✅ Flex Point {
    x: 15.0,
    y: 15.0,
}
✅ Grid Point {
    x: 15.0,
    y: 15.0,
}

top and left = 100%

Main

✅ Block Point {
    x: 85.0,
    y: 85.0,
}
❌ Flex Point {
    x: 115.0,
    y: 115.0,
} 
✅ Grid Point {
    x: 85.0,
    y: 85.0,
}

Current

✅ Block Point {
    x: 85.0,
    y: 85.0,
}
✅ Flex Point {
    x: 85.0,
    y: 85.0,
}
✅ Grid Point {
    x: 85.0,
    y: 85.0,
}

top and left with scrollbar_width = 10 and parent size += 10

Main

✅ Block Point {
    x: 85.0,
    y: 85.0,
}
❌ Flex Point {
    x: 125.0,
    y: 125.0,
} 
❌ Grid Point {
    x: 95.0,
    y: 95.0,
}

Current

✅ Block Point {
    x: 85.0,
    y: 85.0,
}
✅  Flex Point {
    x: 85.0,
    y: 85.0,
}
❌ Grid Point {
    x: 95.0,
    y: 95.0,
}

TLDR

[x] absolute children in flex layout should be relative to container size - border [ ] absolute children in grid layout doesn't account for scrollbar [ ] Block layout absolute children with auto insets returns unexpected values (not sure why)

julia-script commented 2 months ago

Ok, fixed those issues and added some tests (I didn't, but I guess I could remove the handwritten test I added since now it's a bit redundant?).

I'm less familiar with the ins and outs of the grid layout algorithm so I'm not 100% confident my change there is the correct way to approach it, even though tests are passing, so please, pay special attention to that ❤️