Support `Overflow::Hidden` by nicoburns · Pull Request #424 · DioxusLabs/taffy

nicoburns · 2023-04-10T14:28:48Z

Objective

A partial fix for Support the overflow property #304
I have also included some updates to the gentest infrastructure for overflow:scroll/overflow:auto. Notably support for a scrollbar-width property which measures the actual size of the scrollbars used in the browser that runs the gentests so that they can be accounted for in the layout. One thing to note is that the gentests will now fail to run if run in a browser using overlay scrollbars.. This is neccessary to ensure that our tests for layouts including scrollbars are actually testing things properly (because if the scrollbars take up no space then no special support is required).

Context

Style.overflow not hidding overflowed nodes bevyengine/bevy#8016
This is split out from WIP: Support overflow property #412. I intend to submit separate PRs for overflow: hidden, overflow: scroll, and overflow: auto as the changes required for each are quite separate.

Notes

There is one disabled failing test:

<div id="test-root" style="flex-direction: row;">
  <div style="flex-direction: column;flex-basis: 0px;overflow: hidden;">
    <div style="width: 100px; height: 50px;"></div>
  </div>
</div>

Chrome gives the root node a width of 100px. Which seems wrong to me both intuitively and from reading the spec, as the combination of overflow:hidden and flex-basis: 0px should clamp every except the inner node at 0px width. This is what Taffy currently does.

In any case, I feel like this is a pretty obscure case and that it therefore shouldn't block this PR.

Feedback wanted

General PR review.

Weibye

Overall this looks very good 👍

scripts/gentest/src/main.rs

test_fixtures/grid_fit_content_percent_indefinite_max_content_hidden.html

src/style/mod.rs

src/geometry.rs

src/compute/grid/track_sizing.rs

Weibye

Required change: HTML fixtures closing head tag should be fixed

TimJentzsch · 2023-04-10T17:35:40Z

Interesting behavior on the failing test.
Firefox also sizes at 100px width:

TimJentzsch

I can't say that I fully understand everything, but I didn't find anything looking wrong either

src/geometry.rs

src/style/mod.rs

nicoburns · 2023-04-10T18:34:05Z

Interesting behavior on the failing test. Firefox also sizes at 100px width.

Yeah, Chrome and Firefox seem to be in agreement on this one. I can't see how it can possibly be right though. I think I might report this one to Chrome/Firefox and see what their devs say.

Relevant part of the spec is here btw (https://www.w3.org/TR/css-flexbox-1/#intrinsic-item-contributions):

The main-size min-content contribution of a flex item is the larger of its outer min-content size and outer preferred size (its width/height as appropriate) if that is not auto, clamped by its flex base size as a maximum (if it is not growable) and/or as a minimum (if it is not shrinkable), and then further clamped by its min/max main size.

(emphasis mine). This element is not growable (as flex-grow default to 0) and it's flex-basis is trivially 0px, so as far as I can tell it's content contributions ought to be capped at 0px.

…_hidden_parent test

…rflow values

… with non-visible overflow

…rflow values

…h non-visible overflow

These tests were generated by temporarily setting overflow:hidden on every node across the entire generated test suite and then manually creating copies of the tests with overflow:hidden for those testse that failed.

…to tracks, even when sizing under a min- or max-content constraint

…croll container

… base size

TimJentzsch · 2023-04-10T19:01:57Z

I think I might report this one to Chrome/Firefox and see what their devs say.

Sounds like a good idea :)
I also don't see an apparent reason why they size it like that...

nicoburns · 2023-04-10T19:20:44Z

Chrome bug report: https://bugs.chromium.org/p/chromium/issues/detail?id=1431963
Firefox bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1827269

nicoburns · 2023-04-10T22:38:30Z

Chrome have merged my issue into https://bugs.chromium.org/p/chromium/issues/detail?id=240765 (which seems to be a WIP implementation of flexbox algorithm changes) with the comment "This passes with --enable-blink-features=NewFlexboxSizing". Which would suggest that the Taffy result is correct.

Would interesting to try the gentest suite with --enable-blink-features=NewFlexboxSizing!

nicoburns added enhancement New feature or request breaking-change A change that breaks our public interface labels Apr 10, 2023

nicoburns added this to the 0.4 "Make taffy easier to embed" milestone Apr 10, 2023

nicoburns requested review from Weibye and alice-i-cecile April 10, 2023 14:28

Weibye approved these changes Apr 10, 2023

View reviewed changes

Weibye self-requested a review April 10, 2023 15:02

Weibye suggested changes Apr 10, 2023

View reviewed changes

nicoburns force-pushed the feat/overflow-hidden branch from 25d096c to 809b496 Compare April 10, 2023 16:05

nicoburns requested a review from Weibye April 10, 2023 16:56

Weibye approved these changes Apr 10, 2023

View reviewed changes

nicoburns requested a review from TimJentzsch April 10, 2023 17:22

TimJentzsch approved these changes Apr 10, 2023

View reviewed changes

src/geometry.rs Show resolved Hide resolved

src/style/mod.rs Show resolved Hide resolved

nicoburns added 15 commits April 10, 2023 19:34

Impl Default for Point. Use derive for Rect and Size Default impls

f8297e8

Add overflow style property

749ac4f

Add scrollbar width property

67d6439

Add/fix support for overflow property to gentest script

308b930

Enable do_not_clamp_height_of_absolute_node_to_height_of_its_overflow…

0f643a4

…_hidden_parent test

Add tests for automatic minimum size of flex items with different ove…

04dbc9b

…rflow values

Implement setting automatic minimum size of flex items to 0 for nodes…

7d0e59a

… with non-visible overflow

Flexbox: only compute/store main axis resolved minimum size

601cac6

Derive Serialize/Deserialize for Point<T> when serde feature is enabled

8343441

Add tests for automatic minimum size of grid items with different ove…

a73eafa

…rflow values

Add get and set methods to Point

affabc6

Implement resolving automatic minimum size to zero for grid items wit…

3993323

…h non-visible overflow

Optimise style resolution when sizing grid items

8be594f

Ensure that scrollbars take up space when generating tests

f4a0d06

Add support for scrollbar_width property to gentest script

5915b0d

nicoburns added 8 commits April 10, 2023 19:37

Add tests for overflow:hidden

db03a2f

These tests were generated by temporarily setting overflow:hidden on every node across the entire generated test suite and then manually creating copies of the tests with overflow:hidden for those testse that failed.

Use 0 when distributing intrinsic minimums of scroll containers to au…

5d432f1

…to tracks, even when sizing under a min- or max-content constraint

Apply fit-content limit to min-content contributions when item is a s…

34244c4

…croll container

Always use indefinite main axis available space when determining flex…

84b6f43

… base size

Remove WIP overflow:scroll and overflow:auto support

b045d52

Add extra feature flags for flexbox-specific code

8c97772

Replace <head/> with </head> in test fixtures

ae857c5

Add note about allow(dead_code)

2011868

nicoburns force-pushed the feat/overflow-hidden branch from 2c590c1 to 2011868 Compare April 10, 2023 18:39

nicoburns merged commit b9fce82 into DioxusLabs:main Apr 10, 2023

nicoburns mentioned this pull request Apr 10, 2023

Support the overflow property #304

Closed

3 tasks

This was referenced Apr 11, 2023

WIP: Use --enable-blink-features=NewFlexboxSizing when running gentests #433

Closed

Support Overflow::Scroll #446

Merged

nicoburns mentioned this pull request Apr 27, 2023

WIP: Support scrolling (Overflow::Scroll) in bevy_ui bevyengine/bevy#8104

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `Overflow::Hidden`#424

Support `Overflow::Hidden`#424
nicoburns merged 23 commits intoDioxusLabs:mainfrom
nicoburns:feat/overflow-hidden

nicoburns commented Apr 10, 2023

Uh oh!

Weibye left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Weibye left a comment

Uh oh!

TimJentzsch commented Apr 10, 2023

Uh oh!

TimJentzsch left a comment

Uh oh!

Uh oh!

Uh oh!

nicoburns commented Apr 10, 2023

Uh oh!

TimJentzsch commented Apr 10, 2023

Uh oh!

nicoburns commented Apr 10, 2023

Uh oh!

nicoburns commented Apr 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

nicoburns commented Apr 10, 2023

Objective

Context

Notes

Feedback wanted

Uh oh!

Weibye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Weibye left a comment

Choose a reason for hiding this comment

Uh oh!

TimJentzsch commented Apr 10, 2023

Uh oh!

TimJentzsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicoburns commented Apr 10, 2023

Uh oh!

TimJentzsch commented Apr 10, 2023

Uh oh!

nicoburns commented Apr 10, 2023

Uh oh!

nicoburns commented Apr 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants