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.

Sign up for GitHub

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

Jump to bottom

Margin auto allows negative values #978

Closed
1 task done
jacobp100 opened this issue Mar 2, 2020 · 6 comments · May be fixed by #1014
Closed
1 task done

Margin auto allows negative values #978

jacobp100 opened this issue Mar 2, 2020 · 6 comments · May be fixed by #1014
Labels
Algorithm

Comments

@jacobp100
Copy link

Report

Issues and Steps to Reproduce

Create a setup like follows

<View style={{ height: 500 }}>
  <View style={{ flex: 0, height: 300 }} />
  <View style={{ flex: 0, marginTop: 'auto', height: 300 }} />
</View>

Or use playground link

Expected Behavior

The bottom view should overflow the parent (as happens on the web)

Actual Behavior

The bottom view gets a negative top margin applied, and overlaps the top view. Nothing overflows the parent

Link to Code

https://yogalayout.com/playground?eyJ3aWR0aCI6NTAwLCJoZWlnaHQiOjUwMCwibWluV2lkdGgiOm51bGwsIm1pbkhlaWdodCI6bnVsbCwibWF4V2lkdGgiOm51bGwsIm1heEhlaWdodCI6bnVsbCwiZmxleERpcmVjdGlvbiI6MCwicG9zaXRpb24iOnsidG9wIjpudWxsLCJyaWdodCI6bnVsbCwiYm90dG9tIjpudWxsLCJsZWZ0IjpudWxsfSwiY2hpbGRyZW4iOlt7IndpZHRoIjoiIiwiaGVpZ2h0IjoiMzAwIiwibWluV2lkdGgiOm51bGwsIm1pbkhlaWdodCI6bnVsbCwibWF4V2lkdGgiOm51bGwsIm1heEhlaWdodCI6bnVsbCwicG9zaXRpb24iOnsidG9wIjpudWxsLCJyaWdodCI6bnVsbCwiYm90dG9tIjpudWxsLCJsZWZ0IjpudWxsfSwiZmxleFNocmluayI6IjAifSx7IndpZHRoIjoiIiwiaGVpZ2h0IjoiMzAwIiwibWluV2lkdGgiOm51bGwsIm1pbkhlaWdodCI6bnVsbCwibWF4V2lkdGgiOm51bGwsIm1heEhlaWdodCI6bnVsbCwibWFyZ2luIjp7InRvcCI6ImF1dG8ifSwicG9zaXRpb24iOnsidG9wIjpudWxsLCJyaWdodCI6bnVsbCwiYm90dG9tIjpudWxsLCJsZWZ0IjpudWxsfSwiZmxleFNocmluayI6IjAifV19

Notes

If you remove marginTop: auto you get the correct behaviour

I did find these few lines

I'm not sure what the logic of remainingFreeSpace is - if it should allow negative values or not. If it does allow them, it might just need a max(0, ...)
@woehrl01 woehrl01 mentioned this issue Jun 9, 2020
Open
@woehrl01
Copy link
Contributor

woehrl01 commented Jun 9, 2020

Hi @jacobp100

Thank you for reporting this. I just pushed a PR for that bug fix.

@jacobp100
Copy link
Author

Amazing. Thanks so much!

@jacobp100
Copy link
Author

@NickGerleman is this something you'd be willing to accept a PR for Yoga 3? Or is it unlikely to get accepted if it'll break things at Meta

https://www.yogalayout.dev/playground?code=DwGQhgng9grgLgAgMZQHYDMCWBzAvAb3xgGcBTAdVICMARU9MGAGzmIC4EGmyBfHgPgBQCBMAByUACakExOBCakC+AO6ZJcABYcAzAFYADABoEm0jk1xdhkwAcwkyZlTYOAJgN8hIkeKky5BSVCNQ1tBB0DY1NzbEsOABYokwBbMAAnbGcAISg4OCgUjgByRgLivgQAem8fP2lZeUVlUK1dZJiLKwQARijKmuFRKolpIWAq8Gh4fiA

@NickGerleman
Copy link
Contributor

I’d be willing to give it another go. I’ve been leaning a bit on merging things where existing coverage doesn’t show lots of product changes, and this one might not be too bad.

@jacobp100
Copy link
Author

I think it's literally just these two lines

https://github.com/facebook/yoga/blob/main/yoga/algorithm/CalculateLayout.cpp#L1046
https://github.com/facebook/yoga/blob/main/yoga/algorithm/CalculateLayout.cpp#L1062

Need a max(flexLine.layout.remainingFreeSpace, 0)

@NickGerleman NickGerleman mentioned this issue Apr 13, 2024
Closed
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Apr 13, 2024
@NickGerleman @facebook-github-bot
Fixup margin: auto and justification behavior for overflowed containers
5baf25d 
Summary:
Fixes facebook/yoga#978

1. Don't allow auto margin spaces to become a negative length
2. Replicate fallback alignment behavior specified by box-alignment spec that we are using for align-content.

Differential Revision: D56091577
NickGerleman added a commit to NickGerleman/yoga that referenced this issue Apr 13, 2024
@NickGerleman @facebook-github-bot
Fixup margin: auto and justification behavior for overflowed containers
4d9db51 
Summary:
Fixes facebook#978

1. Don't allow auto margin spaces to become a negative length
2. Replicate fallback alignment behavior specified by box-alignment spec that we are using for align-content.

Differential Revision: D56091577
@NickGerleman NickGerleman mentioned this issue Apr 13, 2024
Closed
NickGerleman added a commit to NickGerleman/yoga that referenced this issue Apr 17, 2024
@NickGerleman @facebook-github-bot
Fixup margin: auto and justification behavior for overflowed containe…
1fb58e2 
…rs (facebook#1646)

Summary:

X-link: facebook/react-native#44069

Fixes facebook#978

1. Don't allow auto margin spaces to become a negative length
2. Replicate fallback alignment behavior specified by box-alignment spec that we are using for align-content.

Reviewed By: joevilches

Differential Revision: D56091577
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Apr 17, 2024
@NickGerleman @facebook-github-bot
Fixup margin: auto and justification behavior for overflowed containe…
30bf453 
…rs (facebook#44069)

Summary:
X-link: facebook/yoga#1646


Fixes facebook/yoga#978

1. Don't allow auto margin spaces to become a negative length
2. Replicate fallback alignment behavior specified by box-alignment spec that we are using for align-content.

Reviewed By: joevilches

Differential Revision: D56091577
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Apr 18, 2024
@NickGerleman @facebook-github-bot
Fixup margin: auto and justification behavior for overflowed containe…
f21d9af 
…rs (#44069)

Summary:
X-link: facebook/yoga#1646

Pull Request resolved: #44069

Fixes facebook/yoga#978

1. Don't allow auto margin spaces to become a negative length
2. Replicate fallback alignment behavior specified by box-alignment spec that we are using for align-content.

Reviewed By: joevilches

Differential Revision: D56091577

fbshipit-source-id: 3c02f81f969bb947cdc5c80b15faaa0b0d39c0c2
facebook-github-bot pushed a commit that referenced this issue Apr 18, 2024
@NickGerleman @facebook-github-bot
Fixup margin: auto and justification behavior for overflowed containe…
932361c 
…rs (#1646)

Summary:
Pull Request resolved: #1646

X-link: facebook/react-native#44069

Fixes #978

1. Don't allow auto margin spaces to become a negative length
2. Replicate fallback alignment behavior specified by box-alignment spec that we are using for align-content.

Reviewed By: joevilches

Differential Revision: D56091577

fbshipit-source-id: 3c02f81f969bb947cdc5c80b15faaa0b0d39c0c2
@NickGerleman
Copy link
Contributor

I think the change will stick, but fixing justify-content overflow behavior in particular did subtlety break a couple real-world UIs at Meta. Namely, there were cases where main axis would overflow, and justification would happen to remove space between items to make it not noticeable. Should be rare though I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Algorithm
Projects
None yet
Milestone
No milestone
3 participants
@NickGerleman @woehrl01 @jacobp100