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

New Issue

2016-03-16T07:59:43-07:00

javache commented

2016-03-16 07:59:43 -07:00

(Migrated from github.com)

Seeing this issues after pulling in https://github.com/facebook/css-layout/pull/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.

Seeing this issues after pulling in https://github.com/facebook/css-layout/pull/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 commented

2016-03-16 08:00:19 -07:00

(Migrated from github.com)

cc @lucasr @jsendros

alebo commented

2016-03-18 16:37:35 -07:00

(Migrated from github.com)

@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 commented

2016-03-21 16:36:02 -07:00

(Migrated from github.com)

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.

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 commented

2016-03-21 16:48:31 -07:00

(Migrated from github.com)

@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 commented

2016-03-21 17:04:06 -07:00

(Migrated from github.com)

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.

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 commented

2016-03-21 17:53:45 -07:00

(Migrated from github.com)

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 commented

2016-03-25 12:49:28 -07:00

(Migrated from github.com)

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.

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 in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#175