fix: margin auto conversion in fabric #35635
Conversation
facebook-github-bot
added
the
CLA Signed
Base commit: 14307b8 |
Base commit: 14307b8 |
|
PR build artifact for 2a15f91 is ready. |
|
Thanks for this fix! cc @sammy-SC as well. This definitely seems like a change we should make. It changes the layout behavior of existing apps on Fabric, but in OSS everyone is still on paper, so it is still best to break current Fabric behavior for correctness and to preserve compatibility. There are some apps inside of Meta that have been exclusively on Fabric much longer, which introduces some risk of regression. For those, we will need a way to disable the change until we can run an experiment to find out if there is any broken product code relying on this. The way RN reads C++ feature flags is via https://github.com/facebook/react-native/blob/main/ReactCommon/react/config/ReactNativeConfig.h. The current API lets you query for a flag, defaulting to false if it is not found. Could we add a query for a flag |
|
@NickGerleman if auto margins have always been broken on Fabric, maybe we could also merge facebook/yoga#1014 and put that on the same flag |
|
For the more general case of fixing long-standing layout bugs, I'm hoping that we add the StrictLayout API next half. Though I think this case is particularly interesting because it is specific to Fabric, which means even for compat with existing code we should enable it by default in OSS. |
|
@NickGerleman @sammy-SC updated, let me know if this looks good! |
|
PR build artifact for 473179f is ready. |
| @@ -382,7 +383,8 @@ inline void fromRawValue( | |||
| } else if (value.hasType<std::string>()) { | |||
| const auto stringValue = (std::string)value; | |||
| if (stringValue == "auto") { | |||
| result = YGValueUndefined; | |||
| auto reactNativeConfig = context.contextContainer.at<std::shared_ptr<ReactNativeConfig const>>("ReactNativeConfig"); | |||
There was a problem hiding this comment.
The conversion code here is going to be a relatively hot path, so it might make sense to cache this (e.g. in a static variable), since I think the config values shouldn't change over app lifetime. Can make that change when importing if I get around to it before EOW, but wanted to give a quick heads up.
|
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@NickGerleman merged this pull request in 5a912cc. |
Summary:
Auto margin is not working as expected in fabric views. Below snippet results in different outputs in fabric and non-fabric runtime.
```jsx
<View style={{ backgroundColor: "black", width: "100%", height: "100%" }}>
<View style={{ marginLeft: "auto", width: 100, height: 100, backgroundColor: "pink" }} />
</View>
```
### Current Result | Expected Result
<div style="display: flex;">
<img width="100" alt="Screenshot 2022-12-14 at 11 00 22 AM" src="https://user-images.githubusercontent.com/23293248/207513912-633910e2-cf92-4f50-b839-c5abfa8041fb.png">
<img width="100" alt="Screenshot 2022-12-14 at 10 55 31 AM" src="https://user-images.githubusercontent.com/23293248/207513196-94650c60-ffe5-4475-9a95-2a59f8f0e9d9.png">
</div>
## Issue
This issue doesn't happen on non-fabric as it uses [YGNodeStyleSetMarginAuto](https://github.com/facebook/react-native/blob/9f9111bd7b75fb5f60b74b127b417512e29afb67/ReactCommon/yoga/yoga/Yoga.cpp#L749) to set the auto margins. In fabric, it uses the [convert function](https://github.com/intergalacticspacehighway/react-native/blob/2a15f9190266d90abbe3fc7e5437ea77e99e4290/ReactCommon/react/renderer/components/view/conversions.h#L375). The change in this PR can affect some other styles e.g. padding, borders. We could also create a different setter just for margins.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry.
Pick one each for the category and type tags:
For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[GENERAL] [FIXED] - margin auto style conversion in fabric
Pull Request resolved: facebook#35635
Test Plan:
- Test above snippet in fabric and non fabric runtime.
- I think we should have test cases for RN + layouts. Maybe we can use something like https://github.com/FormidableLabs/react-native-owl
Reviewed By: sammy-SC
Differential Revision: D42080908
Pulled By: NickGerleman
fbshipit-source-id: 6715c292fc40d82c425d79867099d8a6a3663dba
Summary
Auto margin is not working as expected in fabric views. Below snippet results in different outputs in fabric and non-fabric runtime.
Current Result | Expected Result
Issue
This issue doesn't happen on non-fabric as it uses YGNodeStyleSetMarginAuto to set the auto margins. In fabric, it uses the convert function. The change in this PR can affect some other styles e.g. padding, borders. We could also create a different setter just for margins.
Changelog
[GENERAL] [FIXED] - margin auto style conversion in fabric
Test Plan
Test above snippet in fabric and non fabric runtime.
I think we should have test cases for RN + layouts. Maybe we can use something like https://github.com/FormidableLabs/react-native-owl