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

AdaptiveLayoutAnchor does not constraint oversized contents #413

Closed
warpling opened this issue Dec 4, 2020 · 10 comments · Fixed by #518
Closed

AdaptiveLayoutAnchor does not constraint oversized contents #413

warpling opened this issue Dec 4, 2020 · 10 comments · Fixed by #518

Comments

@warpling
Copy link
Contributor

warpling commented Dec 4, 2020

Description

When using VC's that are taller than the device window and an AdaptiveLayoutGuide the panel can extend completely of screen making it nearly impossible to dismiss.

Expected behavior

Panels (even those with adaptive layout) should not be able to expand beyond the size of the window.

Actual behavior

The Panel goes way off screen.

Steps to reproduce

I have an example based on the current Adaptive Anchor example I can upload but am curious if there is anything I should try first that will help constraint the panel to a reasonable size :P

@warpling
Copy link
Contributor Author

warpling commented Dec 4, 2020

Here's an example of a UITableView with an intrinsic size much larger than the window that overflows and should instead be contained. https://github.com/warpling/FloatingPanel/tree/AdaptiveOversizeIssue

@scenee
Copy link
Owner

scenee commented Dec 7, 2020

Thank you for your good try in the example. I understand your expectation but I don't think the lib should limit a panel's content size because some users might not want to do it like #349.

Before the lib had a limit of the panel boundary in the same reason as you. But according to the issue, I knew there was a use case which the limit can obstruct and then I got rid of it. Therefore the boundary should be cared by a lib's consumer.

@warpling
Copy link
Contributor Author

warpling commented Dec 7, 2020

Having a panel that extends past the window size instead of using a UIScrollView seems like very niche and non-native behavior to me? Perhaps it would be better as a flag that can be disabled to allow the panel to extend past the window size? With the old fixed anchors it was typical to set the inset from top to 0 to prevent the panel from extending outside of the bounds where it could be reasonably pulled on to be closed? The only way to work around this current (that I could think of) is very awkward and involves looking at the UIWindow's root ViewController's height, insetting its safe areas, etc to adjust a layout constraint every time subviews are laid out? This feels very hacky and wrong.

Even if the current behavior is desired there is a glitch when pulling down on this oversized content to close the panel where the content jumps and the panel does not react for awhile before starting to follow the pan gesture.

@scenee
Copy link
Owner

scenee commented Dec 8, 2020

I am sorry if I might offend you by what I said. I agree the panel of this example is a glitch and should be fixed. However it's just caused by the programer error because the contentSize.height is 2000.0 at this line. The lib's adaptive layout respects codes of the library's consumer expectedly so it's ok.

@warpling
Copy link
Contributor Author

warpling commented Dec 8, 2020

No worries! Sorry if I came across as accusatory.
I'm still a little confused. As far as I understand panels should not extend off screen but instead should always use a UIScrollView to contain content that extends outside of the window bounds. That is why scroll views are tracked so that the library can handle proper dismissals. In this example a very tall UITableView is used and because the panel height is no constrained, AutoLayout lets the intrinsicContentSize.height of UITableView expand to completely fit its contents, this then makes the table view not scrollable and introduces a glitch into the dismissal. If the ViewController's height was constraint with a high priority the table view would also be constrained and would allow its contents to scroll while still trying to expand to fit the space as well as it can.

I would recommend we add a flag to allow consumers to opt out of the behavior where the panel can be extended beyond the opposite side it originates from unless there is an alternative way for consumers to do this that we can document? I could not think of one since any childVC can't constraint itself to the surface area and I was unable to find an easy way to add this to the library since I don't know the internal workings very well.

@scenee
Copy link
Owner

scenee commented Dec 11, 2020

I'm sorry I'm still not able to understand why we should add a flag into the lib. If we would like to prevent the panel is extended beyond the screen size in the example, we can just add the constraint as blow. What's not good in this way?

index a4369a7..e0f6a75 100644
--- a/Examples/Samples/Sources/ViewController.swift
+++ b/Examples/Samples/Sources/ViewController.swift
@@ -1369,6 +1369,7 @@ class ImageViewController: UIViewController, UITableViewDelegate, UITableViewDat

         NSLayoutConstraint.activate([
             tableView.heightAnchor.constraint(lessThanOrEqualTo: view.heightAnchor),
+            stackView.heightAnchor.constraint(lessThanOrEqualToConstant: UIScreen.main.bounds.height),
             guide.topAnchor.constraint(equalTo: stackView.topAnchor),
             guide.leftAnchor.constraint(equalTo: stackView.leftAnchor),
             guide.bottomAnchor.constraint(equalTo: stackView.bottomAnchor),

@warpling
Copy link
Contributor Author

That is what I ended up doing but it seems improper for the childVC to have to know about the UIScreen/UIWindow size, no? Especially considering there can be multiple of them and that this doesn't easily support resizing/rotation because the constraint uses a constant (using UIScreen/UIWindow as an anchor is impossible since it is in a different view hierarchy).

@warpling
Copy link
Contributor Author

I guess I'm mainly failing to see why something like a bottom panel should be able to extend it's top past the window top, considering the way FloatingPanel already supports scrolling to handle contents that are larger than the panel.

@scenee
Copy link
Owner

scenee commented Dec 12, 2020

That is what I ended up doing but it seems improper for the childVC to have to know about the UIScreen/UIWindow size, no? Especially considering there can be multiple of them and that this doesn't easily support resizing/rotation because the constraint uses a constant (using UIScreen/UIWindow as an anchor is impossible since it is in a different view hierarchy).

That is true of the lib, right? In addition, the lib will have to take care of all expectations the boundary, not just UIScreen or UIWindow and also ones on the device rotation and scene resizing. For example, some users might expect the boundary should be FloatingPanelController.view's bounds. In my experience on the lib, the lib should not assume that because it's impossible for us to know all of them.

Also the lib doesn't not supports just a bottom panel, but also top, left and right positioned panel. For now the lib consumers are easy to define a boundary of a panel as they want.

Thank you for this discussion. I would like to close this issue.

@scenee scenee closed this as completed Dec 12, 2020
@warpling
Copy link
Contributor Author

Bottom panel was just an example since it's hard to talk about all of them abstractly ;)

For future record what is the best practice for this? I don't think the VC should have to make these constraints since the VC might be presented modally one place, in a popover another place, in a FloatingPanel in some other, etc. Should these constraints be applied to the SurfaceView?

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.

2 participants