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

Baseline support #317

Closed
wants to merge 33 commits into from
Closed

Baseline support #317

wants to merge 33 commits into from

Conversation

woehrl01
Copy link
Contributor

@woehrl01 woehrl01 commented Jan 4, 2017

Added baseline support (see #132)

You have the ability for a custom baseline function (float(*YGBaselineFunc)(YGNodeRef node);) to return whatever baseline you want.

@woehrl01
Copy link
Contributor Author

woehrl01 commented Jan 4, 2017

Not sure if we should cache the baseline, as we get the baseline twice. First to get the maximum and second to position it.

I'm thinking about something like computedFlexBasisGeneration just named baselineCalculationGeneration and a cached value property.

Copy link
Contributor

@emilsjolander emilsjolander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Thanks! I'll do a more thorough review later this week. Please run format.sh in the mean time.

@@ -334,6 +335,20 @@ YGMeasureFunc YGNodeGetMeasureFunc(const YGNodeRef node) {
return node->measure;
}


void YGNodeSetBaselineFunc(const YGNodeRef node, YGBaselineFunc baselineFunc) {
if (baselineFunc == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node->baseline = baselineFunc; is enough

}

YGNodeRef baselineChild = NULL;
for(unsigned int i = 0; i < YGNodeGetChildCount(node); ++i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint32_t

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i++

@woehrl01
Copy link
Contributor Author

woehrl01 commented Jan 5, 2017

Please run format.sh in the mean time.

I'd love to do this, but the effort to create the environment to run this on windows, is currently too much work for me. As it seems I need a full linux runtime to execute this.

@emilsjolander
Copy link
Contributor

@woehrl01 having clang installed should be enough 👍 But that's fine, I can run it before merging. Could you try to adhere to the code style of the rest of the project though?

@woehrl01
Copy link
Contributor Author

woehrl01 commented Jan 5, 2017

@emilsjolander clang to the rescue! 👍 strangely clang-format on windows doesn't allow wildcard. So I hope I haven't missed a file.

@emilsjolander
Copy link
Contributor

@woehrl01 That's not clang, most likely your shell is not doing the expansion.

@woehrl01
Copy link
Contributor Author

woehrl01 commented Jan 5, 2017

most likely your shell is not doing the expansion.

true 😄

Copy link
Contributor

@emilsjolander emilsjolander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests for column direction as well as row direction. Also please add tests for align-self usage.

if (YGNodeIsLayoutDimDefined(child, crossAxis)) {
lineHeight = fmaxf(lineHeight,
child->layout.measuredDimensions[dim[crossAxis]] +
YGNodeMarginForAxis(child, crossAxis, availableInnerWidth));
}
if (performLayout && YGNodeAlignItem(node, child) == YGAlignBaseline) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performLayout check is not needed here as it is already checked in the containing if statement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't performlayout checked at line 2526 a little bit down below?

@@ -941,6 +950,39 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,
}
}

static float YGBaselineOfFirstLine(const YGNodeRef node, const YGFlexDirection crossAxis) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YGNodeBaseline

@@ -929,7 +938,12 @@ static inline float YGNodePaddingAndBorderForAxis(const YGNodeRef node,
}

static inline YGAlign YGNodeAlignItem(const YGNodeRef node, const YGNodeRef child) {
return child->style.alignSelf == YGAlignAuto ? node->style.alignItems : child->style.alignSelf;
YGAlign align =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

if (node->baseline != NULL) {
const float baseline = node->baseline(node);
if (YGFloatIsUndefined(baseline)) {
return node->layout.measuredDimensions[dim[crossAxis]];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will always be height as baseline is undefined for column flex direction. right? in that case I think it is better to be clear and use YGDimensionHeight instead of dim[crossAxis]

const YGFlexDirection crossAxis) {
if (node->baseline != NULL) {
const float baseline = node->baseline(node);
if (YGFloatIsUndefined(baseline)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a good reason to allow undefined return value for baseline? I would prefer asserting and crashing as it would probably indicate a bug. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure my self about this. I thought this could be a kind of "feature", so if you return undefined, you simply use the nodes height. But crashing would be fine too for me. What I'm not sure about is if we explicitly add the padding-top here or if the implementation of the custom function needs to consider this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom function should not take padding into account. We don't expect this for the measure function so I would like to preserve that here if possible.

Let's crash for now. If we find a valid reason to have this feature we can implement it later.

YGNodeRef baselineChild = NULL;
for (uint32_t i = 0; i < YGNodeGetChildCount(node); i++) {
const YGNodeRef child = YGNodeGetChild(node, i);
if (child->style.positionType == YGPositionTypeAbsolute || child->lineIndex > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why child->lineIndex > 0? I understand that it skips multiline children but i'm not sure why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use only the first line of the children for base layout alignment. At least this is how chrome handles it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but this looks like we are skipping over children with multiple lines instead of just looking at their first line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, yep. We should break instead of continue here!

}

if (baselineChild == NULL) {
return node->layout.measuredDimensions[dim[crossAxis]];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above regarding dim[crossAxis]

continue;
}

if (YGNodeAlignItem(node, child) == YGAlignBaseline) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The baseline of a container is defined by its first baseline aligned child, but when there is no baseline aligned child then the container's baseline is defined by it's last child. This seems odd. Are you sure we have test cases covering edge cases here to make sure this is how it works on the web?

Copy link
Contributor Author

@woehrl01 woehrl01 Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no it's defined by its first child on the first line or the first baseline aligned child if there is one (one the first line). I'll add a test for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the current code is wrong i think. Right? As it will pick the last child in the case no child is baseline aligned?

Copy link
Contributor Author

@woehrl01 woehrl01 Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no as we only use the child if baselineChild is still NULL. Which is false as seen as we find the first one. We still need to iterate to take any baseline aligned child into account.

}

const float baseline = YGBaseline(baselineChild, crossAxis);
return baseline + baselineChild->layout.position[pos[crossAxis]];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above regarding pos[crossAxis]

return true;
}
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: empty line above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what you mean, should I add a empty line above return false?

@@ -49,6 +49,9 @@ typedef YGSize (*YGMeasureFunc)(YGNodeRef node,
YGMeasureMode widthMode,
float height,
YGMeasureMode heightMode);

typedef float (*YGBaselineFunc)(YGNodeRef node);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove added empty lines

if (YGNodeIsLayoutDimDefined(child, crossAxis)) {
lineHeight = fmaxf(lineHeight,
child->layout.measuredDimensions[dim[crossAxis]] +
YGNodeMarginForAxis(child, crossAxis, availableInnerWidth));
}
if (YGNodeAlignItem(node, child) == YGAlignBaseline) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above. Don't reference crossAxis as it confuses the fact that baseline is only defined for row layouts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I use YGFlexDirectionColumn here on the YGNodeLeadingMargin and YGNodeMarginForAxis calls?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes :) and replace dim[crossAxis]

@emilsjolander
Copy link
Contributor

@woehrl01 If you push all updates in one go (Ok if they are different commits) that would make it more clear to me and others when you are finished addressing comments so we can have another look 👍

@woehrl01
Copy link
Contributor Author

woehrl01 commented Jan 5, 2017

@emilsjolander sorry, will do next time!

@woehrl01
Copy link
Contributor Author

woehrl01 commented Jan 5, 2017

I think I got all your points, please have a look 😄

@@ -49,6 +49,8 @@ typedef YGSize (*YGMeasureFunc)(YGNodeRef node,
YGMeasureMode widthMode,
float height,
YGMeasureMode heightMode);
typedef float (*YGBaselineFunc)(YGNodeRef node);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove added empty line

@@ -2501,6 +2574,12 @@ static void YGNodelayoutImpl(const YGNodeRef node,
// (auto) crossAxis dimension.
break;
}
case YGAlignBaseline: {
child->layout.position[pos[crossAxis]] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could pos[crossAxis] be hardcoded? Same with crossAxis a couple lines below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I just wanted to be the same like the others, to not cause confusion. But I'll replace them, to be explicit that it is only used on YGEdgeTop

return true;
}
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this 👍

YGNodeRef baselineChild = NULL;
for (uint32_t i = 0; i < YGNodeGetChildCount(node); i++) {
const YGNodeRef child = YGNodeGetChild(node, i);
if (child->lineIndex > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not quite sure why we break if a child has more than one line. That would mean if the first child of this node has two lines then the baseline would default to the height of the node. I'm not sure why that would be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we break as only childs on the first line which have align baseline are taken into account. I already added a testcase for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I see the confusion, child->lineIndex does not mean that it has more than one line. It means it is itself on the seconds (or later) line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@woehrl01 yeah that was my confusion, i'm not super familiar with the multi line code and read it too quickly 🐱

@woehrl01
Copy link
Contributor Author

woehrl01 commented Jan 5, 2017

@emilsjolander I additionally modified YGIsBaselineLayout to shortcut to false if it's a column layout.

@facebook-github-bot
Copy link
Contributor

@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@splhack splhack mentioned this pull request Jan 5, 2017
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jan 6, 2017
Summary:
Added baseline support (see #132)

You have the ability for a custom baseline function (```float(*YGBaselineFunc)(YGNodeRef node);```) to return whatever baseline you want.
Closes facebook/yoga#317

Reviewed By: splhack

Differential Revision: D4385061

Pulled By: emilsjolander

fbshipit-source-id: cb8a59a09237c840fa3e21753ab68239997dab0c
facebook-github-bot pushed a commit that referenced this pull request Jan 7, 2017
Summary:
same as Measure function, based on #317
Closes #321

Reviewed By: emilsjolander

Differential Revision: D4385336

Pulled By: splhack

fbshipit-source-id: b583ec79861d2e9abba31a72503e2f706bfda8e8
@anql
Copy link

anql commented Feb 24, 2017

@woehrl01 The baseline feature is really awesome, but I'm not sure how to implement the function correctly. Would you give me some suggestions? Thanks.

@woehrl01
Copy link
Contributor Author

woehrl01 commented Feb 24, 2017

@anql: Sure! You must return the vertical position inside the node you want to align.

If you do not have a baseline function set, it uses the nodes height. That means that all the nodes are align at their bottom line.

If you have a text node where you want to align your nodes across each others, you need to return the position of the baseline of the firstline inside the node.

I unpacked my "awesome" paint skills 😂, to make this a bit clearer:

image

If the black numbers are the height of the nodes, you need to return the red values, to align the nodes across. If you have multiline text you need to return the value of the first line.

@anql
Copy link

anql commented Feb 24, 2017

The paint is so clearer, it's very helpful for me. 👍 @woehrl01

vincentriemer pushed a commit to vincentriemer/yoga-dom that referenced this pull request May 9, 2018
Summary:
Added baseline support (see #132)

You have the ability for a custom baseline function (```float(*YGBaselineFunc)(YGNodeRef node);```) to return whatever baseline you want.
Closes facebook/yoga#317

Reviewed By: splhack

Differential Revision: D4385061

Pulled By: emilsjolander

fbshipit-source-id: cb8a59a09237c840fa3e21753ab68239997dab0c
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.

4 participants