DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

Fix aspect ratio when stretching with main axis margin #834

Closed
emilsjolander wants to merge 2 commits from aspect-ratio-with-margin into master
pull from: aspect-ratio-with-margin
merge into: DaddyFrosty:master
Conversation 8 Commits 2 Files Changed 2 +31
emilsjolander commented 2018-11-24 06:52:50 -08:00 (Migrated from github.com)
Copy Link

I've noticed that when a child's size is determined by align-items: stretch in combination with aspect-ratio its size is wrongly calculated to account for margin in the main axis when there is more than enough space.

See playground: https://goo.gl/tgW6cD

I've yet to figure out exactly how to solve this but i've started by writing a failing test when can be seen in the first commit here.

I assumed I had found the bug here https://github.com/facebook/yoga/blob/master/yoga/Yoga.cpp#L1838 where margin is being subtracted from the desired width even though the measure mode tells it to measure to exactly that size. However, if we don't remove this margin from the available width then 15 tests fail (including the one I just added) not quite figured out why yet. I'm also a bit confused at to why this would only happen for nodes with aspect-ratio and not for nodes where an explicit height and width is set.

I've noticed that when a child's size is determined by `align-items: stretch` in combination with `aspect-ratio` its size is wrongly calculated to account for margin in the main axis when there is more than enough space. See playground: https://goo.gl/tgW6cD I've yet to figure out exactly how to solve this but i've started by writing a failing test when can be seen in the first commit here. I assumed I had found the bug here https://github.com/facebook/yoga/blob/master/yoga/Yoga.cpp#L1838 where margin is being subtracted from the desired width even though the measure mode tells it to measure to exactly that size. However, if we don't remove this margin from the available width then 15 tests fail (including the one I just added) not quite figured out why yet. I'm also a bit confused at to why this would only happen for nodes with `aspect-ratio` and not for nodes where an explicit height and width is set.
facebook-github-bot commented 2018-11-24 06:53:09 -08:00 (Migrated from github.com)
Copy Link

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired. Before we can review or merge your code, **we need you to email [cla@fb.com](mailto:cla@fb.com?subject=CLA%20for%20facebook%2Fyoga%20%23834)** with your details so we can update your status.
emilsjolander commented 2018-11-24 07:55:59 -08:00 (Migrated from github.com)
Copy Link

Ok the test I added is passing now.I noticed that this is not just an issue with stretch as seen here: https://goo.gl/Dqbpji

However I was not able to create a failing test for this, so my guess is that this was probably fixed previously and the yoga version used in the layout editor just hasn't been updated yet. Would like to confirm this though.

Ok the test I added is passing now.I noticed that this is not just an issue with `stretch` as seen here: https://goo.gl/Dqbpji However I was not able to create a failing test for this, so my guess is that this was probably fixed previously and the yoga version used in the layout editor just hasn't been updated yet. Would like to confirm this though.
priteshrnandgaonkar commented 2018-11-26 03:35:55 -08:00 (Migrated from github.com)
Copy Link

The yoga used in playground has not been updated since a long time. There are few bugs which were fixed and still its reproducible on playground.

The yoga used in playground has not been updated since a long time. There are few bugs which were fixed and still its reproducible on playground.
👍 1
emilsjolander commented 2018-11-26 03:54:26 -08:00 (Migrated from github.com)
Copy Link

@priteshrnandgaonkar yeah that was my guess

@priteshrnandgaonkar yeah that was my guess
facebook-github-bot commented 2018-11-27 13:48:43 -08:00 (Migrated from github.com)
Copy Link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
facebook-github-bot (Migrated from github.com) reviewed 2018-11-27 14:22:43 -08:00
facebook-github-bot (Migrated from github.com) left a comment
Copy Link

@davidaurelio has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@davidaurelio has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.internmc.facebook.com/D13223579).
astreet commented 2018-12-07 09:40:13 -08:00 (Migrated from github.com)
Copy Link

Nice catch! This fix doesn't seem right though (being constrained to just stretch) -- are you still working on a better fix or do you think this is all that's needed?

Nice catch! This fix doesn't seem right though (being constrained to just stretch) -- are you still working on a better fix or do you think this is all that's needed?
emilsjolander commented 2018-12-09 15:39:48 -08:00 (Migrated from github.com)
Copy Link

@astreet I think this is all that's needed. The fix is specifically for stretch as that is the case where it needs to be set as an exact measurement. Not sure what else would need to be covered by this.

@astreet I think this is all that's needed. The fix is specifically for stretch as that is the case where it needs to be set as an exact measurement. Not sure what else would need to be covered by this.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
CLA Signed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#834
No description provided.
Delete Branch "aspect-ratio-with-margin"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?