When paddingStart is 0, it should override paddingHorizontal #816

2018-09-24T19:00:23-07:00

rigdern commented

2018-09-24 19:00:23 -07:00

(Migrated from github.com)

Fixes #815

Problem

Imagine a node with this style: { paddingHorizontal: 10, paddingStart: 0 }.

After running layout on this node, we expect its computed paddingStart to be 0. However, it is actually 10.

Fix

Consider the expression paddingEdgeStart.getValue() > 0.0f in getLeadingPadding. Why is 0 handled like a negative number rather than a positive number? I suspect this should be >= so 0 is handled like the positive numbers (this is how getTrailingPadding works).

History

It looks like 3a82d2b1a8 (diff-07b4949bf42749fde386e769ff08a124) changed the operator from >= to > in getLeadingPadding. I suspect it was a mistake. getTrailingPadding still uses >=.

Testing

I manually verified this using the code in #815 and added some unit tests to catch this bug and other similar issues.

Adam Comella
Microsoft Corp.

Fixes #815 ### Problem Imagine a node with this style: `{ paddingHorizontal: 10, paddingStart: 0 }`. After running layout on this node, we expect its computed `paddingStart` to be `0`. However, it is actually `10`. ### Fix Consider the expression `paddingEdgeStart.getValue() > 0.0f` in [`getLeadingPadding`](https://github.com/facebook/yoga/blob/328ec7dc4d104b42b836d5ccebff04033d045133/yoga/YGNode.cpp#L461). Why is `0` handled like a negative number rather than a positive number? I suspect this should be `>=` so `0` is handled like the positive numbers (this is how `getTrailingPadding` works). ### History It looks like https://github.com/facebook/yoga/commit/3a82d2b1a8f5b65a2c3bd1407552d3ed2226238e?diff=unified&w=1#diff-07b4949bf42749fde386e769ff08a124 changed the operator from `>=` to `>` in `getLeadingPadding`. I suspect it was a mistake. `getTrailingPadding` still uses `>=`. ### Testing I manually verified this using the code in #815 and added some unit tests to catch this bug and other similar issues. Adam Comella Microsoft Corp.

woehrl01 commented

2018-09-24 21:21:50 -07:00

(Migrated from github.com)

Hi Adam, great finding! Not every test is generated from the web, I think YGComputedPaddingTest.cpp is a good fit for this case.

Hi Adam, great finding! Not every test is generated from the web, I think ```YGComputedPaddingTest.cpp``` is a good fit for this case.

rigdern commented

2018-09-25 09:38:26 -07:00

(Migrated from github.com)

@woehrl01 thanks for the suggestion. I added some unit tests to catch this bug and other similar issues.

facebook-github-bot (Migrated from github.com) reviewed 2018-10-09 16:48:48 -07:00

facebook-github-bot (Migrated from github.com) left a comment

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

shergin has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.internmc.facebook.com/D10282617).

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#816