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

Positioning issues for centered items when nested under stretched items #175

Closed
javache opened this issue Mar 16, 2016 · 7 comments · Fixed by #178
Closed

Positioning issues for centered items when nested under stretched items #175

javache opened this issue Mar 16, 2016 · 7 comments · Fixed by #178

Comments

@javache
Copy link
Member

javache commented Mar 16, 2016

Seeing this issues after pulling in #171 in RN on iOS.

When there's a container view with unspecified cross-axis dimension, even though its calculated dimensions are known, it will perform a second recursive call. This second recursive call doesn't reset the position of recursively laid out children, and the offset to align any centered children will be applied twice.

Changing isStyleDimDefined on https://github.com/facebook/css-layout/blob/master/src/Layout.c#L1039 to isLayoutDimDefined mitigates the issue for us, but I'm not sure if the underlying issue of offsets being applied twice is just hidden by this.

@javache
Copy link
Member Author

javache commented Mar 16, 2016

cc @lucasr @jsendros

@alebo
Copy link
Contributor

alebo commented Mar 18, 2016

@javache could you provide a specific layout, please? #127 involves a centered item inside a stretched one and this example works fine in the latest version.

@javache
Copy link
Member Author

javache commented Mar 21, 2016

The critical difference might be that in Layout.js we reset all child layouts here: https://github.com/facebook/css-layout/blame/master/src/Layout.js#L1217-L1223

but in the native versions (.c/.java) we do not do this.

@vjeux
Copy link
Contributor

vjeux commented Mar 21, 2016

@javache: we do reset it but at a different place. Not on a computer right now but it's somewhere in the glue code that we have that's react native specific

@javache
Copy link
Member Author

javache commented Mar 22, 2016

Here's a test that shows this issue: https://gist.github.com/javache/f38f48b85d497da832ec

It works perfectly in JS (running grunt test-javascript) but fails when ran in C (using grunt transpile).

@vjeux we're probably missing that reset then somewhere now the layout is re-entrant for stretch.

@vjeux
Copy link
Contributor

vjeux commented Mar 22, 2016

We reset it here, which is after the entire pass happened: https://github.com/facebook/react-native/blob/master/React/Views/RCTShadowView.m#L160-L170

@javache
Copy link
Member Author

javache commented Mar 25, 2016

Hmm, the Java version of this test does pass, but I don't see any reset of properties happening there either. Any idea why it would be different @vjeux?

edit: Ah, we actually do at the start of layoutNodeImpl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants