taffy icon indicating copy to clipboard operation
taffy copied to clipboard

scrollable_overflow implementation

Open wlott opened this issue 3 months ago • 2 comments

Objective

Replaces content_size: Size<f32> with scrollable_overflow: Rect<f32> and tries to match the definition of "scrollable overflow rectangle" in CSS Overflow 3, 2.2. Scrollable Overflow.

Fixes #871

  • [ ] Changes that will affect external library users must update RELEASES.md before they will be merged.

Context

See #871.

Feedback wanted

I added several helper/utility Rect methods to geometry.rs. I wasn't sure if they are of sufficient generality to be methods directly on Rect or whether they should be helper functions in some utility module. Also, if they are Rect methods, I wasn't sure if they should be pub or just pub(crate). I saw examples of both, so I couldn't just do the same as the others.

I made some changes to gentest/src/main.rs (which unfortunately means that every generated file shows up in this commit) but I don't think they should be controversial:

  • I pulled long string literals for leaf nodes out onto their own line to make rustfmt not throw up its hands in despair.
  • I changed the assertion logic to check every node then assert once instead of asserting at each comparison. This way you can see all the things you got wrong, not just the first.
  • And I (tentatively) turned on the scroll_width/scroll_height checks for all nodes instead of limiting them to scroll containers. Hopefully I'll be able to fix the last few fails and won't have to turn that back off.

The actual logic for the scrollable overflow computations follows a very similar structure to the content_size computations that I replaced. There were some subtleties that took some trial-and-error to figure out, like the fact that the scrollable overflow from inflow child elements is expanded by the container's padding but the scrollable overflow from out-of-flow elements is taken as is. But what it is doing is pretty straight forward. The most non-obvious bit I think is that LayoutOutput only contains some of the scrollable overflow and it is up to the caller of tree.perform_child_layout to fill in the rest before calling tree.set_unrounded_layout. The compute_*_layout functions just don't have enough information to compute it all in one place.

And finally, I'm sure this stuff needs more test cases to be fully exercised, but I just don't know enough CSS to figure out all that might entail. I know a million times more about CSS layout now than I did when I started this work, but, well, that's like saying 10^-6 is a million times more than 10^-12: both are still small numbers.

wlott avatar Oct 16 '25 06:10 wlott

Oh, something else I forgot to mention. By the time I got done with this work, the text "content_size" shows up pretty much only in #[cfg(feature = "content_size")] annotations. It really isn't the "content_size" feature any more. Couple of options:

  • We could add a content_size field back into Layout that tries to duplicate the prior semantics.
  • We could change all my scrollable overflow stuff to gate on a new feature, say scrollable_overflow.
  • We could do that but also put back all the content_size stuff so people could choose between the two feature sets.
  • scrollable_overflow feature with just the new stuff then a content_size feature that requires scrollable_overflow and just adds a content_size field that tries to duplicate prior semantics.

I guess it boils down to separate questions:

  • do we add a new scrollable_overflow feature?
  • do we remove the old content_size feature?
  • do we preserve the old content_size field in Layout (and possibly LayoutOutput)
  • if there is a content_size field, do we try to compute it from the scrollable overflow or do we preserve both sets of code under different feature gates?

My personal bias is towards renaming the feature and pretending content_size never existed. But I come out of a career of smallish game shops where there were no external clients for the code so reducing maintenance burden was much more valuable than backwards compatibility.

wlott avatar Oct 16 '25 06:10 wlott

Looking into the justify_content_row_max_width_and_margin test. Taffy's result seems to agree with Firefox but not Chrome here. Additionally, Chrome starts giving this results if overflow is set to hidden/scroll/auto.

nicoburns avatar Oct 20 '25 11:10 nicoburns