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

Presenting in popover on iOS 13 displays with incorrect insets #68

Open
colinhumber opened this issue Aug 19, 2019 · 7 comments
Open

Presenting in popover on iOS 13 displays with incorrect insets #68

colinhumber opened this issue Aug 19, 2019 · 7 comments

Comments

@colinhumber
Copy link
Contributor

colinhumber commented Aug 19, 2019

On iOS 13, the view that's displayed within a popover now extends to include the popover arrow. As a result, additional safeAreaInsets are applied to include the arrow.

When presenting an AloeStackView in a popover with the arrow pointing left or right, this currently isn't being taken into account. As a result, the cell insets (content and separator) are too close to the edges. I've tracked this down to insetsLayoutMarginsFromSafeArea = false being partially the culprit, as well as the separator leading/trailing constraints not taking the safe area into account.

I haven't seen any visual issues in the sample app, or my own projects with insetsLayoutMarginsFromSafeArea enabled, so I'm not sure if there is a specific reason for this being disabled in the Airbnb app. When I test this out with the default value set (true), everything works as expected (tested on iOS 11, 12, and 13).

I've included a screenshot of the issue on iOS 13. As you can see, the content is too close to the left side of the popover.
image

With insetsLayoutMarginsFromSafeArea enabled and the separators taking the safe area into account, everything looks as expected (tested on iOS 11, 12, and 13).
image

Ignore the switch cell extending off the right hand side. That's not an issue with the stack view, just some internal constraints in one of the sample cells.

@colinhumber
Copy link
Contributor Author

colinhumber commented Aug 19, 2019

This could certainly be fixed in a custom StackViewCell subclass. Mostly curious if this is something that could be included in the base implementation without needed to implement a custom cell class.

@colinhumber
Copy link
Contributor Author

Playing around with this some more, since the separator view and constraints are all private, fixing this in a custom cell subclass would require a bunch of messy code and workarounds.

@colinhumber
Copy link
Contributor Author

I was able to get this sorted by responding to safeAreaInsetsDidChange in a custom AloeStackView subclass and adjusting each row's separator and row inset to adjust for the new safeAreaInsets.

Since this is likely a common issue in popovers on iOS 13, it would be awesome if this was built into the base class. Any feedback on this would be great and I'd be happy to work on a fix.

@apang42
Copy link
Collaborator

apang42 commented Aug 29, 2019

hey @colinhumber thanks for reporting this! We'll take a look at this - if you could share some of your code for a fix, we'll get back to you with a proposed solution for this issue.

@colinhumber
Copy link
Contributor Author

colinhumber commented Aug 29, 2019

Awesome!

This fixes it if the fix is directly within StackViewCell.

// this could be removed, since insetsLayoutMarginsFromSafeArea = false is the default 
if #available(iOS 11.0, *) {
  insetsLayoutMarginsFromSafeArea = true
}

private func setUpSeparatorViewConstraints() {
  if #available(iOS 11.0, *) {
    separatorTopConstraint = separatorView.topAnchor.constraint(equalTo: safeAreaLayoutGuide.topAnchor)
    separatorBottomConstraint = separatorView.bottomAnchor.constraint(equalTo: safeAreaLayoutGuide.bottomAnchor)
    separatorLeadingConstraint = separatorView.leadingAnchor.constraint(equalTo: safeAreaLayoutGuide.leadingAnchor)
    separatorTrailingConstraint = separatorView.trailingAnchor.constraint(equalTo: safeAreaLayoutGuide.trailingAnchor)
  } else {
    separatorTopConstraint = separatorView.topAnchor.constraint(equalTo: topAnchor)
    separatorBottomConstraint = separatorView.bottomAnchor.constraint(equalTo: bottomAnchor)
    separatorLeadingConstraint = separatorView.leadingAnchor.constraint(equalTo: leadingAnchor)
    separatorTrailingConstraint = separatorView.trailingAnchor.constraint(equalTo: trailingAnchor)

  }
}

Since the separator constraints are private, I wasn't able to fix it in a StackViewCell subclass, so I created an AloeStackView subclass to handle it instead. This fix assumes all rows are added upfront before any safeAreaInsets would change.

class SafeAreaAdjustingStackView: AloeStackView {
  @available(iOS 11.0, *)
  public override func safeAreaInsetsDidChange() {
    super.safeAreaInsetsDidChange()

    getAllRows().forEach {
      guard let cell = $0.superview as? StackViewCell else { return }

      // adjust separator inset for the new safe area
      let currentSeparatorInset = cell.separatorInset
      let adjustedSeparatorInset = UIEdgeInsets(top: currentSeparatorInset.top,
                                                left: currentSeparatorInset.left + safeAreaInsets.left,
                                                bottom: currentSeparatorInset.bottom,
                                                right: currentSeparatorInset.right + safeAreaInsets.right)
      setSeparatorInset(forRow: $0, inset: adjustedSeparatorInset)

      // adjust row inset for the new safe area
      let currentRowInset = cell.rowInset
      let adjustedRowInset = UIEdgeInsets(top: currentRowInset.top,
                                          left: currentRowInset.left + safeAreaInsets.left,
                                          bottom: currentRowInset.bottom,
                                          right: currentRowInset.right + safeAreaInsets.right)
      setInset(forRow: $0, inset: adjustedRowInset)
    }
  }
}

@colinhumber
Copy link
Contributor Author

@apang42 Hey! Just wanted to follow up and see if there had been any internal discussions about this. Thanks!

@colinhumber
Copy link
Contributor Author

Any updates on this fix?

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

2 participants