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

[YogaKit] Removal of element messes up layout #606

Closed
1 task done
kovpas opened this issue Jul 20, 2017 · 10 comments
Closed
1 task done

[YogaKit] Removal of element messes up layout #606

kovpas opened this issue Jul 20, 2017 · 10 comments

Comments

@kovpas
Copy link

kovpas commented Jul 20, 2017

Report

Issues and Steps to Reproduce

  1. The initial state of the app:
  • A red view with padding = 20
  • A button that adds a label inside the red view

  1. Once you press the button, a label (violet) is added inside the red view:

  1. Once you press the button again, the label gets removed from the red view:

Expected Behavior

State 3 should look exactly like state 1

Actual Behavior

The layout of the state 3 gets messed up. Looks like yoga thinks that the label is still there, also padding gets added twice to the height of the red view.

Link to Code

Demo project is here

@artman
Copy link

artman commented Jul 24, 2017

Facing this issue, too.

@d16r
Copy link
Contributor

d16r commented Jul 26, 2017

@emilsjolander I'm kinda stumped about this one...
This is the correct layout when the Violet button is inserted.

1.{*wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 736.000000 initial
2.{*wm: EXACTLY, hm: AT_MOST, aw: 414.000000 ah: 736.000000 measure
2.}*wm: EXACTLY, hm: AT_MOST, d: (414.000000, 90.000000) measure
2.{wm: EXACTLY, hm: EXACTLY, aw: 414.000000 ah: 90.000000 flex
2.}wm: EXACTLY, hm: EXACTLY, d: (414.000000, 90.000000) flex
2.{[skipped] wm: EXACTLY, hm: EXACTLY, aw: 414.000000 ah: 50.000000 => d: (414.000000, 50.000000) flex
2.{wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 90.000000 stretch
3.{*wm: EXACTLY, hm: EXACTLY, aw: 374.000000 ah: 50.000000 flex
3.}*wm: EXACTLY, hm: EXACTLY, d: (374.000000, 50.000000) flex
3.{[skipped] wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 374.000000 ah: 50.000000 => d: (374.000000, 50.000000) stretch
2.}wm: LAY_EXACTLY, hm: LAY_EXACTLY, d: (414.000000, 90.000000) stretch
2.{[skipped] wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 50.000000 => d: (414.000000, 50.000000) stretch

This is the changes when you remove the violet button. Looks like the container isn't being remeasured.

1.}*wm: LAY_EXACTLY, hm: LAY_EXACTLY, d: (414.000000, 736.000000) initial

1.{*wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 736.000000 initial 2.{*wm: EXACTLY, hm: AT_MOST, aw: 414.000000 ah: 736.000000 measure 2.}*wm: EXACTLY, hm: AT_MOST, d: (414.000000, 130.000000) measure 2.{[skipped] wm: EXACTLY, hm: EXACTLY, aw: 414.000000 ah: 130.000000 => d: (414.000000, 130.000000) flex 2.{[skipped] wm: EXACTLY, hm: EXACTLY, aw: 414.000000 ah: 50.000000 => d: (414.000000, 50.000000) flex 2.{[skipped] wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 130.000000 => d: (414.000000, 130.000000) stretch 2.{[skipped] wm: LAY_EXACTLY, hm: LAY_EXACTLY, aw: 414.000000 ah: 50.000000 => d: (414.000000, 50.000000) stretch 1.}*wm: LAY_EXACTLY, hm: LAY_EXACTLY, d: (414.000000, 736.000000) initial

@emilsjolander
Copy link
Contributor

Perhaps some dirty flag isn't being set correctly? However I would be very surprised if this is a bug in the C code and not the objc bindings as we aren't facing any similar issues on other platforms.

@woehrl01
Copy link
Contributor

@kovpas Does the behaviour change if you both call applyLayout(preservingOrigin: false) (see the false)

@tadeuzagallo
Copy link
Contributor

@woehrl01 Changing preserveOrigin: false doesn't modify the behaviour.

@emilsjolander even if I manually mark all the views as dirty it still doesn't change the layout. The curious bit is that this doesn't happen with React Native, which is using the same bindings, right?

@lucdion
Copy link
Contributor

lucdion commented Jul 27, 2017

This is maybe also related to this issue #603, which is the reversed problem. Adding an element don't relayout its siblings.

@kovpas
Copy link
Author

kovpas commented Jul 27, 2017

@woehrl01 no, as @tadeuzagallo said, nothing changes if I do preserveOrigin: false

@lucdion I saw #603... The reason I decided to fill a separate issue is that I don't really think it's related: In the case of #603, the view hierarchy remains the same, only yoga layout hierarchy is changed (by setting isIncludedInLayout to false).

In my case, view hierarchy changes which leads to the messed up layout.

@d16r
Copy link
Contributor

d16r commented Jul 27, 2017

So, this seems like a UIKit issue. If you set a breakpoint in YGLayout's YGMeasureView function you can see that when we call sizeThatFits: on a plain UIView with no subviews, it is returning a height of 90.

That height of 90 + 40 padding (top and bottom, 20) explains the height of 130. It's unclear why UIView is returning 90 as a height. But it makes it clear that it isn't a yoga problem.

@kovpas
Copy link
Author

kovpas commented Jul 28, 2017

@dshahidehpour according to the documentation, this is intended behavior:

The default implementation of this method returns the existing size of the view.

I think what happens is that when there's the violet label inside the red view, sizeThatFits: gets invoked for this label, and red view's size becomes equal to the label's size. If there are no subviews, UIView's implementation of sizeThatFits: just returns bounds.size.

kovpas pushed a commit to kovpas/yoga that referenced this issue Jul 28, 2017
@kovpas kovpas mentioned this issue Jul 28, 2017
@artman
Copy link

artman commented Apr 14, 2018

Any progress on getting this merged in?

mulle-kybernetik-tv pushed a commit to mulle-kybernetik-tv/yoga that referenced this issue Mar 12, 2019
Summary:
Fixes facebook#606.

If there are no subviews in `UIView`, yoga assumes that `sizeThatFits:` returns `CGSizeZero`. However, according to [the documentation](https://developer.apple.com/documentation/uikit/uiview/1622625-sizethatfits), `UIView` returns current size if there are no subviews.

This diff adds a check - if there are no subviews, `sizeThatFits:` doesn't get called, and CGSizeZero is returned.
Pull Request resolved: facebook#610

Reviewed By: davidaurelio

Differential Revision: D6807406

Pulled By: priteshrnandgaonkar

fbshipit-source-id: 9189cf14c393f840122bc365d3827881bf03548c
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

No branches or pull requests

7 participants