DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

YogaKit doesn't remember the origin of the root view. #378

New Issue
Closed
opened 2017-02-07 07:28:43 -08:00 by d16r · 19 comments
d16r commented 2017-02-07 07:28:43 -08:00 (Migrated from github.com)
Copy Link

Report

When you apply layout to a root view of layout with an origin that is not CGPointZero, in some scenarios it seems that this would be the incorrect behavior.

Issues and Steps to Reproduce

  1. Clone or download this
  2. Go into YogaKit directory and run pod install.
  3. Open the workspace and run the app.
  4. Click the last cell in table view.
  5. Click the button at the bottom of the screen.

Expected Behavior

The bordered view should stay on screen (ideally).

Actual Behavior

The bordered view's .frame.origin is set to (0,0).

# Report When you apply layout to a root view of layout with an origin that is not `CGPointZero`, in some scenarios it seems that this would be the incorrect behavior. - [x] I have searched [existing issues](https://github.com/facebook/yoga/issues) and this is not a duplicate # Issues and Steps to Reproduce 1. Clone or download [this](https://github.com/dshahidehpour/yoga/tree/space_repro) 2. Go into `YogaKit` directory and run `pod install`. 3. Open the workspace and run the app. 4. Click the last cell in table view. 5. Click the button at the bottom of the screen. # Expected Behavior The bordered view should stay on screen (ideally). # Actual Behavior The bordered view's .frame.origin is set to (0,0). ![](https://media.giphy.com/media/l3q2CaaUj6CvvY9d6/giphy.gif)
d16r commented 2017-02-07 07:29:32 -08:00 (Migrated from github.com)
Copy Link

cc @hartbit Have you run into this? I'm trying to decide if there is a better way to fix this, other than requiring users to manually set the inset on a yoga view.

cc @hartbit Have you run into this? I'm trying to decide if there is a better way to fix this, other than requiring users to manually set the inset on a yoga view.
woehrl01 commented 2017-02-07 10:15:00 -08:00 (Migrated from github.com)
Copy Link

@dshahidehpour does this problem persist if you use some thing different thanspace-around on the root node? My theory is that on the second click the root height is diffierent/bigger. And so you position the blue square at a lower position/centered.

@dshahidehpour does this problem persist if you use some thing different than```space-around``` on the root node? My theory is that on the second click the root height is diffierent/bigger. And so you position the blue square at a lower position/centered.
d16r commented 2017-02-07 10:36:41 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 let me spit out some debug info for you.

Here's the output when the screen loads (before I hit the button):

 1.{*wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 736.000000 initial
  2.{*wm: AT_MOST, hm: AT_MOST, aw: 414.000000 ah: 736.000000 measure
  2.}*wm: AT_MOST, hm: AT_MOST, d: (106.000000, 30.000000) measure
  2.{*wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 300.000000 flex
  2.}*wm: LAY_EXACTLY, hm: LAY_EXACTLY, d: (414.000000, 300.000000) flex
  2.{[skipped] wm: LAY_AT_MOST, hm: LAY_EXACTLY, aw: 414.000000 ah: 30.000000 => d: (106.000000, 30.000000) flex
 1.}*wm: LAY_EXACTLY, hm: LAY_EXACTLY, d: (414.000000, 736.000000) initial
{layout: {width: 414, height: 736, top: 0, left: 0}, flexDirection: 'column', justifyContent: 'space-around', alignItems: 'stretch', flexGrow: 0, flexShrink: 0, overflow: 'visible', children: [
  {layout: {width: 414, height: 300, top: 101.5, left: 0}, flexDirection: 'column', alignItems: 'stretch', flexGrow: 0, flexShrink: 0, overflow: 'visible', width: 414px, height: 300px, },
  {layout: {width: 106, height: 30, top: 604.5, left: 154}, flexDirection: 'column', alignItems: 'stretch', alignSelf: 'center', flexGrow: 0, flexShrink: 0, overflow: 'visible', },
]},

Here's the output when I tap the button (and the root view shifts):

 1.{wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 672.000000 initial
  2.{[skipped] wm: AT_MOST, hm: AT_MOST, aw: 414.000000 ah: 672.000000 => d: (106.000000, 30.000000) measure
  2.{[skipped] wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 300.000000 => d: (414.000000, 300.000000) flex
  2.{[skipped] wm: LAY_AT_MOST, hm: LAY_EXACTLY, aw: 414.000000 ah: 30.000000 => d: (106.000000, 30.000000) flex
 1.}wm: LAY_EXACTLY, hm: LAY_EXACTLY, d: (414.000000, 672.000000) initial
{layout: {width: 414, height: 672, top: 0, left: 0}, flexDirection: 'column', justifyContent: 'space-around', alignItems: 'stretch', flexGrow: 0, flexShrink: 0, overflow: 'visible', children: [
  {layout: {width: 414, height: 300, top: 85.5, left: 0}, flexDirection: 'column', alignItems: 'stretch', flexGrow: 0, flexShrink: 0, overflow: 'visible', width: 414px, height: 300px, },
  {layout: {width: 106, height: 30, top: 556.5, left: 154}, flexDirection: 'column', alignItems: 'stretch', alignSelf: 'center', flexGrow: 0, flexShrink: 0, overflow: 'visible', },
]},

So yeah it looks like the computed height is changing from 736 to 672.

@woehrl01 let me spit out some debug info for you. Here's the output when the screen loads (before I hit the button): ``` 1.{*wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 736.000000 initial 2.{*wm: AT_MOST, hm: AT_MOST, aw: 414.000000 ah: 736.000000 measure 2.}*wm: AT_MOST, hm: AT_MOST, d: (106.000000, 30.000000) measure 2.{*wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 300.000000 flex 2.}*wm: LAY_EXACTLY, hm: LAY_EXACTLY, d: (414.000000, 300.000000) flex 2.{[skipped] wm: LAY_AT_MOST, hm: LAY_EXACTLY, aw: 414.000000 ah: 30.000000 => d: (106.000000, 30.000000) flex 1.}*wm: LAY_EXACTLY, hm: LAY_EXACTLY, d: (414.000000, 736.000000) initial {layout: {width: 414, height: 736, top: 0, left: 0}, flexDirection: 'column', justifyContent: 'space-around', alignItems: 'stretch', flexGrow: 0, flexShrink: 0, overflow: 'visible', children: [ {layout: {width: 414, height: 300, top: 101.5, left: 0}, flexDirection: 'column', alignItems: 'stretch', flexGrow: 0, flexShrink: 0, overflow: 'visible', width: 414px, height: 300px, }, {layout: {width: 106, height: 30, top: 604.5, left: 154}, flexDirection: 'column', alignItems: 'stretch', alignSelf: 'center', flexGrow: 0, flexShrink: 0, overflow: 'visible', }, ]}, ``` Here's the output when I tap the button (and the root view shifts): ``` 1.{wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 672.000000 initial 2.{[skipped] wm: AT_MOST, hm: AT_MOST, aw: 414.000000 ah: 672.000000 => d: (106.000000, 30.000000) measure 2.{[skipped] wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 300.000000 => d: (414.000000, 300.000000) flex 2.{[skipped] wm: LAY_AT_MOST, hm: LAY_EXACTLY, aw: 414.000000 ah: 30.000000 => d: (106.000000, 30.000000) flex 1.}wm: LAY_EXACTLY, hm: LAY_EXACTLY, d: (414.000000, 672.000000) initial {layout: {width: 414, height: 672, top: 0, left: 0}, flexDirection: 'column', justifyContent: 'space-around', alignItems: 'stretch', flexGrow: 0, flexShrink: 0, overflow: 'visible', children: [ {layout: {width: 414, height: 300, top: 85.5, left: 0}, flexDirection: 'column', alignItems: 'stretch', flexGrow: 0, flexShrink: 0, overflow: 'visible', width: 414px, height: 300px, }, {layout: {width: 106, height: 30, top: 556.5, left: 154}, flexDirection: 'column', alignItems: 'stretch', alignSelf: 'center', flexGrow: 0, flexShrink: 0, overflow: 'visible', }, ]}, ``` So yeah it looks like the computed height is changing from 736 to 672.
d16r commented 2017-02-07 10:37:35 -08:00 (Migrated from github.com)
Copy Link

I guess I'm surprised that simply recalculating the layout of the root view without making any changes to the YGNodeRef causes the view to change it's height.

I guess I'm surprised that simply recalculating the layout of the root view without making any changes to the `YGNodeRef` causes the view to change it's height.
woehrl01 commented 2017-02-07 11:22:04 -08:00 (Migrated from github.com)
Copy Link

@dshahidehpour not sure, as I'm not that familiar with YogaKit, but as the difference is 64, this looks to me like the height of the button which isn't explicitly set. I experienced on other ui frameworks, that the height is sometimes only predictable after the ui has been visible the first time. Do you have the same problem if you set the buttons height to a fixed value?

@dshahidehpour not sure, as I'm not that familiar with YogaKit, but as the difference is 64, this looks to me like the height of the button which isn't explicitly set. I experienced on other ui frameworks, that the height is sometimes only predictable after the ui has been visible the first time. Do you have the same problem if you set the buttons height to a fixed value?
hartbit commented 2017-02-07 13:43:34 -08:00 (Migrated from github.com)
Copy Link

@dshahidehpour Sounds weird to me. I don't have time to check it out this evening, I want to get the percentage value PR posted, but have you tried nailing down the problem to the button's sizeThatFits returning different values?

@dshahidehpour Sounds weird to me. I don't have time to check it out this evening, I want to get the percentage value PR posted, but have you tried nailing down the problem to the button's `sizeThatFits` returning different values?
d16r commented 2017-02-09 06:59:37 -08:00 (Migrated from github.com)
Copy Link

@woehrl01, @hartbit It's not the button. When I hit the "reapply layout" button, it's not trying to remeasure the button.

Here's my debug:

> ViewController is Loading
> The measured size of <UIButton: 0x7fd94e5038c0; frame = (0 0; 0 0); opaque = NO; layer = <CALayer: 0x60000003b580>> is 106.00x30.00
> Reapply Button was tapped.

Notice that when I tap the button, it's not remeasuring that node.

@woehrl01, @hartbit It's not the button. When I hit the "reapply layout" button, it's not trying to remeasure the button. Here's my debug: ``` > ViewController is Loading > The measured size of <UIButton: 0x7fd94e5038c0; frame = (0 0; 0 0); opaque = NO; layer = <CALayer: 0x60000003b580>> is 106.00x30.00 > Reapply Button was tapped. ``` Notice that when I tap the button, it's not remeasuring that node.
d16r commented 2017-02-09 07:33:55 -08:00 (Migrated from github.com)
Copy Link

So here's the problem:

The frame of the my rootView is {.origin = {0, 64}, .size = {414, 672}}. When I reapply the layout, we only pass in the size {414, 672}. As a result, when it computes the layout, it had no idea that the rootView's origin was set outside of YogaKit, so it sets the new frame to {0, 0, 414, 672}.

I'm thinking that we should add some functionality so that we can choose to preserve the origin of the root of layout. I'll put up a PR for this.

So here's the problem: The frame of the my rootView is `{.origin = {0, 64}, .size = {414, 672}}`. When I reapply the layout, we only pass in the size `{414, 672}`. As a result, when it computes the layout, it had no idea that the rootView's origin was set outside of YogaKit, so it sets the new frame to `{0, 0, 414, 672}`. I'm thinking that we should add some functionality so that we can choose to preserve the origin of the root of layout. I'll put up a PR for this.
woehrl01 commented 2017-02-09 07:37:41 -08:00 (Migrated from github.com)
Copy Link

@dshahidehpour Oh, I see! It overrides the predefined position. Couldn't this be solved by setting the root view with top: 64 (yoga) ?

@dshahidehpour Oh, I see! It overrides the predefined position. Couldn't this be solved by setting the root view with ```top: 64``` (yoga) ?
d16r commented 2017-02-09 07:47:26 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 Yeahp, that would solve it. Another way is to create a "wrapper" view that goes inside the root view. Then suddenly the coordinate frame of the wrapper view has no idea the rootView has a predefined offset.

I'm also thinking about updating the applyLayout() function so that it takes a parameter that allows you apply the predefined origin to the root. What do you think about @hartbit?

@woehrl01 Yeahp, that would solve it. Another way is to create a "wrapper" view that goes inside the root view. Then suddenly the coordinate frame of the wrapper view has no idea the rootView has a predefined offset. I'm also thinking about updating the `applyLayout()` function so that it takes a parameter that allows you apply the predefined origin to the root. What do you think about @hartbit?
hartbit commented 2017-02-09 08:10:01 -08:00 (Migrated from github.com)
Copy Link

Is there a case where we do NOT want it to preserve the root view's origin? It might be better to fix it by always preserving the origin.

On 9 Feb 2017, at 16:47, Dustin Shahidehpour notifications@github.com wrote:

@woehrl01 Yeahp, that would solve it. Another way is to create a "wrapper" view that goes inside the root view. Then suddenly the coordinate frame of the wrapper view has no idea the rootView has a predefined offset.

I'm also thinking about updating the applyLayout() function so that it takes a parameter that allows you apply the predefined origin to the root. What do you think about @hartbit?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Is there a case where we do NOT want it to preserve the root view's origin? It might be better to fix it by always preserving the origin. > On 9 Feb 2017, at 16:47, Dustin Shahidehpour <notifications@github.com> wrote: > > @woehrl01 Yeahp, that would solve it. Another way is to create a "wrapper" view that goes inside the root view. Then suddenly the coordinate frame of the wrapper view has no idea the rootView has a predefined offset. > > I'm also thinking about updating the applyLayout() function so that it takes a parameter that allows you apply the predefined origin to the root. What do you think about @hartbit? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub, or mute the thread. >
d16r commented 2017-02-09 08:13:21 -08:00 (Migrated from github.com)
Copy Link

I can't think of a use-case at the moment. I'll put up a PR, and we can see what happens.

I can't think of a use-case at the moment. I'll put up a PR, and we can see what happens.
d16r commented 2017-02-09 09:44:14 -08:00 (Migrated from github.com)
Copy Link

Diff coming, I'm going to close this.

Diff coming, I'm going to close this.
woehrl01 commented 2017-02-09 22:35:45 -08:00 (Migrated from github.com)
Copy Link

@dshahidehpour I looked at your diff and it seems like if you now set top via yoga, you will move it down every time you call applyLayout. As we add the position to the previous position.

@dshahidehpour I looked at your diff and it seems like if you now set top via yoga, you will move it down every time you call applyLayout. As we add the position to the previous position.
d16r commented 2017-02-10 08:16:58 -08:00 (Migrated from github.com)
Copy Link

@woehrl01 That's totally true. That being said there are ways around fixing that (just like there were for the original problem). Overall, I felt like this behavior will solve the most amount of bugs on iOS pertaining to root views on iOS. But if seems to persist as a problem, I'll definitely revisit it.

@woehrl01 That's totally true. That being said there are ways around fixing that (just like there were for the original problem). Overall, I felt like this behavior will solve the most amount of bugs on iOS pertaining to root views on iOS. But if seems to persist as a problem, I'll definitely revisit it.
woehrl01 commented 2017-02-10 08:31:32 -08:00 (Migrated from github.com)
Copy Link

@dshahidehpour maybe we should add a comment to applyLayout which documents this?

@dshahidehpour maybe we should add a comment to ```applyLayout``` which documents this?
👍 1
d16r commented 2017-02-10 08:49:52 -08:00 (Migrated from github.com)
Copy Link

I was also thinking of making the preservation optional

[view.yoga applyLayoutWithPreservedOrigin];
[view.yoga applyLayout];

OR

[view.yoga applyLayout:YES]
// [view.yoga applyLayout:NO] looks really weird tho

Swift could be view.yoga.applyLayout(preverseOrigin: true);

I was also thinking of making the preservation optional ``` [view.yoga applyLayoutWithPreservedOrigin]; [view.yoga applyLayout]; ``` OR ``` [view.yoga applyLayout:YES] // [view.yoga applyLayout:NO] looks really weird tho ``` Swift could be `view.yoga.applyLayout(preverseOrigin: true)`;
woehrl01 commented 2017-02-10 09:06:22 -08:00 (Migrated from github.com)
Copy Link

I'd prefer the first, it's the most descriptive one and not shorter than the swift version, but also applies to obj-c.

I'd prefer the first, it's the most descriptive one and not shorter than the swift version, but also applies to obj-c.
hartbit commented 2017-02-10 10:48:33 -08:00 (Migrated from github.com)
Copy Link

[view.yoga applyLayoutPreservingOrigin:YES];
view.yoga.applyLayout(preservingOrigin: true)

If you want to follow Swift naming conventions by making it more grammatically correct.

On 10 Feb 2017, at 17:49, Dustin Shahidehpour notifications@github.com wrote:

I was also thinking of making the preservation optional

[view.yoga applyLayoutWithPreservedOrigin];
[view.yoga applyLayout];
OR

[view.yoga applyLayout:YES]
// [view.yoga applyLayout:NO] looks really weird tho
Swift could be view.yoga.applyLayout(preverseOrigin: true);

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[view.yoga applyLayoutPreservingOrigin:YES]; view.yoga.applyLayout(preservingOrigin: true) If you want to follow Swift naming conventions by making it more grammatically correct. > On 10 Feb 2017, at 17:49, Dustin Shahidehpour <notifications@github.com> wrote: > > I was also thinking of making the preservation optional > > [view.yoga applyLayoutWithPreservedOrigin]; > [view.yoga applyLayout]; > OR > > [view.yoga applyLayout:YES] > // [view.yoga applyLayout:NO] looks really weird tho > Swift could be view.yoga.applyLayout(preverseOrigin: true); > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub, or mute the thread. >
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
YogaKit
bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#378
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?