ios: Fix app-unusable bug with "Prefer Cross-Fade Transitions" setting #5163
Conversation
chrisbobbe
changed the title
chrisbobbe
force-pushed
the
pr-keyboard-avoiding-fix
branch
3 times, most recently
from
7fc6250
to
5bce956
Compare
|
Greg and I discussed a while in the office and landed on forking |
chrisbobbe
force-pushed
the
pr-keyboard-avoiding-fix
branch
2 times, most recently
from
1cc9fe9
to
40f8118
Compare
There was a problem hiding this comment.
Thanks @chrisbobbe for getting to the bottom of this! This code looks good; comments below on a comment, and on some license metadata.
| if (keyboardFrame.height === 0) { | ||
| // Empirically, in default environments, the keyboard animation is | ||
| // slide-in and slide-out. The off-screen keyboard frame is a | ||
| // downward translation of the on-screen frame, to just off the | ||
| // screen, with no change in height or width. | ||
| // | ||
| // Empirically, if the UIAccessibilityPrefersCrossFadeTransitions | ||
| // setting is set (see | ||
| // https://developer.apple.com/documentation/uikit/3584818-uiaccessibilitypreferscrossfadet?language=objc) | ||
| // ), then the keyboard fades in and out instead of sliding. In the | ||
| // off-screen state, the keyboard-frame values (`height`, `width`, | ||
| // `screenX`, and `screenY`) all take the special value of zero. | ||
| // | ||
| // So, recognize that special representation as a keyboard-closed | ||
| // state. (We choose `height` for the conditional because it seems | ||
| // least likely to collide with some valid non-off-screen state.) | ||
| // See zulip/zulip-mobile#5162 and facebook/react-native#29974. | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
I think this can explain much more directly why this logic is correct. In particular the correctness of it doesn't depend on those empirical observations (always a bit concerning of a thing to have to rely on) or on very much detail at all.
Then the explanation of why it's needed comes after that. The possibility that it's redundant is lower-stakes than the possibility that it's incorrect, so the reader can be more relaxed about it.
Here's one version:
| if (keyboardFrame.height === 0) { | |
| // Empirically, in default environments, the keyboard animation is | |
| // slide-in and slide-out. The off-screen keyboard frame is a | |
| // downward translation of the on-screen frame, to just off the | |
| // screen, with no change in height or width. | |
| // | |
| // Empirically, if the UIAccessibilityPrefersCrossFadeTransitions | |
| // setting is set (see | |
| // https://developer.apple.com/documentation/uikit/3584818-uiaccessibilitypreferscrossfadet?language=objc) | |
| // ), then the keyboard fades in and out instead of sliding. In the | |
| // off-screen state, the keyboard-frame values (`height`, `width`, | |
| // `screenX`, and `screenY`) all take the special value of zero. | |
| // | |
| // So, recognize that special representation as a keyboard-closed | |
| // state. (We choose `height` for the conditional because it seems | |
| // least likely to collide with some valid non-off-screen state.) | |
| // See zulip/zulip-mobile#5162 and facebook/react-native#29974. | |
| return 0; | |
| } | |
| if (keyboardFrame.height === 0) { | |
| // The keyboard is hidden, and occupies no height. | |
| // | |
| // This condition is needed because in some circumstances when the | |
| // keyboard is hidden we get both `screenY` and `height` (as well as | |
| // `screenX` and `width`) of zero. In particular, this happens when | |
| // the UIAccessibilityPrefersCrossFadeTransitions setting is true, | |
| // i.e. when the user has enabled both "Reduce Motion" and "Prefer | |
| // Cross-Fade Transitions" under Settings > Accessibility > Motion. | |
| // See zulip/zulip-mobile#5162 and facebook/react-native#29974. | |
| } |
(Another thing there is that I think saying how to find the setting in the UI is more helpful than that API docs page; we don't want to have any code looking for that setting in any case, at least not in connection with this logic. The page is easy to find via Google given the identifier, anyway.)
There was a problem hiding this comment.
Oh, and this component is written to be cross-platform, right? We only use it on iOS, but the conditional in componentDidMount is meant to operate on Android too.
So this description of the relevant circumstance should mention iOS. Adding "on iOS" after "In particular, this happens" should do it.
| /** | ||
| * Copyright (c) Facebook, Inc. and its affiliates. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. |
There was a problem hiding this comment.
This should be updated; for one thing, with this file in a different source tree, there is a LICENSE file in the root directory but it's a different license.
Relatedly, we'll want to add an entry in docs/THIRDPARTY.
I can make those edits before merging.
There was a problem hiding this comment.
In the commit message, let's mention the exact RN version, and path in its tree, that this file is copied from. That makes it easy to compare, and also to look for upstream changes in the future.
There was a problem hiding this comment.
I can make those edits before merging.
Cool, thanks!
chrisbobbe
force-pushed
the
pr-keyboard-avoiding-fix
branch
from
40f8118
to
d12e90d
Compare
|
Thanks! Revision pushed.
Great, thank you! |
It's 2021, and we've kept making changes, so update the copyright
years for our own work.
We've also made a number of changes to the redux-persist-migrate
code. Like all our software, we make that work available under
the Apache License, Version 2.0 ("Apache-2"). Because the MIT
license (which the upstream version of that code is provided
under) is compatible with Apache-2, the combination is available
under simply the same Apache-2 license.
There's also some more third-party code for which we forgot to
update this file: in src/third/redux-persist/, added as e5409c5
in 2020-05.
* The upstream for that code has the MIT license, with a
copyright notice "Copyright (c) 2015-present Zack Story".
* The last upstream commit (github:rt2zz/redux-persist@v4.10.2)
for that code is dated 2017, so "present" really means 2017.
* Because again the MIT license is compatible with our Apache-2
license, the new versions with our changes are under the same
Apache-2 license as the rest of our code.
Finally, we deleted vendor/intl/, in 30e4d19 in 2020-08.
From node_modules/react-native/Libraries/Components/Keyboard/KeyboardAvoidingView.js with v0.64.2 of `react-native`, adjusting only the license pointer in the copyright header. We'll make the fork usable soon, not in this pure copy-paste commit. We don't like this implementation (see, e.g., 70eca07, which took a lot of painstaking investigation), but we want to fix zulip#5162 as soon as we can. For a not-quite-perfect attempt at reimplementing it from scratch in our style (and with zulip#5162 fixed), see my branch `reimplement-keyboard-avoiding-view` on GitHub. We can't easily make a jank-free solution in a normal React Native way because we can't have perfect information about the layout on every frame. React Native exposes it to us by consuming event listeners and providing asynchronous query functions, and we end up having to learn about different aspects of the layout at different times. The best hope for a jank-free solution is to use native iOS APIs. If we're feeling adventurous and we find the time, we should really try hard to make React Native play along with iOS's `keyboardLayoutGuide` API (iOS 15+). The first four minutes of this video should be really useful background on the `keyboardLayoutGuide` feature: https://developer.apple.com/videos/play/wwdc2021/10259/ It works within iOS's "Auto Layout" system: https://developer.apple.com/library/archive/documentation/UserExperience/Conceptual/AutolayoutPG/index.html But then we'd have to go and (a) Make a native component: https://reactnative.dev/docs/native-components-ios (b) Figure out how to make React Native's propagate-from-JavaScript layout system (Yoga) not fight with the native iOS layout system, including "Auto Layout" and `keyboardLayoutGuide`. Possibly we can pick up some clues from `react-native-safe-area-context` for this? From another angle, since the jank is regularly seen on screen orientation changes between portrait and landscape, we could consider just unsupporting the landscape orientation. On very common device types, like phones, it's just not as easy to use the app in, especially for composing messages.
Just make the minimal changes necessary to get type-checking working (with a few suppressions, though) and to have the imports make sense.
Fixes: zulip#5162 Co-authored-by: Greg Price <greg@zulip.com>
gnprice
force-pushed
the
pr-keyboard-avoiding-fix
branch
from
d12e90d
to
3b8caa5
Compare
|
OK, and done! Thanks again for fixing this. |
Fork React Native's
KeyboardAvoidingViewand apply a fix.Fixes: #5162