Skip to content

Commit

Permalink
Rename ambiguous leading/trailingEdge functions (facebook#41017)
Browse files Browse the repository at this point in the history 
Summary:
X-link: facebook/yoga#1423

Pull Request resolved: facebook#41017

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

fbshipit-source-id: 1e83a885876af9cf363822ebdbb64537f4784520
  • Loading branch information
@joevilches @Othinn
joevilches authored and Othinn committed Oct 30, 2023
1 parent 207084c commit f6bfa69
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 52 deletions.
52 changes: 26 additions & 26 deletions packages/react-native/ReactCommon/yoga/yoga/algorithm/CalculateLayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ static void setChildTrailingPosition(
const float size = child->getLayout().measuredDimension(dimension(axis));
child->setLayoutPosition(
node->getLayout().measuredDimension(dimension(axis)) - size -
child->getLayout().position[leadingEdge(axis)],
trailingEdge(axis));
child->getLayout().position[flexStartEdge(axis)],
flexEndEdge(axis));
}

static void constrainMaxSizeForMode(
Expand Down Expand Up @@ -456,22 +456,22 @@ static void layoutAbsoluteChild(
mainAxis, direction, isMainAxisRow ? width : height) -
child->getTrailingPosition(
mainAxis, isMainAxisRow ? width : height),
leadingEdge(mainAxis));
flexStartEdge(mainAxis));
} else if (
!child->isLeadingPositionDefined(mainAxis) &&
node->getStyle().justifyContent() == Justify::Center) {
child->setLayoutPosition(
(node->getLayout().measuredDimension(dimension(mainAxis)) -
child->getLayout().measuredDimension(dimension(mainAxis))) /
2.0f,
leadingEdge(mainAxis));
flexStartEdge(mainAxis));
} else if (
!child->isLeadingPositionDefined(mainAxis) &&
node->getStyle().justifyContent() == Justify::FlexEnd) {
child->setLayoutPosition(
(node->getLayout().measuredDimension(dimension(mainAxis)) -
child->getLayout().measuredDimension(dimension(mainAxis))),
leadingEdge(mainAxis));
flexStartEdge(mainAxis));
} else if (
node->getConfig()->isExperimentalFeatureEnabled(
ExperimentalFeature::AbsolutePercentageAgainstPaddingEdge) &&
Expand All @@ -485,7 +485,7 @@ static void layoutAbsoluteChild(
mainAxis,
direction,
node->getLayout().measuredDimension(dimension(mainAxis))),
leadingEdge(mainAxis));
flexStartEdge(mainAxis));
}

if (child->isTrailingPosDefined(crossAxis) &&
Expand All @@ -498,7 +498,7 @@ static void layoutAbsoluteChild(
crossAxis, direction, isMainAxisRow ? height : width) -
child->getTrailingPosition(
crossAxis, isMainAxisRow ? height : width),
leadingEdge(crossAxis));
flexStartEdge(crossAxis));

} else if (
!child->isLeadingPositionDefined(crossAxis) &&
Expand All @@ -507,15 +507,15 @@ static void layoutAbsoluteChild(
(node->getLayout().measuredDimension(dimension(crossAxis)) -
child->getLayout().measuredDimension(dimension(crossAxis))) /
2.0f,
leadingEdge(crossAxis));
flexStartEdge(crossAxis));
} else if (
!child->isLeadingPositionDefined(crossAxis) &&
((resolveChildAlignment(node, child) == Align::FlexEnd) ^
(node->getStyle().flexWrap() == Wrap::WrapReverse))) {
child->setLayoutPosition(
(node->getLayout().measuredDimension(dimension(crossAxis)) -
child->getLayout().measuredDimension(dimension(crossAxis))),
leadingEdge(crossAxis));
flexStartEdge(crossAxis));
} else if (
node->getConfig()->isExperimentalFeatureEnabled(
ExperimentalFeature::AbsolutePercentageAgainstPaddingEdge) &&
Expand All @@ -529,7 +529,7 @@ static void layoutAbsoluteChild(
crossAxis,
direction,
node->getLayout().measuredDimension(dimension(crossAxis))),
leadingEdge(crossAxis));
flexStartEdge(crossAxis));
}
}

Expand Down Expand Up @@ -1308,7 +1308,7 @@ static void justifyMainAxis(
node->getLeadingBorder(mainAxis) +
child->getLeadingMargin(
mainAxis, direction, availableInnerWidth),
leadingEdge(mainAxis));
flexStartEdge(mainAxis));
}
} else {
// Now that we placed the element, we need to update the variables.
Expand All @@ -1322,9 +1322,9 @@ static void justifyMainAxis(

if (performLayout) {
child->setLayoutPosition(
childLayout.position[leadingEdge(mainAxis)] +
childLayout.position[flexStartEdge(mainAxis)] +
flexLine.layout.mainDim,
leadingEdge(mainAxis));
flexStartEdge(mainAxis));
}

if (child != flexLine.itemsInFlow.back()) {
Expand Down Expand Up @@ -1378,9 +1378,9 @@ static void justifyMainAxis(
}
} else if (performLayout) {
child->setLayoutPosition(
childLayout.position[leadingEdge(mainAxis)] +
childLayout.position[flexStartEdge(mainAxis)] +
node->getLeadingBorder(mainAxis) + leadingMainDim,
leadingEdge(mainAxis));
flexStartEdge(mainAxis));
}
}
}
Expand Down Expand Up @@ -1856,18 +1856,18 @@ static void calculateLayoutImpl(
node->getLeadingBorder(crossAxis) +
child->getLeadingMargin(
crossAxis, direction, availableInnerWidth),
leadingEdge(crossAxis));
flexStartEdge(crossAxis));
}
// If leading position is not defined or calculations result in Nan,
// default to border + margin
if (!isChildLeadingPosDefined ||
yoga::isUndefined(
child->getLayout().position[leadingEdge(crossAxis)])) {
child->getLayout().position[flexStartEdge(crossAxis)])) {
child->setLayoutPosition(
node->getLeadingBorder(crossAxis) +
child->getLeadingMargin(
crossAxis, direction, availableInnerWidth),
leadingEdge(crossAxis));
flexStartEdge(crossAxis));
}
} else {
float leadingCrossDim = leadingPaddingAndBorderCross;
Expand Down Expand Up @@ -1975,9 +1975,9 @@ static void calculateLayoutImpl(
}
// And we apply the position
child->setLayoutPosition(
child->getLayout().position[leadingEdge(crossAxis)] +
child->getLayout().position[flexStartEdge(crossAxis)] +
totalLineCrossDim + leadingCrossDim,
leadingEdge(crossAxis));
flexStartEdge(crossAxis));
}
}
}
Expand Down Expand Up @@ -2092,7 +2092,7 @@ static void calculateLayoutImpl(
currentLead +
child->getLeadingMargin(
crossAxis, direction, availableInnerWidth),
leadingEdge(crossAxis));
flexStartEdge(crossAxis));
break;
}
case Align::FlexEnd: {
Expand All @@ -2102,7 +2102,7 @@ static void calculateLayoutImpl(
crossAxis, direction, availableInnerWidth) -
child->getLayout().measuredDimension(
dimension(crossAxis)),
leadingEdge(crossAxis));
flexStartEdge(crossAxis));
break;
}
case Align::Center: {
Expand All @@ -2111,15 +2111,15 @@ static void calculateLayoutImpl(

child->setLayoutPosition(
currentLead + (lineHeight - childHeight) / 2,
leadingEdge(crossAxis));
flexStartEdge(crossAxis));
break;
}
case Align::Stretch: {
child->setLayoutPosition(
currentLead +
child->getLeadingMargin(
crossAxis, direction, availableInnerWidth),
leadingEdge(crossAxis));
flexStartEdge(crossAxis));

// Remeasure child with the line height as it as been only
// measured with the owners height yet.
Expand Down Expand Up @@ -2275,9 +2275,9 @@ static void calculateLayoutImpl(
if (child->getStyle().positionType() != PositionType::Absolute) {
child->setLayoutPosition(
node->getLayout().measuredDimension(dimension(crossAxis)) -
child->getLayout().position[leadingEdge(crossAxis)] -
child->getLayout().position[flexStartEdge(crossAxis)] -
child->getLayout().measuredDimension(dimension(crossAxis)),
leadingEdge(crossAxis));
flexStartEdge(crossAxis));
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions packages/react-native/ReactCommon/yoga/yoga/algorithm/FlexDirection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ inline FlexDirection resolveCrossDirection(
: FlexDirection::Column;
}

inline YGEdge leadingEdge(const FlexDirection flexDirection) {
inline YGEdge flexStartEdge(const FlexDirection flexDirection) {
switch (flexDirection) {
case FlexDirection::Column:
return YGEdgeTop;
Expand All @@ -62,7 +62,7 @@ inline YGEdge leadingEdge(const FlexDirection flexDirection) {
fatalWithMessage("Invalid FlexDirection");
}

inline YGEdge trailingEdge(const FlexDirection flexDirection) {
inline YGEdge flexEndEdge(const FlexDirection flexDirection) {
switch (flexDirection) {
case FlexDirection::Column:
return YGEdgeBottom;
Expand All @@ -77,7 +77,7 @@ inline YGEdge trailingEdge(const FlexDirection flexDirection) {
fatalWithMessage("Invalid FlexDirection");
}

inline YGEdge leadingLayoutEdge(
inline YGEdge inlineStartEdge(
const FlexDirection flexDirection,
const Direction direction) {
if (isRow(flexDirection)) {
Expand All @@ -87,7 +87,7 @@ inline YGEdge leadingLayoutEdge(
return YGEdgeTop;
}

inline YGEdge trailingLayoutEdge(
inline YGEdge inlineEndEdge(
const FlexDirection flexDirection,
const Direction direction) {
if (isRow(flexDirection)) {
Expand Down
46 changes: 24 additions & 22 deletions packages/react-native/ReactCommon/yoga/yoga/node/Node.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,48 +87,48 @@ YGEdge Node::getLeadingLayoutEdgeUsingErrata(
FlexDirection flexDirection,
Direction direction) const {
return hasErrata(Errata::StartingEndingEdgeFromFlexDirection)
? leadingEdge(flexDirection)
: leadingLayoutEdge(flexDirection, direction);
? flexStartEdge(flexDirection)
: inlineStartEdge(flexDirection, direction);
}

YGEdge Node::getTrailingLayoutEdgeUsingErrata(
FlexDirection flexDirection,
Direction direction) const {
return hasErrata(Errata::StartingEndingEdgeFromFlexDirection)
? trailingEdge(flexDirection)
: trailingLayoutEdge(flexDirection, direction);
? flexEndEdge(flexDirection)
: inlineEndEdge(flexDirection, direction);
}

bool Node::isLeadingPositionDefined(FlexDirection axis) const {
auto leadingPosition = isRow(axis)
? computeEdgeValueForRow(
style_.position(), YGEdgeStart, leadingEdge(axis))
: computeEdgeValueForColumn(style_.position(), leadingEdge(axis));
style_.position(), YGEdgeStart, flexStartEdge(axis))
: computeEdgeValueForColumn(style_.position(), flexStartEdge(axis));

return !leadingPosition.isUndefined();
}

bool Node::isTrailingPosDefined(FlexDirection axis) const {
auto trailingPosition = isRow(axis)
? computeEdgeValueForRow(style_.position(), YGEdgeEnd, trailingEdge(axis))
: computeEdgeValueForColumn(style_.position(), trailingEdge(axis));
? computeEdgeValueForRow(style_.position(), YGEdgeEnd, flexEndEdge(axis))
: computeEdgeValueForColumn(style_.position(), flexEndEdge(axis));

return !trailingPosition.isUndefined();
}

float Node::getLeadingPosition(FlexDirection axis, float axisSize) const {
auto leadingPosition = isRow(axis)
? computeEdgeValueForRow(
style_.position(), YGEdgeStart, leadingEdge(axis))
: computeEdgeValueForColumn(style_.position(), leadingEdge(axis));
style_.position(), YGEdgeStart, flexStartEdge(axis))
: computeEdgeValueForColumn(style_.position(), flexStartEdge(axis));

return resolveValue(leadingPosition, axisSize).unwrapOrDefault(0.0f);
}

float Node::getTrailingPosition(FlexDirection axis, float axisSize) const {
auto trailingPosition = isRow(axis)
? computeEdgeValueForRow(style_.position(), YGEdgeEnd, trailingEdge(axis))
: computeEdgeValueForColumn(style_.position(), trailingEdge(axis));
? computeEdgeValueForRow(style_.position(), YGEdgeEnd, flexEndEdge(axis))
: computeEdgeValueForColumn(style_.position(), flexEndEdge(axis));

return resolveValue(trailingPosition, axisSize).unwrapOrDefault(0.0f);
}
Expand Down Expand Up @@ -159,32 +159,34 @@ float Node::getTrailingMargin(

float Node::getLeadingBorder(FlexDirection axis) const {
YGValue leadingBorder = isRow(axis)
? computeEdgeValueForRow(style_.border(), YGEdgeStart, leadingEdge(axis))
: computeEdgeValueForColumn(style_.border(), leadingEdge(axis));
? computeEdgeValueForRow(
style_.border(), YGEdgeStart, flexStartEdge(axis))
: computeEdgeValueForColumn(style_.border(), flexStartEdge(axis));

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

float Node::getTrailingBorder(FlexDirection axis) const {
YGValue trailingBorder = isRow(axis)
? computeEdgeValueForRow(style_.border(), YGEdgeEnd, trailingEdge(axis))
: computeEdgeValueForColumn(style_.border(), trailingEdge(axis));
? computeEdgeValueForRow(style_.border(), YGEdgeEnd, flexEndEdge(axis))
: computeEdgeValueForColumn(style_.border(), flexEndEdge(axis));

return maxOrDefined(trailingBorder.value, 0.0f);
}

float Node::getLeadingPadding(FlexDirection axis, float widthSize) const {
auto leadingPadding = isRow(axis)
? computeEdgeValueForRow(style_.padding(), YGEdgeStart, leadingEdge(axis))
: computeEdgeValueForColumn(style_.padding(), leadingEdge(axis));
? computeEdgeValueForRow(
style_.padding(), YGEdgeStart, flexStartEdge(axis))
: computeEdgeValueForColumn(style_.padding(), flexStartEdge(axis));

return maxOrDefined(resolveValue(leadingPadding, widthSize).unwrap(), 0.0f);
}

float Node::getTrailingPadding(FlexDirection axis, float widthSize) const {
auto trailingPadding = isRow(axis)
? computeEdgeValueForRow(style_.padding(), YGEdgeEnd, trailingEdge(axis))
: computeEdgeValueForColumn(style_.padding(), trailingEdge(axis));
? computeEdgeValueForRow(style_.padding(), YGEdgeEnd, flexEndEdge(axis))
: computeEdgeValueForColumn(style_.padding(), flexEndEdge(axis));

return maxOrDefined(resolveValue(trailingPadding, widthSize).unwrap(), 0.0f);
}
Expand Down Expand Up @@ -418,15 +420,15 @@ YGValue Node::marginLeadingValue(FlexDirection axis) const {
if (isRow(axis) && !style_.margin()[YGEdgeStart].isUndefined()) {
return style_.margin()[YGEdgeStart];
} else {
return style_.margin()[leadingEdge(axis)];
return style_.margin()[flexStartEdge(axis)];
}
}

YGValue Node::marginTrailingValue(FlexDirection axis) const {
if (isRow(axis) && !style_.margin()[YGEdgeEnd].isUndefined()) {
return style_.margin()[YGEdgeEnd];
} else {
return style_.margin()[trailingEdge(axis)];
return style_.margin()[flexEndEdge(axis)];
}
}

Expand Down

0 comments on commit f6bfa69

Please sign in to comment.