Change behaviour of overflow to be more consistent

[emilsjolander]

Overflow behavior was not correct with the last diff to fix measuring behavior.

This PR changes overflow to be respected for the cross axis. This is not exactly how things work on the web but given our more limited API and focus on just the flex box portion of css I think it should be alright.

The behavior after this PR is that if a node is overflow: visible; its children will be able to overflow in the cross axis of the parent node. It does not make a lot of sense for children to overflow in the main axis as that would mean they would overlap their siblings. In the future we might implement overflow-x and overflow-y support for more control over how this behaves.

@rigdern would love to get your opinion on this.

[rigdern]

@emilsjolander I will try this out tomorrow.

I tried running the JavaScript tests and 7 of them failed.

[emilsjolander]

Those test failures are expected as we currently have overflow visible as
default and this changes the behavior of overflow. This change also had
some internal breakages but very few, most of them related to RN scrollview
which i'll put up a fix for.
On Tue, 12 Jul 2016 at 04:56, Adam Comella notifications@github.com wrote:

@emilsjolanderhttps://github.com/emilsjolander I will try this out
tomorrow.

I tried running the JavaScript tests and 7 of them failed.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABdIpEZnDQGwJnlhejijvhngfnOGEKEAks5qUxCFgaJpZM4JJO4Y
.

[emilsjolander]

This change gives us a consistent way to reason about overflow and custom
measure functions (something the web doesn't have). Overflow is currently
broken in master due to a fix for how flex basis auto is measured. I'm open
to other ways of solving this though.
On Tue, 12 Jul 2016 at 09:05, Emil Sjölander sjolander.emil@gmail.com
wrote:

Those test failures are expected as we currently have overflow visible as
default and this changes the behavior of overflow. This change also had
some internal breakages but very few, most of them related to RN scrollview
which i'll put up a fix for.
On Tue, 12 Jul 2016 at 04:56, Adam Comella notifications@github.com
wrote:

@emilsjolanderhttps://github.com/emilsjolander I will try this out
tomorrow.

I tried running the JavaScript tests and 7 of them failed.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABdIpEZnDQGwJnlhejijvhngfnOGEKEAks5qUxCFgaJpZM4JJO4Y
.

[rigdern]

@emilsjolander I tested this change in our app. Compared to d878091, several things broke:

• Some ScrollViews no longer scroll (maybe this is related to what you mentioned above).
• Some text nodes are no longer visible because their height and width are now 0.
• Some text nodes are vertically clipped (I'm not sure whether their height or their ancestor's height is now shorter)

I will investigate some more tomorrow.

I have some questions for you:

• You mentioned that you will put up a fix for the ScrollView breakage. When can I expect to see that?
• What's the relationship between this change and Correctly size cross axis when measuring flex basis #199?
• What's the motivation for making this change to how overflow works?
• You mentioned that this behaves differently than how it behaves on the web. If that's the case, how did you decide on the behavior to implement?
• Can you provide a small example layout that is fixed by this change?

[emilsjolander]

Never mind this. Closing this PR as the issues is more complex than this fix

[rigdern]

@emilsjolander If you can give me some details on what problem you are trying to solve, I can try to provide some advice. A small example of a layout you'd like to fix would also be very helpful. What's the relationship between this and #199?

[emilsjolander]

The relationship is that 199 broke overflow. I reverted it today as there
is no quick fix for now. I plan to look into this again in a bit. I've
talked to some people and it seems we are handling overflow / measure
functions differently than browsers. Layout should not change based on
overflow, overflow hidden only applies clipping on the web as far as I can
tell. Css-layout does not handle rendering so it should probably not care
about overflow at all.
On Wed, 13 Jul 2016 at 22:09, Adam Comella notifications@github.com wrote:

@emilsjolanderhttps://github.com/emilsjolander If you can give me some
details on what problem you are trying to solve, I can try to provide some
advice. A small example of a layout you'd like to fix would also be very
helpful. What's the relationship between this and #199
#199?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABdIpIUXZ8gpSFLJSafdmRLAtQi-GVwXks5qVVP8gaJpZM4JJO4Y
.

[rigdern]

@emilsjolander There are some cases where oveflow affects layout on the web. For example, take a look at https://jsfiddle.net/9cj1368n/1/. You'll notice that .child has a width of 200px. If you change overflow to hidden on .child, you'll see that its width shrinks to 100px.

I believe this behavior is due to section 4.5 (implied minimum size of flex items) of the flexbox spec. Let's use this section of the spec to interpret the results of the example I shared.

In the example, you'd expect .child to have a width of 100 because its flex is 1 and its parent's width is 100. However, when .child's overflow is visible, section 4.5 applies and its min size is based on its content. Its content has a width of 200 so .child gets a min size of 200. Consequently, .child's width is 200 instead of 100.

When .child's overflow is hidden, the rule from the spec doesn't apply and its min size is 0. Thus, .child's width is 100 as you'd expect.

This is how the web works but we didn't quite implement this behavior in css-layout. We decided against it because it added extra complexity to the code, slowed the code down, and we weren't seeing any benefit of implementing this behavior in practice.

Can you provide a small example of a layout that you are trying to fix?

[emilsjolander]

In your fiddle enabling overflow just clipped the grand child for me. I'll
take a look at the spec.

What i'm looking to fix is not overflow itself right now but actually how
custom nodes are measured. This behavior differs from the web where the
constraints passed to the node are treated more as suggestions instead of
constraints. Another thing is that when determining the flex basis we don't
correctly set up all the constraints on both axis which causes some nodes
to be measured with the wrong constraints (possibly due to the cache not
thinking it needs to be re-measured). I don't have a specific layout
example right now.

This is not super urgent right. Right now i'm focusing on restructuring the
project to make things easier to work with, once that is in a good place i
suspect fixing these sorts of things will be much easier.

Thanks for the help!

On Thu, 14 Jul 2016 at 00:06, Adam Comella notifications@github.com wrote:

@emilsjolanderhttps://github.com/emilsjolander There are some cases
where oveflow affects layout on the web. For example, take a look at
https://jsfiddle.net/9cj1368n/1/. You'll notice that .child has a width
of 200px. If you change overflow to hidden on .child, you'll see that its
width shrinks to 100px.

I believe this behavior is due to section 4.5 (implied minimum size of
flex items) https://www.w3.org/TR/css-flexbox-1/#min-size-auto of the
flexbox spec. Let's use this section of the spec to interpret the results
of the example I shared.

In the example, you'd expect .child to have a width of 100 because its
flex is 1 and its parent's width is 100. However, when .child's overflow
is visible, section 4.5 applies and its min size is based on its content.
Its content has a width of 200 so .child gets a min size of 200.
Consequently, .child's width is 200 instead of 100.

When .child's overflow is hidden, the rule from the spec doesn't apply
and its min size is 0. Thus, .child's width is 100 as you'd expect.

This is how the web works but we didn't quite implement this behavior in
css-layout. We decided against it because it added extra complexity to
the code, slowed the code down, and we weren't seeing any benefit of
implementing this behavior in practice.

Can you provide a small example of a layout that you are trying to fix?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABdIpK4Qx6y9n4I6XdY-Y3E_ddW2u6sOks5qVW-LgaJpZM4JJO4Y
.

[rigdern]

@emilsjolander The width of .child is displayed below the green+orange box. You'll see this value changes from 200 to 100 when switching overflow to hidden. Here's another fiddle which just puts a red border around .child to emphasize the width change: https://jsfiddle.net/9cj1368n/2/

[emilsjolander]

Hehe i believe we are running this is different browsers. I'm running it in
mobile safari and am not getting those results.
On Thu, 14 Jul 2016 at 00:26, Adam Comella notifications@github.com wrote:

@emilsjolanderhttps://github.com/emilsjolander The width of .child is
displayed below the green+orange box. You'll see this value changes from
200 to 100 when switching overflow to hidden. Here's another fiddle which
just puts a red border around .child to emphasize the width change:
https://jsfiddle.net/9cj1368n/2/

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABdIpPLQHNADiWP478xWtN7TLJ7voUuVks5qVXQ8gaJpZM4JJO4Y
.

[rigdern]

Oh, you are right! Safari on Mac also gives the results you are seeing. Chrome, Firefox, and Edge all behave the way I described.

[emilsjolander]

Well that explains the confusion. As I said i'll look into this once the
restructuring is in a good place. I don't think it is super urgent but it
should definitely be fixed.

The restructuring will cause some delay in things getting in but will speed
up after as the goal is for changes to be much faster to make after the
restructure.
On Thu, 14 Jul 2016 at 00:36, Adam Comella notifications@github.com wrote:

Oh, you are right! Safari on Mac also gives the results you are seeing.
Chrome, Firefox, and Edge all behave the way I described.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABdIpNEw6sO8O_x81WrnINtn69qx6Jjxks5qVXaLgaJpZM4JJO4Y
.

[rigdern]

That sounds good to me.