Skip to content

Commit

Permalink
Remove row-reverse errata (#42251)
Browse files Browse the repository at this point in the history
Summary:
X-link: facebook/yoga#1547

Pull Request resolved: #42251

Yoga has an odd behavior, where `start`/`end` edges under row-reverse are relative to flex-direction, instead of writing direction.

While Yoga doesn't actually document what this behavior is supposed to be, it goes against CK documentation, historic RN documentation, and the behavior valid on the web. It is also applied inconsistently (e.g. sometimes only on container, sometimes on child). It really is a bug, instead of an intended behavior.

We changed the default behavior for Yoga, but left the existing one behind an errata (so existing fbsource users got old behavior). We have previously seen this behavior show up in product code, including CK when running on FlexLayout.

`row-reverse` is surprisingly uncommon though:
1. Litho has <40 usages
2. RN has ~40 usages in `RKJSModules`,~30 in `arvr/js`, ~6 in `xplat/archon`
3. CK has ~80 usages
4. NT has ~40 usages

There are few enough, mostly simple components, that we can inspect through each of them, looking for signs they will hit the issue (at the potential chance of missing some).

CK accounts for 10/14 usages that I could tell would trigger the issue, since it only exposes start/end edge, and not left/right. It might make sense to make it preserve behavior instead, to reduce risk a bit.

FlexLayout is now separately powering Bloks, which wasn't surveyed, so I didn't touch CK behavior under Bloks.

There could also be other usages in other frameworks/bespoke usages, and this has implications for OSS users. But based on our own usage, of many, many components, this seems rare.

Changelog:
[General][Breaking] - Make `start/end` in styles always refer to writing direction

Reviewed By: pentiumao, joevilches

Differential Revision: D52698130

fbshipit-source-id: 2a9ac47e177469f30dc988d916b6c0ad95d53461
  • Loading branch information
NickGerleman authored and facebook-github-bot committed Jan 12, 2024
1 parent 07a676a commit e859f6c
Show file tree
Hide file tree
Showing 7 changed files with 48 additions and 86 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@
public enum YogaErrata {
NONE(0),
STRETCH_FLEX_BASIS(1),
STARTING_ENDING_EDGE_FROM_FLEX_DIRECTION(2),
POSITION_STATIC_BEHAVES_LIKE_RELATIVE(4),
ABSOLUTE_POSITIONING(8),
POSITION_STATIC_BEHAVES_LIKE_RELATIVE(2),
ABSOLUTE_POSITIONING(4),
ALL(2147483647),
CLASSIC(2147483646);

Expand All @@ -32,9 +31,8 @@ public static YogaErrata fromInt(int value) {
switch (value) {
case 0: return NONE;
case 1: return STRETCH_FLEX_BASIS;
case 2: return STARTING_ENDING_EDGE_FROM_FLEX_DIRECTION;
case 4: return POSITION_STATIC_BEHAVES_LIKE_RELATIVE;
case 8: return ABSOLUTE_POSITIONING;
case 2: return POSITION_STATIC_BEHAVES_LIKE_RELATIVE;
case 4: return ABSOLUTE_POSITIONING;
case 2147483647: return ALL;
case 2147483646: return CLASSIC;
default: throw new IllegalArgumentException("Unknown enum value: " + value);
Expand Down
2 changes: 0 additions & 2 deletions packages/react-native/ReactCommon/yoga/yoga/YGEnums.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,6 @@ const char* YGErrataToString(const YGErrata value) {
return "none";
case YGErrataStretchFlexBasis:
return "stretch-flex-basis";
case YGErrataStartingEndingEdgeFromFlexDirection:
return "starting-ending-edge-from-flex-direction";
case YGErrataPositionStaticBehavesLikeRelative:
return "position-static-behaves-like-relative";
case YGErrataAbsolutePositioning:
Expand Down
5 changes: 2 additions & 3 deletions packages/react-native/ReactCommon/yoga/yoga/YGEnums.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,8 @@ YG_ENUM_DECL(
YGErrata,
YGErrataNone = 0,
YGErrataStretchFlexBasis = 1,
YGErrataStartingEndingEdgeFromFlexDirection = 2,
YGErrataPositionStaticBehavesLikeRelative = 4,
YGErrataAbsolutePositioning = 8,
YGErrataPositionStaticBehavesLikeRelative = 2,
YGErrataAbsolutePositioning = 4,
YGErrataAll = 2147483647,
YGErrataClassic = 2147483646)
YG_DEFINE_ENUM_FLAG_OPERATORS(YGErrata)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -920,13 +920,9 @@ static void justifyMainAxis(
const auto& style = node->getStyle();

const float leadingPaddingAndBorderMain =
node->hasErrata(Errata::StartingEndingEdgeFromFlexDirection)
? node->getInlineStartPaddingAndBorder(mainAxis, direction, ownerWidth)
: node->getFlexStartPaddingAndBorder(mainAxis, direction, ownerWidth);
node->getFlexStartPaddingAndBorder(mainAxis, direction, ownerWidth);
const float trailingPaddingAndBorderMain =
node->hasErrata(Errata::StartingEndingEdgeFromFlexDirection)
? node->getInlineEndPaddingAndBorder(mainAxis, direction, ownerWidth)
: node->getFlexEndPaddingAndBorder(mainAxis, direction, ownerWidth);
node->getFlexEndPaddingAndBorder(mainAxis, direction, ownerWidth);

const float gap = node->getGapForAxis(mainAxis);
// If we are using "at most" rules in the main axis, make sure that
Expand Down
1 change: 0 additions & 1 deletion packages/react-native/ReactCommon/yoga/yoga/enums/Errata.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ namespace facebook::yoga {
enum class Errata : uint32_t {
None = YGErrataNone,
StretchFlexBasis = YGErrataStretchFlexBasis,
StartingEndingEdgeFromFlexDirection = YGErrataStartingEndingEdgeFromFlexDirection,
PositionStaticBehavesLikeRelative = YGErrataPositionStaticBehavesLikeRelative,
AbsolutePositioning = YGErrataAbsolutePositioning,
All = YGErrataAll,
Expand Down
92 changes: 34 additions & 58 deletions packages/react-native/ReactCommon/yoga/yoga/node/Node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,52 +81,41 @@ Style::Length Node::computeEdgeValueForColumn(Edge edge) const {
}
}

Edge Node::getInlineStartEdgeUsingErrata(
FlexDirection flexDirection,
Direction direction) const {
return hasErrata(Errata::StartingEndingEdgeFromFlexDirection)
? flexStartEdge(flexDirection)
: inlineStartEdge(flexDirection, direction);
Edge Node::getInlineStartEdge(FlexDirection flexDirection, Direction direction)
const {
return inlineStartEdge(flexDirection, direction);
}

Edge Node::getInlineEndEdgeUsingErrata(
FlexDirection flexDirection,
Direction direction) const {
return hasErrata(Errata::StartingEndingEdgeFromFlexDirection)
? flexEndEdge(flexDirection)
: inlineEndEdge(flexDirection, direction);
Edge Node::getInlineEndEdge(FlexDirection flexDirection, Direction direction)
const {
return inlineEndEdge(flexDirection, direction);
}

Edge Node::getFlexStartRelativeEdgeUsingErrata(
Edge Node::getFlexStartRelativeEdge(
FlexDirection flexDirection,
Direction direction) const {
return hasErrata(Errata::StartingEndingEdgeFromFlexDirection)
? Edge::Start
: flexStartRelativeEdge(flexDirection, direction);
return flexStartRelativeEdge(flexDirection, direction);
}

Edge Node::getFlexEndRelativeEdgeUsingErrata(
Edge Node::getFlexEndRelativeEdge(
FlexDirection flexDirection,
Direction direction) const {
return hasErrata(Errata::StartingEndingEdgeFromFlexDirection)
? Edge::End
: flexEndRelativeEdge(flexDirection, direction);
return flexEndRelativeEdge(flexDirection, direction);
}

bool Node::isFlexStartPositionDefined(FlexDirection axis, Direction direction)
const {
auto leadingPosition = isRow(axis)
? computeEdgeValueForRow<&Style::position>(
getFlexStartRelativeEdgeUsingErrata(axis, direction),
flexStartEdge(axis))
getFlexStartRelativeEdge(axis, direction), flexStartEdge(axis))
: computeEdgeValueForColumn<&Style::position>(flexStartEdge(axis));

return leadingPosition.isDefined();
}

bool Node::isInlineStartPositionDefined(FlexDirection axis, Direction direction)
const {
Edge startEdge = getInlineStartEdgeUsingErrata(axis, direction);
Edge startEdge = getInlineStartEdge(axis, direction);
Style::Length leadingPosition = isRow(axis)
? computeEdgeValueForRow<&Style::position>(Edge::Start, startEdge)
: computeEdgeValueForColumn<&Style::position>(startEdge);
Expand All @@ -138,16 +127,15 @@ bool Node::isFlexEndPositionDefined(FlexDirection axis, Direction direction)
const {
auto trailingPosition = isRow(axis)
? computeEdgeValueForRow<&Style::position>(
getFlexEndRelativeEdgeUsingErrata(axis, direction),
flexEndEdge(axis))
getFlexEndRelativeEdge(axis, direction), flexEndEdge(axis))
: computeEdgeValueForColumn<&Style::position>(flexEndEdge(axis));

return !trailingPosition.isUndefined();
}

bool Node::isInlineEndPositionDefined(FlexDirection axis, Direction direction)
const {
Edge endEdge = getInlineEndEdgeUsingErrata(axis, direction);
Edge endEdge = getInlineEndEdge(axis, direction);
Style::Length trailingPosition = isRow(axis)
? computeEdgeValueForRow<&Style::position>(Edge::End, endEdge)
: computeEdgeValueForColumn<&Style::position>(endEdge);
Expand All @@ -161,8 +149,7 @@ float Node::getFlexStartPosition(
float axisSize) const {
auto leadingPosition = isRow(axis)
? computeEdgeValueForRow<&Style::position>(
getFlexStartRelativeEdgeUsingErrata(axis, direction),
flexStartEdge(axis))
getFlexStartRelativeEdge(axis, direction), flexStartEdge(axis))
: computeEdgeValueForColumn<&Style::position>(flexStartEdge(axis));

return leadingPosition.resolve(axisSize).unwrapOrDefault(0.0f);
Expand All @@ -172,7 +159,7 @@ float Node::getInlineStartPosition(
FlexDirection axis,
Direction direction,
float axisSize) const {
Edge startEdge = getInlineStartEdgeUsingErrata(axis, direction);
Edge startEdge = getInlineStartEdge(axis, direction);
Style::Length leadingPosition = isRow(axis)
? computeEdgeValueForRow<&Style::position>(Edge::Start, startEdge)
: computeEdgeValueForColumn<&Style::position>(startEdge);
Expand All @@ -186,8 +173,7 @@ float Node::getFlexEndPosition(
float axisSize) const {
auto trailingPosition = isRow(axis)
? computeEdgeValueForRow<&Style::position>(
getFlexEndRelativeEdgeUsingErrata(axis, direction),
flexEndEdge(axis))
getFlexEndRelativeEdge(axis, direction), flexEndEdge(axis))
: computeEdgeValueForColumn<&Style::position>(flexEndEdge(axis));

return trailingPosition.resolve(axisSize).unwrapOrDefault(0.0f);
Expand All @@ -197,7 +183,7 @@ float Node::getInlineEndPosition(
FlexDirection axis,
Direction direction,
float axisSize) const {
Edge endEdge = getInlineEndEdgeUsingErrata(axis, direction);
Edge endEdge = getInlineEndEdge(axis, direction);
Style::Length trailingPosition = isRow(axis)
? computeEdgeValueForRow<&Style::position>(Edge::End, endEdge)
: computeEdgeValueForColumn<&Style::position>(endEdge);
Expand All @@ -211,8 +197,7 @@ float Node::getFlexStartMargin(
float widthSize) const {
auto leadingMargin = isRow(axis)
? computeEdgeValueForRow<&Style::margin>(
getFlexStartRelativeEdgeUsingErrata(axis, direction),
flexStartEdge(axis))
getFlexStartRelativeEdge(axis, direction), flexStartEdge(axis))
: computeEdgeValueForColumn<&Style::margin>(flexStartEdge(axis));

return leadingMargin.resolve(widthSize).unwrapOrDefault(0.0f);
Expand All @@ -222,7 +207,7 @@ float Node::getInlineStartMargin(
FlexDirection axis,
Direction direction,
float widthSize) const {
Edge startEdge = getInlineStartEdgeUsingErrata(axis, direction);
Edge startEdge = getInlineStartEdge(axis, direction);
Style::Length leadingMargin = isRow(axis)
? computeEdgeValueForRow<&Style::margin>(Edge::Start, startEdge)
: computeEdgeValueForColumn<&Style::margin>(startEdge);
Expand All @@ -236,8 +221,7 @@ float Node::getFlexEndMargin(
float widthSize) const {
auto trailingMargin = isRow(axis)
? computeEdgeValueForRow<&Style::margin>(
getFlexEndRelativeEdgeUsingErrata(axis, direction),
flexEndEdge(axis))
getFlexEndRelativeEdge(axis, direction), flexEndEdge(axis))
: computeEdgeValueForColumn<&Style::margin>(flexEndEdge(axis));

return trailingMargin.resolve(widthSize).unwrapOrDefault(0.0f);
Expand All @@ -247,7 +231,7 @@ float Node::getInlineEndMargin(
FlexDirection axis,
Direction direction,
float widthSize) const {
Edge endEdge = getInlineEndEdgeUsingErrata(axis, direction);
Edge endEdge = getInlineEndEdge(axis, direction);
Style::Length trailingMargin = isRow(axis)
? computeEdgeValueForRow<&Style::margin>(Edge::End, endEdge)
: computeEdgeValueForColumn<&Style::margin>(endEdge);
Expand All @@ -257,7 +241,7 @@ float Node::getInlineEndMargin(

float Node::getInlineStartBorder(FlexDirection axis, Direction direction)
const {
Edge startEdge = getInlineStartEdgeUsingErrata(axis, direction);
Edge startEdge = getInlineStartEdge(axis, direction);
Style::Length leadingBorder = isRow(axis)
? computeEdgeValueForRow<&Style::border>(Edge::Start, startEdge)
: computeEdgeValueForColumn<&Style::border>(startEdge);
Expand All @@ -268,15 +252,14 @@ float Node::getInlineStartBorder(FlexDirection axis, Direction direction)
float Node::getFlexStartBorder(FlexDirection axis, Direction direction) const {
Style::Length leadingBorder = isRow(axis)
? computeEdgeValueForRow<&Style::border>(
getFlexStartRelativeEdgeUsingErrata(axis, direction),
flexStartEdge(axis))
getFlexStartRelativeEdge(axis, direction), flexStartEdge(axis))
: computeEdgeValueForColumn<&Style::border>(flexStartEdge(axis));

return maxOrDefined(leadingBorder.value().unwrap(), 0.0f);
}

float Node::getInlineEndBorder(FlexDirection axis, Direction direction) const {
Edge endEdge = getInlineEndEdgeUsingErrata(axis, direction);
Edge endEdge = getInlineEndEdge(axis, direction);
Style::Length trailingBorder = isRow(axis)
? computeEdgeValueForRow<&Style::border>(Edge::End, endEdge)
: computeEdgeValueForColumn<&Style::border>(endEdge);
Expand All @@ -287,8 +270,7 @@ float Node::getInlineEndBorder(FlexDirection axis, Direction direction) const {
float Node::getFlexEndBorder(FlexDirection axis, Direction direction) const {
Style::Length trailingBorder = isRow(axis)
? computeEdgeValueForRow<&Style::border>(
getFlexEndRelativeEdgeUsingErrata(axis, direction),
flexEndEdge(axis))
getFlexEndRelativeEdge(axis, direction), flexEndEdge(axis))
: computeEdgeValueForColumn<&Style::border>(flexEndEdge(axis));

return maxOrDefined(trailingBorder.value().unwrap(), 0.0f);
Expand All @@ -298,7 +280,7 @@ float Node::getInlineStartPadding(
FlexDirection axis,
Direction direction,
float widthSize) const {
Edge startEdge = getInlineStartEdgeUsingErrata(axis, direction);
Edge startEdge = getInlineStartEdge(axis, direction);
Style::Length leadingPadding = isRow(axis)
? computeEdgeValueForRow<&Style::padding>(Edge::Start, startEdge)
: computeEdgeValueForColumn<&Style::padding>(startEdge);
Expand All @@ -312,8 +294,7 @@ float Node::getFlexStartPadding(
float widthSize) const {
auto leadingPadding = isRow(axis)
? computeEdgeValueForRow<&Style::padding>(
getFlexStartRelativeEdgeUsingErrata(axis, direction),
flexStartEdge(axis))
getFlexStartRelativeEdge(axis, direction), flexStartEdge(axis))
: computeEdgeValueForColumn<&Style::padding>(flexStartEdge(axis));

return maxOrDefined(leadingPadding.resolve(widthSize).unwrap(), 0.0f);
Expand All @@ -323,7 +304,7 @@ float Node::getInlineEndPadding(
FlexDirection axis,
Direction direction,
float widthSize) const {
Edge endEdge = getInlineEndEdgeUsingErrata(axis, direction);
Edge endEdge = getInlineEndEdge(axis, direction);
Style::Length trailingPadding = isRow(axis)
? computeEdgeValueForRow<&Style::padding>(Edge::End, endEdge)
: computeEdgeValueForColumn<&Style::padding>(endEdge);
Expand All @@ -337,8 +318,7 @@ float Node::getFlexEndPadding(
float widthSize) const {
auto trailingPadding = isRow(axis)
? computeEdgeValueForRow<&Style::padding>(
getFlexEndRelativeEdgeUsingErrata(axis, direction),
flexEndEdge(axis))
getFlexEndRelativeEdge(axis, direction), flexEndEdge(axis))
: computeEdgeValueForColumn<&Style::padding>(flexEndEdge(axis));

return maxOrDefined(trailingPadding.resolve(widthSize).unwrap(), 0.0f);
Expand Down Expand Up @@ -578,14 +558,10 @@ void Node::setPosition(
const float relativePositionCross =
relativePosition(crossAxis, directionRespectingRoot, crossSize);

const Edge mainAxisLeadingEdge =
getInlineStartEdgeUsingErrata(mainAxis, direction);
const Edge mainAxisTrailingEdge =
getInlineEndEdgeUsingErrata(mainAxis, direction);
const Edge crossAxisLeadingEdge =
getInlineStartEdgeUsingErrata(crossAxis, direction);
const Edge crossAxisTrailingEdge =
getInlineEndEdgeUsingErrata(crossAxis, direction);
const Edge mainAxisLeadingEdge = getInlineStartEdge(mainAxis, direction);
const Edge mainAxisTrailingEdge = getInlineEndEdge(mainAxis, direction);
const Edge crossAxisLeadingEdge = getInlineStartEdge(crossAxis, direction);
const Edge crossAxisTrailingEdge = getInlineEndEdge(crossAxis, direction);

setLayoutPosition(
(getInlineStartMargin(mainAxis, direction, ownerWidth) +
Expand Down
16 changes: 6 additions & 10 deletions packages/react-native/ReactCommon/yoga/yoga/node/Node.h
Original file line number Diff line number Diff line change
Expand Up @@ -350,18 +350,14 @@ class YG_EXPORT Node : public ::YGNode {
Direction direction,
const float axisSize) const;

Edge getInlineStartEdgeUsingErrata(
FlexDirection flexDirection,
Direction direction) const;
Edge getInlineEndEdgeUsingErrata(
FlexDirection flexDirection,
Direction direction) const;
Edge getFlexStartRelativeEdgeUsingErrata(
FlexDirection flexDirection,
Direction direction) const;
Edge getFlexEndRelativeEdgeUsingErrata(
Edge getInlineStartEdge(FlexDirection flexDirection, Direction direction)
const;
Edge getInlineEndEdge(FlexDirection flexDirection, Direction direction) const;
Edge getFlexStartRelativeEdge(
FlexDirection flexDirection,
Direction direction) const;
Edge getFlexEndRelativeEdge(FlexDirection flexDirection, Direction direction)
const;

void useWebDefaults() {
style_.setFlexDirection(FlexDirection::Row);
Expand Down

0 comments on commit e859f6c

Please sign in to comment.