1182673 - Snapkit Errors on rotation in all devices except iPhone 6

Status: RESOLVED FIXED

(Firefox for iOS :: Browser, defect)

Description

[Bryan Munar (:bkmunar)]

1. navigate to a web page
2. rotate the phone and there a whole bunch of complaints about conflicting constraints from Snapkit
3. worth trying to resolve/fix?

Comment 1

[Aaron Train [:aaronmt]]

Non-fatal I assume?

Comment 2

[Stephan Leroux [:sleroux]]

I would nominate this since conflicting constraints usually manifest themselves as broken layout/ui.

Comment 3

[Stephan Leroux [:sleroux]]

Attachment: PR https://github.com/mozilla/firefox-ios/pull/811

Spent time time trying to resolve the constraint error but couldn't find the exact issue was besides it being between the lock image view/text field inside BrowserLocationView. I just converted it to frame layout instead and it works fine for now. I feel like this might be a deep rooted issue with the way we're setting up constraints at the BrowserViewController/URLBarView level and I didn't really want to change that at this point.

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 8639436 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/811

I'd really like to avoid switching to frames as a workaround if possible -- it feels like we're digging ourselves deeper into a hole. It's definitely frustrating trying to iron out these layout issues, but band-aid fixes just create technical debt for the future, making it harder for ourselves later as the layout grows more complex.

I spent a lot of yesterday and this morning looking into this, and I think I know what's going on. When we rotate, the toolbar layout is updated immediately before the new orientation is set. That means the back/forward/reload/etc. buttons are briefly crammed into the top URL bar before rotating, so the the text field doesn't fit with all of the fixed-width images around it (note that this error only appears if the reader *and* lock icons are showing).

I'm not yet sure of the best fix, though. The gist here partially fixes the issue:

https://gist.github.com/thebnich/eafbba0acd9a7eab4c25

The errors no longer appear for portrait -> landscape transition, but still happen for landscape -> portrait (I think it's because it waits longer to do the constraint changes, which helps us when the URL bar gets bigger, but hurts us when it gets smaller while the toolbar views are showing).

Hopefully this helps -- I can keep digging for a proper fix if you want!

Comment 5

[Stephan Leroux [:sleroux]]

Good catch - that's probably what's happening. I would much rather go with sticking with Autolayout as well. Feel free to take this one to dig into if you want!

Comment 6

[Stephan Leroux [:sleroux]]

Another trick that might help but might involve a bit more code is instead of removing/replacing constraints, we can keep the same constraints all the time but update the value of the button widths to 0 when we want them hidden. I find a lot of the times you can get away with a set of static constraints and just update their values as needed.

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/821

Ugh, this bug was a bear to fix, and I'm still not sure I'm happy with the result. I think my initial guess was correct, but I couldn't figure a clean way to do this. Basically, when transitioning from portrait -> landscape, we need to wait until the rotation has happened before updating the toolbar (since updating will show the toolbar, and we can't show it in portrait), but when transitioning from landscape -> portrait, we need to update it before the rotation (since updating will hide the toolbar, and we can't show it in portrait).

Happy to hear any ideas for improving this. The Apple-recommended way is to use multiple view controllers to avoid all these conditional layouts, though that wouldn't be very straightforward either...

Comment 8

[Stephan Leroux [:sleroux]]

Attachment: patch.diff

Figured out a way to avoid it to complain in a clean way. The problem is that when we call setShowToolbars, we're not just calling setNeedsUpdateConstraints but we're forcing it to updateConstraintsIfNeeded right after. Forcing these updates to occur is what's messing up the timing. Instead, we can call self.urlBar.layoutIfNeeded within the willTransition coordinator's animation block to tell the system to relayout the changes during the animation. If we just call setNeedsUpdateConstraints and take out the updateConstraintIfNeeded, the layoutIfNeeded call with update the constraints for us alongside the animation. I've attached a .diff file you can apply with the changes if you want.

Besides that, the rest of the patch looks good. I like that we're moving to more static constraints with updates instead of remaking the constraints each time.

Comment 9

[Brian Nicholson (:bnicholson)]

Hm, I tried the given patch, but I'm still seeing the SnapKit errors. Testing on an iPhone 5s on mozilla.org (where both the lock and reader icons are active).

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/827

What do you think of this alternate fix? By using lessThanOrEqualTo, we can set a size without strictly enforcing it when it doesn't fit. This keeps setShowToolbar simple without having to forward the coordinator, which I really didn't like.

Comment 11

[Stephan Leroux [:sleroux]]

Comment on attachment 8640738 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/827

Ah you know what - I was testing on an iPhone 6 :( I like this approach better than that one before but I was able to find a strange bug that made the back/refresh buttons result in a 0 width on landscape.

STR:

1. Navigate to mozilla.org
2. Rotate to landscape
3. Tap url bar to edit
4. Tap empty area in top sites to hide keyboard
5. Rotate to portrait
6. Navigate to mozilla.org from top site tile
7. Rotate to landscape

Expected: 

Toolbar buttons appear to the left of the url bar

Actual:

URL bar extends to the left edge of the screen

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 8640738 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/827

Nice catch -- thanks for those STR! Looks like the backButtonLeftConstraint was causing some issues. I removed it and don't see any regressions from it, but let me know if we still need it for some reason...

Comment 13

[Stephan Leroux [:sleroux]]

Comment on attachment 8640738 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/827

LGTM

Comment 14

[Brian Nicholson (:bnicholson)]

https://github.com/mozilla/firefox-ios/commit/be497a3d40cdae23d6fdfd9e36773d5f7b072411