scrollable_overflow implementation #875

wlott · 2025-10-16T06:25:25Z

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 · 2025-10-16T06:48:41Z

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.

nicoburns

This is overall looking good. I've left some comments.

We should compare benchmark results against main. To run benchmarks: cargo bench -p taffy_benchmarks.

I haven't dug into the failing tests yet.

nicoburns · 2025-10-16T11:22:50Z

scripts/gentest/src/main.rs

+    let gen_check = |expr: TokenStream, expected: f32| {
+        let cmp = if use_rounding {
+            quote!(layout.#expr != #expected)
+        } else {
+            quote!((layout.#expr - #expected).abs() >= 0.1)
+        };
+        quote!(
+            if #cmp {
+                mismatches += 1;
+                eprintln!("{:?}.{} mismatch: expected {} actual {}", #ident, stringify!(#expr), #expected, layout.#expr);
+            }
+        )
+    };


This is very nice! At some point I am hoping to be able to print an actual diff between the expected and actual trees. But this is a much easier way of getting some of the benefit of that.

nicoburns · 2025-10-16T11:24:40Z

scripts/gentest/src/main.rs

                "hidden" => Some(quote!(taffy::style::Overflow::Hidden)),
                "scroll" => Some(quote!(taffy::style::Overflow::Scroll)),
                "auto" => Some(quote!(taffy::style::Overflow::Auto)),
+                "clip" => Some(quote!(taffy::style::Overflow::Clip)),


Minor nit: maybe we should still explicitly set this to visible even though it's the default?

I'm a fan of explicit.

nicoburns · 2025-10-16T11:28:51Z

scripts/gentest/src/main.rs

+    let string_lit = quote!(#trimmed_text_content);
+
+    // If the string literal is too long, rustfmt gives up on the
+    // whole line.  So pull long string literals out onto their own
+    // line so that the Style constructor still gets formatted.
+    if string_lit.to_string().len() > 100 {


Here you're quoting the string then converting it back to string to check the length. I think it would be better to do the the length check directly on trimmed_text_content

Alternatively you could just format it on a new line unconditionally? It probably doesn't matter too much if we also do this for short strings?

The quote->format->length bit is because rustfmt gives up when the source representation of the string literal is too long, not when the string contents are too long. There were strings like "HH\u{200b}HH\u{200b}HH..." that overflowed rustfmt's cutoff but still had a low character count.

Another option would be to use the prettyplease crate which appears to have been purpose-built to handle good-enough formatting for generated token streams.

I would prefer to avoid prettyplease if possible for test generator compile time reasons. Where I actually want to go with this is:

To have the test generator generate a JSON (or XML or some other common data format) representation of the test cases.

Have a single implementation of the test running code

Have each generated test fn simply call into this single function with the path/key of the data

I think that would greatly simplify the test generator and also improve compile times of the tests themselves which are currently generating a lot of duplicate code.

nicoburns · 2025-10-16T11:35:52Z

src/tree/layout.rs

-        #[cfg_attr(not(feature = "content_size"), allow(unused_variables))] content_size: Size<f32>,
+        #[cfg(feature = "content_size")] descendent_scrollable_overflow: Rect<f32>,


I'm a little wary about changing the public API based on a feature flag, as this can cause breakage for users if an unrelated crate enables the feature flag.

Ah. I had not realized that was a possibility. How about I make this field (and scrollable_overflow in Layout) always contain Rect::new_empty() or Rect::zero() when the feature isn't enabled?

That sounds good!

nicoburns · 2025-10-16T11:39:30Z

src/tree/layout.rs

+    /// of the scrollable overflow below the top padding edge.  The
+    /// scroll origin is currently always the padding box's top-left


Could we be explicit that "Taffy assumes that..."? I have found that using "is" can lead to confusion as whether a sentence is describing the implementation or the spec.

Good point.

nicoburns · 2025-10-16T11:50:01Z

src/compute/flexbox.rs

+    // so we do it here before adding the out-of-flow (absolute)
+    // children.
+    if constants.is_scroll_container {
+        scrollable_overflow = scrollable_overflow.inset_by(-constants.padding);


This should probably be a new outset_by rather than a negative inset.

I will also note that I believe for a fully correct implementation, we need extra accounting for in-flow position: relative nodes with inset properties set.

The scroll overflow contribution of these nodes needs to be counted twice:

The static position (ignoring the inset) + the containers padding (as you are currently doing)

The inset-modified position (without adding the containers padding similar to position: absolute nodes)

We don't currently support position: static, but you could use inset == 0 or similar as a proxy. We could also definitely land this PR without this bit, but mentioning for completeness.

I was worried that I had already added enough Rect methods so was hesitant to add another with a single character operator worked just as well. But I do agree that having it .outset_by would make the intent more clear. I'll assume you suggestion is licence to add yet another Rect method.

As for position: relative, this is one of those areas where my lack of familiarity with CSS makes it hard for me to intuit things like that. Adding the inset-modified overflow will require a bit of restructuring because I accumulate all of the in-flow contributions, expand that with the padding, then add the out-of-flow contributions. To have in-flow contributions with and without padding, we'd need to either:

scan the in-flow list twice, once before and once after adding the padding.

remove the pad all inflow at once code and have each inflow item add padding if desired. Commutivity of union makes them equivalent.

maintain two separate accumulators, one that gets padding added after and one that doesn't.

The third option seems to heavyweight to be desirable. Between the first two, it is the relative cost of a second scan vs the cost of extra math. I'm thinking the extra math is probably cheaper given that the extra math is just 4 comparisons per node while the extra scan would stress memory latency more.

But I'm also good with adding it as a follow-up after landing this first PR.

As for position: relative, this is one of those areas where my lack of familiarity with CSS makes it hard for me to intuit things like that

This is not intuitive at all! I also missed this at my first pass of this feature and I have almost 20 years of experience with CSS.

To be clear, I think the logic for position: relative nodes is that you need the union of:

The "static position" (position before it is modified by relative positioning) + padding

The "inset-modified position" (position after it is modified by relative positioning).

I also don't think we should be scanning the list twice, so either option 2 or 3 works.

(leaving it for a follow-up is still fine)

nicoburns · 2025-10-16T12:09:33Z

src/compute/flexbox.rs

+        // Now that we've picked a final size for the item, add the
+        // resultant padding box to the scrollable overflow.
+        let scrollable_overflow = layout_output
+            .descendent_scrollable_overflow
+            .union(Rect::from_origin_and_size(final_size).inset_by(border).shrunk_by(scrollbar_size));


FWIW, you actually can rely on the child knowing it's own final size during the "final layout" phase of layout. Containers will always pass it to children as LayoutInput::known_dimensions in this phase. It would be impossible for a child container to lay out it's own children without this.

(the setup you have with descendent_scrollable_overflow is probably still fine though)

I don't recall the details, but I originally started with including the node's own border box in the LayoutOutput::scrollable_overflow but that caused some test fixtures to fail and that I pivoted to this split responsibility setup (and renamed scrollable_overflow to descendent_scrollable_overflow to make the distinction clear) in order to get those tests to pass. I'm pretty sure I tracked it down to there being a difference the size in the LayoutOutput returned by perform_child_layout and the size passed to set_unrounded_layout.

Interesting. Well the way you have it is fine. So lets leave it like that if that's what works!

nicoburns · 2025-10-16T12:24:30Z

src/compute/common/content_size.rs

+    // From CSS Overflow 3, section 2.2:
+    //   The scrollable overflow area is the union of:
+    //     ...
+    //   - The border boxes of all boxes for which it is the
+    //     containing block and whose border boxes are positioned not
+    //     wholly in the unreachable scrollable overflow region,
+    //     accounting for transforms by projecting each box onto the
+    //     plane of the element that establishes its 3D rendering
+    //     context. [CSS3-TRANSFORMS]
+    if item_rect.right < container_scroll_origin.x || item_rect.bottom < container_scroll_origin.y {
+        return;
+    }


For our purposes I think we may want to skip this check. But perhaps only if overflow: visible. Certainly for hit testing we'd want something that was absolutely positioned in the negative overflow area such that it was visible on screen to be click/hoverable.

I'm pretty sure this test is needed from some of the fixtures to match chrome's results. But I agree, it does diminish the utility of scrollable overflow for hit testing. It is unfortunate that they stuck this wart in the spec just to accommodate legacy web pages that were abusing negative positions.

These tests fail with that conditional disabled but pass with it enabled:

generated::grid::grid_absolute_container_negative_position::grid_absolute_container_negative_position__border_box generated::grid::grid_absolute_container_negative_position::grid_absolute_container_negative_position__content_box generated::grid::grid_absolute_container_negative_position_margin::grid_absolute_container_negative_position_margin__border_box generated::grid::grid_absolute_container_negative_position_margin::grid_absolute_container_negative_position_margin__content_box

nicoburns · 2025-10-16T12:26:29Z

src/compute/common/content_size.rs

+    // From CSS Overflow 3, section 2.2:
+    //   The scrollable overflow area is the union of:
+    //     ...
+    //   - The margin areas of grid item and flex item boxes for
+    //     which the box establishes a containing block.
+
+    // TODO: open questions:
+    // - what does this mean?
+    // - is this predicated the same as the previous item?
+    // - assuming the margins are positive, don't this subsume the
+    //   previous item?


Are you missing margin-box vs border-box here? This seems to be saying that margins should extend the overflow scroll contribution for flex and grid items. Whereas they would be ignored for other kinds of box.

I am. Given that both grid and flex do strange and complicated things with item margins, I wasn't sure what to do with this item. At one time in development, this function had another parameter for extra padding and I was trying to address the inflow vs out-of-flow padding distinction by passing in the containers padding or zeros. I pivoted away from that and switched to the current setup of adding padding to inflow in the container instead of here for some reasons that I don't recall. Pivoting back would let flex/grid containers pass in the appropriate margins--and would also be useful in that position: relative double contribution detail you mentioned elsewhere.

It's the margins of the items (children) that are relevant here I think (which you should already have available I think).

src/geometry.rs

Added `Rect::outset_by` and changed `inset_by(-something)` to use it instead of `-`. Made `Layout` and `LayoutOutput` include the overflow fields even when the content_size feature isn't enabled. I'm not 100% happy with the `..default`s I added but it was either that or add lots of `#[cfg(not(feature="content_size"))]`. I went with the use of `..default` as the lesser of two evils.

nicoburns · 2025-10-20T11:27:30Z

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.

scrollable_overflow WIP snapshot

2633496

wlott mentioned this pull request Oct 16, 2025

content_size for internal nodes incorrectly includes the node's left/top border. #871

Open

nicoburns reviewed Oct 16, 2025

View reviewed changes

src/geometry.rs Outdated Show resolved Hide resolved

William Howe-Lott added 2 commits October 16, 2025 11:53

gentest: explicitly set overflow instead relying on defaults.

4216099

		#[cfg_attr(not(feature = "content_size"), allow(unused_variables))] content_size: Size<f32>,
		#[cfg(feature = "content_size")] descendent_scrollable_overflow: Rect<f32>,

		/// of the scrollable overflow below the top padding edge. The
		/// scroll origin is currently always the padding box's top-left

scrollable_overflow implementation #875

Are you sure you want to change the base?

scrollable_overflow implementation #875

Uh oh!

Conversation

wlott commented Oct 16, 2025

Objective

Context

Feedback wanted

Uh oh!

wlott commented Oct 16, 2025

Uh oh!

nicoburns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wlott Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoburns Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wlott Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoburns Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicoburns commented Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

wlott Oct 16, 2025 •

edited

Loading

nicoburns Oct 16, 2025 •

edited

Loading

wlott Oct 16, 2025 •

edited

Loading

nicoburns Oct 16, 2025 •

edited

Loading