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

disable keyboard controller in the composer screen #4399

Merged
merged 7 commits into from
Jun 6, 2024

Conversation

haileyok
Copy link
Contributor

@haileyok haileyok commented Jun 6, 2024

Why

See https://kirillzyusko.github.io/react-native-keyboard-controller/blog/set-enabled

It appears that on some devices, the react-native-keyboard-controller library is setting some padding on the bottom of the composer. This breaks the composer, for example as seen here:

image

Instead, what we can do is enable it in the app, but specifically disable it in this screen. If we do that, this is the result:

image

Test Plan

  • Test on simulator, medium device, API level 28 ✅
  • Test on device one (personal) ✅
  • Test on device two (personal) ✅

Copy link

render bot commented Jun 6, 2024

Copy link

github-actions bot commented Jun 6, 2024

Old size New size Diff
7.39 MB 7.39 MB 127 B (0.00%)

src/view/com/composer/Composer.tsx Outdated Show resolved Hide resolved
@haileyok haileyok merged commit 885ad2c into main Jun 6, 2024
6 checks passed
@kirillzyusko
Copy link
Contributor

Hello, maintainer and creator of react-native-keyboard-controller is here 👋

I believe I fixed a very similar problem in kirillzyusko/react-native-keyboard-controller#207 (a year ago), but maybe your problem is slightly different than a mentioned one 🤔

Anyway, since your codebase is open source I can try to tackle this problem 👀 Can we have a chat in discord so you can answer on my questions (if I have them) on how to setup a project/which credentials to use/etc.?

@@ -129,6 +130,17 @@ export const ComposePost = observer(function ComposePost({
const {closeAllDialogs} = useDialogStateControlContext()
const t = useTheme()

// Disable this in the composer to prevent any extra keyboard height being applied.
// See https://github.com/bluesky-social/social-app/pull/4399
const {setEnabled} = useKeyboardContext()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small hint - for toggling enabled/disabled states it's better to use useKeyboardController hook. Was there any reasons why you preferred to consume context directly?

I mean, it's not forbidden and eventually useKeyboardController is also a simple context consumer, but consuming context directly it's kind of a last resort and was designed to be used in class-components (and in general useKeyboardContext is not mentioned in API section, so it can be a subject to internal changes). For functional components I think it would be better to use useKeyboardController hook which has a defined area of responsibility (toggling enabled/disabled state).

But let me know if I'm missing something important and in which cases better to consume a context 🙌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's good to know! We ran into some nasty problems yesterday and had to get something through quickly. I had been toying around with the library the other day and knew about the context but wasn't aware of the useKeyboardController hook itself 😅. Thanks for catching that! Have not tried it yet but I assume it works the same way according to the docs 🙌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched this here, thanks for the tip 🙏 a5434e9

@haileyok
Copy link
Contributor Author

haileyok commented Jun 7, 2024

Hi @kirillzyusko! We'd love it if you would like to take a look! Regarding credentials, you can create an account at https://bsky.app, no need for a phone number or anything. You can send me a message on Discord too (haileyok is my username) if you'd like.

Is there anything specific that would help for you to understand? This problem originally came up whenever we switched from using a bottom sheet to a RN Modal for the composer input. On iOS, this works great with the keyboard controller enabled, though on Android we would see some devices - such as this emulator - where it looks like an additional keyboard's worth of height is being added :(

It does appear that maybe Android is adding that padding, though we're not sure. Additionally, the autoFocus seems to be a bit wonky after we moved to using Modal, though that looks more like an RN specific problem than something going on in the library, but would love to know your input on that too!

@kirillzyusko
Copy link
Contributor

kirillzyusko commented Jun 10, 2024

This problem originally came up whenever we switched from using a bottom sheet to a RN Modal for the composer input.

Yeah, keyboard-controller doesn't work well with Modals on Android yet 😔 Basically the problem comes with the fact, that in native view hierarchy Modals are not attached to app rootView and instead they create they own rootView and has own window instance. So you have to track when Modal gets shown and attach callbacks to a new rootView, move new window to edge-to-edge mode. But the problem is that RN encapsulated everything pretty well and it's hard to get a necessary Modal view/window. I found one way, and on paper it was working quite good, but on fabric architecture it wasn't working.

I'll clone your app and see what's the root cause in your app. And yeah, probably I need to allocate some time and finally fix the problem with Modals, because it bothers a lot of devs 😀

the autoFocus seems to be a bit wonky after we moved to using Modal, though that looks more like an RN specific problem than something going on in the library, but would love to know your input on that too!

Yeah, I don't think keyboard-controller somehow intercepts that, so most likely it's a problem of RN (I remember autoFocus always worked not very reliably - at least I always experienced problems with it)

@kirillzyusko
Copy link
Contributor

So the problem comes from the fact, that when Modal is shown - edge-to-edge mode is not applied automatically to this view (since it lives in its own hierarchy). The view gets automatically resized (RN specifies adjustResize behavior to modals by default) + callbacks on API < 30 are fired (because prior to Android 11 the keyboard movement is actually emulated).

Because of these two facts we apply double padding (on API > 29 it doesn't happen, because keyboard movement is not emulated and callbacks are not getting fired, so we have only one padding applied by adjustResize automatically by OS). I fixed the integration with modals in kirillzyusko/react-native-keyboard-controller#466 but there are still some open issues that needs to be resolved before merging. With this fix in place Composer screen looks like:

image

I hope I can merge and publish a new version soon 🤞

@haileyok
Copy link
Contributor Author

Really appreciate you looking into this so quickly! We've reverted this modal on Android for now, but definitely would love to use it if you're able to get it working. Thanks for all this info, amazing! 🙌

@kirillzyusko
Copy link
Contributor

@haileyok slightly late, but it's better than never I believe 😅

I fixed a problem of Modal integration - basic integration is available starting from 1.13 and a more robust solution (which works better and handles more edge-cases) is published under 1.14.1. Would you mind to try it out and, if possible, revert this PR? 🙈

If you encounter any issues - please don't hesitate to post them here or send me a DM in discord 👀

@haileyok
Copy link
Contributor Author

@kirillzyusko Hey! And I'm a little late too...no worries about being late haha. My attention is pretty much fully on other tasks right now but I definitely want to revisit this. Going to add a todo to come back to this and will reach out to you in discord if anything comes up 🙏

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 this pull request may close these issues.

4 participants