Closed Bug 1182673 Opened 6 years ago Closed 6 years ago
Snapkit Errors on rotation in all devices except i
47 bytes, text/x-github-pull-request
|Details | Review|
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?
Non-fatal I assume?
I would nominate this since conflicting constraints usually manifest themselves as broken layout/ui.
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.
Attachment #8639436 - Flags: review?(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!
Attachment #8639436 - Flags: review?(bnicholson) → review-
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!
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.
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...
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.
Attachment #8640184 - Flags: review?(sleroux) → review+
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).
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.
Attachment #8640738 - Flags: review?(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 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...
Attachment #8640497 - Attachment is obsolete: true
Attachment #8640184 - Attachment is obsolete: true
Comment on attachment 8640738 [details] [review] Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/827 LGTM
Attachment #8640738 - Flags: review?(sleroux) → review+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.