Fix issue where paddingStart and paddingEnd were swapped with row reverse #41023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D50348995 |
Base commit: 2018a82 |
|
This pull request was exported from Phabricator. Differential Revision: D50348995 |
joevilches
added a commit
to joevilches/react-native
that referenced
this pull request
Oct 17, 2023
…erse (facebook#41023) Summary: Pull Request resolved: facebook#41023 X-link: facebook/yoga#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995 fbshipit-source-id: be2b8b75fd7a7053f681275ccfd835640ab4a03f
joevilches
force-pushed
the
export-D50348995
branch
from
b195bae to
2b4f9d7
Compare
joevilches
added a commit
to joevilches/yoga
that referenced
this pull request
Oct 17, 2023
…erse (facebook#1426) Summary: X-link: facebook/react-native#41023 Pull Request resolved: facebook#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995 fbshipit-source-id: 0ee19df00403d678c235fe62968f7f8c196d7669
joevilches
added a commit
to joevilches/yoga
that referenced
this pull request
Oct 17, 2023
…erse (facebook#1426) Summary: X-link: facebook/react-native#41023 Pull Request resolved: facebook#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995 fbshipit-source-id: 8892db893927e4f72c198eb76d1d146f2064d293
|
This pull request was exported from Phabricator. Differential Revision: D50348995 |
joevilches
added a commit
to joevilches/react-native
that referenced
this pull request
Oct 17, 2023
…erse (facebook#41023) Summary: Pull Request resolved: facebook#41023 X-link: facebook/yoga#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995 fbshipit-source-id: 5ce9d1ef98e8cb02d7270cbd2c09a1bee8a44c55
joevilches
force-pushed
the
export-D50348995
branch
from
2b4f9d7 to
154c628
Compare
joevilches
added a commit
to joevilches/yoga
that referenced
this pull request
Oct 17, 2023
…erse (facebook#1426) Summary: X-link: facebook/react-native#41023 Pull Request resolved: facebook#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995 fbshipit-source-id: 50c7bd4e1567bbd86ff603640d600d93c7c76d4c
|
This pull request was exported from Phabricator. Differential Revision: D50348995 |
joevilches
force-pushed
the
export-D50348995
branch
from
154c628 to
38f1797
Compare
joevilches
added a commit
to joevilches/react-native
that referenced
this pull request
Oct 17, 2023
…erse (facebook#41023) Summary: Pull Request resolved: facebook#41023 X-link: facebook/yoga#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995 fbshipit-source-id: fdca6b10617d3e63ec007b672c0106015299c0fc
joevilches
pushed a commit
to joevilches/react-native
that referenced
this pull request
Oct 17, 2023
…erse (facebook#41023) Summary: X-link: facebook/yoga#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995
joevilches
pushed a commit
to joevilches/react-native
that referenced
this pull request
Oct 17, 2023
…erse (facebook#41023) Summary: X-link: facebook/yoga#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995
joevilches
pushed a commit
to joevilches/react-native
that referenced
this pull request
Oct 17, 2023
…erse (facebook#41023) Summary: X-link: facebook/yoga#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995
added 4 commits
October 17, 2023 18:10
Summary: X-link: facebook/yoga#1423 Before resolving facebook/yoga#1208 yoga was in a state where "leading" and "trailing" only referred to the main-start and main-end directions ([definition in spec](https://drafts.csswg.org/css-flexbox/#box-model)). That is, the start/end of the layout of flex items in a container. This is distinct from something like inline-start/inline-end which is the [start of text layout as defined by direction](https://drafts.csswg.org/css-writing-modes-3/#inline-start). The bug linked above happened because "leading" and "trailing" functions are referring to the wrong directions in certain cases. So in order to fix this we added a new set of functions to get the "leading" and "trailing" edges according to what inline-start/inline-end would refer to - i.e. those defined by the direction (ltr | rtl). In this state I think it is confusing to understand which function refers to which direction and more specific names could help that. This diff just renames the following 4 FlexDirection.h functions: * **leadingEdge** -> **flexStartEdge** * **trailingEdge** -> **flexEndEdge** * **leadingLayoutEdge** -> **inlineStartEdge** * **trailingLayoutEdge** -> **inlineEndEdge** The spec calls the start/end directions as dictated by the flex-direction attribute "main-start" and "main-end" respectively, but mainStartEdge might be a bit confusing given it will be compared to a non-flexbox-specific name in inlineStartEdge. As a result I landed on flexStart/flexEnd similar to what values are used with alignment attributes (justify-content, align-content). I chose to get rid of the "leading" and "trailing" descriptors to be more in line with what terminology the spec uses. Next diff will be to rename the functions in Node.cpp to adhere to the above patterns. Reviewed By: NickGerleman Differential Revision: D50342254
…ok#41018) Summary: X-link: facebook/yoga#1424 See D50344874 Reviewed By: NickGerleman Differential Revision: D50344874
…se (facebook#41022) Summary: X-link: facebook/yoga#1425 Just like D50140503 where marginStart and marginEnd were not working with row reverse, borderStart and borderEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of border (and also padding) there is a callsite that actually wants to get the flex item layout's leading/trailing border and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348085
…erse (facebook#41023) Summary: X-link: facebook/yoga#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995
joevilches
pushed a commit
to joevilches/yoga
that referenced
this pull request
Oct 18, 2023
…erse (facebook#1426) Summary: X-link: facebook/react-native#41023 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995
joevilches
force-pushed
the
export-D50348995
branch
from
38f1797 to
1ddc569
Compare
joevilches
pushed a commit
to joevilches/react-native
that referenced
this pull request
Oct 18, 2023
…erse (facebook#41023) Summary: X-link: facebook/yoga#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995
|
This pull request was exported from Phabricator. Differential Revision: D50348995 |
joevilches
pushed a commit
to joevilches/yoga
that referenced
this pull request
Oct 18, 2023
…erse (facebook#1426) Summary: X-link: facebook/react-native#41023 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995
joevilches
force-pushed
the
export-D50348995
branch
from
1ddc569 to
eecea5c
Compare
|
This pull request was exported from Phabricator. Differential Revision: D50348995 |
facebook-github-bot
pushed a commit
to facebook/litho
that referenced
this pull request
Oct 18, 2023
…erse Summary: X-link: facebook/react-native#41023 X-link: facebook/yoga#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995 fbshipit-source-id: 85717df23de7cf5f66b38d3ff28435b053a4e68e
facebook-github-bot
pushed a commit
to facebook/yoga
that referenced
this pull request
Oct 18, 2023
…erse (#1426) Summary: X-link: facebook/react-native#41023 Pull Request resolved: #1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995 fbshipit-source-id: 85717df23de7cf5f66b38d3ff28435b053a4e68e
|
This pull request has been merged in 2bf1a8f. |
Othinn
pushed a commit
to Othinn/react-native
that referenced
this pull request
Oct 30, 2023
…erse (facebook#41023) Summary: Pull Request resolved: facebook#41023 X-link: facebook/yoga#1426 Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set. One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out. Reviewed By: NickGerleman Differential Revision: D50348995 fbshipit-source-id: 85717df23de7cf5f66b38d3ff28435b053a4e68e
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Just like D50140503 where marginStart and marginEnd were not working with row reverse, paddingStart and paddingEnd are not working either with row reverse either. The solution is similar - we were checking the flex item layout starting/ending edges and not the general layout starting/ending edges. This change makes it so that we look at the proper edge according to what direction is set.
One caveat is that in the case of padding (and also border) there is a callsite that actually wants to get the flex item layout's leading/trailing padding and not the one dictated by direction. So, I made a new function to accommodate this and just swapped that callsite out.
Differential Revision: D50348995