Let measure behave more like on the web #499

woehrl01 · 2017-04-02T09:42:37Z

Nodes with a measure function needs to be measured even so it seems there is no available space. So it behaves more like on the web. Fix #488

emilsjolander

Could you explain why? The test is not obvious to me.

emilsjolander · 2017-04-04T08:54:30Z

tests/YGMeasureTest.cpp

+  YGNodeStyleSetAlignItems(root, YGAlignStretch);
+  YGNodeStyleSetAlignContent(root, YGAlignStretch);
+  YGNodeStyleSetFlexGrow(root, 0);
+  YGNodeStyleSetOverflow(root, YGOverflowHidden);


remove overflow line. Should have no effect on test

emilsjolander · 2017-04-04T08:54:44Z

tests/YGMeasureTest.cpp

+  YGNodeStyleSetFlexGrow(root, 0);
+  YGNodeStyleSetOverflow(root, YGOverflowHidden);
+  YGNodeStyleSetAlignSelf(root, YGAlignAuto);
+  YGNodeStyleSetPositionType(root, YGPositionTypeRelative);


relative is default, no need to set it

emilsjolander · 2017-04-04T08:55:00Z

tests/YGMeasureTest.cpp

+  const YGNodeRef root = YGNodeNew();
+  YGNodeStyleSetHeight(root, 200);
+  YGNodeStyleSetFlexDirection(root, YGFlexDirectionColumn);
+  YGNodeStyleSetAlignItems(root, YGAlignStretch);


stretch is default for align items. no need to set it

You're right, I adopted all the values form the issue.

emilsjolander · 2017-04-04T08:55:16Z

tests/YGMeasureTest.cpp

+  YGNodeStyleSetOverflow(root, YGOverflowHidden);
+  YGNodeStyleSetAlignSelf(root, YGAlignAuto);
+  YGNodeStyleSetPositionType(root, YGPositionTypeRelative);
+  YGNodeStyleSetJustifyContent(root, YGJustifyFlexStart);


flex start is default for justify content. no need to set it

emilsjolander · 2017-04-04T08:55:53Z

tests/YGMeasureTest.cpp

+  YGNodeStyleSetAlignSelf(root_child0, YGAlignStretch);
+  YGNodeStyleSetJustifyContent(root_child0, YGJustifyFlexStart);
+  YGNodeStyleSetOverflow(root_child0, YGOverflowHidden);
+  YGNodeStyleSetPadding(root_child0, YGEdgeLeft, 100);


Use YGEdgeALL instead?

woehrl01 · 2017-04-04T09:33:31Z

The test is not that obvious correct. The problem is:
The parent height is 200 and the padding is 100 on both sides so the available size is 0, the measure size is non zero. But as we haven't constraint the other side, We should be still growable in the width dimension.

In my opinion a measured node could still overflow it's parent, isn't it?

emilsjolander · 2017-04-04T09:39:22Z

Ok thanks for explaining. Growing the parent width makes sense in that case. Regarding overflow I don't know. I would look to web browsers for guidance on this.

woehrl01 · 2017-04-04T09:43:29Z

According to this fiddle it's overflowing.

woehrl01 · 2017-04-04T09:43:58Z

Btw. I'll update the test and remove the default values.

emilsjolander · 2017-04-04T11:51:59Z

Why should this only handle at_most and not undefined?

woehrl01 · 2017-04-05T18:13:52Z

You're right it should also measure on the undefined mode, too.

woehrl01 · 2017-04-05T20:49:59Z

Sorry, I actually was wrong. We don't need to measure here, we just need to return the correct size in the "no available size" skipping. I moved the skipping to the calculation step to perform the same calculations. I just don't call measure but use a new gYGSizeZero for the measurement instead.

facebook-github-bot · 2017-04-07T08:35:04Z

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

emilsjolander · 2017-04-07T12:10:44Z

Seems measure_zero_space_should_grow test is failing after the last update. Could you have a look?

woehrl01 · 2017-04-07T12:30:57Z

It's failing as we don't measure anymore. I reduce the testcase for now. As enabling always measure would lead to a possible layoutbreaking change. This fixes the current behaviour.

We need to discuss how the final behaviour should be and possibly introduce changes as some kind of flag?

I think this should be fixed/handled in a separete PR.

woehrl01 · 2017-04-07T12:36:28Z

I expanded the jsfiddle to some testcases which some of them definetly aren't rendered correct currently https://jsfiddle.net/uh9whoom/6/

emilsjolander · 2017-04-07T12:46:01Z

@woehrl01 As long as we are moving towards better web compatibility and not breaking any major existing use cases I'm happy. I would rather not introduce new flags unless absolutely necessary.

facebook-github-bot · 2017-04-07T12:46:39Z

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

emilsjolander · 2017-04-07T15:09:04Z

This is causing some test failures on integration tests internally. I haven't tracked down why but it seems this causes nodes to be measured to larger sizes than before.

woehrl01 · 2017-04-07T15:46:33Z

Yes, it returns now a different size, it now behaves from the size like a node without a measure function. If there isn't any space.

woehrl01 · 2017-04-07T15:47:47Z

But if this is introducing a breaking change. Maybe we should directly move to a more web compatible behaviour. What do you think?

emilsjolander · 2017-04-07T16:14:03Z

But if this is introducing a breaking change. Maybe we should directly move to a more web compatible behaviour. What do you think?

Yeah that sounds good, much easier to land if we only breaks things once.

woehrl01 · 2017-04-07T16:16:54Z

Ok, but this will lead to that we will measure more often. I'll put up a bigger test suite and put up a PR. Could take me a bit 👍 .

emilsjolander · 2017-04-07T16:42:23Z

ok Thanks! Regarding increased measurements as long as it is in a couple edge cases that is fine. We will have a look once the code is up and i'll do some internal logging to figure out if we regressed # of measure calls in practice.

facebook-github-bot · 2017-04-11T09:52:31Z

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

Summary: Nodes with a measure function needs to be measured even so it seems there is no available space. So it behaves more like on the web. Fix facebook/yoga#488 Closes facebook/yoga#499 Differential Revision: D4850458 Pulled By: emilsjolander fbshipit-source-id: be5e35a670ddcbf3cd426fc3c2a0c9b60a874cdc

emilsjolander · 2017-04-13T22:30:58Z

Re-opening this as well. This caused fewer breakages but still some issues which I don't have time to resolve right away. I will try re-commiting this next week. Sorry about that :)

Summary: Nodes with a measure function needs to be measured even so it seems there is no available space. So it behaves more like on the web. Fix facebook/yoga#488 Closes facebook/yoga#499 Differential Revision: D4850458 Pulled By: emilsjolander fbshipit-source-id: be5e35a670ddcbf3cd426fc3c2a0c9b60a874cdc

Summary: Nodes with a measure function needs to be measured even so it seems there is no available space. So it behaves more like on the web. Fix facebook/yoga#488 Closes facebook/yoga#499 Reviewed By: astreet Differential Revision: D4954008 Pulled By: emilsjolander fbshipit-source-id: 5b6d9afae0cdebe33f8b82b67620b3b4527d1efc

Summary: Nodes with a measure function needs to be measured even so it seems there is no available space. So it behaves more like on the web. Fix facebook/yoga#488 Closes facebook/yoga#499 Differential Revision: D4850458 Pulled By: emilsjolander fbshipit-source-id: be5e35a670ddcbf3cd426fc3c2a0c9b60a874cdc

Summary: Nodes with a measure function needs to be measured even so it seems there is no available space. So it behaves more like on the web. Fix facebook/yoga#488 Closes facebook/yoga#499 Reviewed By: astreet Differential Revision: D4954008 Pulled By: emilsjolander fbshipit-source-id: 5b6d9afae0cdebe33f8b82b67620b3b4527d1efc

Summary: Nodes with a measure function needs to be measured even so it seems there is no available space. So it behaves more like on the web. Fix facebook/yoga#488 Closes facebook/yoga#499 Differential Revision: D4850458 Pulled By: emilsjolander fbshipit-source-id: be5e35a670ddcbf3cd426fc3c2a0c9b60a874cdc

Summary: Nodes with a measure function needs to be measured even so it seems there is no available space. So it behaves more like on the web. Fix facebook/yoga#488 Closes facebook/yoga#499 Reviewed By: astreet Differential Revision: D4954008 Pulled By: emilsjolander fbshipit-source-id: 5b6d9afae0cdebe33f8b82b67620b3b4527d1efc

measure nodes on AT_MOST with zero space

b7f4350

facebook-github-bot added the CLA Signed label Apr 2, 2017

emilsjolander suggested changes Apr 4, 2017

View reviewed changes

remove default values

6dfbf67

woehrl01 added 2 commits April 5, 2017 20:10

apply optimisation only for exactly measure

7f23b3f

dots

6e7cd2a

woehrl01 changed the title ~~Measure nodes on AT_MOST with zero space~~ Skip measuring of nodes with zero space only on EXACTLY Apr 5, 2017

woehrl01 added 2 commits April 5, 2017 22:41

reduce measurement

4b876ba

fix vcxproj

ee6a348

woehrl01 changed the title ~~Skip measuring of nodes with zero space only on EXACTLY~~ Fix skipping of measurement by returning correct size for non EXACT measurements Apr 5, 2017

fix test

07f70c7

make measuring behaviour more like on the web

6af70cf

woehrl01 changed the title ~~Fix skipping of measurement by returning correct size for non EXACT measurements~~ Let measure behave more like on the web Apr 7, 2017

woehrl01 added 2 commits April 8, 2017 09:04

remove unused variable

f20ce50

format

e2410aa

facebook-github-bot closed this in 7c57245 Apr 13, 2017

emilsjolander reopened this Apr 13, 2017

lacker closed this in lacker/react-native@62360e8 Apr 21, 2017

emilsjolander reopened this Apr 21, 2017

facebook-github-bot closed this in f6b1718 Apr 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let measure behave more like on the web #499

Let measure behave more like on the web #499

woehrl01 commented Apr 2, 2017 •

edited

Loading

emilsjolander left a comment

emilsjolander Apr 4, 2017

emilsjolander Apr 4, 2017

emilsjolander Apr 4, 2017

woehrl01 Apr 4, 2017

emilsjolander Apr 4, 2017

emilsjolander Apr 4, 2017

woehrl01 commented Apr 4, 2017

emilsjolander commented Apr 4, 2017

woehrl01 commented Apr 4, 2017 •

edited

Loading

woehrl01 commented Apr 4, 2017

emilsjolander commented Apr 4, 2017

woehrl01 commented Apr 5, 2017

woehrl01 commented Apr 5, 2017

facebook-github-bot commented Apr 7, 2017

emilsjolander commented Apr 7, 2017

woehrl01 commented Apr 7, 2017 •

edited

Loading

woehrl01 commented Apr 7, 2017

emilsjolander commented Apr 7, 2017

facebook-github-bot commented Apr 7, 2017

emilsjolander commented Apr 7, 2017

woehrl01 commented Apr 7, 2017

woehrl01 commented Apr 7, 2017 •

edited

Loading

emilsjolander commented Apr 7, 2017

woehrl01 commented Apr 7, 2017

emilsjolander commented Apr 7, 2017

facebook-github-bot commented Apr 11, 2017

emilsjolander commented Apr 13, 2017

Let measure behave more like on the web #499

Let measure behave more like on the web #499

Conversation

woehrl01 commented Apr 2, 2017 • edited Loading

emilsjolander left a comment

Choose a reason for hiding this comment

emilsjolander Apr 4, 2017

Choose a reason for hiding this comment

emilsjolander Apr 4, 2017

Choose a reason for hiding this comment

emilsjolander Apr 4, 2017

Choose a reason for hiding this comment

woehrl01 Apr 4, 2017

Choose a reason for hiding this comment

emilsjolander Apr 4, 2017

Choose a reason for hiding this comment

emilsjolander Apr 4, 2017

Choose a reason for hiding this comment

woehrl01 commented Apr 4, 2017

emilsjolander commented Apr 4, 2017

woehrl01 commented Apr 4, 2017 • edited Loading

woehrl01 commented Apr 4, 2017

emilsjolander commented Apr 4, 2017

woehrl01 commented Apr 5, 2017

woehrl01 commented Apr 5, 2017

facebook-github-bot commented Apr 7, 2017

emilsjolander commented Apr 7, 2017

woehrl01 commented Apr 7, 2017 • edited Loading

woehrl01 commented Apr 7, 2017

emilsjolander commented Apr 7, 2017

facebook-github-bot commented Apr 7, 2017

emilsjolander commented Apr 7, 2017

woehrl01 commented Apr 7, 2017

woehrl01 commented Apr 7, 2017 • edited Loading

emilsjolander commented Apr 7, 2017

woehrl01 commented Apr 7, 2017

emilsjolander commented Apr 7, 2017

facebook-github-bot commented Apr 11, 2017

emilsjolander commented Apr 13, 2017

woehrl01 commented Apr 2, 2017 •

edited

Loading

woehrl01 commented Apr 4, 2017 •

edited

Loading

woehrl01 commented Apr 7, 2017 •

edited

Loading

woehrl01 commented Apr 7, 2017 •

edited

Loading