Absolute Insets should be resolved against the container size minus border

[julia-script]

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?

[nicoburns]

This looks like another great change - thank you and well spotted!

I have a couple of requests:

• Could you implement the same fix for (absolute children) in the Grid and Block algorithms? I'm betting they have the same problem.

• Can we turn the test(s) into generated tests (+ add tests for block and grid). This allows us to assert that our implementation matches Chrome rather than just that it matches whatever we've asserted in a manually written test. This installing chromedriver, adding HTML files to the test_fixtures directory (start by duplicating an existing one), and then running cargo gentest to turn the test fixture into a test (further instructions available in https://github.com/DioxusLabs/taffy/blob/main/CONTRIBUTING.md#testing).

If you have any issues then feel free to ask for help either here or in #taffy in the Dioxus discord.

[julia-script]

Hi 👋

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

[julia-script]

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.

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]

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 ❤️

[nicoburns]

Please can you remove the manual tests? I would rather not commit to maintaining those now we have generated tests covering the same use cases. Especially as we've had issues before with manual tests asserting incorrect layouts!

^ This is the only change I'm actually requiring here.

---

I've put a suggestion as a comment on the code. I'd prefer that change to be made, but would be willing to merge the PR without it.

Most of our generated tests are much smaller than the one you have created (where we have lots of things to test like this, we've separated them out into multiple tests). This can be quite helpful for tracking down failures. And part of me thinks we ought to split these new ones up too. However, seeing as they're currently passing I don't think there's much harm in leaving them as they are. If we need to debug parts of them later then we can split them then.

[nicoburns]

This is a bit of a "nit" (and I would accept this PR without this change), but I don't think the name "committed_offset" really makes sense for the x axis here. Because it is supposed to capture the idea that we iterate over the items and add to ("commit to") the y offset each time. Which we don't do for the x axis.

Or to think about it another way. Note that committed_offset.x is mutable here even though it is never actually mutated.

My recommendation is that line 357 is changed to be item.static_position = resolved_content_box_inset.left. And then I believe all the other "committed offset"-related changes in this file can be undone (so bonus: much smaller diff!)