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

Implement flexbox automatic min-size #12610

Open
shinglyu opened this issue Jul 27, 2016 · 15 comments
Open

Implement flexbox automatic min-size #12610

shinglyu opened this issue Jul 27, 2016 · 15 comments

Comments

@shinglyu
Copy link
Contributor

https://drafts.csswg.org/css-flexbox/#min-size-auto

As @stshine said in #12453 :

To do this you may need to change the type of min-{width, height} in position.mako.rs to LengthOrPercentageOrAuto. Then in flex.rs when the computed value of min_inline_size is Auto and overflow is visible, set the min_size to a implied size according to the rules. No need to deal with minimal block size yet.
This is not tightly relevant to flexbox, but to make it functional it still needs #12230 merged first.

@shinglyu
Copy link
Contributor Author

Gecko's reference implementation is in nsFlexContainerFrame::ResolveAutoFlexBasisAndMinSIze()

@shinglyu
Copy link
Contributor Author

shinglyu commented Jul 29, 2016

@stshine
Copy link
Contributor

stshine commented Jul 29, 2016

IMO for inline mode the intrinsic_inline_sizes.minimum_inline_size should have enough information.
For block mode things are much more troublesome since it needs the item to be layouted first using max-content width. I'll discuss that in a new issue.

@shinglyu
Copy link
Contributor Author

Here are my planned step to implement this (at least the inline mode):

  1. Change the type of the min_inline_size to LengthOrPercentageOrAuto
  2. Provide fallback (to 0) for other flow types that doesn't have Auto for min-{width,height}
  3. Resolve the Auto size when needed (probably in flex_resolve(), there is a item.main_size = ... line)

@shinglyu
Copy link
Contributor Author

Done 1 and 2. 3 is harder then I think, there are too many conditional branches in the spec, I'll start by implementing the branch that doesn't require the aspect ratio first.

@shinglyu
Copy link
Contributor Author

shinglyu commented Nov 7, 2016

Spec bug filed: w3c/csswg-drafts#671

@shinglyu
Copy link
Contributor Author

shinglyu commented Nov 7, 2016

I sort out the test cases as a giant truth table:

done  main size  aspect ratio? max size   cross size  min cross size  max cross size
      definite?                definite?  definite?   definite?       definite?

      # Has definite specified size
      T          T             T          X           T               T              => specified = min(main_size, max_size)
                                                                                        content = min-content, clamped by (cross min & max) * aspect, clamped by max size
                                                                                        auto min size = min(specified, content)
      T          T             T          X           T               F              => as above, except not clamped by max cross
      T          T             T          X           F               T              => as above, except not clamped by min cross
      T          T             T          X           F               F              => as above, except not clamped by any cross
      T          F             T          X           X               X              => as above, except not clamped by any cross (flexbox-min-width 001)

      T          T             F          X           T               T              => specified = main_size
                                                                                        content = min-content, clamped by (cross min & max) * aspect
                                                                                        auto min size = min(specified, content)
      T          T             F          X           T               F              => as above, except not clamped by max cross (flexbox-min-width 002b)
      T          T             F          X           F               T              => as above, except not clamped by min cross (flexbox-min-width 002c)
      T          T             F          X           F               F              => as above, except not clamped by any cross (flex-minimun-width 004, 005, 006) (flexbox-min-with 002a)
      T          F             F          X           X               X              => as above, except not clamped by any cross (flex-minimum-width 002, 003) (flexbox-min-width 001)

      # Has transferred size
      F          T             T          T           T               T              => content = min-content, clamped by (cross min & max) * aspect, clamped by max size
                                                                                        transferred = (cross size, clamped by cross min & max) * aspect ratio
                                                                                        auto min size = min(content, transferred)

      F          T             T          T           T               F              => as above, except not clamped by max cross
      F          T             T          T           F               T              => as above, except not clamped by min cross
      F          T             T          T           F               F              => as above, except not clamped by any cross

      F          T             F          T           T               T              => as above, except not clamped by max size
      F          T             F          T           T               F              => as above, except not clamped by max size & min cross (flexbox-min-width 002b)
      F          T             F          T           F               T              => as above, except not clamped by max size & max cross (flexbox-min-width 002c)
      F          T             F          T           F               F              => as above, except not clamped by max size & any cross (flex-minimum-width 007, 008) (flexbox-min-width 002a)

      # Neither specified nor transferred
V     F          F             T          X           X               X              => content = min-content, clamped by max size
                                                                                        auto min size = content
      F          T             T          F           X               X              => same as above
      F          T             F          F           X               X              => same as above, except not clamped by max size
V     F          F             F          X           X               X              => same as above, except not clamped by max size  (flex-mininum-width 001)

overflow-x is not visible => 0 (flexbox-min-width 003, 004)

@stshine
Copy link
Contributor

stshine commented Nov 8, 2016

@shinglyu Impressive sum up! That said, we do need some methods to handle the min-max size clamping more carefully, which currently layout does not pay much attention to(And that's the reason why I still left the AxisSize struct there).

On the other hand, IMHO most of the complexity of intrinsic size calculation involved with intrinsic aspect ratio goes to bubble_inline_size(); especially, in this issue the bubble_inline_sizes_for_block() method in block.rs has some bugs on handling intrinsic size calculation for replaced block. So I suggest you not to worry to much about the intrinsic ratio thing too much, as they should go into a seperate issue.

@shinglyu
Copy link
Contributor Author

shinglyu commented Nov 8, 2016

@stshine I noticed that if I specify width: 200px for an image, who's size is smaller then 200px, the intrisic_inline_size fields will all become 200px. Is that a bug? That makes me unable to get the "content size", or maybe the "content size" is something else.

@stshine
Copy link
Contributor

stshine commented Nov 8, 2016

@shinglyu I believe that is the desired behavior since 'content size' is what we use to avoid fall into zero length when the style does not specify the actual size. And that is why the content size of element with intrinsic ratio is so complex since is may be determined by size property in the other direction.

@stshine
Copy link
Contributor

stshine commented Nov 12, 2016

One thing I found worth to mention that may be confusing when I was reviewing flex.rs for column flexbox:
For a flex item, whether content size is needed is determined by the flex-basis property.
Or more concretely, in my current design, content size is needed when the from_flex_basis() returns MaybeAuto::Auto. And it may need enhancement to be able to handle intrinsic aspect ratio. I suggest you to take a closer look at it.

@shinglyu
Copy link
Contributor Author

@stshine I assume you are referencing the Gecko implementation, which skips content-size calculation if it's not needed? But I wants to follow the Chromium implementation, which is more like the spec. I write some pseudo logic here, would you mind giving me some feedback?

I'm also confused about the role of flex-basis in here, the gecko code says we need to use the used flex-basis, but I didn't see where it is defined in the spec.

I'm also not quite sure how we would get the min-content-size in Servo.

Based on your knowledge about Servo's status, which conditions in the algorithm is doable? Which is blocked by other issue? Seems like I need to land a patch that implement part of the conditions first, then follow up on other blockers.

@stshine
Copy link
Contributor

stshine commented Nov 14, 2016

Oops... sorry! I have not read flexbox spec quite a while, just thought about the main size; please ignore my previous two comments.

(stshine felt so embarrassed.)

So I took a closer look at the bubble_inline_sizes() methods and the compute_intrinsic_inline_sizes() methods, and what they compute are actually Intrinsic Size Contributions; that is, it first consult the specified size, and when it does not exist it will use the content size . Sadly this can not fulfill our requirements.

To fix this, note that since elements in flex container is blockified, the only possible child types is BlockFlow, TableFlow, MultiColFlow and FlexFlow. So one possible way is observe how their bubble_inline_sizes() works when their content_inline_sizes() in not available, and copy them in you method. But IMO eventually we need distinguish methods between Intrinsic Sizes and Intrinsic Size Contributions.

IMO your logic is correct, but the code can be much simplified.

Sorry for the confusion again.

@shinglyu shinglyu changed the title Implement Auto for implied min size for flex items Implement flexbox automatic min-size Nov 16, 2016
@stshine
Copy link
Contributor

stshine commented Dec 8, 2016

@shinglyu I created #14490 , which exposed the intrinsic aspect ratio info with has_intrinsic_ratio() method on Fragment, and you can use it to calculate the size of replaced elements in flexbox if you like (Maybe factor some code out and make it into a seperate method).
My next plan is to make a clear distinction between Intrinsic Size Contribution and Intrinsic Sizes, so we can get both info at the same time.

@shinglyu
Copy link
Contributor Author

I find it challenging to implement all the cases in one run. Any suggestions on how to splitting it into smaller chunks? I tried implement one of the condition at a time, but I keep regressing the reftests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants