Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Overflow::Hidden #424

Merged
merged 23 commits into from
Apr 10, 2023
Merged

Conversation

nicoburns
Copy link
Collaborator

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

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.

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

@Weibye Weibye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks very good 👍

scripts/gentest/src/main.rs Show resolved Hide resolved
src/style/mod.rs Show resolved Hide resolved
src/geometry.rs Show resolved Hide resolved
src/compute/grid/track_sizing.rs Show resolved Hide resolved
@Weibye Weibye self-requested a review April 10, 2023 15:02
Copy link
Collaborator

@Weibye Weibye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required change: HTML fixtures closing head tag should be fixed

@TimJentzsch
Copy link
Collaborator

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

Copy link
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/geometry.rs Show resolved Hide resolved
src/style/mod.rs Show resolved Hide resolved
@nicoburns
Copy link
Collaborator Author

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.

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
@TimJentzsch
Copy link
Collaborator

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
Copy link
Collaborator Author

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 nicoburns merged commit b9fce82 into DioxusLabs:main Apr 10, 2023
@nicoburns nicoburns mentioned this pull request Apr 10, 2023
3 tasks
@nicoburns
Copy link
Collaborator Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A change that breaks our public interface enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants