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.

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

Fix issue where paddingStart and paddingEnd were swapped with row reverse #1426

Closed
wants to merge 5 commits into from

Conversation

joevilches
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50348995

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
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50348995

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
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
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50348995

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
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
Joe Vilches added 5 commits October 17, 2023 18:10
Summary:

X-link: facebook/react-native#41017

Before resolving facebook#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#1424)

Summary:
X-link: facebook/react-native#41018


See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
…se (facebook#1425)

Summary:
X-link: facebook/react-native#41022


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
Summary: this is fixed now so we can turn it on

Reviewed By: NickGerleman

Differential Revision: D50348206
…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 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 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
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50348995

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
@facebook-github-bot
Copy link
Contributor

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
Copy link
Contributor

This pull request has been merged in 7502595.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Oct 18, 2023
…erse (#41023)

Summary:
Pull Request resolved: #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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants