Skip to content

Commit

Permalink
fix: margin auto conversion in fabric (#35635)
Browse files Browse the repository at this point in the history
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: #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
  • Loading branch information
intergalacticspacehighway authored and facebook-github-bot committed Jan 6, 2023
1 parent 44cb07b commit 5a912cc
Showing 1 changed file with 8 additions and 1 deletion.
9 changes: 8 additions & 1 deletion ReactCommon/react/renderer/components/view/conversions.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <folly/Conv.h>
#include <folly/dynamic.h>
#include <glog/logging.h>
#include <react/config/ReactNativeConfig.h>
#include <react/debug/react_native_assert.h>
#include <react/renderer/components/view/primitives.h>
#include <react/renderer/core/LayoutMetrics.h>
Expand Down Expand Up @@ -376,13 +377,19 @@ inline void fromRawValue(
const PropsParserContext &context,
const RawValue &value,
YGStyle::ValueRepr &result) {
// For bug compatibility, pass "auto" as YGValueUndefined
static bool treatAutoAsUndefined =
context.contextContainer
.at<std::shared_ptr<ReactNativeConfig const>>("ReactNativeConfig")
->getBool("react_fabric:treat_auto_as_undefined");

if (value.hasType<Float>()) {
result = yogaStyleValueFromFloat((Float)value);
return;
} else if (value.hasType<std::string>()) {
const auto stringValue = (std::string)value;
if (stringValue == "auto") {
result = YGValueUndefined;
result = treatAutoAsUndefined ? YGValueUndefined : YGValueAuto;
return;
} else {
if (stringValue.back() == '%') {
Expand Down

0 comments on commit 5a912cc

Please sign in to comment.